Bug: Incorrect Caret Position After Undo in `CodeArea`

[Symeon94]

I've encountered an unexpected bug in the undo behavior of the CodeArea. It seems that when pressing Enter after selecting text, and then performing an undo (CTRL+Z), the caret is not restored to its original position. Instead, it ends up one character before the end of the original selection.

Steps to Reproduce

1. In a CodeArea, type any text — for example: Something,
2. Select a portion of the text — e.g., select "eth" in Som[eth]ing,
3. Press Enter — the result will be Som\ning with the caret positioned at the start of the second line,
4. Press CTRL+Z to undo — the text reverts to Something, but the caret is now placed after the letter 't' instead of its original position and there is no selection.

Expected Behavior

The undo operation should restore both:

• The original selection range
• The caret position exactly as it was before pressing Enter

Additional Notes

• Selection direction matters: selecting from 'e' to 'h' vs. 'h' to 'e' should not affect the final caret position after undo.
• The caret should consistently return to its pre-Enter location regardless of selection direction.

Update: I suspect the issue is in the UndoManager, so it might be a bug to be reported there instead, I will look for the root cause.

[Symeon94]

From what I have seen when I undo, the UndoManager has the position, the removed and the added text. But it does not contain the previous caret position (in PlainTextChange).

It seems to me though that there are two issues here:

1. The caret is not at the right position after undo (which can be fixed independantly of knowing the previous caret position)
2. Restoring the caret at the exact position it was before

[Symeon94]

After some research, it seems like the caret position is moved after the redo in RedoUtils method applyMultiRichTextChange by calling moveToChange.

In the example I gave the change is (start: 3, end:4, inserted: "eth", removed: "\n"), so logically speaking it should do:

int pos = chg.getPosition(); // 3
int len = chg.getNetLength(); // 2 (see below)
if ( len > 0 ) pos += len; // pos = 3 + 2 = 5

area.moveTo( Math.min( pos, area.getLength() ) ); // Move to min(5, ?)

The thing is, I debugged the code and chg.getNetLength() is equal to 2. Because it is removing \n and adding eth. That is how the cursor is moved to the fifth character in "Something".

public int getNetLength() {
    return this.insertedLength() - this.removedLength();
}

This is where the bug comes from.

[Symeon94]

I will add some Unit tests for all cases, for me there are 4:

1. Add some text
2. Delete some text with "delete" key
3. Delete some text with "backspace" key
4. Replace some text

Type	From	To	Position	Removed	Added	Net length	Caret after undo
Add	"Hat"	"Heat"	1	-	e	-1	1 (ok)
Delete	"Heat"	"Hat"	1	e	-	1	2 (not ok)
Backspace	"Heat"	"Hat"	1	e	-	1	2 (ok)
Replace	"Slate"	"Site"	1	la	i	1	2 (not ok)

Note that for the addition it does not add the length due to the if ( len > 0 )

[Symeon94]

I'm actually wrong when stating we can divide the problem. I could easily fix the replace issue but the delete needs the information about the previous position of the caret. Delete with "backspace" or "delete" for the same letter produce different results, and there is no way to know where the caret must be after the undo without having this information saved (see my example above, we remove the same letter and the position/removed/added is exactly the same).

I'll see if I can easily add the caret position in the undo record, if not we can already push the easy fix for the replace.

[Jugen]

Here is some relevant code in CaretNode, just in case this is helpful and you haven't seen it: https://github.com/FXMisc/RichTextFX/blob/40fc429ab82b24ef53191910a386ce94b99e6401/richtextfx/src/main/java/org/fxmisc/richtext/CaretNode.java#L161-L194

[Symeon94]

Thanks for that tip, I'll have a look. In the meantime I have extended the UT to include more cases. There is one more wrong thing I have seen. If you do a selection, normally you would expect that after the undo action you would get back that selection (meaning that if you select "Som[eth]ing" you want the selection back after the undo).

So that would be a third problem.

For the UT, I made sure to prepare them to pass against the old code before doing a change, so we are sure of the previous behaviour.

[Symeon94]

@Jugen I looked at that code which seems to define how the caret is repositioned after the change. But that is not relevant in that case, as the caret positioning is not in question. The problem is that some information are not recorded in the undo records, making it impossible to properly undo.

[Jugen]

I'm confused, I thought that's the whole problem is that the caret is positioned incorrectly after an undo. So I thought the code in CaretNode may be helpful in the calculation as it's different from in UndoUitls. For instance, it seems to take the current caret position into account. BTW there's similar code for selection in SelectionImpl.

