pedrosanta / quill-sharedb-cursors

Collaborative editing with multi cursors sync using Quill and ShareDB.
https://quill-sharedb-cursors.herokuapp.com
MIT License
138 stars 19 forks source link

Selection/cursor-change and edits racing condition #1

Open pedrosanta opened 7 years ago

pedrosanta commented 7 years ago

A known issue of this approach/example is a racing condition which makes cursors stuck/out-of-correct-position sometimes when a selection-change cursor update (which goes through cursors socket) is quickly followed by some edits (which goes through ShareDB and its socket) - like using the back arrow key with Ctrl to go to the beginning of a word and immediately start typing.

How to replicate:

  1. Open two side-by-side windows on https://quill-sharedb-cursors.herokuapp.com;
  2. On one of them try to move to the beginning of a word with the keyboard shortcut (Ctrl/Option+Left Arrow) and inserting some text immediately after;
  3. If you do these things quick enough, the cursor on the other client might become off-position.

My take is that:

One solution for this was if... cursor updates could be transmitted through ShareDB (probably as ephemeral metadata like stated on share/sharedb#128).

Still needs more testing.

sferoze commented 7 years ago

@pedrosanta

I noticed this issue too. The cursor is sometimes sent before the sharedb delta operation is updated. So it sometimes forces the cursor onto a new line or it hides the cursor.

What do you think about this as a temp solution?

the quill.on 'selection-change' even is only called on initial focus, when you have selected or deselected text, and when you place caret in a different spot. It is not called on each character typed though. Was this your initial assumption when writing the code?

So the cursor updating is really dependent on doc.on 'nothing pending', debouncedSendCursorData event. Since the original timeout was really high, it caused issues.

So I reduced the timeout to 1ms. Basically always updating the cursor whenever operations have competed. This way the cursor always is updating right after the delta's have been applied.

@pedrosanta In my testing it seems this solutions works better, and just wondering what your thoughts are?

debouncedSendCursorData = utils.debounce((->
      range = quill.getSelection()
      if range
        console.log '[cursors] Stopped typing, sending a cursor update/refresh.'
        sendCursorData range
      return
    ), 1)
    doc.on 'nothing pending', debouncedSendCursorData

quill.on 'selection-change', (range, oldRange, source) ->
      sendCursorData range
      return
sferoze commented 7 years ago

But I do notice one downside is the constant sending of cursor data via sockets...

Otherwise the cursor module shifts the cursor to the correct spot depending on the delta, not requiring the cursor update.

So reducing the timeout does help cursor placement but also uses more network resources...

update:

in my testing reducing debounce timeout to 10ms doesn't make the cursor update method called significantly more. I think it is best to reduce the debounce timeout.

pedrosanta commented 7 years ago

Hey @sferoze, thank you for the comments. I've been a bit low on time in the latest few days, but let me catch this up and I'll get back to you. 👍