Add support for nonlinear undo/redo (clones don't currently handle undo/redo correctly)

[JordanMartinez]

#232 (solution to #152) has been implemented. However, as noted by Tomas:

If the document is edited from both areas, undo will not work very well. One area's undo will be regarded as a new edit in the other area. It is a complex problem on its own, so for now I would just say that such a use is not supported.

[JordanMartinez]

I think the only way to account for this is to use one UndoManager that is shared between multiple StyledTextAreaModels. However, the tricky part lies in making sure all the other Models' getUndoManager() returns the same undoManager after one of them sets it to something else.

So, it seems like the best way to get around this is to wrap the UndoManager in wrapper class. See my betterCloningUndoing branch for an example of this.

However, there is an additional problem to solve. As of now, when the undo manager is created, it is passed a lambda that calls the replace() method of the StyledTextAreaModel that initially constructed it when a change is re-done.

So, if there are three StyledTextAreaModels, two of which are clones of the original....

StyledTextAreaModel initial;
StyledTextAreaModel clone1;
StyledTextAreaModel clone2;

....when clone1 or clone2 call getUndoManager().undo(), it is initial whose replace() method is called, not either of the clones.

So, what happens when initial gets disposed? When redo() is called, the disposed and thus garbage-collected initial won't exist and the attempt to call its replace() method will result in a NullPointerException.

[JordanMartinez]

The only way around this second part is if the apply part of createRich/PlainTextChange(UndoManagerFactory factory) doesn't rely upon StyledTextAreaModel's code in any way. In this way, it would need to actively call EditableStyledDocument's code and any caret/selection values would need to be updated via the code that updates these values when EditableStyledDocument's plainTextChange emits a new change.

[TomasMikula]

The same effect as by sharing an UndoManager can also be achieved by only one of the areas having a real UndoManager and the others having a fake one:

area.setUndoManager(UndoManagerFactory.zeroHistoryFactory())

Then no undo/redo will be available on the other areas, but the keyboard shortcuts can be overridden to trigger undo/redo on the area with the true UndoManager.

[JordanMartinez]

What happens when the initial area is disposed though? And doesn't that limit the developer if they want to be able to undo something using either of the areas?

[TomasMikula]

What happens when the initial area is disposed though?

Yeah, that would be a problem.

And doesn't that limit the developer if they want to be able to undo something using either of the areas?

The developer would have to know to call undo() on the correct area.

When first implementing the undo functionality, I almost wanted the UndoManager to live outside the StyledTextArea. I think in the end the main reason why StyledTextArea holds a reference to UndoManager was to have the expected behavior for Ctrl+Z etc. out of the box. Maybe it's time to rethink this.

[JordanMartinez]

What if it was placed within EditableStyledDocument? Even the code I've implemented in my branch could be refactored to place it there.

[TomasMikula]

Sorry for not being responsive. The reason is that I don't know a good solution off the top of my head, and I didn't have time to think deeply about the problem.

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.

[JordanMartinez]

Sorry for not being responsive. The reason is that I don't know a good solution off the top of my head, and I didn't have time to think deeply about the problem.

No worries! :-) You're busy and I didn't realize just how complicated the implications could become

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.

So, you're suggesting something like a "plug-in" sort of idea...? We can provide the current version of UndoFX as a basic "undo manager plug-in" but allow others to develop their own outside of RichTextFX. Thus, the way the undo/redo is handled is determined by the developer.

Edit: Perhaps "plug-in" isn't the best term to use for the idea your suggesting. However, I get the basic idea of what you're proposing.

[JordanMartinez]

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).

So, it should remain inside StyledTextAreaModel as it does now.

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).

Doesn't this necessarily mean that UndoActions#undo and UndoActions#redo should include the StyledTextAreaModel that called the undo as an argument? Then, if one does want to use a shared UndoManager for handling a directed acyclic graphic undos/redos, they already have the foundation upon which to build their custom handling.

public interface UndoActions {
    public void undo(StyledTextAreaModel model) { getUndoManager().undo(model); }
}

However, as soon as we go that direction, now we run across the requirement of using generics: if one subclasses StyledTextAreaModel and provides some other functionality that is relevant to that model, a generic would be necessary to return the correct object without needing to cast it to its actual type:

