-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Revamp DeviceEvent::Button to be consistent with the pointer event overhaul
#4388
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?
Revamp DeviceEvent::Button to be consistent with the pointer event overhaul
#4388
Conversation
Could you expand on this? What are the differing semantics? We talked about it in our maintainer meeting, I think we'd prefer: DeviceEvent::PointerButton {
button: ButtonSource,
state: ElementState,
}This would also sidestep your issue with |
|
The semantic difference is that My motivation for having |
Maybe these shouldn't emit a |
|
Maybe. I'm coming more from the perspective of |
We have touched on this and I think there is some sort of consensus that this is what it should be. Unfortunately that would require a much bigger revamp of |
a395f2a to
76f8583
Compare
|
I have updated the PR to the suggested API, and |
|
FYI this will clash with #4324. I suggest waiting until that is merged since it was already approved. |
|
Said PR got merged. |
a50daf5 to
ec172d4
Compare
|
Updated the PR |
I have tested the change on Windows, but not on other platforms.
changelogmodule if knowledge of this change could be valuable to usersI revamped
DeviceEvent::Buttonto make it consistent with the overhauled pointer events, and to make it usable without having to implement platform-specific code to recognize the same buttons.The API looks like this:
Initially, I was just going to reuse
ButtonSourcefor the for thebuttonfield instead of creating a newDeviceButtonSourceenum, but I decided to createDeviceButtonSourcebecause theUnknownvariant has different semantics for device button inputs compared to window button inputs.Two considerations:
button_id? The raw input API sends a bitfield containing mouse button states, and there are specific down/up flags for the 5 supported mouse buttons. This means that there aren't any "raw" button identifiers for mouse buttons on Windows. Current version chooses to send a button "index" as the button id, butDeviceButtonSourcemakes that unnecessary. I've opted to always send0as the button id on Windows, since there are no benefits for any users to use the button index whenDeviceButtonSourceexists, it would only make their code less cross-platform compatible for no reason.DeviceButtonSource::Unknownhere makes sense, and if an user wants to specifically detect X11 scroll button inputs, they can handle that themselves using the button id.