[Symeon94]

You are correct about the issue. The issue is that we are missing information, I can fix the undo issue for the replace by doing:

private static void moveToChange( GenericStyledArea area, TextChange chg )
{
    int pos = chg.getInsertionEnd();
    area.moveTo( Math.min( pos, area.getLength() ) );
}

But that does not fix the problem of caret position for pressing the "delete" key.

The code in CaretNode is reevaluating what it calls finalPosition which I assume to be the last position of the text. For this it has all the needed information. Let me illustrate: for word "Something" if caret is at 4 ("Some|thing"), and:

• You press "delete", it removes "t" (len=1) at position 4,
• You press "backspace", it removes "e" (len = 1) at position 3.

It doesn't need to know where the caret was in that case, it knows the position and the length of the removed content.

When doing "undo", the information "e" at position 3 could either be:

• A "delete" when the caret was at position 3
• A "backspace" when the caret was at position 4

I hope I'm clear enough. Let me know if you think I'm missing something there.

My plan is to try to add the selection information in the TextChange and see if that can be done with minimal changes.

[Symeon94]

I'm working on UT the PlainTextChange before adding data in TextChange to make sure nothing will break. Especially that merge method which is rather complex and which I'm trying to fully cover. But by covering it, I noticed some cases are not covered by that method, and I wanted your opinion @Jugen about extending that code to cover more cases. For instance, imagine you do the following replace:

• From ABCDEF you replace CD by GH getting ABGHEF
• Then you replace BGHE by IJ getting AIJF Currently the code is not able to merge this case, but we could merge it as a simple replace of BCDE by IJ.

Why I'm asking that? Because the simpler to code (and more general) the easier it will be to extend it without breaking it.

For now, I'll stick to keep the code in its current behaviour.

[Symeon94]

@Jugen I'm adding some UT to some parts of the code and I found something that might be incorrect.

Now, my experience when working with legacy code like this one is: first cover the code you intend to change with tests defining the current behaviour and then only change. I was looking at ReadOnlyStyledDocument.Pos which has a method clamp. Current behaviour is not in line with the interface specification of that method, if you clamp a when pointing to a character outside the paragraph, it will not clamp to that paragraph but will point to next one.

Now, this is far from essential to my changes, so I'll put UT and a comment stating it's wrong, but this might actually be a hidden bug. (now fixing a hidden bug can sometimes break other things, that's also why I probably won't touch that part).

[Symeon94]

It appears that placing the caret within TextChange or its subclass RichTextChange isn't an option, since these are part of the document model and inherently unaware of the caret's position.

A more appropriate solution would be to encapsulate the caret in a wrapper object used by applyMultiRichTextChange. However, this would require modifying the signature of multiRichChanges to use that wrapper class instead of RichTextChange, which could have widespread impact on whoever depends on the signature of this.

Unfortunately, multiRichChanges() currently returns a concrete implementation (RichTextChange), which is an internal class of the document. This design choice makes it harder to introduce changes, as it exposes more than what most consumers actually need.

I'll explore less disruptive alternatives, but for now, we may need to live with the bug.

[Symeon94]

@Jugen I pushed a PR.

If we could get that change in a build that would be most appreciated.

Note that after all these changes, the one that I really need is UndoUtils and its associated UT/IT. I can maybe do two PR, because the rest is some cleanup that, in the end, was not necessary.

The problems highlighted before are not fixed, but at least the UndoUtils change makes it acceptable now.

Edit: I created a separate PR, once approved I'll propose the refactor in a different one.

[Symeon94]

@Jugen would it be possible to get a build with that simple fix? I would like to integrate in my release.

Also, is there a way to get the right to perform merges and builds?

[Jugen]

Thanks @Symeon94, will publish a release tomorrow (20th Nov). I've noticed your participation and contributions recently and am glad you asked for merge and build rights - will look into it.

[Symeon94]

@Jugen thanks.

I hope I didn't overwhelm you with the details in this ticket. I noticed you closed it, but there are still a few issues that are pending with this ticket (the change is only limited to what was easily feasible with minimal changes). Do you want me to open separate ticket for each of these?

For the build rights, I'm working on a project that uses this library, so I'm just a bit worried of customer facing issues that might appear and for which I might need to do a quick fix (I can obviously always do a temporary local build on my system too).

[Jugen]

The close was unintentional, I didn't realise that closing tags are recognised in the title as well. :-)

Also, I would like for you to have access rights as I'm currently the only maintainer - I just need to figure out how to go about it.