public class ModelSubclass extend StyledTextAreaModel {
    // class content...
}

public interface UndoActions<PS, S, M extends StyledTextAreaModel<PS, S>> {
    public void undo(M model) { getUndoManager().undo(model); }
}

Adding another thought: Additionally, if we go that route, now UndoFX doesn't support that. It's #undo doesn't take any arguments. We could write a subclass of UndoManagerImpl within RichTextFX and add that functionality, but then we have an #undo method that might be mistakenly called.

[TomasMikula]

So, it should remain inside StyledTextAreaModel as it does now.

Or maybe further out, to StyledTextArea.

Doesn't this necessarily mean that UndoActions#undo and UndoActions#redo should include the StyledTextAreaModel that called the undo as an argument?

That would not be necessary, or desired. (Why would a user calling StyledTextArea#undo have to pass in something?)

if one subclasses StyledTextAreaModel

I'm not yet really on board with allowing subclassing of StyledTextAreaModel. I somewhat lost track here, why is it even public?

[JordanMartinez]

Or maybe further out, to StyledTextArea.

Isn't that breaking separation of concerns? UndoManager seems to operate on the model, not necessarily the view. Why stick it inside the view?

I'm not yet really on board with allowing subclassing of StyledTextAreaModel. I somewhat lost track here, why is it even public?

I was thinking in terms of separation of concerns. Why store model-content of a subclass of StyledTextArea within that subclass? Shouldn't it be stored in a subclass of StyledTextAreaModel?

That would not be necessary, or desired. (Why would a user calling StyledTextArea#undo have to pass in something?)

I was thinking of an approach where the clones share one UndoManager. The public API for StyledTextArea would still show undo() without any arguments. However, the implementation of it within the StyledTextAreaModel would need to call UndoManager.undo(source) where source is the model. I saw that as necessary so that if there were multiple views of the same document, the shared UndoManager would know which area's undo history to use to undo (or redo) some action.

[TomasMikula]

UndoManager seems to operate on the model, not necessarily the view. Why stick it inside the view?

While this is correct, in trying to give the user complete control over how to instantiate the undo manager, for StyledTextArea the undo manager would be nothing more than a handler of the Ctrl+Z/Shift+Ctrl+Z events.

I was thinking in terms of separation of concerns. Why store model-content of a subclass of StyledTextArea within that subclass? Shouldn't it be stored in a subclass of StyledTextAreaModel?

I didn't imagine there would be different model for different subclasses of StyledTextArea. I don't think subclassing StyledTextArea should be encouraged.

I saw that as necessary so that if there were multiple views of the same document, the shared UndoManager would know which area's undo history to use to undo (or redo) some action.

Each area's UndoManager can be a thin wrapper around the shared UndoManager, that in addition remembers the source.

[JordanMartinez]

While this is correct, in trying to give the user complete control over how to instantiate the undo manager, for StyledTextArea the undo manager would be nothing more than a handler of the Ctrl+Z/Shift+Ctrl+Z events.

Ok. That is true and would give more control over how that works.

I didn't imagine there would be different model for different subclasses of StyledTextArea. I don't think subclassing StyledTextArea should be encouraged.

So, when a developer does subclass StyledTextArea, and he/she wants to distinguish the view from the model, they will have to create a second model within the subclassed view?

For example, let's say a developer subclasses StyledTextArea for use as an area where the user can select some word/term and then define that word/term. Then, if the user hovers their mouse over an instance of one of those terms anywhere in the area, a popup will appear displaying its definition.

If the model cannot be subclassed, we'd have to take the following approach:

// store the terms that have been defined in their own model
public class SubModel {
    private final Map<String, Definition> termsToDefinitions;
    public Definition getDefinitionOf(String term) { return termsToDefinitions.get(term); }

    // rest of the class...
}
public class SubStyledTextArea<PS, S> extends StyledTextArea<PS, S> {
    private final subModel = // a second model stored within the view

    // which model do we get? Two models might lead to confusion...
    // get SubStyledTextArea's model
    protected final SubModel getSubModel() { return subModel; }
    // get StyledTextArea's model
    protected final getModel() { return model; }

    public Definition getDefinitionOf(String term) { return subModel.getDefinitionOf(term); }

    public SubStyledTextArea(/* args */) {}

    // rest of class...
}

But if the model could be subclassed, we'd only have one model in the view

public class SubModel<PS, S> extends StyledTextAreaModel<PS, S> {
    private final Map<String, Definition> termsToDefinitions;
    public Definition getDefinitionOf(String term) { return termsToDefinitions.get(term); }

    // rest of the class...
}

public class SubStyledTextArea<PS, S> extends StyledTextArea<PS, S, SubModel<PS, S> {

    // get the only model within the view
    public final getModel() { return model; }    

    public Definition getDefinitionOf(String term) { return getModel().getDefinitionOf(term); }
}

Each area's UndoManager can be a thin wrapper around the shared UndoManager, that in addition remembers the source.

Ok. That works!

[TomasMikula]

Why store the dictionary of term definitions inside StyledTextArea?

[JordanMartinez]

Why store the dictionary of term definitions inside StyledTextArea?

If you were implementing the above idea, would you compose a new class like so?

class ReaderArea<PS, S> extends Region implements Virtualized {
    StyledTextArea<PS, S> area;
    HashMap<String, Definition> termsToDefs;

    ReaderArea(/* args */) {
        getChildren().add(area);
    }

    // rest of class code...
    protected void layoutChildren() {
        // layout area correctly...
    }
}

I don't know if my previous comment's example was a good one, but I was simply trying to show that when StyledTextArea is subclassed, the developer will be forced to account for two models, especially when cloning the subclassed area:

public class SubClassModel {
    // same object is shared across clones...
    protected final ObservableMap<String, Definition> getDefinitions() { return map; }

}
public class SubStyledTextArea<PS, S> extends StyledTextArea<PS, S> {
    private final SubClassModel subModel;

    public SubStyledTextArea(ObservableMap<String, Definition> termsToDefs, 
            PS initialParagraphStyle, BiConsumer<TextFlow, PS> applyParagraphStyle,
            S initialTextStyle, BiConsumer<? super TextExt, S> applyStyle,
            boolean preserveStyle) {
        // constructor code
        subModel = new SubModel(termsToDefs, /* additional args... */);
    };

}
SubStyledTextArea area = // creation code....
SubStyledTextArea clone = new SubStyledTextArea<>(
        area.getSubModel().getDefinitions(),
        area.getInitialParagraphStyle(), area.getApplyParagraphStyle(),
        area.getInitialTextStyle(), area.getApplyStyle(),
        area.getModel().getContent(), area.isPreserveStyle())
);

[TomasMikula]

If you were implementing the above idea, would you compose a new class like so?

I would implement it like this

class MyApp {
    StyledTextArea<Foo, Bar> area;
    Map<String, Definition> termsToDefs;
}

No extending Region or Virtualized. Just an encapsulation of two objects. Composition over inheritance.

[JordanMartinez]

Oh... ok.

[JordanMartinez]

Composition over inheritance.

You're probably getting annoyed with the number of times you've stated this principle to me 😜

[TomasMikula]

This is only about the second time I guess, or I have a short memory ;)

