Refactor: create Caret class

[JordanMartinez]

Coming from #417, it was proposed that we refactor the caret-related code into its own class. I've opened this issue to provide a place for discussing such a change's pros, cons, and implementation.

[ghost]

// Caret scope is the current paragraph.
Caret caret = editor.getCaret();

// Line number within the current paragraph.
int line = caret.getLineNumber();

// Get the number of columns for the current line.
int column = caret.getColumnCount();

// Get the number of columns for the third line in the paragraph (0-based index).
int lineColumns = caret.getColumnCount(2);

// Move the caret to the start of the current line.
caret.moveTo( 0 );

// Move the caret to the end of the current line.
caret.moveTo( column );

// And convenience methods:
caret.moveToLineStart();
caret.moveToLineEnd();

To address multiple carets:

List<Caret> carets = editor.getCarets();
Caret caret1 = editor.getCaret(0);
Caret caret2 = editor.getCaret(1);

And other functionality such as:

// Equivalent to editor.getCaret(0);
Caret caret = editor.getCaret();

// Updates the CSS?
caret.setBlinkOnRate(500);
caret.setBlinkOffRate(500);

As well as:

Paragraph p = editor.getParagraph( 1 );
Caret caret = editor.getCaret();
if( p.contains( caret ) ) {
  // The paragraph contains the given caret instance...
}

[JordanMartinez]

Aye. the blinkrate, whether it blinks at all, and its position (position in text, paragraph position, column position, line position?) would all be in this one class.

[JordanMartinez]

Additionally, I think one could have access to CaretOffsetX if we chose to do that, but that's just an idea that came to mind. I don't know the full issues that might surround that.

[ghost]

As in:

caret.getOffsetX();
caret.getOffsetY();

If those are returning the relative coordinates from the view panel, then it might be nice to have it encapsulated as:

Position pos = caret.getViewPortPosition();

[JordanMartinez]

@DaveJarvis CaretOffsetX is used to keep the caret at the same column position when one presses up/down using the arrow keys.

Position pos = caret.getViewPortPosition();

Actually, this would just be its bounds position, which is another element that would be included that I forgot to list in my previous comment.

[ghost]

Ah. Consider:

Caret caret = editor.getCaret();
Position pos = caret.getColumnPosition();

Then pos would contain the X and Y values for the caret's current column. The Y might be superfluous, but maybe someone would find that value useful. A future Z position value, for example, might allow the caret to be positioned above or below other elements.

[JordanMartinez]

Could you explain what you mean by Position? Because when I read it, I think position between letters: |t|e|x|t where | represents such a position. However, you seem to mean Point2D (or its 3D variant).

If we did encapsulate the caret-related elements, it might also be easier to position a caret within a table.

[ghost]

Yes, I meant java.awt.Point. It is unfortunate that Point3D doesn't extend from Point. Might be a Liskov issue. Either way, definitions would be informative, such as:

• Position - Information a caret carries with it, which could include index, offset, column, and point
• Index - Absolute location into the text (0 is start and editor.text.length - 1 is end)
• Offset - Relative location into the text, depending on context:
  • For the entire text, offset has the same meaning as index
  • For a paragraph, 0 is start of paragraph and max(offset) == paragraph.length() - 1
  • For a line, 0 is start of paragraph and max(offset) == line.length() - 1
• Column - Relative location into a line of text (0 is start of line, editor.paragraph.line.length - 1 is end)
• Point - An X/Y pair, typically for view port values

The definitions aren't as important as documenting them somewhere and using them consistently.

[JordanMartinez]

A couple of things to keep in mind:

• every caret would also need its own anchor so that we can track each caret's selection and its range. In that case, we would not be extracting just the caret; we'd also extract all related fields into its own class. Therefore, we shouldn't call the class Caret, but something else that encapsulates all of this. Perhaps it should be called Selection?
• in keeping the separation of concerns principle, the StyledTextAreaModel would need to hold its own model-related info while GenericStyledArea would hold that model's view-related info (i.e. caret/selection bounds).

[JordanMartinez]

I checked to see how hard it would be to implement this. Seems a piece of cake. Below is an initial run through where the "caret" model (SelectionModel) would be stored in StyledTextAreaModel as mainCaret and its corresponding "caret" view (SelectionView) would be stored in GenericStyledArea:

package org.fxmisc.richtext.model;

import javafx.beans.value.ObservableValue;
import javafx.scene.control.IndexRange;
import org.fxmisc.richtext.model.TwoDimensional.Bias;
import org.fxmisc.richtext.model.TwoDimensional.Position;
import org.reactfx.Guard;
import org.reactfx.Subscription;
import org.reactfx.Suspendable;
import org.reactfx.value.SuspendableVal;
import org.reactfx.value.SuspendableVar;
import org.reactfx.value.Val;
import org.reactfx.value.Var;

