-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Real-time collaboration: [pr-72604] Replace fast-diff in quill-delta (with cursor hint) #72604
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
Real-time collaboration: [pr-72604] Replace fast-diff in quill-delta (with cursor hint) #72604
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| yblocks: YBlocks, | ||
| incomingBlocks: Block[], | ||
| lastSelection: WPBlockSelection | null | ||
| yblocks: Y.Array< YBlock >, // yblocks represent the blocks in the local Y.Doc |
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.
can you revert this type change?
| yblocks: Y.Array< YBlock >, // yblocks represent the blocks in the local Y.Doc | |
| yblocks: YBlocks, // yblocks represent the blocks in the local Y.Doc |
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.
Fixed (along with the duplicate comments) in b89cffb
packages/sync/src/index.ts
Outdated
| OpIterator, | ||
| AttributeMap, |
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.
Why are these exported? They seem unused
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.
Op too
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.
Looks like they are creating some missing documentation as well: https://github.com/WordPress/gutenberg/pull/72604/files#diff-da401d4179b4bbc25579caf51d49a6fbeda9f60d7d4c3349fd2996bf955f6e7f
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.
Good catch! Fixed in 0d36056.
771b65f to
b694c7d
Compare
0d36056 to
15db094
Compare
…umptions about results
This reverts commit cc304ee.
57ef102 to
b6ec9be
Compare
…(with cursor hint) (#72604) * Include quill-delta with fast-diff replacement * Update generated docs * Remove "mixed operations" tests because they have hardcoded order assumptions about results * Reformat code in quill-delta * Revert "Reformat code in quill-delta" This reverts commit cc304ee. * Run 'lint:js:fix' * Replace non-type-safe comparisons with type-safe comparisons * Fix type errors in Delta * Revert type change in mergeCrdtBlocks() * Only export Delta from sync package * Add diff types to sync package * Fix remaining != vs !== issue * Add docs comment for Delta export * Add README to explain quill-delta library fork --------- Co-authored-by: chriszarate <[email protected]>
…(with cursor hint) (#72604) * Include quill-delta with fast-diff replacement * Update generated docs * Remove "mixed operations" tests because they have hardcoded order assumptions about results * Reformat code in quill-delta * Revert "Reformat code in quill-delta" This reverts commit cc304ee. * Run 'lint:js:fix' * Replace non-type-safe comparisons with type-safe comparisons * Fix type errors in Delta * Revert type change in mergeCrdtBlocks() * Only export Delta from sync package * Add diff types to sync package * Fix remaining != vs !== issue * Add docs comment for Delta export * Add README to explain quill-delta library fork --------- Co-authored-by: chriszarate <[email protected]>
…(with cursor hint) (WordPress#72604) * Include quill-delta with fast-diff replacement * Update generated docs * Remove "mixed operations" tests because they have hardcoded order assumptions about results * Reformat code in quill-delta * Revert "Reformat code in quill-delta" This reverts commit cc304ee. * Run 'lint:js:fix' * Replace non-type-safe comparisons with type-safe comparisons * Fix type errors in Delta * Revert type change in mergeCrdtBlocks() * Only export Delta from sync package * Add diff types to sync package * Fix remaining != vs !== issue * Add docs comment for Delta export * Add README to explain quill-delta library fork --------- Co-authored-by: chriszarate <[email protected]>
What?
This is a follow-up to @ingeniumed's work in Automattic#43 for the
wpvip/rtc-pluginbranch. This branch:quill-deltato the sync package.fast-difflibrary used inquill-delta, which is incompatible with GPLv2 due to the Apache 2 license. Instead we usediff, which has a compatible license.fast-diffcalleddiffWithCursor()that attempts to match insert/deletion location with user cursor position. More information on this below.Why?
For multi-user editing within a single rich text area, we want to send incremental updates instead of entirely new strings in Yjs. This makes update operations more efficient and allows us to support relative positioning.
Y.Textnatively supports applying changes asDeltas along withquill-delta, but we're unable to directly use thequill-deltalibrary due to license constraints.What does the new
diffWithCursor()function do? The per-character function provided bydiffisdiffChars(), which takes two strings as input but does not accept a cursor position. This can cause ambiguous changes when there are repeated characters or substrings.For example, a user enters an
ain the middle of a run of the same character:diffChars( 'aaa', 'aaaa' )gives this result:This indicates adding an
ato the end of the string. BecausediffChars()only accepts two strings and has no cursor awareness, it can only guess where theawas added, and always selects the last position. This results in cursors "moving backwards" when another user types:The "Chrome" user's cursor should move forward when my user is typing, but it doesn't because the
ais being appended to the end of the string and the relative positioning of cursor fails to match user expectations.The new
diffWithCursor()function wraps the initialdiffresult and attempts to move deletions or insertions to the user's cursor position. Here's what the processed result ofdiffChars()looks like:This matches the actual user's experience and where the character is added. The result, with the character in the right position, is relative postioning working as expected:
How?
Here's a runthrough of the
diffWithCursor()function. I'm not sure it handles every case, but it handles a lot of cases better thandiffalone.First, we convert incoming
Deltaoperations to strings we can use withdiffChars().For example, if we have the string
jajajaand a user has pastedjaat cursor position 2, (ja|jaja->jaja|jajawhere|is the cursor),this.opswill be{ insert: "jajaja" }andother.opswill be{ insert: "jajajaja" }.deltasToStrings()will process these operations into two strings,"jajaja"and"jajajaja".Next,
diffChars()is run against those two strings which is provided by thediffpackage. It gives this result:The
jaaddition has incorrectly been placed at the end of the diff.
Next, we post-process this diff to see if we can move the changes to the position of the cursor. Each diff object is iterated with these rules:
Path 1: If the current segment is unchanged, but we see the cursor ended in this segment, we try to see if we can move the insertion from the next segment.
For the example above, we encounter this path on the first diff segment.
cursorAfterChangeis4, indicating the cursor ended in the middle of the firstjajajasegment, but there is no insertion there. There is also an insertion directly after this segment, so we calltryMoveInsertionToCursor()which will check if the insertion after this segment ({count:2, added:true, value:"ja"}) can be moved to match the cursor location. If it can, then it'll:{count:2, value:"ja"}{count:2, added:true, value:"ja"}{count:2, value:"jaja"}with this result:This segment and the next is then consumed.
Path 2: Works similarly to part 1, but checking for deletions. Here's an example for the text
aaaawhere anais deleted in the second position (aa|aa->a|aa):We process the original diff of:
On processing the second segment, we see a deletion where the cursor was in the previous segment. Calling into
tryMoveDeletionToCursor()will test if the deletion string can be moved to the prior segment. If this is possible, we split the prior segment and add the deletion with this result:If the segment doesn't match either case above, add it as-is to the processed segments.
Lastly we convert the result of the processed diffs to
Deltaformat withconvertChangesToDelta(). For example:Before processing:
After processing as a
Delta:This indicates that the original text stays the same for the first two characters, and adds
ja, matching our original user behavior.Testing Instructions
aaaaaorjajajajaand insert multiple characters with pastes and ensure the second user's cursor in awareness moves as expected.diffWithCursor()tests can be run via: