Generate more accurate deltas from typing

[dgreensp]

This PR fixes issue #746 for real this time, adds tests, and also handles a wider variety of cases. Fast-diff's cursor-position hint is no longer used; instead, the editor looks at both the old and new selection (cursor or range) to determine the best delta to represent the change.

There are many examples of ambiguous diffs that can only be disambiguated using selection information. For example, the minimal insertion in aaa to aaaa could be at index 0, 1, 2, or 3. The same is true for the two-character insertion in bob to bobob, or the single-character deletion in aaaa to aaa. Because of backwards deletion (control-D or fn-delete on a Mac), both the old and new cursor are needed to get the delta right. In order to handle the case where foo is selected and the letter f is typed (as distinct from deleting oo), the full selection range for the old selection is required.

There were a few issues with the previous implementation:

• The cursor position passed to fast-diff was not correctly calculated for the mutation fast path (so typing on the first line of the document might work but typing on the second line wouldn't)
• Fast-diff did not correctly take the cursor position into account for some outputs of the diff optimizer, sometimes as simple as deleting and re-inserting the middle character in strings like aab or abb. An extra character before or after the repeated character causes the diff optimizer to bias towards a different repeated character. This proved hard to fix in fast-diff.
• Only the old cursor position was taken into account, not the new position or old selection range

This new solution is implemented at the editor level rather than the diff level, because it's not clear what taking a cursor location into account for an arbitrary diff ultimately means; a lot of edge cases come up that probably aren't of any practical importance in Quill, but it's hard to be certain of, and capture, this fact.

For Slab, there will be a direct benefit for displaying remote collaborator cursors and selections more accurately. For example, if selecting foo and typing f is considered to be a deletion of oo, it will look to collaborators afterwards as if you have selected the f. In other cases, collaborator cursors will be shown in a slightly wrong location after typing. Author attributes will also be more accurate.

[jhchen]

Since there's no substantial else branch we can we instead check the opposite and return right away and save an indent on a lot of code? Also the inner if + else if both check for the same second condition which could be made more obvious by checking outside/immediately too so adding both of this together we could do something like:

const [oldSelection, newSelection] = selectionInfo || [];
if (selectionInfo == null || newSelection.length > 0) {
  return oldDelta.diff(newDelta);
}

[dgreensp]

The branches of the big if/else don't always return; sometimes they fall through. So the diff has to be at the bottom to catch those cases.

The way I think of this method is as two special cases to check for, each with its own conditions, checks, and guards. It's possible that the number of special cases or the conditions and guards on each case could change in the future. Maybe this would be clearer if the big if/else were actually two ifs. selectionInfo has to be present to even consider the special cases, and to calculate the variables that are used by both. There's no way to know if a special case will apply or not, though, without running them.

[jhchen]

Yes I was thinking the last return oldDelta.diff(newDelta); would still be there at the end of the function. Also the if + elseif could turn into and if + else so the relationship between the branches is perfectly clear. There is still the inner conditions that may not be satisfied but the last return oldDelta.diff(newDelta); would take care of this.

[dgreensp]

I don't feel too strongly, but is duplicating code better than having an extra level of indentation? If the non-special case needs to change, it would have to change in two places.

The fact that the two special cases (insertion or deletion before or after the cursor; typing over a range) both make certain requirements of newSelection and oldSelection, and these requirements are partly the same and partly exclusive, seems totally coincidental to me, so at least the way I've been thinking about it, highlighting the common part and the exclusive part doesn't add clarity. But maybe just making the structure of the code simpler makes it easier to read. We can do it.

[dgreensp]

I mean maybe there is a logic to it, like if the new selection is a range then logically, this isn't a splice caused by someone typing. And if the old selection is a range that's a different case than the old selection being a cursor, and the two diff strategies basically correspond to those two cases. I guess I just didn't have those thoughts at the time. :)

[jhchen]

I don't feel strongly on my particular recommendation but I do feel something should be done to make the if branch easier to read. At 70 lines long I cannot see the full if branch on a 28" monitor. A couple other ideas:

• The two inner if else branches can be their own functions
• Instead of calling return oldDelta.diff(newDelta); at the end we call diffDeltas(oldDelta, newDelta) (removing the selectionInfo parameter even if one is supplied). If function overloading were a thing in JS this would be a clear candidate for it so this approach somewhat approximates that

[dgreensp]

Oh I see. I’ll do something about that!

…

On Sat, Aug 18, 2018 at 4:06 PM Jason Chen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/editor.js <#2252 (comment)>: > @@ -328,4 +338,84 @@ function normalizeDelta(delta) { }, new Delta()); } +function splitDelta(delta, index) { + return [delta.slice(0, index), delta.slice(index)]; +} + +function diffDeltas(oldDelta, newDelta, selectionInfo = undefined) { + if (selectionInfo) { I don't feel strongly on my particular recommendation but I do feel *something* should be done to make the if branch easier to read. At 70 lines long I cannot see the full if branch on a 28" monitor. A couple other ideas: - The two inner if else branches can be their own functions - Instead of calling return oldDelta.diff(newDelta); at the end we call diffDeltas(oldDelta, newDelta) (removing the selectionInfo parameter even if one is supplied) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2252 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJgpmGlT9KsEygCAy3qUsxBzOei3z4gks5uSJ3-gaJpZM4V_FSY> .

[dgreensp]

30 of those lines weren't there before running the linter ;)

[dgreensp]

Ok, see how this version strikes you. I also changed { index: ..., length: ... } to new Range(...), which helped with the monstrosity that was relativeSelectionInfo after prettier (edit: besides being the correct way to create a range, I believe).

[NHypothesis]

@jhchen What are the chances of getting this fix cherry-picked into a 1.3.x release? I've been running into issue #746 while working on a feature that maintains multiple ranges, roughly analogous to multi-cursor and multi-select. My choices are otherwise waiting on Quill 2.0 or maintaining a custom build, neither of which are ideal, so I figure it's worth asking. Either way, thanks!