Richtextfx: Unexpected change in UndoManager

[garybentley]

I'm getting the following exception from the UndoManager:

java.lang.IllegalArgumentException: Unexpected change received.
Expected:
[RichTextChange{
        position: 980
        removed: Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@d2a730f style=14,Georgia,0x000000ff)]
        inserted: Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
}]
Received:
[RichTextChange{
        position: 980
        removed: Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@53d2627e style=14,Georgia,0x000000ff), StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@2295669d style=14,Georgia,0x000000ff)]
        inserted: Par[[]; StyledSegment(segment=com.quollwriter.ui.fx.components.TextEditor$TextSegment@67d7424 style=14,Georgia,0x000000ff)]
}]
        at org.fxmisc.undo.impl.UndoManagerImpl.changeObserved(UndoManagerImpl.java:234)

This is because it is trying to undo a change to text that has a style that has not been recorded.

The style in this case is a "spelling error" indicator for a word.

I'm adding the style to the word using:

this.suspendUndos.suspendWhile (() -> {
   this.getContent ().setStyleSpans (r.getStart (), this.getContent ().getStyleSpans (r.getStart (), r.getEnd ()).mapStyles (ss -> {
   TextStyle _s = new TextStyle (ss);
    _s.setSpellingError (true);
    return _s;
 }));
  });

The suspendWhile is blocking the change from being recorded.

My undo manager is created using:

UndoManagerFactory.unlimitedHistoryFactory ().createMultiChangeUM(this.multiRichChanges ().conditionOn (this.suspendUndos), TextChange::invert, UndoUtils.applyMultiRichTextChange(this), TextChange::mergeWith, TextChange::isIdentity)

