-
Notifications
You must be signed in to change notification settings - Fork 54
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
Comments
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 |
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 |
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 |
IMO, an |
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. |
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
tou64
. 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
toNonZeroU128
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 ofOption<NodeId>
in Rust. But with the node structure refactor that we introduced earlier this year, we no longer need to store manyOption<NodeId>
fields. So now I'm OK with using a plainu64
.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 au32
, 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
The text was updated successfully, but these errors were encountered: