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.
matthew-carroll
force-pushed
the
undo-redo_snapshot-with-commands
branch
from
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.
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.
Do we want a try/finally to still end the transaction even if something crashes in between?
|
|
||
| editor.endTransaction(); |
There was a problem hiding this comment.
Do we want a try/finally to still end the transaction even if something crashes in between?
| // editor.execute([ | ||
| // ChangeSelectionRequest( | ||
| // insertionSelection, | ||
| // SelectionChangeType.placeCaret, | ||
| // SelectionReason.userInteraction, | ||
| // ), | ||
| // ]); |
There was a problem hiding this comment.
Please remove the commented code.
| final history = <CommandTransaction>[]; | ||
| final future = <CommandTransaction>[]; |
There was a problem hiding this comment.
Did you intend to leave these properties public?
| return; | ||
| } | ||
|
|
||
| print("STARTING TRANSACTION"); |
There was a problem hiding this comment.
Please remove this and the other print statements.
| void main() { | ||
| group("Super Editor > undo redo >", () { | ||
| group("text insertion >", () { | ||
| testWidgets("insert a word", (widgetTester) async { |
There was a problem hiding this comment.
Shouldn't we use tester instead of widgetTester to match our existing tests?
|
|
||
| // Type text that causes a conversion to a header node. | ||
| await widgetTester.typeImeText("--"); | ||
|
|
||
| // Ensure that the paragraph is now a header. | ||
| expect(SuperEditorInspector.findTextInComponent("1").text, "—"); |
There was a problem hiding this comment.
These comments don't look correct, since we are not dealing with headers here.
| expect(SuperEditorInspector.findTextInComponent("1").text, "Hello"); | ||
|
|
||
| // Undo the typing. | ||
| print("------------ UNDO -------------"); |
There was a problem hiding this comment.
Should we have this print statement here?
| // Move caret to start of paragraph. | ||
| await widgetTester.placeCaretInParagraph("1", 0); | ||
|
|
||
| // Type a few more characters |
There was a problem hiding this comment.
Please end the comment with a dot.
| }); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we have a test to ensure that, when we undo or redo, we sync our editing value with the IME ?
|
@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. |
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
Editablestate snapshots, combined with a history of one-wayEditCommands.With this approach, when the user wants to undo a change, the
Editables are completely reverted to their most recent "snapshot" and then all historicalEditCommands are replayed, except the most recentEditCommand, thus achieving undo.This approach requires that
Editables 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. EachEditableneeds 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.