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

feat: Identify modifiers in Wayland/X11 #4629

Closed
wants to merge 19 commits into from

Conversation

gabyx
Copy link
Contributor

@gabyx gabyx commented Nov 30, 2023

Fixes: #4626

  • This implementation is the first modifier detection client-side workaround as Kitty does for Wayland.
  • For X11 the code checks the modifier table and tries to match a single virtual modifiers to a single real modifier (ctrl, shift, capslock, mod1...mod5).

The solution is encapsulated out in file modifiers.rs for both X11 and Wayland.

  • I am sure about how to test this, since it uses xkb and its state...
  • @wez: A review is appreciated to let me know if we should go on with this, or there is any better idea from your side.
    This PR contains the port of Kittys C code for Wayland.

@wez wez marked this pull request as draft December 1, 2023 14:29
@gabyx gabyx force-pushed the feature/identify-modifiers-wayland branch 2 times, most recently from 49f2879 to a758a9b Compare December 1, 2023 19:02
@gabyx gabyx marked this pull request as ready for review December 1, 2023 19:02
@gabyx gabyx changed the title feat: identify modifiers wayland feat: Identify modifiers wayland Dec 1, 2023
@gabyx gabyx changed the title feat: Identify modifiers wayland feat: Identify modifiers in Wayland Dec 1, 2023
Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

At a high level, I think it is reasonable to do this kind of probe to reason about the keymap. We do something similar to detect dead keys in the Windows implementation.

I can't follow the code well enough to really understand it, and I don't understand why Wayland needs to be treated differently from X11 when the data model for both is the same keyboard map and state.

Most of my comments are suggestions for making it easier to follow and may not be the most constructive wrt. the higher level goal.

window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
wezterm-input-types/src/lib.rs Outdated Show resolved Hide resolved
wezterm-input-types/src/lib.rs Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
window/src/os/x11/modifiers.rs Outdated Show resolved Hide resolved
@gabyx
Copy link
Contributor Author

gabyx commented Dec 2, 2023

@wez: Thanks! for helping out on this: input really appreciated, also my compile switch is still wrong in the code (the code needs to switch in the ctor of the keyboard to select the appropriate impl for the modifier detection)
as far I could grasp, the wayland key press probing I understood, but the X11 analogous and no-workaround code is obviously not possible in Wayland (the reason I need to search again for, I guess its due to the calls which are not all in the xkb:: namespace and more in the x11 namespace. Once I implemented the X11 approach in the next days I probably understand more, why this is maybe not callable on wayland, I am on wayland and can test it.

I moved the code to modifiers.ra for the reason that its not that easy to understand, and that we can simply drop it or exchange it, or use the initi_modifiers_fallback which is the current thing wezterm does.
Also I try to not overdocument it but add some more key comments what we try to achieve. I thought it might be a good idea to keep as close to the kitty code as possible such that we can still look at it in the future and reason about it (esoecially the wayland probing)

@gabyx
Copy link
Contributor Author

gabyx commented Dec 2, 2023

@wez: the PR is far from finished, once X11 is implemented it needs documentation for the user what to consider about modifiers on wayland etc

@gabyx gabyx marked this pull request as draft December 2, 2023 18:36
@gabyx gabyx marked this pull request as ready for review December 2, 2023 21:19
@gabyx gabyx mentioned this pull request Dec 4, 2023
@gabyx gabyx changed the title feat: Identify modifiers in Wayland feat: Identify modifiers in Wayland/X11 Dec 4, 2023
@gabyx
Copy link
Contributor Author

gabyx commented Dec 4, 2023

@wez: The implementation for X11 is finished, last commit.. I think this code is better to understand and also gives some reason why it does not work on Wayland, xlib calls I think.

I added scopeguard as a new dependency as I was not sure what you use for that purpose. I needed a proper defer statement. Not sure maybe there is already the same in your library then I can remove it or maybe do it differently. I didnt find something related to defer or scope/guard...

I have not answered the Windows and MacOs question about the Led stuff i removed. I still have to investigate this.

Implementation wise I think everything is now there similar to what kitty does, which is probably a good thing.
It would be nice if somebody could test the startup on X11. I might be able to pull it off by just switching on NixOS.

Thanks for the review.

I think I still need to add some documentation to highlight what is done on startup for X11 and Wayland such that users understand. Do you have a suggestion where I could add this?

@gabyx gabyx force-pushed the feature/identify-modifiers-wayland branch from 8c5a6d2 to fcd45b7 Compare December 7, 2023 14:31
@gabyx gabyx closed this Dec 7, 2023
@gabyx
Copy link
Contributor Author

gabyx commented Dec 7, 2023

@wez Could you please reopen the PR. Github had some troubles not updating the commits ?? I tried to close and reopen it. BR.

@wez
Copy link
Owner

wez commented Dec 7, 2023

It won't let me; I think you'll just have make a new PR from that branch

image

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

Successfully merging this pull request may close these issues.

fix: Implement modifier detection on wayland/x11
2 participants