-
Notifications
You must be signed in to change notification settings - Fork 940
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
api: add DeviceId::{into,from}_raw
#3941
Conversation
Whats the reason for introducing the methods? I would prefer keeping it opaque for the same reasons discussed in #3795. |
@daxpedda we need a way to build those types when backends will be split. |
08ebda5
to
293736b
Compare
I can make those functions |
No, we can have Winit introduce its own Generally speaking I'm always leaning towards making compromises internally rather then on the public API, which is to me the most important part of the whole project. I'm in favor of keeping them private as long as the types are still in the |
I mean, if the core types are in Like with a layout like // winit-core
pub trait DeviceEventHandler: ApplicationHandler {
fn device_event(&mut self, device_id: Option<DeviceId>, event: Event);
}
// winit-wayland
{
// Should be `DeviceId`, but no way to build?
state.device_event(device_id???, event)
} It's true that if you re-export everything possible inside the Unless you just do |
293736b
to
ca3162f
Compare
The same as for `WindowId`.
ca3162f
to
63f4e84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that if you re-export everything possible inside the
winit
and wrap everything, you can probably avoid certain things, but however you can not create traits with types like that, or at least there still should be a type that can be build from e.g.u64
.Unless you just do
dyn
, butdyn
forId
doesn't make any sense.
Yeah, makes sense.
Only reviewed the public API and the Web backend, LGTM!
I've removed public API for now, so should be less concerning. |
The same as for
WindowId
.This also removes the internal platform ID.
--
I'm not sure what type to pick here, since some use u32, some i32. I think
i64
would be the best, but then, negative values for DeviceId are strange?Also, it's not that clear to what IDs should be limited.
For now I'm just using
u64
.