import static org.fxmisc.richtext.model.StyledTextAreaModel.EMPTY_RANGE;
import static org.fxmisc.richtext.model.TwoDimensional.Bias.Backward;
import static org.fxmisc.richtext.model.TwoDimensional.Bias.Forward;

final class SelectionModel {

    /**
     * Private helper method.
     */
    protected static int clamp(int min, int val, int max) {
        return val < min ? min
                : val > max ? max
                : val;
    }

    // caret position
    private final Var<Integer> internalCaretPosition = Var.newSimpleVar(0);
    private final SuspendableVal<Integer> caretPosition = internalCaretPosition.suspendable();
    public final int getCaretPosition() { return caretPosition.getValue(); }
    public final ObservableValue<Integer> caretPositionProperty() { return caretPosition; }

    // selection anchor
    private final SuspendableVar<Integer> anchor = Var.newSimpleVar(0).suspendable();
    public final int getAnchor() { return anchor.getValue(); }
    public final ObservableValue<Integer> anchorProperty() { return anchor; }

    // selection
    private final Var<IndexRange> internalSelection = Var.newSimpleVar(EMPTY_RANGE);
    private final SuspendableVal<IndexRange> selection = internalSelection.suspendable();
    public final IndexRange getSelection() { return selection.getValue(); }
    public final ObservableValue<IndexRange> selectionProperty() { return selection; }

    // selected text
    private final SuspendableVal<String> selectedText;
    public final String getSelectedText() { return selectedText.getValue(); }
    public final ObservableValue<String> selectedTextProperty() { return selectedText; }

    // current paragraph index
    private final SuspendableVal<Integer> currentParagraph;
    public final int getCurrentParagraph() { return currentParagraph.getValue(); }
    public final ObservableValue<Integer> currentParagraphProperty() { return currentParagraph; }

    // caret column
    private final SuspendableVal<Integer> caretColumn;
    public final int getCaretColumn() { return caretColumn.getValue(); }
    public final ObservableValue<Integer> caretColumnProperty() { return caretColumn; }

    private Position selectionStart2D;
    final Position getSelectionStart2D() { return selectionStart2D; }
    private Position selectionEnd2D;
    final Position getSelectionEnd2D() { return selectionEnd2D; }

    private final StyledTextAreaModel<?, ?, ?> model;
    private final Subscription subscription;

    SelectionModel(StyledTextAreaModel<?, ?, ?> model) {
        this.model = model;
        Val<Position> caretPosition2D = Val.create(
                () -> offsetToPosition(internalCaretPosition.getValue(), Forward),
                internalCaretPosition, model.getParagraphs()
        );
        currentParagraph = caretPosition2D.map(Position::getMajor).suspendable();
        caretColumn = caretPosition2D.map(Position::getMinor).suspendable();

        selectionStart2D = position(0, 0);
        selectionEnd2D = position(0, 0);
        internalSelection.addListener(obs -> {
            IndexRange sel = internalSelection.getValue();
            selectionStart2D = offsetToPosition(sel.getStart(), Forward);
            selectionEnd2D = sel.getLength() == 0
                    ? selectionStart2D
                    : selectionStart2D.offsetBy(sel.getLength(), Backward);
        });

        selectedText = Val.create(
                () -> model.getText(internalSelection.getValue()),
                internalSelection, model.getParagraphs()).suspendable();

        // when content is updated by an area, update the caret
        // and selection ranges of all the other
        // clones that also share this document
        subscription = model.plainTextChanges().subscribe(plainTextChange -> {
            int changeLength = plainTextChange.getInserted().length() - plainTextChange.getRemoved().length();
            if (changeLength != 0) {
                int indexOfChange = plainTextChange.getPosition();
                // in case of a replacement: "hello there" -> "hi."
                int endOfChange = indexOfChange + Math.abs(changeLength);

                // update caret
                int caretPosition = getCaretPosition();
                if (indexOfChange < caretPosition) {
                    // if caret is within the changed content, move it to indexOfChange
                    // otherwise offset it by changeLength
                    positionCaret(
                            caretPosition < endOfChange
                                    ? indexOfChange
                                    : caretPosition + changeLength
                    );
                }
                // update selection
                int selectionStart = getSelection().getStart();
                int selectionEnd = getSelection().getEnd();
                if (selectionStart != selectionEnd) {
                    // if start/end is within the changed content, move it to indexOfChange
                    // otherwise, offset it by changeLength
                    // Note: if both are moved to indexOfChange, selection is empty.
                    if (indexOfChange < selectionStart) {
                        selectionStart = selectionStart < endOfChange
                                ? indexOfChange
                                : selectionStart + changeLength;
                    }
                    if (indexOfChange < selectionEnd) {
                        selectionEnd = selectionEnd < endOfChange
                                ? indexOfChange
                                : selectionEnd + changeLength;
                    }
                    selectRange(selectionStart, selectionEnd);
                } else {
                    // force-update internalSelection in case caret is
                    // at the end of area and a character was deleted
                    // (prevents a StringIndexOutOfBoundsException because
                    // selection's end is one char farther than area's length).
                    int internalCaretPos = internalCaretPosition.getValue();
                    selectRange(internalCaretPos, internalCaretPos);
                }
            }
        });
    }

