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

RFC: Switch NodeId from NonZeroU128 to u64 #275

Closed
mwcampbell opened this issue Aug 17, 2023 · 5 comments · Fixed by #276
Closed

RFC: Switch NodeId from NonZeroU128 to u64 #275

mwcampbell opened this issue Aug 17, 2023 · 5 comments · Fixed by #276

Comments

@mwcampbell
Copy link
Contributor

This is currently a request for comments; I haven't made a final decision to do this. But I'm opening it as an issue because, if we move forward with it, it will be actionable. And I want to make sure this gets resolved one way or the other.

I'm thinking of switching the underlying type for AccessKit node IDs from NonZeroU128 to u64. The primary reason is that most type systems, for both programming languages and statically typed serialization formats, don't have a native type for an unsigned 128-bit integer. The need to have a struct for a node ID is one thing that makes the C API cumbersome, and the same concern applies to the work-in-progress Java API.

The original reason for switching from NonZeroU64 to NonZeroU128 last year was to allow a node ID to be constructed from an unsigned 64-bit integer that might be zero, such as a Rust hash code, without any risk of either losing data or breaking the "non-zero" requirement. This is an important concern, which is why I didn't hesitate to make the switch. But I've since reconsidered whether we really need to require that the node ID be non-zero. The original reason for that requirement was to optimize the size of Option<NodeId> in Rust. But with the node structure refactor that we introduced earlier this year, we no longer need to store many Option<NodeId> fields. So now I'm OK with using a plain u64.

My only concern at this point is that this change will interfere with the way slint's AccessKit integration currently composes a node ID from a component ID plus an index of an item within the component. The item index is currently a usize, which is 64-bit on modern general-purpose machines, meaning the node ID has to be more than 64-bit. One option would be to specify that the item index must be a u32, as it already is on some machines that slint targets (specifically, microcontrollers). Another solution would be for slint's AccessKit integration to maintain a complete mapping between items and node IDs. A final option I'm considering is some kind of subtree support, where each subtree has a node ID within the parent tree, and each subtree has its own node ID space. The problem that immediately comes to mind with that solution is that, with the current schema, there wouldn't be a way for a node in one subtree to refer to a node in a different subtree, such as when a control and its label are in different subtrees.

What we have right now works, but it's arguably a wart in the current design, particularly when considering FFI and serialization. We need to address all known warts as we move toward stabilization.

cc @raphlinus @wtholliday @tronical

@DataTriny
Copy link
Member

For reference, #99 was about storing UUIDs. I know that GTK4 use these, but I don't know how prevalent it is in the GUI toolkit space.

Also worth noting that we will be forced to use i32 on Android.

@tronical
Copy link

I think u64 is fine for Slint. More than 2^32 components and 2^32 items seems unlikely to me. The earlier encoding would have required a u128 but your solution fixed that. Cc @ogoffart

@wtholliday
Copy link
Contributor

IIRC in #99, I was concerned about being able to easily express IDs using hashes of paths down the view tree. I think @raphlinus advised me to use something more than a u64 to prevent collisions. I suppose I could always maintain a hash table from the bigger IDs to (allocated) AccessKit IDs.

If Android forces i32, maybe it should be i32 as a lowest common denominator?

@mwcampbell
Copy link
Contributor Author

IMO, an i32 is too restrictive. The Android platform adapter can maintain a mapping between AccessKit node IDs and Android-specific IDs. But we shouldn't require all other platforms to pay for that constraint.

@raphlinus
Copy link

I'm in favor of moving this to u64. My advice to @wtholliday was not to rely on 64-bit hashes to enforce uniqueness, as the birthday paradox gets you a nontrivial probability of collision, especially as the app scales up. That said, speaking bluntly, hashes of paths is a somewhat oddball choice for a UI toolkit (I am unaware of any other than rui that use this), so it seems reasonable to me to require the client to do its own mapping.

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 a pull request may close this issue.

5 participants