Remove a cursor with an invalid index in _updateCursor()

[thomsbg]

An example scenario:

• Two clients (client A and client B) are connected to the same document in such a way that client A's changes take 5 seconds to propagate to client B and vice versa.
• Let the initial length of the document be 100.
• Client A changes her cursor position to 95. At the same time, Client B deletes 20 characters from the beginning of the document.
• By the time Client B receives A's new cursor position, the document length is 80 characters, and the cursor position of 95 is invalid (beyond the end of the document).

The right thing to do in this scenario would be to transform Client A's cursor against the 20 character deletion. However, the amount of bookkeeping necessary to compute this diff is not trivial, and so for this edge case I prefer to just remove the offending cursor.

[jhchen]

Deltas have a method for transforming a cursor position so whatever is currently being used to resolve text conflicts should also be able to resolve cursor positions.

[thomsbg]

I am aware of Delta#transformPosition(), and am using it to great effect. However, I still think this change is necessary. I will try to describe why.

Imagine an OT system with two clients A and B, and a server S. In this system, as A makes changes, I keep track of the pending diff (unsavedDelta) between A and the S before the S replies "OK Saved". If B moves her cursor, it is broadcast to A, who calls unsavedDelta.transformPosition(cursor). This works well 90% of the time.

However, there is a brief window of time after S has replied "OK Saved" to A, but before B is notified of the change. If B moves her cursor during this time, A will receive B's cursor-change broadcast, but have no unsavedDelta to transform it against. If B's out-of-date cursor is beyond the length of A's document, calling setCursor will throw an Invalid index error.

I tried surrounding my call to setCursor with a try/catch, but found that further changes by A will trigger more Invalid index errors as B's invalid cursor is shifted to compensate. These Invalid index errors crucially prevent my text-change listeners from firing and saving document changes.

Therefore, I think it is important for _updateCursor to never throw Invalid index errors.

[jhchen]

I'm not understanding how text transformation could work but cursor transformation contains a window that doesn't work 10% of the time. The cursor information could be represented as a text insertion of a unique character so it's really just a special case of text transformation. Issues do arise if cursor updates are sent/received on different a different channel than text.

Regardless, multi-cursor's updateCursor call is very brittle and should be improved. I'm not sure why getBounds throws an error (as that's not idiomatic in Quill's codebase) but it should probably return null instead and updateCursor on a null/invalid boundary could remove the cursor. Would you be amenable to these changes instead?

[thomsbg]

I'm not sure why getBounds throws an error (as that's not idiomatic in Quill's codebase)

This is why: https://github.com/quilljs/quill/blob/ea8e31c4d20c853aa3374dea1de4559bc4421abf/src/core/editor.coffee#L88

but it should probably return null instead and updateCursor on a null/invalid boundary could remove the cursor. Would you be amenable to these changes instead?

I can change editor.getBounds() to return null instead. Since quill.getBounds() is a public API method, changing from throwing an error to returning null sounds like a significant change. Shall we proceed with that strategy?

[jhchen]

I meant why I coded getBounds to throw an error since almost nowhere in Quill does it throw errors. In any case getBounds is not yet documented so there's more flexibility to make changes (intentionally for this reason).

[thomsbg]

Updated to have quill.getBounds() return null in case of an invalid index.

[jhchen]

Great thanks!