Extract caret and selection to separate classes

[JordanMartinez]

Address #429.

[JordanMartinez]

@davejarvis What's your feedback on this API?

I'm also wondering where the default methods for the caret/selection API that was extracted into TextEditingArea should go since it blurs both model and view API in the same interface (is that desired?). If we put them into NavigationActions, that makes sense, but what about their bounds-related API? I don't think we should split the methods up between two interfaces (e.g. NavigationActions and ViewActions).

I don't have fancier methods like caret.moveToLineStart() implemented on the Caret interface or selectLine() implemented on the *Selection interfaces. It shouldn't be too hard to implement.

[ghost]

Regarding Caret.java:

• Public methods should have full comments describing the purpose of the method, pre-conditions, post-conditions, what to expect from return types, and the meaning of parameter values.
• dirtyEvents() seems like something that shouldn't need to be part of a public interface.
• Perhaps add some definitions about the meaning of various relative position types to the interface itself (e.g., "within the text" means anywhere in the document, "paragraph" means the caret's current paragraph, "line" means the current line. Line 56, "Moves the caret to the given position in the area." isn't clear for the meaning of "area" -- line, paragraph, or entire text?).

In BoundedSelection.java:

• Method and interface comments, as above.
• There's some duplication in the signatures for the selectRange methods

The (int anchorParagraph, int anchorColumn, int caretParagraph, int caretColumn) looks like it would be more refined as (Location anchor, Location caret).

The (int anchorPosition, int caretPosition) doesn't make clear the relationship between anchorPosition and caretPosition. Typically a selection goes from a starting point to an ending point. Thus, (int startPosition, int endPosition). Again, this is with the view that a range selection needn't have any relation to a caret, leading to:

/**
 * Selects a range in the current document using absolute values. If the
 * document is empty, this performs a no-op.
 * 
 * @param start The starting character position for the selection.
 * @param end The ending character position for the selection (must be greater than
 * or equal to start).
 * @param moveCaret If true, move the caret to the end position, otherwise
 * retain the caret's position independently from the selection (to allow for
 * multiple selections).
 */
void selectRange( int start, int end, boolean moveCaret );

Lines 150 to 160 of BoundedSelectionImpl.java contain duplication that can be eliminated with a variable assignment.

[JordanMartinez]

Thanks for the feedback!

Caret

Public methods should have full comments describing the purpose of the method, pre-conditions, post-conditions, what to expect from return types, and the meaning of parameter values.

I agree that documentation should be added to explain the purpose of the code at both the class/interface and method levels. At the same time, I'm not sure about the extent that you're describing when it comes to documenting pre-conditions, post-conditions, and the meaning of parameter values. Tomas believes in self-documenting code; I agree with it in principle, but still would like some documentation to explain what's going on. I think that's accomplished when the purpose is documented along with clear parameter names.
The pre-conditions and post-conditions, while good and helpful, are something that don't exist in the majority of this project's code base. I understand this argument is purely a "Why start now?" argument, but perhaps that task should be tackled in a separate PR to insure consistent style and content. On another hand, perhaps this doesn't seem needed to me because I have much greater familiarity with the code than others. So perhaps I'm just biased.

Perhaps add some definitions about the meaning of various relative position types to the interface itself (e.g., "within the text" means anywhere in the document, "paragraph" means the caret's current paragraph, "line" means the current line. Line 56, "Moves the caret to the given position in the area." isn't clear for the meaning of "area" -- line, paragraph, or entire text?).

Aye, I'd like to put that in the interface level since this would be the best place to document that. Originally, int position was called textPosition to distinguish it from another (later removed) variable TwoDimensional.Position 2DPosition (the paragraphIndex-columnPosition position). However, the extra word made the code that much longer (e.g. startTextPositionProperty(), start2DPositionProperty()) and position is what's been used throughout the project's history thus far. Since I didn't think 2DPosition would be used that much, I decided to remove it.

dirtyEvents() seems like something that shouldn't need to be part of a public interface.

I'm iffy on this. I added that in case one needed to use it as a trigger for something else. However, I can't think of any use cases off the top of my head, and I suppose they could just use either the caret's position or bounds property as an equivalent trigger.

Selection

The (int anchorParagraph, int anchorColumn, int caretParagraph, int caretColumn) looks like it would be more refined as (Location anchor, Location caret).

The right way would be (TwoDimensional.Position anchorPosition, TwoDimensional.Position caretPosition), and I initially had another method for that in each implementation. However, I thought that most people wouldn't go to the effort of constructing such objects and using them to select some range. Should I readd that?

There's some duplication in the signatures for the selectRange methods

That's intentional. It allows one to store a list of Selection (whether bounded or not) objects and use the same API to select some range. At the same time, that's also problematic because the arguments for UnboundedSelection work differently than when used for the BoundedSelection.

The (int anchorPosition, int caretPosition) doesn't make clear the relationship between anchorPosition and caretPosition. Typically a selection goes from a starting point to an ending point. Thus, (int startPosition, int endPosition). Again, this is with the view that a range selection needn't have any relation to a caret...

True, but that's why UnboundedSelection exists as a forward-thinking foundation for implementing #222. The goal of this API is merely to refactor the code into separate interfaces without breaking changes in the area's API. Since the area both selects content and moves the caret via selectRange(int anchorPos, int caretPos), I am not reworking the behavior to completely change that. If we did move away from that concept, so that only Caret and UnboundedSelection were used independent of one another, I'm not sure whether that would be better or actually worse than the current behavior.

[JordanMartinez]

Lines 150 to 160 of BoundedSelectionImpl.java contain duplication that can be eliminated with a variable assignment.

Not sure I understand you. Could you rewrite the method in your way to explain?

[JordanMartinez]

• I changed UnboundedSelection's selectRange(int, int) signature to selectRange0(int, int) while BoundedSelection still uses the regular selectRange(int, int). Personally, I feel like the unbounded one should use no suffix and the bounded one should either use a suffix or have a different select-name altogether. However, for the sake of maintaining source compatibility, I'm not doing that.
• I added documentation. I hope that makes enough sense

[JordanMartinez]

in cef005b, I made a typo: selectRange was used instead of selectRange0

[JordanMartinez]

@davejarvis If you have additional feedback, please share. Otherwise, I'll merge this sometime tomorrow.

[JordanMartinez]

One issue I just realized: selectedText works if one is using StyledTextArea<PS, S>, but what if one is uses custom objects via GenericStyledArea<PS, SEG, S>? Shouldn't there be a selectedDocument getter and property, too?
However, as soon as I do that, I have to add all the boilerplate generic code (i.e., BoundedSelection<PS, SEG, S>, UnboundedSelection<PS, SEG, S>). :-/

[JordanMartinez]

One issue I just realized: selectedText works if one is using StyledTextArea<PS, S>, but what if one is uses custom objects via GenericStyledArea<PS, SEG, S>? Shouldn't there be a selectedDocument getter and property, too?
However, as soon as I do that, I have to add all the boilerplate generic code (i.e., BoundedSelection<PS, SEG, S>, UnboundedSelection<PS, SEG, S>). :-/

I think for now, I'll leave that out. Only those who use custom objects in an area might use it, and of those that create such an area, most will probably not use it.

[JordanMartinez]

One issue I just realized: selectedText works if one is using StyledTextArea<PS, S>, but what if one is uses custom objects via GenericStyledArea<PS, SEG, S>? Shouldn't there be a selectedDocument getter and property, too?
However, as soon as I do that, I have to add all the boilerplate generic code (i.e., BoundedSelection<PS, SEG, S>, UnboundedSelection<PS, SEG, S>). :-/

I think for now, I'll leave that out. Only those who use custom objects in an area might use it, and of those that create such an area, most will probably not use it.

On second thought, one could write BoundedSelection<?, ?, ?> selection = area.getMainSelection() to interact with that object for anything unrelated to the selectedDocument-related getter and property. If we document that pattern in the javadoc, it'll reduce the generic boilerplate one would have to use.