Skip to content

Conversation

@TheZipCreator
Copy link
Contributor

@TheZipCreator TheZipCreator commented Oct 2, 2025

This makes it so you can use strings that don't necessarily end with a null terminator. Useful for C libraries that offer that kind of API, or for bindings to languages other than C through the C binding.

Note: I'm not a Rust programmer, so I'm not sure if my implementation is the best (or most correct) one, but it seems to work.

Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

These changes make sense to me.

For consistency, I think we also need to add accesskit_macos_add_focus_forwarder_to_window_class_with_length in the macos module.

We also have nul-terminated strings in accesskit_tree and accesskit_custom_action structs though.

Looking back, I think calling CStr::to_string_lossy was not a great idea as it wouldn't panic if given invalid UTF-8 strings, something that UI toolkit developers may not realize until an end-user finds replacement characters in their UI. I think we should panic instead and add a note in the documentation of every functions taking a string parameter that valid UTF-8 strings are expected. Something easy you can do in another PR if you're interested, otherwise I'll do it myself later.

@DataTriny DataTriny changed the title Allow usage of slice-style strings rather than just null-sentinel strings feat: Allow usage of char slices in addition to null-terminated strings Oct 7, 2025
Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

I didn't expect you to handle accesskit_custom_action as part of this PR, sorry if this wasn't clear. Please revert these changes.

You should add the same notice on the new functions:

Caller is responsible for freeing the memory pointed by `value`.

@DataTriny
Copy link
Member

I don't see the comment here so maybe you figured it out yourself, just in case:

I'm not really sure how accesskit_string_free works with non-null-terminated strings (since it seems to expect a C string to drop). would passing a non-null-terminated string to that function result in UB?

Users must never do that. This is bad for at least two reasons: there would be no way of knowing what would be freed exactly, but it would likely try to mess up the memory adjacent to the slice until a zero byte is encountered. Secondly, the slice would not have been allocated by Rust so there is a chance that it was allocated by a different memory allocator, that could have nasty consequences as well.

@TheZipCreator
Copy link
Contributor Author

TheZipCreator commented Oct 7, 2025

I deleted the comment because I realized the better way to deal with custom_action would just be to allocate a new null-terminated string (custom_action_new already seems to allocate a string if I understand it right, so doing this should be fine). So the other question was no longer relevant.

I can remove accesskit_custom_action_new_with_length, but can I ask why? the implementation of "copy to a null-terminated string" seems fine to me (I can't see a situation in which one would want \x00 in an action description anyway).

@DataTriny
Copy link
Member

DataTriny commented Oct 7, 2025

I'd like you to remove the new constructor because I think the API for accesskit_custom_action should be redesigned to not expose the fields directly, this way we can have two setters for the description, one of which would allow a char slice. But this is a breaking change so I'd prefer to do that in another PR.

@DataTriny
Copy link
Member

Thanks @TheZipCreator for this contribution. I'd like to have this for the next release so I'm doing the final changes myself. Please note that this pull request got closed by GitHub as I had to rebase your main branch. I'll add you as co-author on the other pull request. In the future, please do not open pull requests from the main branch of your fork.

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.

2 participants