reedsy / quill-cursors

A multi cursor module for Quill text editor.
MIT License
250 stars 53 forks source link

Move during transformation can lead to wrong result #56

Closed Krizai closed 4 years ago

Krizai commented 4 years ago

Theoretically, following situation is possible:

Result - incorrect cursor position.

As a solution I'm ignoring moves during transform on my fork, but in this way we can lose some move, which is ok for my case, but not for a general case.

alecgibson commented 4 years ago

Yes, I've been thinking about this a lot lately. Updating a client's position whilst updates are happening is an extremely difficult problem. This library currently recommends working around it with timeouts and debounces, but it's not a robust solution.

However, the answer is that it shouldn't be a concern of this library.

The real solution is complex, and requires intimate knowledge about the underlying operations, and I'm actively working on implementing this robust solution in ShareDB (if you want an idea of how complex this problem is, just have a read of the descriptions of the test cases).

The future of this library is probably to become far "dumber" out-of-the-box, and to leave the heavy lifting of automatic cursor updates to the underlying OT library. The main concern of this library will be sort of: "Given some cursor range, let's just be good at displaying it, and not worry about how we got that range."

I'll probably issue a breaking change at some point that will mean that using quill-cursors with its default configuration will work well with using ShareDB's upcoming presence functionality.

Krizai commented 4 years ago

Good advice, thank you.

I'm already doing transformations on my own, as I'm not using ShareDB. So just switched off the embedded transformation, moved them to my code and applied some logic there.

alecgibson commented 4 years ago

As a point of interest, if you're not using ShareDB, I've seen an interesting take on presence, which uses OT itself to handle presence, which does take a lot of the edge cases and complexity out of handling it (at the trade-off of committing your transient presence data in OT).

Krizai commented 4 years ago

Thank you. Will check. By the way, I didn't really get one thing - why do we have setTimeout in transformation. Which cases can/will break without it?

alecgibson commented 4 years ago

Which setTimeout? Both of them have comments above them explaining why they're there.

Krizai commented 4 years ago

This one https://github.com/reedsy/quill-cursors/blob/master/src/quill-cursors/quill-cursors.ts#L148 I saw the comment

    // Wrap in a timeout to give the text change an opportunity to finish
    // before checking for the current selection

But still don't understand which actions are we waiting to finish and what can go wrong without the timeout.

alecgibson commented 4 years ago

I think the main thing is to do with the fact that JS event emitters execute their handlers synchronously, so we want to give the text-change event a chance to finish everything it's doing, before we check the selection in _emitSelection(), and trigger a new chain of handlers.

However, this sort of confusing behaviour is exactly the sort of stuff I'd like to cut when we make the library "dumber", so I wouldn't worry about it too much right now.

Krizai commented 4 years ago

Thank you. I thought the same, but just wanted to double-check.