-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Revise MouseButton type #4324
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
base: master
Are you sure you want to change the base?
Revise MouseButton type #4324
Conversation
Prior art: the |
This is not really true if we're speaking from the point of Wayland protocol. https://wayland.app/protocols/wayland#wl_pointer:event:button:arg:button . I'd not tie here to kernel, but rather what written in the spec. Honestly speaking, I feel that the proposed design is better, since it tends to solve the issue of adding a button, assuming that we don't mess around with undefined button values. |
Quoting the Wayland protocol docs:
I don't feel that this is very useful: The existing approach in winit (map both pairs of buttons to back/forward) may be the most compatible but it also prevents other usages of these buttons.
That looks like it was written for a state-map and based on Windows button naming? We could use a 32-variant enum, but I'd like (1) to use Another approach I would consider is to report both a named enumeration (the existing Or, possibly, drop the last two commits from this PR: in this case My expectations on the API are essentially:
|
The thing that should be preserved IMO, ability to bind unknown codes by e.g. let user just press the button. So you can not really recognize, but you can kind of identify, so we should just never throw things away and be able to distinguish them. Which is still the thing with your patch from what I can see, so it's fine. Just to make it clear, is the point to kind of normalize buttons so they can be kind of used in cross platform manner, or at least make them more likely to be usable? Like it certainly can not work from(since you can not really guarantee that people on every platform will do things exactly the same). |
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.
ability to bind unknown codes
From what I could tell most of these "unknown codes" should never be encountered in practice, though it's hard to rule out that ever happening.
is the point to kind of normalize buttons so they can be kind of used in cross platform manner
This + more precisely specify which button codes are likely to be seen in practice. I.e. it is now almost guaranteed that any observable mouse button code will fit in a u8
and probably be <32.
winit-core/src/event.rs
Outdated
ButtonSource::Mouse(mouse) => mouse, | ||
ButtonSource::Touch { .. } => MouseButton::Left, | ||
ButtonSource::Unknown(button) => match button { | ||
0 => MouseButton::Left, | ||
1 => MouseButton::Middle, | ||
2 => MouseButton::Right, | ||
3 => MouseButton::Back, | ||
4 => MouseButton::Forward, | ||
_ => MouseButton::Other(button), | ||
}, | ||
ButtonSource::Touch { .. } => MouseButton::LEFT, | ||
ButtonSource::Unknown(_) => MouseButton::UNKNOWN, |
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.
The old mapping of Unknown
codes made no sense since values were entirely unspecified.
However, there were several things mapping to Unknown(0)
; that might need changing.
match button.0 { | ||
0 => MB::LEFT.into(), | ||
1 => MB::MIDDLE.into(), | ||
2 => MB::RIGHT.into(), | ||
3 => MB::BACK.into(), | ||
4 => MB::FORWARD.into(), | ||
// Codes above 4 are not observed on Firefox or Chromium. 5 is defined as an eraser, | ||
// which is not a mouse button. No other codes are defined. | ||
i => ButtonSource::Unknown(i), | ||
} |
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.
The "eraser" button could map to ButtonSource::Unknown
on web, except that this code path shouldn't be used for non-mouse input. In my testing (using web forms not winit) I never saw any other codes output. So I don't think web platforms can produce any Unknown
output codes in practice, though that could conceivably change in the future.
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.
eraser button is a tablet
button, so it shouldn't be here in the first place, I think?
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 part of "the button property" spec:
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.
But it says pen eraser button
, so I'd assume that it's pen and will go there?
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.
Yes, so I don't think it would occur here.
winit-win32/src/event_loop.rs
Outdated
// 1 is defined as back, 2 as forward; other codes are unexpected. | ||
button: MouseButton(xbutton as u8 + MouseButton::BACK.0 - 1).into(), |
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 well documented exactly which codes are produced here, and my testing agreed: this code should only ever yield MouseButton::BACK
and FORWARD
.
if (BTN_MOUSE..BTN_JOYSTICK).contains(&button) { | ||
// Mapping orders match | ||
MouseButton((button - BTN_MOUSE) as u8).into() | ||
} else { | ||
ButtonSource::Unknown(button as u16) | ||
} |
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.
This goes beyond the Wayland specification. I haven't seen any evidence that codes outside this range would be produced from mouse input, but maybe if a mouse has more than 16 buttons? Maybe there is some way buttons on a mouse could be mapped to other codes?
If anyone does have evidence that Wayland can produce codes outside this range (0x110..=0x11F
) I'd like to see it.
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 mean, it's possible, I guess, but not right now. Like we can use u16
for button just in case, since it doesn't really matter, I guess, just so we don't have a breaking change in the future if the problem appears?
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 put this in ButtonSource::Unknown
instead of within MouseButton
for a reason. Allowing the latter to contain undocumented u16
codes defeats that reason.
I would prefer to revisit this when we have evidence of such values, then map those to values within MouseButton
. I view changes to Unknown
codes as non-breaking (well, maybe not appropriate for patch releases), while changes to MouseButton
codes would be breaking changes.
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 mean, we can generally just map to u8
one away or another anyway, so don't think it'll be a problem tbf.
We discussed this at our meeting yesterday, and while we can see the technical point that there might be unknown mouse buttons, it is unclear that this is actually a problem in practice? Are there variants that we'd want to expose, that we don't expose yet? The unfortunate effect of this is that it makes things like rust-analyzer less able to fill out variants when matching on the mouse button; structs with consts are generally less supported than true enums. If this were not the case, such as if RFC 3803 was implemented, then this would definitely be a no-brainer. (That said, there are a few cleanups in this PR that would be worthwhile to land anyhow). |
Whether the buttons should be named or just numbered I don't know, but I'd like to see support for at least a dozen buttons; maybe 32 like MacOS.
Personally I'd like to be able to pull out a small enum or number. E.g.: enum Button {
Primary,
Secondary,
Middle,
Back,
Forward,
Button6,
// ...
Button31,
} would be fine. I don't like the existing
I'll try to find time this week. Still not 100% sure of the direction; should I use an enum like |
I would prefer the 32-variant enum you propose, yes. |
Just in addition, we can also number the enum fields, like |
…own buttons This may change codes converted to ButtonSource::Unknown and ButtonId.
8f13667
to
cffd5a0
Compare
cffd5a0
to
0f3f8c2
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.
Rebased and updated.
pub fn mouse_button(self) -> MouseButton { | ||
pub fn mouse_button(self) -> Option<MouseButton> { |
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.
ButtonSource::mouse_button
now returns None
on ButtonSource::Unknown
(no other reasonable option).
impl MouseButton { | ||
/// Construct from a `u8` if within the range `0..=31` | ||
pub fn try_from_u8(b: u8) -> Option<MouseButton> { |
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.
An alternative would be to use the num_enum
crate, I guess. I didn't use TryFrom
since that returns a Result
.
changelog
module if knowledge of this change could be valuable to usersCreated or updated an example program if it would help users understand this functionalityIt turns out that at least Wayland, X11 and macOS support more than 5 mouse buttons. Previously, these all used incompatible
Mouse::Other(_)
codes, whileMouse::Other
was also used for potential errors.Now,
MouseButton
is a wrapper aroundu8
with portable button mappings, modelled on AppKit codes.Tested:
BTN_SIDE
andBTN_EXTRA
for back/forward, notBTN_BACK
/BTN_FORWARD
. My mice report up to button 10 (code 0x1A0); since the next named code isBTN_JOYSTICK = 0x120
I hypothesize that 16 buttons are usable.Alternative
We could keep using an enum and name more buttons, but how many would be named? Note that while we can now report consistent numbers for more than 5 buttons across platforms, these buttons don't have consistent names.
Further work
ButtonId
could obviously be aMouseButton
in some cases, aButtonSource::Unknown(_)
in others. But I don't want to replace it withButtonSource
since that has a lot of extra data in theTouch
variant.