    Suspendable omniSuspendable() {
        return Suspendable.combine(caretPosition, anchor, selection, selectedText, currentParagraph, caretColumn);
    }

    Position position(int row, int col) {
        return model.position(row, col);
    }

    Position offsetToPosition(int charOffset, Bias bias) {
        return model.offsetToPosition(charOffset, bias);
    }

    /**
     * Returns the selection range in the given paragraph.
     */
    public IndexRange getParagraphSelection(int paragraph) {
        int startPar = selectionStart2D.getMajor();
        int endPar = selectionEnd2D.getMajor();

        if(paragraph < startPar || paragraph > endPar) {
            return EMPTY_RANGE;
        }

        int start = paragraph == startPar ? selectionStart2D.getMinor() : 0;
        int end = paragraph == endPar ? selectionEnd2D.getMinor() : model.getParagraph(paragraph).length();

        // force selectionProperty() to be valid
        getSelection();

        return new IndexRange(start, end);
    }

    public void selectRange(int anchor, int caretPosition) {
        try(Guard g = suspend(
                this.caretPosition, currentParagraph,
                caretColumn, this.anchor,
                selection, selectedText)) {
            this.internalCaretPosition.setValue(clamp(0, caretPosition, model.getLength()));
            this.anchor.setValue(clamp(0, anchor, model.getLength()));
            this.internalSelection.setValue(IndexRange.normalize(getAnchor(), getCaretPosition()));
        }
    }

    /**
     * Positions only the caret. Doesn't move the anchor and doesn't change
     * the selection. Can be used to achieve the special case of positioning
     * the caret outside or inside the selection, as opposed to always being
     * at the boundary. Use with care.
     */
    public void positionCaret(int pos) {
        try(Guard g = suspend(caretPosition, currentParagraph, caretColumn)) {
            internalCaretPosition.setValue(pos);
        }
    }

    protected void dispose() {
        subscription.unsubscribe();
    }

    private Guard suspend(Suspendable... suspendables) {
        return Suspendable.combine(model.suspendableBeingUpdatedProperty(), Suspendable.combine(suspendables)).suspend();
    }

}

SelectionView would encapsulate the model and add view-related methods:

package org.fxmisc.richtext;

import javafx.scene.control.SelectionModel;
import org.fxmisc.richtext.GenericStyledArea.CaretVisibility;
import org.reactfx.value.Var;

public class SelectionView {
    
    // ### Model-related properties delegate to model
    private final SelectionModel model;
    
    // ### View-related properties
    // caret line index in paragraph
    
    private final Var<CaretVisibility> showCaret = Var.newSimpleVar(CaretVisibility.AUTO);
    public final CaretVisibility getShowCaret() { return showCaret.getValue(); }
    public final void setShowCaret(CaretVisibility value) { showCaret.setValue(value); }
    public final Var<CaretVisibility> showCaretProperty() { return showCaret; }
    
    // caret blink rate
    
    // caret bounds on screen
    
    // selection bounds on screen
    
}

[JordanMartinez]

Implementing #222 would be more difficult because plain/richTextChanges's types would need to be updated to EventStream<List<PlainTextChange>> and its rich variant in case the feature was used to rename a method in a code editor.

[ghost]

Every caret would also need its own anchor so that we can track each caret's selection and its range. In

Is this the correct separation of concerns? A Caret is a position in the text that has visual properties. A Selection is text that has a start position and an end position and distinctly different visual properties and behaviours. That a Caret and a Selection could have the same starting position is coincidental. Thus:

Caret caret = editor.getCaret();

// Select five characters starting from the caret position.
Selection selection = editor.selectFrom( caret, 5 );

// For convenience, those same five characters could be selected using:
Selection selection = caret.select( 5 );

// For multiple selections (using absolute index):
Selection selection1 = editor.newSelection( 0, 5 );
Selection selection2 = editor.newSelection( 10, 20 );

Paragraph p1 = editor.getParagraph( 0 );
Paragraph p2 = editor.getParagraph( 1 );

// Using offset into different paragraphs:
Selection selection3 = p1.newSelection( 0, 5 );
Selection selection4 = p2.newSelection( 0, 10 );

A Selection shouldn't know anything about a caret's blink rate, for example, nor whether the caret is visible. A programmer, or writer, could select multiple places in the text simultaneously, but the editor might have a single Caret instance. (See below.)

Calling caret.getText() is probably nonsensical (though it could return the character at the caret's position); whereas, selection.getText() would return the selected text, so it seems they are separate classes (much like a circle isn't an ellipse because it breaks Liskov). Both have the concept of a "position", which suggests a third class that contains information about a position in the text. The Caret would have a single "position" instance and a Selection would have two (start and end).

---

