-
Notifications
You must be signed in to change notification settings - Fork 933
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
Remove dummy()
from WindowId
and DeviceId
and use Option<DeviceId>
#3864
Conversation
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.
I think dummy()
on WindowId
was mostly useful back when we had Event::WindowEvent(WindowId, WindowEvent)
, and users wanted to create Event
for testing.
pub(crate) use crate::icon::NoIcon as PlatformIcon; | ||
pub(crate) use crate::platform_impl::Fullscreen; | ||
|
||
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
pub struct DeviceId; | ||
|
||
impl DeviceId { | ||
#[cfg(test)] | ||
pub const fn dummy() -> Self { |
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.
Still need to remove this and other fn dummy()
.
Or do we use it for testing anywhere that I don't know about?
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.
We have some tests that check if things don't panic:
Lines 1166 to 1173 in 7fbc211
let did = crate::event::DeviceId::dummy().clone(); | |
let fid = crate::event::FingerId::dummy().clone(); | |
HashSet::new().insert(did); | |
let mut set = [did, did, did]; | |
set.sort_unstable(); | |
let mut set2 = BTreeSet::new(); | |
set2.insert(did); | |
set2.insert(did); |
But it will be removed eventually when we split the backends out.
I'm happy to remove it now?
Eventually integration tests should check this sort of thing without requiring a dummy implementation for us: #2866.
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.
I don't see that much value in that test compared to the many extra functions that we need to make it work - but I don't have a strong preference either way here.
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.
I feel the same and would be in favor of removing it.
Unless I hear disagreement I'm gonna go ahead with that when the PR is ready.
3ac49d9
to
71faa98
Compare
`::dummy` was used for testing purposes and became redundant after adding e.g. `from_raw` and `into_raw` methods on `Id` types.
07f161b
to
680c52a
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.
Brought it up to date, should be good now.
This was discussed in the last meeting but it was so late only @kchibisov and myself were around so I'm gonna re-iterate what we discussed here.
WindowId
,DeviceId
and the newly addedFingerId
, have only one purpose: making sure that users are able to differentiate between events coming from different devices.dummy()
however entirely circumvents this property, because it is always equal itself. Ideallydummy()
would never be equal itself, but that's not possible while we want to implementHash
(unless we add a counter ...).This is in a similar vein to #3795.
However, this assumption was made under the fact hat adding
Option<DeviceId>
was not necessary because only Web is usingdummy()
internally. Turned out this isn't exactly true: every backend has at least some events where it is unable to emit a uniqueDeviceId
for a device. Some backends, like Wayland and Orbital, don't use anyDeviceId
at all, making all devices appear equal!So with that in mind, I also replaced all
DeviceId
s withOption<DeviceId>
.Surprisingly enough this wasn't necessary for
FingerId
because every backend that implements touch emits a proper finger ID.Based on #3795.