After undo (ctrl-Z), text insertion point jumps to the start of the GenericStyledArea

[sconrad94]

I understand that there's no current maintainer of the project. I'm reporting this in case someone decides to pick it up the project or someone else is looking for a fix for this specific issue. (I edited an hour or two after the initial post because it did the wrong thing after a Ctrl-Y (redo).)

Expected Behavior

The insertion point (i.e. the selected text) in a GenericStyledArea should match the caret.

Actual Behavior

After a ctrl-Z (undo), the caret remains where it was (at least in a relative sense) but the insertion point for new text jumps to position 0.

Reproducible Demo

Run the RichTextDemo:

$ ./gradlew RichTextDemo

type in any text
wait 500 ms or more
type in additional text
type ctrl-Z to undo the last edit.
note that the caret is at the end of the first text that was typed
type in new text
note that the new text is inserted at the start of the text rather than at the position of the caret.

Environment info:

• RichTextFX Version: 0.9.1
• Operating System: Fedora release 23
• Java version: 1.8.0_77

---

Here's my fix to SelectionImpl.java starting at line 221. I didn't always understand the intent of the author, so my change may break desired behavior that I'm unaware of. I tried posting code deltas, but that didn't that didn't play nice with github's markup style.

        manageSubscription(area.multiPlainChanges(), list -> {
            int finalStart = getStartPosition();
            int finalEnd = getEndPosition();
            for (PlainTextChange plainTextChange : list) {
                int netLength = plainTextChange.getNetLength();
                //if (netLength != 0)  Causes IndexOutOfBoundsException in ParagraphText.getRangeShapecursorSafely issue #689
                // but can be safely reimplemented if this causes other issues.
                {
                    int indexOfChange = plainTextChange.getPosition();
                    // use abs() in case of a replacing a longer string with
                    // a shorter one, e.g.: "hello there" -> "hi."
                    int endOfChange = indexOfChange + Math.abs(netLength);

                        /*
                            "->" means add (positive) netLength to position
                            "<-" means add (negative) netLength to position
                            "x" means don't update position

                            "start / end" means what should be done in each case for each anchor if they differ

                            "+a" means one of the anchors was included in the deleted portion of content
                            "-a" means one of the anchors was not included in the deleted portion of content
                            Before/At/After means indexOfChange "<" / "==" / ">" position

                                   |   Before +a   | Before -a |   At   | After
                            -------+---------------+-----------+--------+------
                            Add    |      N/A      |    ->     | -> / x | x
                            Delete | indexOfChange |    <-     |    x   | x
                         */
                    // Note: if both are moved to indexOfChange, selection is empty.
                    if (indexOfChange == finalStart && netLength > 0) {
                        finalStart = finalStart + netLength;
                    } else if (indexOfChange <= finalStart) {
                        finalStart = finalStart < endOfChange
                                ? indexOfChange
                                : finalStart + netLength;
                    }
                    if (indexOfChange == finalEnd && netLength > 0) {
                        finalEnd = finalEnd + netLength;
                    } else if (indexOfChange <= finalEnd) {
                        finalEnd = finalEnd < endOfChange
                                ? indexOfChange
                                : finalEnd + netLength;
                    }

                    // force-update internalSelection in case empty selection is
                    // at the end of area and a character was deleted
                    // (prevents a StringIndexOutOfBoundsException because
                    // end is one char farther than area's length).
                    if (area.getLength() < getEndPosition()) {
                        finalStart = area.getLength();
                        finalEnd = area.getLength();
                    }
                }
            }
            selectRange(finalStart, finalEnd);
        });

Here's the original org.fxmisc.richtext.SelectionImpl code using the wrong getLength() and (possibly) the wrong finalEnd and finalStart checks.

        manageSubscription(area.multiPlainChanges(), list -> {
            int finalStart = getStartPosition();
            int finalEnd = getEndPosition();
            for (PlainTextChange plainTextChange : list) {
                int netLength = plainTextChange.getNetLength();
                //if (netLength != 0)  Causes IndexOutOfBoundsException in ParagraphText.getRangeShapeSafely issue #689
                // but can be safely reimplemented if this causes other issues.
                {
                    int indexOfChange = plainTextChange.getPosition();
                    // in case of a replacement: "hello there" -> "hi."
                    int endOfChange = indexOfChange + Math.abs(netLength);

                    if (getLength() != 0) {

                        /*
                            "->" means add (positive) netLength to position
                            "<-" means add (negative) netLength to position
                            "x" means don't update position

                            "start / end" means what should be done in each case for each anchor if they differ

                            "+a" means one of the anchors was included in the deleted portion of content
                            "-a" means one of the anchors was not included in the deleted portion of content
                            Before/At/After means indexOfChange "<" / "==" / ">" position

                                   |   Before +a   | Before -a |   At   | After
                            -------+---------------+-----------+--------+------
                            Add    |      N/A      |    ->     | -> / x | x
                            Delete | indexOfChange |    <-     |    x   | x
                         */
                        if (indexOfChange == finalStart && netLength > 0) {
                            finalStart = finalStart + netLength;
                        } else if (indexOfChange < finalStart) {
                            finalStart = finalStart < endOfChange
                                    ? indexOfChange
                                    : finalStart + netLength;
                        }
                        if (indexOfChange < finalEnd) {
                            finalEnd = finalEnd < endOfChange
                                    ? indexOfChange
                                    : finalEnd + netLength;
                        }
                    } else {
                        // force-update internalSelection in case empty selection is 
                        // at the end of area and a character was deleted
                        // (prevents a StringIndexOutOfBoundsException because
                        // end is one char farther than area's length).

                        if (getLength() < getEndPosition()) {
                            finalStart = getLength();
                            finalEnd = getLength();
                        }
                    }
                }
            }
            selectRange(finalStart, finalEnd);
        });

[Jugen]

Thanks @sconrad94

There is a pull request #775 that also makes some changes to this same method which will "conflict" with the changes you suggest above. I've had a look at what you propose and suggest the following to resolve the conflict and still fix the bug you are addressing. Could you submit a pull request where you just change:

                    if (getLength() < getEndPosition()) {
                        finalStart = getLength();
                        finalEnd = getLength();
                    }

(at the bottom of the method) to:

                    if (getEndPosition() > 0) {
                        finalStart = area.getLength();
                        finalEnd = finalStart;
                    }

and that should do the trick.

[Jugen]

@sconrad94 I've become a maintainer of the project now, so I would really appreciate it if you could please submit a pull request for this. Thank you.