Skip to content
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

Fix collaborative undo correctness (fixes #2105) #2500

Merged
merged 7 commits into from
Feb 12, 2019
Merged

Conversation

dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Feb 8, 2019

Inspired by #2433. Thank you to @rhysbrettbowen for the bug report and suggested solution. This PR takes a similar approach, but calls guessUndoDelta (now invertDelta) directly from change(...) for simpler logic.

In this PR:

  1. Fix incorrect transformation of the undo and redo stacks, as described in logic error in history module when transforming undo/redo against non-user changes #2105 . (Reasoning: If you are going to apply a delta X to a succession of deltas A, B, C that compose, you have to transform A using X to get A', and also transform X using A to get X', and then transform B using X', not X... transform C against X'', and so on.)
  2. When applying an undo, generate the redo on the spot, and vice versa. (See below for explanation.) . This means the undo and redo stacks are now just arrays of Deltas instead of {undo,redo} records.
  3. Enhance guessUndoDelta to be a real delta inversion, invertDelta, because it was not too far off! This routine could be extracted into quill-delta (see Could we have an undoCompose function? For calculating the reverse of a delta when composed with a base delta. delta#28 ; I think invert is perhaps a better name than undoCompose).
  4. When coalescing undo records, don't create an empty undo record. This improves the undo experience, because applying an empty delta was a source of the cursor jumping on undo.
  5. Take test from Fix the transforms in undo/redo #2433 and add another.

To give an example of why (2) is necessary, even after fixing (1): Suppose you type xxx, and a collaborator comes along and makes it yxyxyxy. You then undo, and the editor correctly removes the x's and leaves you with yyyy. However, when you then go to redo, the correct redo is to insert three x's in different places. If you use the original redo delta with appropriate transformations, what you get is insert xxx transformed against insert yyyy, which is not going to result in inserting the three x's in different places. So with this change, the redo delta is generated at the time the undo is applied. I added this example as a test case.

I have lightly tested undo in Quill after this change, but not in a real collaborative context (i.e. the real the version of what the tests are testing).

@jhchen jhchen merged commit 3795d47 into develop Feb 12, 2019
@jhchen jhchen deleted the dg-collab-undo branch February 12, 2019 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants