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

Punching through uniffi to expose the cursor API #69

Closed
wants to merge 2 commits into from

Conversation

munhitsu
Copy link
Contributor

exposing the Cursor API

@alexjg
Copy link
Collaborator

alexjg commented Aug 10, 2023

Nice! The code LGTM. We may want a little more context in the docs about when one might need a cursor etc, @heckj thoughts?

Copy link
Collaborator

@heckj heckj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor fiddly bit on the how the test is constructed, but otherwise the content looks good. This PR is aimed at that alpha branch though, but anything incoming should be aimed at the main branch, and after it's landed, I can cherry pick the relevant commit back in the alpha branch and cut a new XCFramework and pre-release 0.5.x version.

I'm not fussed about the relative lack of documentation - I'm right in the middle of a big rebuild of the documentation, so it will be easy for me to integrate this in after we land the PR.

@@ -27,6 +27,36 @@ class TextTestCase: XCTestCase {
XCTAssertEqual(try! doc.textAt(obj: text, heads: heads), "hello world!")
}

func testTextCursor() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know a lot of the legacy tests in this repo use the force unwrapping of try statements (try!), but as a general pattern its far easier to diagnose test errors if the test itself is throwing, and then you don't need to wrap the try with !.

@heckj
Copy link
Collaborator

heckj commented Aug 10, 2023

Question about Cursor - I get how it works based on the tests provided, but wanted to verify a few details:

  • Cursor works for both .Text objects and .List objects - it just happens to be more obviously useful for Text cursor tracking
  • Cursor also updates its position if/when you invoke "insert" to add an item to a list
  • There is no "default" cursor that you can retrieve from either Text or List objects - you have to manually set it at some point in time, and it updates with the changes from there
  • If a cursor is near the end of a list, and you spliceText/delete the last word, including where the cursor previously pointed, the cursor position updates to the UTF-8 index end of the text

And a question - as much for @munhitsu as @alexjg - right now we leave the whole tracking of a UTF-8view of a Swift String to the developer to manage the positioning in and out, ignoring the Character indexes (which point to full glyphs, not UTF8 index positions). Would it make sense to have an initializer on Cursor, referencing the Character Index within the String of the .Text object, that we automagically convert to a UTF-8 position when initializing the cursor to better developer ergonomics?

@alexjg
Copy link
Collaborator

alexjg commented Aug 10, 2023

Correct on all four points. The idea of a "default" cursor is interesting, that would be the start of the sequence?

As to the character index stuff, that makes a lot of sense. Can it be done efficiently? Do you not need to track the state of the "current" version of the text in order to translate to UTF-8 indexes?

@heckj
Copy link
Collaborator

heckj commented Aug 10, 2023

The idea of a "default" cursor is interesting, that would be the start of the sequence?

That was sort of the idea that I was starting with, and maybe exposing Cursor as an additional property on the higher-level AutomergeText object, initializing it to index 0 as a starting point. That said, I'm not sure if that makes sense in the scenario where you've created a .Text object, but not added any data into it yet. The cursor would, at that point, be at an invalid position as far as I could see. (Haven't tried anything experimental yet, just thinking through it)

As to the character index stuff, that makes a lot of sense. Can it be done efficiently? Do you not need to track the state of the "current" version of the text in order to translate to UTF-8 indexes?

My biggest concern is the whole "one glyph being multiple UTF-8 code points" scenario with String and UTF-8 positions. I think it makes a HUGE amount of sense to let the cursor shift with Automerge's updates, but when initializing a cursor from a Swift string value, using the character index from the current-value of string to convert into the UTF-8 view to get that appropriate index. If I recall correctly, there's a conversion method within String that lets you convert String.Index to String.UTF8View.Index pretty easily.

@heckj
Copy link
Collaborator

heckj commented Aug 14, 2023

@munhitsu I'm closing this PR and replacing it with #70, which targets the main branch. I cherry picked your commits over to that branch, then added some additional documentation.

@heckj heckj closed this Aug 14, 2023
@heckj heckj mentioned this pull request Aug 14, 2023
@munhitsu
Copy link
Contributor Author

If ever you want to get back to the idea of having the default cursor then bear in mind that UITextField thinks in terms of NSRange. This means 2 cursors.
Using the Cursor in anger made me also realise that spliceText would benefit from having variant with cursor as an argument, but this work should start from rust implementation...

Thank you for the merge!

@heckj
Copy link
Collaborator

heckj commented Aug 15, 2023

I think I'll snag that into a future enhancement issue, thanks @munhitsu

@heckj heckj mentioned this pull request Aug 15, 2023
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.

None yet

3 participants