Move UndoManager into EditableStyledDocument so that one UndoManager …

[JordanMartinez]

…is shared across multiple clones. Clones can be disposed without affecting access to the UndoManager.

See comments in #233

[JordanMartinez]

Hmm.... looks like we have another instance of IllegalArgumentException appearing from #216 through this approach...

[JordanMartinez]

Stracktrace:

11:25:24.979 [DEBUG] [TestEventLogger] org.fxmisc.richtext.StyledTextAreaModelTest > insureCloneUndoRedoWorks FAILED
11:25:24.980 [DEBUG] [TestEventLogger]     java.lang.IllegalArgumentException: Unexpected change received.
11:25:24.980 [DEBUG] [TestEventLogger]     Expected:
11:25:24.981 [DEBUG] [TestEventLogger]     RichTextChange{
11:25:24.981 [DEBUG] [TestEventLogger]          position: 0
11:25:24.981 [DEBUG] [TestEventLogger]          removed: Par["":]
11:25:24.981 [DEBUG] [TestEventLogger]          inserted: Par["A really short text example.":-fx-font-size: 30px;]
11:25:24.981 [DEBUG] [TestEventLogger]     }
11:25:24.981 [DEBUG] [TestEventLogger]     Received:
11:25:24.982 [DEBUG] [TestEventLogger]     RichTextChange{
11:25:24.982 [DEBUG] [TestEventLogger]          position: 0
11:25:24.982 [DEBUG] [TestEventLogger]          removed: Par["":-fx-font-size: 30px;]
11:25:24.982 [DEBUG] [TestEventLogger]          inserted: Par["A really short text example.":-fx-font-size: 30px;]
11:25:24.982 [DEBUG] [org.gradle.process.internal.DefaultExecHandle] Changing state to: SUCCEEDED
11:25:24.982 [DEBUG] [TestEventLogger]     }
11:25:24.983 [DEBUG] [TestEventLogger]         at org.fxmisc.undo.impl.UndoManagerImpl.changeObserved(UndoManagerImpl.java:185)
11:25:24.983 [DEBUG] [TestEventLogger]         at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$278(NotificationAccumulator.java:134)
11:25:24.983 [DEBUG] [TestEventLogger]         at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
11:25:24.983 [DEBUG] [org.gradle.process.internal.DefaultExecHandle] Process 'Gradle Test Executor 1' finished with exit value 0 (state: SUCCEEDED)
11:25:24.983 [DEBUG] [TestEventLogger]         at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.MappedStream.lambda$observeInputs$207(MappedStream.java:25)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$278(NotificationAccumulator.java:134)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
11:25:24.984 [DEBUG] [TestEventLogger]         at org.reactfx.FilterStream.lambda$observeInputs$194(FilterStream.java:20)

[JordanMartinez]

I guess this is another instance of the bug you mentioned in #247: a re-done text (via insertion) will inherit the deleted text's style.

[JordanMartinez]

I did some debugging and now see that subsequence is what's preventing the above test from working:

org.fxmisc.richtext.StyledTextAreaModelTest > insureCloneUndoRedoWorks STANDARD_OUT
    // insert styled text into area
    Emtting Doc from RichChanges: RichTextChange{
        position: 0
        removed: Par["":]
        inserted: Par["A really short text example.":-fx-font-size: 30px;]
    }

    Calling area::undo
    Applying change: RichTextChange{
        position: 0
        removed: Par["A really short text example.":-fx-font-size: 30px;]
        inserted: Par["":]
    }
    Emtting Doc from RichChanges: RichTextChange{
        position: 0
        removed: Par["A really short text example.":-fx-font-size: 30px;]
        inserted: Par["":]
    }

    Calling area::redo
    Applying change: RichTextChange{
        position: 0
        removed: Par["":]
        inserted: Par["A really short text example.":-fx-font-size: 30px;]
    }
    Emtting Doc from RichChanges: RichTextChange{
        position: 0

        // `StyledDocumentBase#subsequence()` picks up the deleted text's style
        removed: Par["":-fx-font-size: 30px;]

        inserted: Par["A really short text example.":-fx-font-size: 30px;]
    }

Gradle Test Executor 1 finished executing tests.

org.fxmisc.richtext.StyledTextAreaModelTest > insureCloneUndoRedoWorks FAILED
    java.lang.IllegalArgumentException: Unexpected change received.
    Expected:
    RichTextChange{
        position: 0
        removed: Par["":]
        inserted: Par["A really short text example.":-fx-font-size: 30px;]
    }
    Received:
    RichTextChange{
        position: 0
        removed: Par["":-fx-font-size: 30px;]
        inserted: Par["A really short text example.":-fx-font-size: 30px;]
    }

One way around this would be to have a different replace() method for undo/redo-related things. In this way, the undos/redos would still change the Paragraphs, but could push their corresponding change directly to the another EventStream specifically for this.

[JordanMartinez]

In light of Tomas' recent comment in the related issue:

Putting UndoManager inside EditableStyledDocument has its own problems. For example, we will then never be able to include selection in the undo/redo, if we ever decide to implement it (and some editors do that).

Another desired feature would be that in one view, you would undo/redo only the edits made in that view. And the other view would undo/redo its own edits. So at least the UndoManager would need to distinguish the origin of the change (this applies whether there is a shared UndoManager or two separate ones). With this feature comes another complication. It is no longer sufficient to store edits as a linear sequence (possibly tagged by origin), as is done by UndoFX, but rather as a directed acyclic graph, in order to not impose some arbitrary ordering on edits that are independent (and thus can be undone independently of each other).

And then someone will come and want to implement a collaborative editor on top of RichTextFX. That necessarily means there will be asynchrony involved, which opens up the possibility of conflicts, so they will need a mechanism to resolve conflicts.

I don't think we can or should resolve all these problems in RichTextFX. I think the best we can do is put the user in charge, by pushing the undo/redo functionality out of StyledTextArea, with convenient factory methods that preserve the current Ctrl+Z behavior.

I'm closing this PR as it doesn't address this issue as well as another approach could.