The Shift key modifies mouse dragging to perform a selection of text relative to the caret position. The Control key typically modifies the interaction to mean "additional". In Windows, for example, dragging a file while holding down Control creates an additional copy when dropped. When selecting from a list box, shift allows selecting a range whereas Control+Shift used to permit selecting additional, non-contiguous ranges. (This behaviour seems to have slowly eroded, possibly because it confused users. The behaviour could have remained a configurable option for power users versus novice users.) The advanced behaviour for an editor would be:

• No modifiers. User drags text with mouse and selection happens as usual.
• Shift modifier. User drags text and if no existing selection, select from caret to mouse click position.
• Shift modifier. User drags text and if an existing selection, select from end of selection to click position (depends on relative offsets and whether the click happens before or after the existing selection).
• Control modifier. User drags text and if click overlaps existing selection, selection is continued while dragging.
• Control modifier. User drags text and if click does not overlap existing selection, an additional selection is added.

[JordanMartinez]

Is this the correct separation of concerns? A Caret is a position in the text that has visual properties. A Selection is text that has a start position and an end position and distinctly different visual properties and behaviours. That a Caret and a Selection could have the same starting position is coincidental.

I was talking about the implementation details. These wouldn't be exposed in the public API but it helps keep track of things in a much easier way. See #430 as an example of what I mean.

Selection selection = editor.selectFrom( caret, 5 );

I'd rename this to editor.selectForward(caret, 5) and editor.selectBackward(caret, 5) as it indicates which direction you're going.

Selection selection = caret.select( 5 );

I'm not sure if that makes much sense... isn't it the area that is doing the selection? Shouldn't the API look more like:

// selects via main CaretSelectionView
public void selectRange(int from, int to)

// selects using one of the caretSelectionViews that were added previously.
public void selectRange(int from, int to, int caretSelectionIndex) 

Or they might need to be changed in name to always indicate whether we mean the main caretSelectionView or one of the ones we added/removed at a later time as this would allow using default methods to call getAbsolutePosition(int row, int col) when overloading these methods. For example:

// selects via main CaretSelectionView
public void selectMainRange(int from, int to)
public void selectMainRange(int fromRow, int fromCol, int toRow, int toCol)

// selects using one of the caretSelectionViews that were added previously.
public void selectAddedRange(int caretSelectionIndex, int from, int to) 
public void selectAddedRange(int caretSelectionIndex, int fromRow, int fromCol, int toRow, int toCol)

[JordanMartinez]

Also @DaveJarvis (since I never fully addressed this above), we use Bounds for the viewport position of the caret because a caret, in the future, could be a block caret, not just a line. Additionally, it allows one to determine whether to position a popup above, below, or to the left/right of caret.
We use Bounds for selection as well, but in this issue, it might make sense to add more API to such an instance so that one knows the bounds of the start and end positions (i.e. the anchor and caret) of the selection, not just the entire selection bounds itself.

[ghost]

I'd rename this to editor.selectForward(caret, 5) and editor.selectBackward(caret, 5) as it indicates which direction you're going.

Another possibility is to use the inherent negative property of integers... ;-)

Selection forward = editor.selectFrom( caret, 5 );
Selection backward = editor.selectFrom( caret, -5 );

---

Shouldn't the API look more like:

// selects via main CaretSelectionView
public void selectRange(int from, int to)

// selects using one of the caretSelectionViews that were added previously.
public void selectRange(int from, int to, int caretSelectionIndex) 

I was hoping that the Caret and Selection classes would be exposed as public APIs. The selectRange makes sense in a couple of contexts: paragraph and editor. Thus:

Paragraph p = editor.getParagraph(0);

// Relative paragraph offsets
p.selectRange(0, 50);
// Absolute indexes
editor.selectRange(0, 50);
// With negative indexes
editor.selectRange(50, -25);

A public Caret class exposes a convenient mechanism for selecting text:

// Selects forward from the current caret position....
Selection s1 = caret.selectRange( 5 );
// Selects backward from the current caret position....
Selection s2 = caret.selectRange( -5 );

// Creates an additional selection.
Selection s3 = caret.selectNewRange( 5 );

// Same as selectNewRange?
Selection s4 = caret.selectRange( 5, true );

// Then extend a selection by 5 characters.
s4.extendRange( 5 );

CaretSelectionView

Seems to mix concerns: a Caret isn't a Selection.

in the future, could be a block caret, not just a line.

All the more reason to expose an encapsulated Caret class? The appearance of the caret and how it works internally (by offset, indexing, relative to paragraphs, having a bounds, blinking, etc.) should be an implementation detail.

[ghost]

public void selectAddedRange(int caretSelectionIndex, int fromRow, int fromCol, int toRow, int toCol)

When developing against APIs like these, it feels like having to juggle a lot of mental math to keep the context of rows and columns in my head. I prefer to use objects and their relationships, rather than numerous index values. The row/col pairing, for example, could be a "position" (or "Point" or what have you), derived from a caret location. Consider:

