X.{A.{{Grid,Tree}Select,Submap},Prompt}: KeyPress handling fixes #686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We didn't clean XKB group bits out of the KeyPress events' state so key bindings only worked in the primary keyboard layout (first XKB group). To fix this, this adds a `cleanKeyMask` function to X.Prelude which is analogous to `cleanMask` but aimed at cleaning regular KeyPress states (as opposed to just KeyPresses from passive key grabs), and this is then used instead of `cleanMask`. Related: xmonad#290 Related: xmonad#590
This replaces the custom `cleanMask` extension in these modules—which only filtered out XKB group bits and Button5Mask¹—with the new `cleanKeyMask` which additionally filters out all mouse buttons, as these aren't relevant for key bindings. ¹) Filtering out Button5Mask was probably an off-by-one mistake. Fixes: xmonad#290 Related: xmonad#590
…imilar This changes KeyPress handling in these modules to behave much closer to how xmonad core itself handles keypresses. The primary difference lies in that xmonad reads raw KeyCode and then converts it to unmodified KeySym, while these modules used `lookupString` to find the actual keysyms. As a consequence, key definitions like `(shiftMap, xK_Tab)` didn't work on many layouts because an actual KeySym for `Shift-Tab` is commonly `ISO_LEFT_TAB`, and not `Tab`. Closes: xmonad#590 Co-authored-by: Tomas Janousek <[email protected]>
This was referenced Feb 10, 2022
slotThe
reviewed
Feb 11, 2022
There was a problem hiding this comment.
Compiles and doesn't seem to break the prompt for me and the code also looks reasonable, so LGTM!
One thing to consider: if we are concerned that unconditionally stripping XKB and mouse state might break someone's workflow, we'll need to make this configurable, and then we'll need to move
cleanKeyMaskout of X.Prelude as we can't import X.U.ExtensibleConf.
Reminds me a bit of this—probably not going to be a problem :)
liskin
force-pushed
the
keypress-fixes
branch
from
bd637ba
to
1b728ff
Compare
|
(merged without the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a cleaned up version of #590 which additionally addresses #290 (I believe nobody really depends on the XKB and mouse state not being stripped so it's okay to change behaviour) and xmonad/xmonad#172 (this needs xmonad/xmonad#374).
Original description from #590 follows:
One thing to consider: if we are concerned that unconditionally stripping XKB and mouse state might break someone's workflow, we'll need to make this configurable, and then we'll need to move
cleanKeyMaskout of X.Prelude as we can't import X.U.ExtensibleConf.Checklist
I've read CONTRIBUTING.md
I've considered how to best test these changes (property, unit, manually, ...) and concluded: I manually tested TreeSelect and Prompt and haven't found any breakage.
I updated the
CHANGES.mdfile