And my segment has the following equals method (which is returning true when it's trying to do an undo):

@Override public boolean equals( Object obj )
{
    if ( obj == this )  return true;
    else if ( obj instanceof AbstractSegment && getClass().equals( obj.getClass() ) )
    {
        boolean v = getText().equals( ((AbstractSegment) obj).getText() );
        return v;
    }
    return false;
}

So the question is how can I add the spelling error style (which I don't want to record in the undo list) but still have the undo work?

[Jugen]

Sorry Gary I've had a look and I don't have any suggestions. (I confess I don't know how the Undo/Redo code works.) In my own code I just used a plain text undo manager to get around it, but then I don't have any other styling needs except for spelling errors. Unfortunately this won't work for you though.

[Jugen]

Hi Gary, after fixing 904 I'm willing to have another crack at it.
Could you please describe the precise behavior you are looking for and also provide step by step test cases (include both undo and redo scenarios).

[cengels]

Hey! I may not be Gary, but as I've recently stumbled upon the same issue (and was able to reproduce it in master using the RichTextDemo) I thought I would chime in.

I believe Gary's solution is derived from #735, which is also where I derived mine from. However, this solution seems to no longer work. Basically, the idea is to pass a suspendable event stream to a custom UndoManager that allows you to temporarily suspend the creation of new undo/redo states while the text area updates automatically computed styles. These styles (in my case) are based on syntax, are not based on user input, and should therefore not be individually undone when you hit undo.

A plain text undo manager is, of course, the ideal solution -- however, that is unfortunately not possible (at least in my case) as I am working on a limited rich-text editor that both allows users to set simple formatting (like bold or italics) while also automatically computing some styles based on token matching (for instance, any text enclosed in square brackets are considered comments and grayed out). The UndoManager should not create new undo states each time this syntax highlighting is updated, only when the user themselves updates the style.

I was able to reproduce this in the RichTextDemo.java by appending the following line to the initialization block of area (starting at line 84):

area.setUndoManager(UndoManagerFactory.unlimitedHistoryFactory().createMultiChangeUM(area.multiRichChanges().conditionOn(allowChange), TextChange::invert, UndoUtils.applyMultiRichTextChange(area), TextChange::mergeWith, TextChange::isIdentity));

allowChange is a field defined as such:

private SuspendableYes allowChange = new SuspendableYes();

Then, all that remains is to use allowChange.suspendWhile() in one of the numerous button handlers. For instance, I used:

private void updateTextColor(Color color) {
    if(!updatingToolbar.get()) {
        allowChange.suspendWhile(() -> updateStyleInSelection(TextStyle.textColor(color)));
    }
}

If you then start the demo, insert text, change its color, and attempt to undo it, you will be hit with an error somewhat like the one at the end of my comment. This should not happen. After this, any change to the text area (including a simple change that adds or removes a character) causes more errors. Because of that, I can't really test the redo, but I imagine it produces a similar error.

What should actually happen is that the UndoManager should essentially "ignore" the style change and just revert to its last valid undo state. For instance:

1. Enter text this is a
2. Wait a few seconds to trigger another undo state and type test
3. Change the font color of test (assuming this is the undo-suspended operation)
4. Undo
5. Now the text area should display this is a again.
6. On redo, the text area should simply return to this is a test (unstyled). Our event stream listeners would then pick up the undo change and recompute the "syntax highlighting", but again: the UndoManager shouldn't know anything of this.

If this is too difficult to implement/fix (although I imagine it worked at some point if it was the recommended course of action 168 revisions ago), might it instead be possible to integrate this kind of functionality directly into the UndoManager? I noticed that the UndoManager already automatically merges changes if they happen in short succession of one another; if my interpretation of this is correct, perhaps this kind of functionality could be publicly exposed in some way? Something like a method mergeNext() to stand as a counter-point to the method preventMerge()? Just a thought.

If there's any other information or assistance you need, please let me know. I really appreciate you wanting to look into this.

Exception in thread "JavaFX Application Thread" java.lang.IllegalArgumentException: Unexpected change received.
Expected:
[RichTextChange{
    position: 0
    removed: Par[; StyledSegment(segment=left(this is a test) style=12,Serif,0x000000ff)]
    inserted: Par[; StyledSegment(segment=left() style=12,Serif,0x000000ff)]
}]
Received:
[RichTextChange{
    position: 0
    removed: Par[; StyledSegment(segment=left(this is a ) style=12,Serif,0x000000ff), StyledSegment(segment=left(test) style=12,Serif,0x990000ff)]
    inserted: Par[; StyledSegment(segment=left() style=12,Serif,0x000000ff)]
}]
    at org.fxmisc.undo.impl.UndoManagerImpl.changeObserved(UndoManagerImpl.java:234)
    at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
    at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
    at org.reactfx.util.QueuingStreamNotifications.lambda$head$0(NotificationAccumulator.java:217)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.SuspendableBase.resume(SuspendableBase.java:64)
    at org.reactfx.CloseableOnceGuard.close(Guard.java:49)
    at org.reactfx.MultiGuard.close(Guard.java:83)
    at org.reactfx.Suspendable$1.resumeSource(Suspendable.java:118)
    at org.reactfx.Suspendable$1.suspendSource(Suspendable.java:104)
    at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
    at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
    at org.reactfx.EventStreams$3.lambda$observeInputs$0(EventStreams.java:105)
    at org.reactfx.value.ChangeListenerWrapper.accept(Val.java:786)
    at org.reactfx.util.AbstractReducingStreamNotifications.lambda$head$0(NotificationAccumulator.java:248)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
    at org.reactfx.value.ValBase.invalidate(ValBase.java:32)
    at org.reactfx.SuspendableBoolean.release(SuspendableBoolean.java:24)
    at org.reactfx.CloseableOnceGuard.close(Guard.java:49)
    at org.reactfx.Suspendable.suspendWhile(Suspendable.java:49)
    at org.fxmisc.richtext.model.GenericEditableStyledDocumentBase.updateMulti(GenericEditableStyledDocumentBase.java:218)
    at org.reactfx.util.Tuple3.exec(Tuple3.java:43)
    at org.fxmisc.richtext.model.GenericEditableStyledDocumentBase.replaceMulti(GenericEditableStyledDocumentBase.java:140)
    at org.fxmisc.richtext.model.GenericEditableStyledDocument.replaceMulti(GenericEditableStyledDocument.java:13)
    at org.fxmisc.richtext.GenericStyledArea.replaceMulti(GenericStyledArea.java:1398)
    at org.fxmisc.richtext.MultiChangeBuilder.commit(MultiChangeBuilder.java:364)
    at org.fxmisc.richtext.util.UndoUtils.lambda$applyMultiRichTextChange$3(UndoUtils.java:220)
    at org.fxmisc.undo.impl.UndoManagerImpl.lambda$applyChange$4(UndoManagerImpl.java:213)
    at org.reactfx.Suspendable.suspendWhile(Suspendable.java:49)
    at org.fxmisc.undo.impl.UndoManagerImpl.applyChange(UndoManagerImpl.java:213)
    at org.fxmisc.undo.impl.UndoManagerImpl.undo(UndoManagerImpl.java:126)
    at org.fxmisc.richtext.UndoActions.undo(UndoActions.java:22)
    at org.fxmisc.richtext.GenericStyledAreaBehavior.lambda$static$4(GenericStyledAreaBehavior.java:88)
    at org.fxmisc.wellbehaved.event.template.InputMapTemplate.lambda$consume$0(InputMapTemplate.java:221)
    at org.fxmisc.wellbehaved.event.template.PatternActionTemplate.lambda$null$1(InputMapTemplate.java:425)
    at java.util.Optional.map(Optional.java:215)
    at org.fxmisc.wellbehaved.event.template.PatternActionTemplate.lambda$getInputHandlerTemplateMap$2(InputMapTemplate.java:425)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:25)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:24)
    at org.fxmisc.wellbehaved.event.template.InputMapTemplate$2.lambda$null$0(InputMapTemplate.java:341)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:24)
    at org.fxmisc.wellbehaved.event.template.InputMapTemplate$1.lambda$null$0(InputMapTemplate.java:202)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:24)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:24)
    at org.fxmisc.wellbehaved.event.template.InputMapTemplate$1.lambda$null$0(InputMapTemplate.java:202)
    at org.fxmisc.wellbehaved.event.template.InputHandlerTemplateMap.lambda$sequence$0(InputHandlerTemplateMap.java:24)
    at org.fxmisc.wellbehaved.event.template.InputMapTemplate$HandlerTemplateConsumer$1.lambda$accept$0(InputMapTemplate.java:103)
    at org.fxmisc.wellbehaved.event.InputHandler.handle(InputHandler.java:50)
    at com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:218)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.Scene$KeyHandler.process(Scene.java:3964)
    at javafx.scene.Scene$KeyHandler.access$1800(Scene.java:3910)
    at javafx.scene.Scene.impl_processKeyEvent(Scene.java:2040)
    at javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2501)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:217)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:149)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$1(GlassViewEventHandler.java:248)
    at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:410)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:247)
    at com.sun.glass.ui.View.handleKeyEvent(View.java:546)
    at com.sun.glass.ui.View.notifyKey(View.java:966)
    at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
    at com.sun.glass.ui.win.WinApplication.lambda$null$4(WinApplication.java:186)
    at java.lang.Thread.run(Thread.java:748)

[Jugen]

What I have found so far is that the current UndoManager implementation does a check after an undo/redo to see if the change it submitted was applied correctly. It does this by comparing the change event emitted afterwards by the text control with what it submitted. If they don't match then the reported exception gets thrown.

In the case of RichTextFX the change event emitted contains a sub-document made up of text as well as styles of the altered area of text. So when the undo manager is suspended and a style change occurs, it isn't registered, with the result being that after the next undo the style is still present in the document and therefore won't match subsequent undo change comparisons for that portion of text.

Unfortunately the current implementation can't be extended and the offending method overridden. So the only way around it at this stage is to create a custom UndoManager implementation that doesn't do the post change comparison check.

I've done a basic test and it seems to work, so could you both please thoroughly test this and give me feedback. (nocheckUndo.zip only contains the following folder org/fxmisc/richtext/util with three files)

Extract into the root of your project and use with:
area.setUndoManager( UndoUtils.richTextUncheckedUndoManager( area, allowChange/suspendUndo ) );

[cengels]

Hey Jugen; I've tested all of my use cases with your files and am happy to report that they all work wonderfully (though, granted, I only really have two use cases for these suspendable changes at the moment). I have also tested "normal" undo/redo operations involving both plain text (removing characters or words or adding them) and rich text (formatting) changes and can report that those also still work as expected.

I will continue to monitor the undos and redos during regular work on the project and will edit this comment or drop another comment if I notice anything strange.

Of course, this does make one wonder—if those undo/redo checks could be safely removed with no adverse effects, then why were they there to begin with? Just for added safety? It seems like it caused more issues than it potentially solved, but it can't be that simple. I feel like we're missing something. Do you have any idea what problems those undo/redo checks could have potentially solved?

Thank you very much for your work.

[Jugen]

Hey cengels, that's great. I think the checks are there to help developers adding UndoFx to their projects as well as to insure the general consistency of the document so that subsequent undo/redo calls produce predictable results.

For instance with the solution provided above if you were to change the actual text while suspending the undo manager then any subsequent undo will probably wreck your text. Without feedback from the undo manager a developer wouldn't have any insight as to why this was happening.

Here is my second attempt which uses plain text comparison and doesn't compare styling.
Could you try it and give me feedback please: suspendableUndo.zip

As before extract into the root of your project and use with:
area.setUndoManager( UndoUtils.richTextSuspendableUndoManager( area, allowChange/suspendUndo ) );

(_Note the different method name_)

[cengels]

Hey again. I tested the new richTextSuspendableUndoManager and found no issues or changes in behaviour compared to the unchecked one. From what I've been able to determine, it works just as well (but is now presumably safer).

[Jugen]

Thanks for the feedback @cengels, I hope to hear from @garybentley as well .... ?
I'll tentatively create a PR for this then in the next couple of days, while awaiting any additional feedback from either of you.

[garybentley]

Hi Jugen,
Don't worry I haven't forgotten, I have just started a university degree and have been trying to get some free time to check this out. I've been following the discussion, just haven't had chance to implement your fixes yet, I'm hoping to set aside some free time in the next couple of days to look into it.

[Jugen]

Wow that's great Gary, for starting a degree. And all the best in getting it done.

[garybentley]

I finally got some time to test your fix Jugen. It looks like it's working, at least I'm not seeing the exception anymore. Thanks for fixing it :)