Position start = editor.getCaret().getPosition();
Position end = editor.newPosition(30, 50);
Selection selection = editor.newSelection( start, end );

[JordanMartinez]

Another possibility is to use the inherent negative property of integers... ;-)
Selection forward = editor.selectFrom( caret, 5 );
Selection backward = editor.selectFrom( caret, -5 );

True, but selectForward/Backward is easier to understand at a glance. selectFrom at a glance could erroneously be interpreted as "select 5 from caret" before one realizes what it actually means. It also gets harder to understand when using bidirectional text.

I was hoping that the Caret and Selection classes would be exposed as public APIs

I think I need you to explain more of what your idealized versions of Caret and Selection would be. I'm thinking in terms of API that tells the area to select something so that it appears visually in the viewport, but my understanding of your code seems to indicate an immutable class that is created every time one calls one of the methods you mentioned (e.g. paragraph.select).

A public Caret class exposes a convenient mechanism for selecting text:

Doesn't that mix concerns as well? ;-)

Seems to mix concerns: a Caret isn't a Selection.

In a way, yes. A caret does not have anything to do with selection, but a selection does need a caret while also adding its own anchor to the mix. However, I'm working within the code that Tomas has already designed, so this was the quickest way to bring it all together.

All the more reason to expose an encapsulated Caret class? The appearance of the caret and how it works internally (by offset, indexing, relative to paragraphs, having a bounds, blinking, etc.) should be an implementation detail.

So, it seems that you want Caret to be its own Node, correct? Would Selection be its own Node as well?

[ghost]

selectForward/Backward is easier to understand at a glance.

Agreed. It will, however, lead to code duplication. That is, if I have to encode logic to determine whether to call forward/backward, someone else will likely have to write that same code. Instead, it should be captured by a single, reusable method that contains said logic in one spot. Perhaps a different name and Javadocs would help?

I think I need you to explain more of what your idealized versions of Caret and Selection would be.

Ideally, I'd like to interact with the API using Position, Caret, Selection, Paragraph, and Editor objects. (I'm not sure if it makes sense to have a separate CaretModel and CaretView since this is mostly view code we're concerned about.) A Caret class could have the following behaviours:

• Can answer whether it is at a given position
• Can change its blink rate
• Can change its presentation (block, line, triangle, happy face, etc.)
• Can change its location in the text
• Can answer whether it is visible (i.e., on the screen)
• Can answer whether it is blinking
• Ability to move to a new position (e.g., moveToColumn, moveToLine, moveTo)
• Optionally, can return its current paragraph (delegates position to the editor)
• Optionally, can create a new selection starting at its position (delegates position to the editor)

And the following attributes:

• A position (index relative to the editor, offset relative to paragraph, column relative to current line).
• View port bounds

Doesn't that mix concerns as well? ;-)

It's delegation for convenience, and it is a slight mixing of concerns, therefore not at all necessary.

In a way, yes. A caret does not have anything to do with selection, but a selection does need a caret while also adding its own anchor to the mix.

Does it need a Caret or does it need two Positions? That is, why does a Selection need to be bound, in any way, to a caret? I understand that what you've accomplished fits within the current code base, but perhaps there's a way to revise the code base that will facilitate migrating to a complete separation of these two concepts in the future?

[JordanMartinez]

Agreed. It will, however, lead to code duplication. That is, if I have to encode logic to determine whether to call forward/backward, someone else will likely have to write that same code. Instead, it should be captured by a single, reusable method that contains said logic in one spot. Perhaps a different name and Javadocs would help?

Unless we implemented such methods as default methods that reroute their calls to one selectFrom method. Then this would reduce code duplication while also increasing clarity.

