Skip to content
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

Closed
wants to merge 7 commits into from
Closed

WIP: initial implementation of cursors #306

wants to merge 7 commits into from

Conversation

geoffreylitt
Copy link

@geoffreylitt geoffreylitt commented Jan 24, 2021

An attempt at a simple initial implementation of cursors, as discussed in #239.

  • For now I just used a plain object to store the Cursor, and added a function 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?
  • I first tried a pure frontend implementation (preserved in commit a80bfe1) but I believe this is limited in how it can handle deleted characters, as mentioned in Exporse oppsite of .getElemId(index) #198. So I then switched to using the backend state to find the closest index in case of deleted characters, based on your suggestion here. This at least avoids any changes to the frontend-backend protocol, but I wonder if there are still other issues caused by involving the backend. In particular, I'm not sure how this change affects frontend perf yet.
  • It seems like the best handling of deleted characters might depend on context of how the cursor is being used. In this PR I provided an option for how to handle the case where the character has been deleted (either return index -1, or try to find the closest one), but not yet sure if this is the best API.
  • I wonder if it would make sense to provide this functionality for both lists and text?

* - consider returning the closest character if deleted
* - consider optimizing the linear search
*/
function findCursorIndex (cursor, doc, findClosest) {
Copy link
Author

@geoffreylitt geoffreylitt Jan 25, 2021

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.

@ept
Copy link
Member

ept commented Jan 26, 2021

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 findCursorIndex function to the top-level Automerge module, which is where the integration of frontend and backend takes place. I renamed it to getCursorIndex and swapped its argument order for consistency with the rest of the API. I also made the API exported by the backend explicit.

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.

@alexjg
Copy link
Contributor

alexjg commented Jan 26, 2021

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?
Copy link
Author

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.

@geoffreylitt
Copy link
Author

@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 getCursorIndex was a function exposed by the backend, and the user could choose to either: 1) synchronously call the backend as in the current PR, 2) wire up their own messaging to make an async call.

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?

@ept
Copy link
Member

ept commented Feb 11, 2021

This PR is based on the Automerge 0.x release series. As we are now focussing our attention on the performance branch and the 1.x series, we can use the implementations of cursors in #307 or #313 instead, so I will close this PR. Thanks for getting us started on cursors, though!

@ept ept closed this Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants