-
Notifications
You must be signed in to change notification settings - Fork 466
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
WIP: initial implementation of cursors #306
Conversation
frontend/index.js
Outdated
* - consider returning the closest character if deleted | ||
* - consider optimizing the linear search | ||
*/ | ||
function findCursorIndex (cursor, doc, findClosest) { |
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.
I think it's odd that this function requires doc
as a separate argument, but I couldn't find any way to navigate from a cursor object to the root doc.
Thanks @geoffreylitt, this is a good start! I pushed a few commits to change some details. In particular, I want to avoid the frontend calling directly into the backend, since that will cause them to become much more closely coupled than they should be. I moved the the We will need to make the equivalent change in the Rust implementation to maintain compatibility between the two implementations. This should not be hard. I will call out that this approach to cursors is incompatible with running frontend and backend on separate threads, since it relies on synchronously calling into the backend. I think that's probably an okay trade-off to make since most Automerge users probably aren't using multiple threads, and having this direct call into the backend is a lot simpler than updating the frontend-backend protocol. But weakening our support for multiple threads is something we should discuss, and make an explicit decision about. |
This is great work, I'm glad someone is taking on the cursor challenge 🙂 I'm also concerned about the explicit coupling to the backend. If we rule out the use of different threads for the frontend/backend protocol (and we would be ruling it out in Rust, the Backend and Frontend components have not been designed to be threadsafe and would require a significant rewrite to enable it) then we lose a lot of advantages the design gives us. I'm currently working on performance for the Rust codebase and aiming to make any change that happens on the UI thread fit within a 16ms budget in order to make 60FPS UI possible, removing multithreading makes this extremely difficult. Is it possible to achieve this same interface by adding an index to the frontend for the element ID <-> list index rather than asking the backend for that information? |
*/ | ||
getCursorAt (index) { | ||
return { | ||
// todo: are there any points in the lifecycle where the Text object doesn't have an ID? |
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.
@ept I see some other places that handle the case where the object ID is not set.
Would it be sensible to add a similar check here and forbid getting a cursor if there's not an object ID? I don't yet understand when/how that case occurs.
@ept thanks, all those updates make sense. re: frontend/backend split -- as an alternative to @alexjg 's suggestion of maintaining more state in the frontend, what if I guess one problem might be that until now the protocol has entirely consisted of change requests going one way and patches going the other, and this would be adding a new kind of message that would need to be handled? |
An attempt at a simple initial implementation of cursors, as discussed in #239.
Automerge.Frontend.findCursorIndex
which operates on that object, because that was easiest to implement. Probably would provide a nicer API to switch it to a custom datatype?