Skip to content
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

Add MouseButton::{Back, Forward} to mouse input events on Wayland, X11, Windows, macOS and Web #2770

Merged
merged 9 commits into from Jun 16, 2023

Conversation

bbb651
Copy link
Contributor

@bbb651 bbb651 commented Apr 14, 2023

Fixes #1785. This is a breaking change because MouseButton isn't #[non_exhaustive],
it's supported on wayland, x11, windows, macos and web, all other platforms (orbital doesn't support these buttons at all, android and ios don't have mouse support in winit).

  • Tested on all platforms changed (currently tested on x11 (xwayland), needs testing on other platforms)
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality (I don't think it's necessary for this kind of change, it's fairly self explanatory)
  • Updated feature matrix, if new features were added or implemented

Copy link
Contributor

@i509VCB i509VCB left a comment

Choose a reason for hiding this comment

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

Tested with xev and the values for X11 appear to be correct.

src/platform_impl/linux/wayland/seat/pointer/handlers.rs Outdated Show resolved Hide resolved
Copy link
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.

macOS impl looks simple enough, though I can't test it since I don't have a mouse with those buttons.

Maybe you could document the variants a bit more to describe what "forward" and "backward" mouse buttons actually mean (or link to a Wikipedia article or something), I suspect I'm not the only one that don't know how those actually look

CHANGELOG.md Outdated Show resolved Hide resolved
@bbb651
Copy link
Contributor Author

bbb651 commented Apr 16, 2023

macOS impl looks simple enough, though I can't test it since I don't have a mouse with those buttons.

Maybe you could document the variants a bit more to describe what "forward" and "backward" mouse buttons actually mean (or link to a Wikipedia article or something), I suspect I'm not the only one that don't know how those actually look

Looking more into it I think macOS doesn't officially support these buttons, I got them from this project which looking at it now it's a hack specifically to address this shortcoming.

I found this documentation page about possible mouse buttons:

Quartz supports up to 32 mouse buttons. The first three buttons are specified using these three constants. Additional buttons are specified in USB order using the integers 3 to 31.

The question now is if treating 3 and 4 as back and forward is a common enough convention that we should adopt it. After searching for way too long I found that at least chromium does it so it seems pretty sensible to support it. But yeah it definitely needs a comment explaining this and documentation for users that it's not guaranteed to work across all configurations. I will fix this and the changelog tomorrow.

Copy link
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.

let me know if you still want to work on that, otherwise I'll push it myself.

examples/mouse_buttons.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM except the nits.

src/platform_impl/web/web_sys/event.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/event.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Member

daxpedda commented Jun 2, 2023

@bbb651 ping me if you want me to take care of the web backend.

@bbb651
Copy link
Contributor Author

bbb651 commented Jun 2, 2023

@daxpedda I'd appreciate it

Copy link
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.

Seems like your rebase went wrong.

CHANGELOG.md Outdated Show resolved Hide resolved
examples/mouse_buttons.rs Outdated Show resolved Hide resolved
src/platform_impl/linux/wayland/seat/pointer/handlers.rs Outdated Show resolved Hide resolved
src/platform_impl/web/web_sys/event.rs Show resolved Hide resolved
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

@bbb651 Web backend is done.
(only approving the Web backend here)

Copy link
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.

MacOS looks good now

Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Windows changes also fine

This is a breaking change because MouseButton isn't #[non_exhaustive],
it's supported on wayland, x11, windows, macos and web, all other platforms (orbital doesn't support these buttons at all, android and ios don't have mouse support in winit).
@kchibisov kchibisov merged commit 4748890 into rust-windowing:master Jun 16, 2023
55 checks passed
@kchibisov
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Cross platfrom mouse button translation
6 participants