Oobranch c : ootext and rangesort

[antalk2]

Adds

• ootext: interpret HTML-like markup into a writer document
• rangesort : sort XTextRange ranges visually and check if they overlap

[Siedlerchr]

please no single character variables, better give it a more meaningful variable. Same applies for others

[antalk2]

renamed to OOText ootext

[Siedlerchr]

can this be removed? Better use LOGGER.debug(...)

[antalk2]

Changed to use LOGGER.debug() and not to use debugThisFun

• I believe LOGGER calls will be included in production code.
• Is this intended?

[Siedlerchr]

Yes, this is fine. debug calls are only enabled with the debug flag starting parameter.

[calixtus]

Just a question: VoidResult feels a bit strange. Using an optional is no alternative?

[antalk2]

Just a question: VoidResult feels a bit strange. Using an optional is no alternative?

That would be Optional<ERRORTYPE>. Its normal use is Optional<RESULTTYPE>

At a call site this would mean using the first alternatives below

 Failure                                                 
 --------------------------------       
 return Optional.of(message)                 
 return VoidResult.error(message)         

 Success
 --------------------------
 return Optional.empty()
 return VoidResult.ok()

Answer:

• Technically: both Optional and VoidResult can store 0 or 1 values,
  in this respect they are equivalent
  • Actually, VoidResult is just a wrapper around an Optional
• In terms of communication to human readers when used:
  • Optional.empty() normally suggests failure, VoidResult.ok() does not.
  • Optional.of(message) may be confused with success,
    VoidResult.error(message) cannot
  • VoidResult is "the other half" (the failure branch) of Result
    • its content is accessed through getError, mapError, ifError, not get, map, ifPresent

Just as a basic enum does not add new capabilities to a set of constants,
and a function does the same as its inlined version, VoidResult does not add new capabilities.
Everything you can do with it can be translated to using Optional (while carefully keeping track of which
Optionals represent values we wanted and which represent errors).
These Optionals can in turn can be translated to using null (again, carefully keeping track of which
values may be null and which ones can be safely dereferenced).

VoidResult does allow

• a clear distinction between success and failure when
  calls to get something that might not be available (Optional) and
  calls to precondition checking where we can only get reasons for failure
  (VoidResult)
  appear together.
  Using Optional for both is possible, but is more error-prone.

• it also allows using uniform verbs (isError, getError, ifError, return {Void}Result.error) for "we have a problem" when

  • checking preconditions (VoidResult) is mixed with
  • "I need an X" orelse "we have a problem" (Result)

• at a functions head:

  • VoidResult<String> function() says: no result, but may get an error message
  • Optional<String> function() says: a String result or nothing.

Summary: technically could use Optional or null but it would be less precise,
leaving more room for confusion and bugs. Forces use of getError instead of get,
and isError or isOk instead of isPresentor isEmpty.

From this and an earlier question it seems that https://en.wikipedia.org/wiki/Result_type
is not well-known among the maintainers. It is not unknown in java:

• https://github.com/bgerstle/result-java/
• https://github.com/MrKloan/result-type
• https://gist.github.com/david-bakin/35d55daeeaee1eb71cea
• https://www.baeldung.com/vavr-try

It can be considered as an extended Optional, that can provide details on "why empty"?
It can be considered as an alternative to throwing an exception: we return an error instead.
It shares the problem that in a function several types of errors may occur,
but we can only return a single error type. Java solves this using checked exceptions
being all descendants of Exception.
Methods throwing checked exceptions however cannot be used with for example List.map.

I used OOError as the unified error type: it can store error messages and wrap exceptions.
It depends on (extends) JabRefException (this forces it to at least logic) and also provides
showErrorDialog (forcing it to gui).

[calixtus]

Please don't suppose that all the maintainers don't know their stuff, it's just me who asked a question, since Im not a computer scientist by education. Please remember that we do this all in our free time and JabRef as a project for people to learn about Open Source and programming. Especially me. Always learning. 😉

[calixtus]

But thanks for the very good answer.

[antalk2]

Please don't suppose that all the maintainers don't know their stuff, it's just me who asked a question,

OK, I just needed some phrase to introduce why am I going into these details.

since Im not a computer scientist by education.

Neither I am. The verbosity and some of the repetition is probably partly due to that: I am clarifying my thoughts
about the details for myself as well.

[koppor]

@antalk2 Would you find time to convert #7788 (comment) into an ADR? 😇 --> https://github.com/adr/adr-manager/ (would be nice for others reading the code later)

[Siedlerchr]

For consistency I would use also Object.equals

[antalk2]

This code is possibly inconsistent with hashCode using position below, while I used position.x and position.y here.
Part of my confusion comes from that I feel "equal" should consider the content part,
however in compareTo I did not want to prescribe comparability for the content.

I think I implemented equals and hashCode because Comparable seems to require it,
in order to make it suitable as a key in TreeMap and HashMap

Although compareTo appeared to me as "natural order", it is probably not that in the sense
as Comparable expects it. Considering right-to-left languages suggests that it is not even
natural in a non-technical sense.

