fix: group contiguous note edits into a single undo step#4524
Conversation
Route note edits through editThought instead of setDescendant so consecutive note changes merge in the undo stack, and mark toggleNote as non-undoable.
|
|
Issue A: Undo is splitted into 2 stacks for replacing a word on NotesStep to Reproduce
Current behaviorThe first Undo the replaced word becomes Fast.type.movSlow.Type.movExpected behaviorThe first Undo should revert to the original |
Issue B: Backspace / Delete is not working and caret is moved to the end of the thought text after Undo on the first thoughtStep to Reproduce
Current behaviorNothing happens. Issue.B.Backspace.Delete.is.not.working.after.Undo.MP4Expected behaviorBackspace/delete should work after Undo action. |
Issue C: Undo a new Notes creation is Undoing the last action prior the creating the new Notes but leaving the empty NotesStep to Reproduce
Current behavior
Issue.C.Undo.a.new.Notes.creation.is.Undoing.the.last.action.prior.the.creating.the.new.Notes.but.leaving.the.empty.Notes.MP4Expected behaviorNewly created empty note should be deleted |
Issue D: Backspace/Delete button on the keyboard deletes the thought space.Step to Reproduce
Current behaviorThought space is deleted. Issue.D.Backspace.Delete.button.on.the.keyboard.deletes.the.thought.space.MP4Expected behaviorBackspace/Delete button on the keyboard should serve to delete the character before the caret. |
BayuAri
left a comment
There was a problem hiding this comment.
List of issues:
- Issue A: Undo is splitted into 2 stacks for replacing a word on Notes
- Issue B: Backspace / Delete is not working and caret is moved to the end of the thought text after Undo on the first thought
- Issue C: Undo a new Notes creation is Undoing the last action prior the creating the new Notes but leaving the empty Notes
- Issue D: Backspace/Delete button on the keyboard deletes the thought space.
@raineorshine
Question about Issue A, is this intended as the implementation of Replacements → separate undo steps
Refer to
Replacements → separate undo steps
#4479 (comment)
This raises a good question, though for the purpose of this issue we are just aiming to reproduce the undo/redo behavior that currently exists for normal thoughts. So for any edit to a note, check how many undo/redo steps are created if the same edit is applied to a normal thought. That should be enough to figure out the intended functionality, but to answer your specific question about Issue A, you have come up with a case that I don't think I had considered yet, and I'm not sure how this is handled in the code currently. I think the code currently categorizes edits based on the string length delta: ADD: Issue A is unique in that it is a replacement, but the number of characters are not the same. I think However, that's all out of scope for this issue, whose main purpose is to implement baseline undo/redo chunking to notes. This was understandably not so clear from my hasty bug report. |
Route note edits through editThought instead of setDescendant so consecutive note changes merge in the undo stack, and mark toggleNote as non-undoable.
This PR fixes #4479.
Changes: