Skip to content

Replace WindowId From/Into u64 with WindowId::into_raw() and from_raw()#3795

Merged
kchibisov merged 6 commits intorust-windowing:masterfrom
daxpedda:window-id-u64
Sep 27, 2024
Merged

Replace WindowId From/Into u64 with WindowId::into_raw() and from_raw()#3795
kchibisov merged 6 commits intorust-windowing:masterfrom
daxpedda:window-id-u64

Conversation

@daxpedda
Copy link
Copy Markdown
Member

@daxpedda daxpedda commented Jul 17, 2024

As there shouldn't really be a need for a user-constructible WindowId, I assume this was used previously because WindowId was passed as a parameter?

I introduced WindowId::to_u64() as a replacement, but I'm not sure if this has any use case.
After the last meeting we decided to add conversion methods called from_raw() and into_raw().

Copy link
Copy Markdown
Contributor

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I think having an easy converstion to/from u64 is nice because it lets you easily store the u64 in FFI.

Copy link
Copy Markdown
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Both are used in alarcitty's IPC.

@daxpedda
Copy link
Copy Markdown
Member Author

@kchibisov @notgull would you mind elaborating on those things?
I have no clue about why Alacritty needs this for IPC or what storing in FFI means exactly.

@kchibisov
Copy link
Copy Markdown
Member

We set a WINDOW_ID in the env variable and then user can use this WINDOW_ID of some window to target it from other window with IPC command (alacritty msg config --window-id WINDOW_ID font.size=10, the point here is that WindowId should be built and passed to HashMap<WindowId, WindowContext> when parsing it from the json payload on the socket.

If you remove the from we probably can use u64 instead(not really nice, since you lose type safety), but we can not do anything without a to for example.

@daxpedda
Copy link
Copy Markdown
Member Author

daxpedda commented Jul 17, 2024

We set a WINDOW_ID in the env variable and then user can use this WINDOW_ID of some window to target it from other window with IPC command (alacritty msg config --window-id WINDOW_ID font.size=10, the point here is that WindowId should be built and passed to HashMap<WindowId, WindowContext> when parsing it from the json payload on the socket.

If you remove the from we probably can use u64 instead(not really nice, since you lose type safety), but we can not do anything without a to for example.

Personally I would consider this still as a good tradeoff.
The to_u64() method makes absolute sense to me, but if we allow arbitrary construction from a u64 we just throw type safety out the window (no pun intended). If WindowId is supposed to represent something, we should make sure it does.
These are just my 2¢, so its all very subjective.

But if this is too contentious, which I would totally understand, I would suggest to at least move away from the From implementations to dedicated methods. Same reasons as above: to make it clear to the user that this is an explicit conversion that is not natural; it doesn't produce necessarily valid WindowIds.

Just to clarify: this PR is not important, if we can't come to a happy consensus we should go ahead and close it.

@kchibisov
Copy link
Copy Markdown
Member

Given that WindowId is not tied to the window's lifetime it's not really type safe in any form, so limiting won't provide much benefit here and given that we don't accept WindowId in any form I probably won't change that for now.

On the other side of things I was thinking to make WindowId opaque with a method to get some kind of id() and use the rest to encode some metadata, etc.

@daxpedda
Copy link
Copy Markdown
Member Author

Given that WindowId is not tied to the window's lifetime it's not really type safe in any form, ...

Its type safe in the sense that you know a WindowId is actually a window ID. Otherwise we could play around with u64s directly as well.

@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Jul 19, 2024
@nicoburns
Copy link
Copy Markdown
Contributor

Constructing WindowId will be needed for 3rd party backend implementations.

@daxpedda
Copy link
Copy Markdown
Member Author

Constructing WindowId will be needed for 3rd party backend implementations.

This is a separate matter that we need to address globally.
We need all types to be constructible for any backend to work, not just 3rd party, because all backends will be in their own crate. But we don't want direct winit users to see the internals or constructors of all these types.

@nicoburns
Copy link
Copy Markdown
Contributor

Another use case for this would be to store the ID as an opaque type somewhere that doesn't otherwise need to depend on Winit. With the functionality I can pass my library code a u64 and get it to pass it back to me when it generates events (e.g. enable IME). It never needs to know that it's a Winit ID, and if I want to integrate it with another system (not winit) then I can.

One pattern I've seen for this that I quite liked is from the slotmap crate which calls these methods as_ffi and from_ffi which really makes it clear that creating the newtype from u64 is only supported if the u64 was created from the newtype in the first place (https://docs.rs/slotmap/latest/slotmap/struct.KeyData.html#method.as_ffi) without actually removing any functionality.

@daxpedda
Copy link
Copy Markdown
Member Author

One pattern I've seen for this that I quite liked is from the slotmap crate which calls these methods as_ffi and from_ffi which really makes it clear that creating the newtype from u64 is only supported if the u64 was created from the newtype in the first place (https://docs.rs/slotmap/latest/slotmap/struct.KeyData.html#method.as_ffi) without actually removing any functionality.

We discussed this in the meeting and are planning to go down exactly this route!

@daxpedda daxpedda removed the C - nominated Nominated for discussion in the next meeting label Aug 13, 2024
@daxpedda daxpedda requested review from kchibisov and notgull August 13, 2024 20:59
@daxpedda daxpedda requested a review from kchibisov August 13, 2024 22:12
@daxpedda daxpedda changed the title Remove WindowId conversions to/from u64 Replace WindowId From/Into u64 with WindowId::into_raw() and from_raw() Aug 13, 2024
@daxpedda daxpedda added the S - api Design and usability label Aug 13, 2024
Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

from_ffi / to_ffi

To my eyes, this looks a bit like these can be used in system-level FFI, i.e. as HWND on Windows and *const NSWindow on macOS (which is not the intention). I think from_raw / into_raw are better.

@kchibisov kchibisov added the C - nominated Nominated for discussion in the next meeting label Aug 23, 2024
@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Sep 8, 2024

@notgull I think we can merge this now? Your review is still marked as "requested changes", but I think your concern has been resolved?

@kchibisov kchibisov dismissed notgull’s stale review September 22, 2024 14:05

resolved, since it's a name change mostly.

@kchibisov kchibisov merged commit 6e1b9fa into rust-windowing:master Sep 27, 2024
@madsmtm madsmtm removed the C - nominated Nominated for discussion in the next meeting label Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S - api Design and usability

Development

Successfully merging this pull request may close these issues.

5 participants