Dropping implements Comparable<ComparableMark<T>>, compareTo, equals and hashCode,
adding private static int compareTopToBottomLeftToRight(ComparableMark a, ComparableMark b)
to RangeSortVisual instead

[antalk2]

@antalk2 Would you find time to convert #7788 (comment) into an ADR? innocent --> https://github.com/adr/adr-manager/ (would be nice for others reading the code later)

I seem to have lost my earlier answer suggesting a description in docs/openoffice instead.

I wrote this: https://github.com/koppor/jabref/blob/69f7be4215bdbb21789a7946c08bc8a9db4c8719/docs/openoffice/ooresult-ooerror.md

I also attempted to compare with other solutions I could come up with: https://github.com/koppor/jabref/blob/69f7be4215bdbb21789a7946c08bc8a9db4c8719/docs/openoffice/ooresult-alternatives.md

I think that OOResult as used in OOBibBase2 resulted in too much boilerplate in the actions,
making them hard to read. The best I could come up with is throwing an exception: basically
throwing OOError instead of returning it allows us to catch and show all of them in a single location.

[k3KAW8Pnf7mkmdSMPHz27]

Overall, looks good to me, and all the comments are very appreciated!

90%+ of my comments are nitpicks, so take the "Request changes" with a grain of salt.

1. Nitpicks = change them if you agree (e.g., do they improve readability?), but I'd be ok with merging without those changes.
2. I'd suggest that you use /** for private methods/variables as well, some tools expect it.
3. Try to avoid having loose comments between methods, it's better if you either make it a JavaDoc or comment inside the method so there is no ambiguity of what it is referring to

[k3KAW8Pnf7mkmdSMPHz27]

@Siedlerchr shouldn't the indention of case fail checkstyle?

[k3KAW8Pnf7mkmdSMPHz27]

nitpick: private final

[antalk2]

ok, private final XTextRangeCompare cmp;

HolderComparatorWithinPartition itself could be private as well.

Changed class comment (on to: HolderComparatorWithinPartition)

    /**
     * Compare two RangeHolders (using RangeHolder.getRange()) within an XText.
     *
     * Note: since we only look at the ranges, this comparison is generally not consistent with
     * `equals` on the RangeHolders. Probably should not be used for key comparison in
     * TreeMap<RangeHolder> or Set<RangeHolder>
     *
     */

Note: TreeMap<RangeHolder> or Set<RangeHolder> is hard to read, but I believe javadoc /** */ expects it.
Is there a way to make the comment both readable and javadoc-compatible?

Maybe, like /* Object.equal */, the content of the "Note:" part it is expected to be well-known among java users,
thus superfluous.

[k3KAW8Pnf7mkmdSMPHz27]

HolderComparatorWithinPartition itself could be private as well.

Fair enough. If you aren't going to use it in a later PR.

Note: TreeMap<RangeHolder> or Set<RangeHolder> is hard to read, but I believe javadoc /** */ expects it.

What about {@code TreeMap<RangeHolder>} or {@code Set<RangeHolder>}?

Maybe, like /* Object.equal */, the content of the "Note:" part it is expected to be well-known among java users,
thus superfluous.

Kind of, but with /* Object.equal */, you are already saying that you are @Override an inherited method. I am also not sure that the concept of consistency with equals is that well-known X)

Perhaps say that the natural ordering isn't consistent with equals? I don't think it is likely to be an issue, and depending on what you are extending with RangeHolder it might not be true.

[k3KAW8Pnf7mkmdSMPHz27]

nitpick: they are not thrown?

[antalk2]

Apparently not. Removed.

Question: is there an option or tool to discover such superfluous exception declarations?

[k3KAW8Pnf7mkmdSMPHz27]

It is built into IntelliJ :/

Perhaps PMD? I haven't tried any except IntelliJ/CheckStyle. I think any code inspection tool should be able to find them (and a lot more).

Perhaps it is something we should look into adding to our checkstyle settings.

[k3KAW8Pnf7mkmdSMPHz27]

This cannot happen? One element is added to positions for each element in inputs? Or is it thrown if there are changes in the document before the method finishes?

[antalk2]

I would say it should not happen.
I think at some point I was losing positions corresponding to some inputs because
set below was a Set and reference marks in footnotes mapped to
the exact same coordinates (of the footnote marker), and the Set silently
ignored the duplicates.

UnoScreenRefresh.hasControllersLocked(doc) was another instance: in this case
the cursor position was not updated, and I think we got the same (wrong) position for
each range we wanted to test.

This cannot happen?

I hope it cannot. If it does, we have a problem.

Or is it thrown if there are changes in the document before the method finishes?

I cannot tell what could happen if the user changes the document or even just the cursor position
while we are trying to use it. Neither the documentation of gotoRange nor of getPosition discusses the possibility of failure. Nor does the getViewCursor documentation mentions it. Yet they can, and did fail,
and I was trying to program against observed (as opposed to documented) behaviour.

On the other hand we have two more checks on the size, probably the last one is enough to discover if there is a problem.

Removing the first two tests, keeping only if (result.size() != inputSize)

[Siedlerchr]

Okay, in order to move forward I am merging this now