-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Undo/Redo (with snapshots + one-way commands) #1900
Conversation
…to execute() so that delta text insertions can execute the delta and then later adjust the composing region and have the whole thing considered to be a single transaction.
…ansactions are applied.
…ash conversion reaction.
…posing region changes to the change list broke reactions
…es in a new transaction, also need checkpoint serialization for editables to avoid full history playback
…added react() changes to new transaction.
4363290
to
50deab3
Compare
@miguelcmedeiros @brian-superlist @knopp - Can you guys give this PR a try in Superlist to see if everything still works on your end? I don't expect that you'll use the new undo feature, but I'm doing surgery on the editor system to implement undo/redo. There's still more work to do in this PR, but I think at this moment we're at a good place to start finding where it breaks things. |
…ly per character.
…hanges - also show history in demo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments. I'm wondering if we should have a way to notify the commands that were undone/redone, for the cases where commands could be used to start actions outside of the editor.
For example, if an app has a reaction that, when an user tag is typed on a task, it runs a command that calls to the backend to assign the user to this task, should the app be able to unassign the user when the command is undone?
// End the editor transaction for all deltas in this call. | ||
editor.endTransaction(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a try/finally to still end the transaction even if something crashes in between?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait until we see such a problem come up. It's not obvious to me what would cause that, and if it happens, how we can confidently recover.
super_editor/lib/src/default_editor/document_ime/document_delta_editing.dart
Outdated
Show resolved
Hide resolved
super_editor/test/super_editor/super_editor_undo_redo_test.dart
Outdated
Show resolved
Hide resolved
super_editor/test/super_editor/super_editor_undo_redo_test.dart
Outdated
Show resolved
Hide resolved
super_editor/test/super_editor/super_editor_undo_redo_test.dart
Outdated
Show resolved
Hide resolved
super_editor/test/super_editor/super_editor_undo_redo_test.dart
Outdated
Show resolved
Hide resolved
}); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a test to ensure that, when we undo or redo, we sync our editing value with the IME ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What might that test look like?
@angelosilvestre I'll check your comments, by this PR is still at about the 75% mark. I just tagged you so you could start to see what's going on. |
@matthew-carroll I haven't gone through every line of this PR but here are my thoughts from the code I have read and the behavior I've observed in the example app so far: Lone selection changes shouldn't be included in the undo stack Based on some of the comments in this PR I think this might be on your radar already but thought I'd call it out anyway. In the current implementation moving the seletion gets included in the history, which is not true of other text editors I've tested against and I believe not what end users would expect. Consider the attached video: during undo/redo the selection moves between the Recording.2024-06-07.151220.mp4Support for other undo algorithms would be nice As-is this PR has a stack based history system baked into the |
@jmatth There's behavior in this PR that merges back to back selection changes. Can you be more specific about which selection changes should and shouldn't be captured? Obviously when the selection changes as the result of a content change, we need to capture that. But please let me know what policies you expect beyond that. WRT a branching history, do you have any resources where I can learn about that concept? |
For selection changes: I don't think undo/redo should ever produce a change that only modifies the selection, which is what happens from about the 6 second mark onward in my video: I delete content from two separate nodes, then repeatedly use It may also make more sense to phrase the requirement in terms of what does need to change for it to register in the history system, such as "an undo/redo operation must always produce a change in the For branching history: I've only ever seen it implemented in Vim/Neovim. Instead of storing the history in stacks and trashing the redo stack when the user begins making new changes, vim stores its history as a tree and new changes just create a new branch. Here is the quickest explanation I could find on Youtube and here is plugin I've seen most people use to visualize the tree. To be clear, I'm not asking for this feature to be included, just that the final API keep in mind that history implementations might need to be more complex than simple undo/redo stacks. |
I'm going to merge undo/redo in its current state so that we can begin to accumulate bug reports and other things. Undo/redo, as it exists right now, accumulates all changes and never collapses change history down to a snapshot. I'll plan to address that next. Since I didn't receive any feedback from stakeholders on this PR, I'm going to assume that everyone thinks that's OK for now. |
No snapshot support yet. All changes are accumulated for every editable.
Is there a way to limit the size of the undo stack to manage memory usage? |
@KevinBrendel yes we can, but we need to decide on what strategy(s) to use. Please comment here with your needs and thoughts: #2154 |
Undo/Redo (with snapshots + one-way commands)
This is an undo/redo concept that's intended to avoid some of the complexities of the reversible command undo/redo version (#1881) by using
Editable
state snapshots, combined with a history of one-wayEditCommand
s.With this approach, when the user wants to undo a change, the
Editable
s are completely reverted to their most recent "snapshot" and then all historicalEditCommand
s are replayed, except the most recentEditCommand
, thus achieving undo.This approach requires that
Editable
s be capable of providing some kind of immutable snapshot. It might be a Dart data structure, it might be JSON, etc. The format doesn't matter. EachEditable
needs to be able to provide some kind of snapshot, and then restore itself with that snapshot. This approach also re-runs a number of commands when undo'ing the previous command. This extra compute cost allows us to avoid the complexity of implementing reversible commands.