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.

Sign up for GitHub

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

Jump to bottom

Use xkeysym for keyboard handling #378

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Use xkeysym for keyboard handling #378

merged 4 commits into from
Sep 11, 2023

Conversation

notgull
Copy link
Member

@notgull notgull commented Apr 24, 2023

Replaces the keysyms module with xkeysym and makes APIs take xkeysym::Keysym instead of alternatives. See rust-windowing/xkeysym#6

@notgull notgull mentioned this pull request Apr 24, 2023
Open
@notgull notgull marked this pull request as draft April 24, 2023 16:40
@kchibisov
Copy link
Member

Could you rebase with the current latest version?

kchibisov
kchibisov reviewed Aug 11, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member

And I guess it's not a draft anymore at this point?

@notgull
Copy link
Member Author

notgull commented Aug 11, 2023

And I guess it's not a draft anymore at this point?

Unfortunately I can't take it out of draft status until rust-x-bindings/xkbcommon-rs#43 is released.

@kchibisov
Copy link
Member

@notgull I mean, we have a release we can use, I don't see any issue with using it here given that it's better than hand rolled solution and it's already used in winit, so we have less code size upstream.

@notgull notgull marked this pull request as ready for review August 12, 2023 16:29
kchibisov
kchibisov reviewed Aug 13, 2023
View reviewed changes
CHANGELOG.md Outdated
@@ -29,6 +29,7 @@
#### Breaking Changes

- `wayland-rs` dependencies are updated to 0.30 and all APIs have changed significantly as a result
- `xkeysym::Keysym` is used as a keyboard key representation instead of `u32`
Copy link
Member

Choose a reason for hiding this comment

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

You should put it in the right place.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a breaking change, I think?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't said that it's not, it just for 0.17 which was released months ago. You should move to the actual unreleased section.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, fixed!

PolyMeilex
PolyMeilex approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@PolyMeilex PolyMeilex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great to me.

EDIT:
Looks like it's not possible to use those with match statements? 🙁

I suppose it's not a big deal as one can just drop down to raw ones for that.

@PolyMeilex PolyMeilex merged commit a85f977 into Smithay:master Sep 11, 2023
@PolyMeilex
Copy link
Member

Thanks!

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

@kchibisov kchibisov kchibisov left review comments

@wash2 wash2 wash2 approved these changes

@PolyMeilex PolyMeilex PolyMeilex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@notgull @kchibisov @PolyMeilex @wash2