[JordanMartinez]

Just FYI. Now that we've migrated to the new approach in RTFX, I'll probably start to look at this again next week. I think this is the last unstable aspect that needs to be fixed before a stable release can be made.

[JordanMartinez]

Just an update. I'm pretty sure I've designed a working non-linear undo/redo system in UndoFX (see FXMisc/UndoFX#11). The only issue is designing a suitable test environment to test whether this system actually works.

[JordanMartinez]

Edit: Reference correct PR
Just an update on this. I've written a RichTextFX-like test environment in FXMisc/UndoFX#13 (my current PR for this feature in UndoFX). There are still a few things to do before it will be ready (finish the FixedSizeNonlinearChangeQueue implementation, use the test environment to prove the code works / work out any other bugs, and then clean up the commit history to clearly show the development of feature).
However, I've concluded that even after I finish that PR, performance will no doubt be a major issue if one decided to use it in RTFX as the code isn't thread safe. I'm not sure how hard it would be or how much time it would take to make this code thread safe.

[TomasMikula]

As long as it is accessed only from the JavaFX application thread, which is the assumption for most things JavaFX, thread-unsafety is not an issue. Anyway, how does that relate to performance? Do you think you could gain performance by parallel implementation?

BTW, it used to be the case that the undo manager had "privileged" access to the stream of changes, in the sense that it would receive the change before any other observer of StyledTextArea#richChanges()/plainTextChanges(). This was lost here eba3671. Without this privileged access, if there are other observers of richChanges(), it is now not defined who gets the notification first. This can be problematic in some scenarios. I didn't like that privilege for undo manager, because it was something the users couldn't achieve themselves (which is also a reason why undo manager had to be baked into StyledTextArea). I wanted to get rid of that ad-hoc privileged access (implemented via a pausable copy of the rich-change stream), but at the same time, I wanted to give users the ability to define themselves who (if anyone) gets privileged access.

[JordanMartinez]

As long as it is accessed only from the JavaFX application thread, which is the assumption for most things JavaFX, thread-unsafety is not an issue. Anyway, how does that relate to performance? Do you think you could gain performance by parallel implementation?

Aye to the parallel implementation. My current implementation in that PR iterates through a graph's queues and has each of them run code to handle their changes' updating as well as recalculating their valid changes (among other things). Theoretically, this process will take longer if such queues have a very large number of changes. Although one would probably only have one to five queues hooked into the graph at a time since one would probably only display five views at most, theoretically, one could have thousands of such queues. In which case, using concurrency would address the resulting performance problem.

Again, it's not something that is desperately needed for GUIs, just my mind realizing that the code could be further optimized if so desired. However, even UndoFX's ReadMe mentions that it's code base could be used for Java applications in general, so perhaps that is desirable to some extent. However, I don't currently have a need to optimize the code in that way.

BTW, it used to be the case that the undo manager had "privileged" access to the stream of changes, in the sense that it would receive the change before any other observer of StyledTextArea#richChanges()/plainTextChanges(). This was lost here eba3671. Without this privileged access, if there are other observers of richChanges(), it is now not defined who gets the notification first. This can be problematic in some scenarios. I didn't like that privilege for undo manager, because it was something the users couldn't achieve themselves (which is also a reason why undo manager had to be baked into StyledTextArea). I wanted to get rid of that ad-hoc privileged access (implemented via a pausable copy of the rich-change stream), but at the same time, I wanted to give users the ability to define themselves who (if anyone) gets privileged access.

Good points and helpful for understanding the thoughts behind your design.

I don't remember if I stated this here or elsewhere, but even if the UndoFX PR on which I'm working was merged into RTFX, one could still use EditableStyledDocument#replace via direct access to that object and not indirect access through a view to make changes to the document and the graph would not be able to account for that, thus screwing up the entire graph. That's partly why I suggested moving the undo code outside of the area in #333.

However, it does seem that the code base you wrote and to which I and some others contributed has reached a state where it truly is a code base.
I wonder how much longer we can keep adding features before those features force us, rather than users of this project, to make design decisions that ultimately help or hinder the users. In other words, a RichTextFX-Extras project does start to sound more and more like a necessity. If this project provides the building blocks of building rich text areas, then the extras project could be the various ways someone constructed those bricks together.
If that approach is taken, then this project should be renamed to RichTextFXBase and the extras project would take this project's name. Unfortunately, I don't know who would maintain the resulting extras project as its scope would be about providing stable and functional rich text areas (standard rich text area, code editor with syntax highlighting, etc).

[JordanMartinez]

The real issue here is whether to include this feature inside the scope of this project. Although such a feature could be added for StyledTextArea-based areas, it wouldn't be very complex and time-consuming to add such a feature to GenericStyledArea.

For now, I'll label it as an enhancement.

[JordanMartinez]

I think this issue itself is actually outside the scope of this project. In reality, a user should be given "full control" (in Tomas' words) over how undo/redo works and what its privilege is. This implies that #333 should be done rather than adding support for nonlinear undo/redos, which are complicated, all of the options and situations for which we will not be able to fully account, and which a developer could implement on top of the area base which RTFX provides.