(I'm not sure if it makes sense to have a separate CaretModel and CaretView since this is mostly view code we're concerned about.

I understand that what you've accomplished fits within the current code base, but perhaps there's a way to revise the code base that will facilitate migrating to a complete separation of these two concepts in the future?

Perhaps that is what we should be discussing instead.
When we made the base area (which was StyledTextArea at the time) stop extending Control so that we could grant access to the view, the resulting code mixed the model and view together. So, we then moved all model-related items from the view over to a new class, StyledTextAreaModel, to increase separation of concerns. However, that was when EditableStyledDocument was a package-private class. Sometime after I implemented #152, EditableStyledDocument became a public interface. As you can see in #430, once one extracts the caret and its selection from StyledTextAreaModel, it really only acts as a class that delegates all calls to the underlying document; there's really not much else in that class.

So, what if we extracted the caret/selection into the view (currently, GenericStyledArea) and removed the StyledTextAreaModel class? Then the base area would delegate its related methods to its underlying EditableStyledDocument.

[ghost]

So, what if we extracted the caret/selection into the view (currently, GenericStyledArea) and removed the StyledTextAreaModel class?

A great question for @TomasMikula .

[JordanMartinez]

@afester Can you give us your thoughts as well?

[JordanMartinez]

So, what if we extracted the caret/selection into the view (currently, GenericStyledArea) and removed the StyledTextAreaModel class? Then the base area would delegate its related methods to its underlying EditableStyledDocument.

This was done in #463. So, I think extracting the caret and its selection would be cleaner to do now. Moreover, when I was working on this on my own local branch, I found that there might be cases where one wants a selection to depend on a caret. In such a case, BoundedSelection can modify the underlying caret's position, and, for other cases, an UnboundedSelection simply selects something and has no dependency on a caret.

[ghost]

In such a case, BoundedSelection can modify the underlying caret's position, and, for other cases, an UnboundedSelection simply selects something and has no dependency on a caret.

Does it make sense to have BoundedSelection subclass of Selection? Or does composition make more sense here? Inheritance is nice because they can share the same code base, with the BoundedSelection updating the caret where the Selection does not, but could lead to Liskov violations.

[JordanMartinez]

I imagine Selection more as an interface than a base class

[JordanMartinez]

Granted, we could still have a SelectionBase class that is package-private to reduce code duplication

[JordanMartinez]

In other words:

public interface Selection {}
class SelectionBase implements Selection {}
public final class UnboundedSelection extends SelectionBase {}
public final class BoundedSelection extends SelectionBase {
    public BoundedSelection(Caret caret) {}
}

[ghost]

Granted, we could still have a SelectionBase class that is package-private to reduce code duplication.

Good idea!

[JordanMartinez]

I think as a first step, we could refactor the code to extract the caret and selection (bounded version) to their own classes. However, we wouldn't be able to implement #222 (e.g. UnboundedSelection) until after we change how getSelectionBoundsOnScreen works. It currently builds the Bounds object based on all the node's selections whereas we'd need it to only get the selections of the nodes that the selection covers.

[ghost]

        double minX = Stream.of(bounds).mapToDouble(Bounds::getMinX).min().getAsDouble();
        double maxX = Stream.of(bounds).mapToDouble(Bounds::getMaxX).max().getAsDouble();
        double minY = Stream.of(bounds).mapToDouble(Bounds::getMinY).min().getAsDouble();
        double maxY = Stream.of(bounds).mapToDouble(Bounds::getMaxY).max().getAsDouble();

Can be:

        double minX = doubleMin(bounds, Bounds::getMinX);
        double maxX = doubleMax(bounds, Bounds::getMaxX);
        double minY = doubleMin(bounds, Bounds::getMinY);
        double maxY = doubleMax(bounds, Bounds::getMaxY);

As well:

 return Optional.of(new BoundingBox(minX, minY, maxX-minX, maxY-minY));

Can be:

 return Optional.of(createBoundingBox(minX, minY, maxX-minX, maxY-minY));

With new BoundingBox replaced by createBoundingBox, which then allows subclasses to override instantiation of bounding boxes with custom implementations, provided createBoundingBox is protected.

On the same token, showCaretAtTop, followCaret(), and showCaretAtBottom() appear to have duplication that can be eliminated.

[JordanMartinez]

Can be:

    double minX = doubleMin(bounds, Bounds::getMinX);
    double maxX = doubleMax(bounds, Bounds::getMaxX);
    double minY = doubleMin(bounds, Bounds::getMinY);
    double maxY = doubleMax(bounds, Bounds::getMaxY);

Where is doubleMin/Max defined?

With new BoundingBox replaced by createBoundingBox, which then allows subclasses to override instantiation of bounding boxes with custom implementations, provided createBoundingBox is protected.

Why would that be needed?

On the same token, showCaretAtTop, followCaret(), and showCaretAtBottom() appear to have duplication that can be eliminated.

How is it duplicated?

[ghost]

I'm a bit of a stickler when it comes to duplication, so feel free to ignore my suggestions. :-) IMO, every requirement should be expressed once and only once within the code base. Here's another example of code duplication within the same class:

    private Optional<Bounds> impl_popup_getSelectionBoundsOnScreen() {
        IndexRange selection = getSelection();
        if(selection.getLength() == 0) {
            return getCaretBoundsOnScreen();
        }

        return impl_getSelectionBoundsOnScreen();
    }

    private Optional<Bounds> impl_bounds_getSelectionBoundsOnScreen() {
        IndexRange selection = getSelection();
        if (selection.getLength() == 0) {
            return Optional.empty();
        }
        return impl_getSelectionBoundsOnScreen();
}

Can be:

    private Optional<Bounds> impl_popup_getSelectionBoundsOnScreen(final Optional<Bounds> noSelection) {
        final IndexRange selection = getSelection();
        return selection.getLength() == 0 ? noSelection : impl_getSelectionBoundsOnScreen();
    }

    private Optional<Bounds> impl_popup_getSelectionBoundsOnScreen() {
        return impl_popup_getSelectionBoundsOnScreen(getCaretBoundsOnScreen());
    }

    private Optional<Bounds> impl_bounds_getSelectionBoundsOnScreen() {
        return impl_popup_getSelectionBoundsOnScreen(Optional.empty());
}

Then, if "getSelection()" ever returns null, there's only one spot in the code to fix. Or maybe a new requirement comes in where someone wants to insert their own behaviour when the selection is empty and a pop-up is invoked.

Where is doubleMin/Max defined?

They aren't. They'd be convenience methods defined elsewhere in the class, similar to the above example. Regarding the showCaret methods, you probably have a sense of the duplication now:

    void showCaretAtBottom() {
        int parIdx = getCurrentParagraph();
        Cell<Paragraph<PS, SEG, S>, ParagraphBox<PS, SEG, S>> cell = virtualFlow.getCell(parIdx);
        Bounds caretBounds = cell.getNode().getCaretBounds();
        double y = caretBounds.getMaxY();
        virtualFlow.showAtOffset(parIdx, getViewportHeight() - y);
    }

    void showCaretAtTop() {
        int parIdx = getCurrentParagraph();
        Cell<Paragraph<PS, SEG, S>, ParagraphBox<PS, SEG, S>> cell = virtualFlow.getCell(parIdx);
        Bounds caretBounds = cell.getNode().getCaretBounds();
        double y = caretBounds.getMinY();
        virtualFlow.showAtOffset(parIdx, -y);
}

Same idea can be applied:

    Bounds getCaretBounds() {
      return getCaretBounds( getCurrentParagraph() );
    }

    Cell<...> getCell(final int paragraph) {
      return virtualFlow.getCell(paragraph);
    }

    Bounds getCaretBounds(final int paragraph) {
      return getCell(paragraph).getNode().getCaretBounds();
    }

    void showCaretAt(double y) {
        final int paragraph = getCurrentParagraph();
        virtualFlow.showAtOffset(paragraph, y);
    }

Now the methods can be simplified to:

    void showCaretAtBottom() {
        final double y = getViewportHeight() - getCaretBounds().getMaxY();
        showCaretAt(y);
    }

    void showCaretAtTop() {
        final double y = getCaretBounds().getMinY();
        showCaretAt(y);
}

In the above, both showCaretAtBottom() and showCaretAtTop() now succinctly define only those operations that are pertinent to the task at hand. It is much easier to see, IMO, what the methods are doing without the clutter of obtaining the caret bounds. Here, getCaretBounds() could be renamed to getCurrentParagraphCaretBounds() for clarity, but that's a minor detail. If there's an issue whereby the code could introduce a race condition, then it might mean passing along the caret bounds into the convenience method, showCaretAt(...).

Notice, too, how the number of direct references to virtualFlow has been halved. By reducing the number of times any instance variable is referenced directly, it decreases the amount of future effort required to refactor the class. (This is one reason for self-encapsulation of instance variables: a little extra effort now can save oodles of time in the future.)

Why would that be needed?

In this case, it might not be needed. I've run into similar situations where all I wanted to do was override a single method to inject my own behaviour. By not allowing subclasses to reuse behaviour, it can introduce tight-coupling. It could also mean the class simply has too many responsibilities and therefore the domain model is incomplete.

[JordanMartinez]

impl_getSelectionBoundsOnScreen is only there so the old Popup API, which was deprecated in #410, still works. The impl_popup version will be removed in future versions and the impl_bounds version will be renamed to the plain getSelectionBoundsOnScreen with the method content of impl_getSelectionBoundsOnScreen() being inlined into that method. Once the caret and selection are extracted, a Selection object will have getBoundsOnScreen method, which will delegate the call to the area.

The code in the showCaret methods could definitely be reduced. Thanks for pointing that out so clearly!

In this case, it might not be needed. I've run into similar situations where all I wanted to do was override a single method to inject my own behaviour. By not allowing subclasses to reuse behaviour, it can introduce tight-coupling. It could also mean the class simply has too many responsibilities and therefore the domain model is incomplete.

That might fall into the area Tomas mentioned about making the API more stable. In any case, assuming we did extract the caret/selection, couldn't you just wrap such an object and override the getBoundsOnScreen method? Or is that what you mean by tight coupling?

[ghost]

Or is that what you mean by tight coupling?

Consider this fictional example:

class SelectionBase implements Selection {
  private Bounds bounds;

  protected void recalculateBounds() {
    this.bounds = new ViewPortBounds();
    this.bounds.setMinX( 0 );
    this.bounds.setMinY( 0 );
    this.bounds.setMaxX( getSelectionEnd().x() );
    this.bounds.setMaxY( getSelectionEnd().y() );
  }

  public void recalculateBounds(Caret caret) {
    this.bounds = new ViewPortBounds();
    this.bounds.setMaxX( caret.x() );
    this.bounds.setMaxY( caret.y() );
  }
}

This code has eliminated the possibility for a developer to easily inject new behaviour every time the bounds for the selection is recalculated. Solutions to this problem include dependency injection, inversion of control, and factory methods, all of which can be a bit heavy. As written, it is not possible to easily add a custom Selection instance that can detect recalculation of the Bounds values. Were it written like:

class SelectionBase implements Selection {
  private Bounds bounds;

  protected void recalculateBounds() {
    this.bounds = createBounds();
    // ...same as before
  }

  public void recalculateBounds(Caret caret) {
    this.bounds = createBounds();
    // ...same as before
  }

  protected Bounds createBounds() {
    return new ViewPortBounds();
  }
}

Now this is possible:

class CustomSelection extends SelectionBase {
  protected Bounds createBounds() {
    return new CustomViewPortBounds();
  }
}

The original behaviour cannot be extended to provide new functionality. For example, maybe the view port bounds are being set by software reading from a socket hooked up to remote touch screen.

Also, technically, the line this.bounds = new ViewPortBounds(); appears twice, which is duplication.

Using the original code, we'd have to override both methods and, worse, re-implement them:

class CustomSelection extends SelectionBase {
  protected void recalculateBounds() { ... }
  public void recalculateBounds(Caret caret) { ... }
}

Here, the CustomSelection has an unnecessary coupling to the Caret interface, whereas with the single protected Bounds createBounds() method, it meant CustomSelection didn't need to reference the Caret interface at all, giving more freedom for the interface and base superclass to change without affecting the subclasses.

In the ResolverYAMLGenerator code I linked to as an example, the purpose of the program is to read and write YAML files. Why does the program need to know about IOContext and ObjectCodec and DumperOptions to write YAML files? It shouldn't, but was forced to because of an inflexible design that was not coded for reuse.

[JordanMartinez]

In other words, you're suggesting we implement hooks into the normal calculations so that one can easily override these values without granting access to anything else, correct? For example, given the code below:

public final class BoundedSelection implements Selection {
    private final Val<Bounds> boundsOnScreen
    
    public BoundedSelection(Caret caret) {
        this.caret = caret;
        this.boundsOnScreen = Val.create (
            () -> area.calculateBoundsOnScreenFor(this), 
            selectionProperty());
}

rather than having area.calculateBoundsOnScreenFor(this), which cannot be quickly overridden without a lot of boilerplate / unnecessarily exposing additional UnboundedSelection info/properties like caret, the class should have a protected recalculateBounds method that gets called instead, which the developer can override (should, for example, he want the bounds to be offset by some value). The default implementation of that method would just call area.calculateBoundsOnScreenFor(this) For example:

public class BoundedSelection implements Selection {
    private final Val<Bounds> boundsOnScreen
    
    public BoundedSelection(Caret caret) {
        this.caret = caret;
        this.boundsOnScreen = Val.create (
            this::recalculateBounds, 
            selectionProperty());

    protected Bounds recalculateBounds() {
        return area.calculateBoundsOnScreenFor(this);
    }
}

Is that correct?

[ghost]

In other words, you're suggesting we implement hooks into the normal calculations so that one can easily override these values without granting access to anything else, correct?

By way of example, I wanted to show how code duplication can be refactored to: (a) make shorter methods; and (b) increase the reusability and extensibility of the interface implementations.

Thus:

public class BoundedSelection implements Selection {
    private final Val<Bounds> boundsOnScreen;
    
    // Use final modifier for method/constructor arguments, as well.
    public BoundedSelection(final Caret caret) {
        // Having "this.caret = caret" in the accessor *and* constructor is duplication.
        setCaret(caret);

        // Subclasses wouldn't necessarily "know" to implement Val.create.
        setBoundsOnScreen( createBounds(
            this::recalculateBounds, 
            selectionProperty()
        ));
    }

    protected Val<Bounds> createBounds( final Bounds bounds ) {
      return Val.create( bounds );
    }

    protected Bounds recalculateBounds() {
      return getArea().calculateBoundsOnScreenFor(this);
    }
}

Assigning local variables is also helpful for debugging:

public class BoundedSelection implements Selection {
    private final Val<Bounds> boundsOnScreen;
    
    // Use final modifier for method/constructor arguments, as well.
    public BoundedSelection(final Caret caret) {
        // Having "this.caret = caret" in the accessor *and* constructor is duplication.
        setCaret(caret);

        //  Break out the bounds stuff.
        initBounds();
    }

    protected void initBounds() {
        final Bounds bounds = recalculateBounds();
        final Property<Selection> selectionProperty = selectionProperty();

        final Val<Bounds> screenBounds = createValBounds( 
            bounds, selectionProperty );

        // Subclasses wouldn't necessarily "know" to implement Val.create.
        setBoundsOnScreen( screenBounds );
    }

    protected Val<Bounds> createValBounds( final Bounds bounds ) {
      return Val.create( bounds );
    }

    protected Bounds recalculateBounds() {
      return getArea().calculateBoundsOnScreenFor(this);
    }
}

[JordanMartinez]

@DaveJarvis Just FYI. I'm finishing up a semester of school and have a couple of papers due in the last few weeks, so I probably won't respond to this (much less start working on implementing what we ultimately come up with) until mid-May.

[JordanMartinez]

Closed by #526