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

XMonad.Prompt.cleanMask doesn't filter out enough modifiers #290

Closed
mgsloan opened this issue Jan 23, 2019 · 5 comments
Closed

XMonad.Prompt.cleanMask doesn't filter out enough modifiers #290

mgsloan opened this issue Jan 23, 2019 · 5 comments

Comments

@mgsloan
Copy link
Contributor

mgsloan commented Jan 23, 2019

For keypresses other than typing, XMonad.Prompt looks up actions in a M.Map (KeyMask,KeySym) (XP ()). Some aspects of KeyMask shouldn't be relevant to keyboard entry, and so it is preprocessed with cleanMask , which has the following definition:

cleanMask :: KeyMask -> XP KeyMask
cleanMask msk = do	
  numlock <- gets numlockMask	
  let highMasks = 1 `shiftL` 12 - 1	
  return (complement (numlock .|. lockMask) .&. msk .&. highMasks)

In essence, what this does is allows all modifiers except for numlock and capslock. The problem is there are a lot of other modifiers that may not be relevant to the prompt. For example, if I hold down the left mouse button and press enter, instead of the prompt confirming, a box like □ gets inserted as if I typed it. Holding mod4 and pressing enter does the same.

I have seen a case in the past where typing and pressing special keys types absolute gibberish, I don't recall whether it was unicode or extended ascii, but I suspect it was related to a stuck modifier, and prevented exiting the prompt. Unfortunately, I have no idea what triggered this behavior.

I have a solution on my patched version of xmonad-contrib, but I'm not sure if it'd be good to merge this. Instead I do the following:

cleanMask msk = return (msk .&. (controlMask .|. mod1Mask))

I happen to know that my config only cares about controlMask and mod1Mask, and so this ignores all the other masks and so doesn't succumb to the sticky mask problem.

I propose the following resolution to this issue:

  1. Have cleanMask use a whitelist, probably something like [controlMask, shiftMask, mod1Mask, mod2Mask, mod3Mask, mod4Mask]. So, not quite as restrictive as my list.

  2. Make the function used for cleaning the keymask be part of the XPrompt configuration

What do y'all think?

@geekosaur
Copy link
Contributor

One problem is those high masks represent keyboard layouts, and this will matter to some people.

@mgsloan
Copy link
Contributor Author

mgsloan commented Jan 24, 2019

@geekosaur I see! I figured it must be that way for some decent reason. I think that Map (KeyMask, KeySym) (XP ()) is really not a great way to represent the keymap, though. For example, to me it is reasonable to bind escape and enter regardless of the keymask state. The map would get quite big indeed in order to have every possible keymask.

It's troublesome to resolve these issues while retaining backwards compatibility. I'm thinking of something along the lines of adding a field of type Maybe (KeyMask -> KeySym -> Action) where Action is a sum of various xprompt actions that can be triggered by keypresses.

@geekosaur
Copy link
Contributor

There's a number of places where backward compatibility is annoying and we may be reaching a point where it's time to either fork it or make a new branch that doesn't guarantee compatibility and might become 1.0 at some point (working a bit like ghc or the linux kernel, odd major number is devel).

But in this case, maybe the minimal solution is to add a new field for the mask, defaulting it as it is currently, and let people who need to override. Then add a mask field when we can break compat. Alternately, a new set of actions which includes a negative mask to apply per entry, which can be populated from the existing one plus any additional entries the user specifies?

@mgsloan
Copy link
Contributor Author

mgsloan commented Jan 24, 2019

I'm definitely in favor of a branch that makes breaking changes in spots that really ought to change, and releasing that as a new major version.

I think the field for the mask would need to be a Maybe KeyMask, since the default for the masking relies on accessing numberlockMask from xmonad's XState. I would really like to have a behavior change by default here, though, as most users probably won't realize that they need to override this to fix the problem. I know that I put up with the problem for a very long time before finally digging into it.

What if the config allowed specifying pre-processing, something along the lines of (KeyMask, KeySym) -> XP (KeyMask, KeySym)?

The main case I'd like to avoid is one where you're stuck in the prompt due to some mask. To solve this, the default for this pre-processing function would be the same as the existing cleanMask, but with one behavior change: by default enter / return / escape would have most masks cleared. I know it's a gnarly hack, but I'd be surprised if this breaks anyone's config, and it might really help in cases of stuckness.

liskin added a commit to liskin/xmonad-contrib that referenced this issue Feb 10, 2022
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
liskin added a commit to liskin/xmonad that referenced this issue Feb 10, 2022
liskin added a commit to liskin/xmonad-contrib that referenced this issue Feb 10, 2022
(Unfortunately MIN_VERSION_xmonad has only 3 parameters and adding a
cabal flag seems too heavyweight.)

Relates: xmonad/xmonad#374
Relates: xmonad/xmonad#172
Relates: xmonad#290
@liskin
Copy link
Member

liskin commented Feb 10, 2022

One problem is those high masks represent keyboard layouts, and this will matter to some people.

I don't think anyone actually relies on this in TreeSelect, GridSelect or Prompt key bindings. I've opened #686 which just unconditionally filters out mouse buttons and XKB groups out of the modifier state.

To make it possible to additionally strip Mod5 as well, I've opened xmonad/xmonad#374


Re branching xmonad into devel/stable: I don't think it's wise to make maintaining xmonad any harder than it already is. At this point in the project's lifecycle, it's probably best to accept some breaking changes rather than forking into stable/devel. Many people build xmonad from source these days (probably involuntarily, because nobody maintains Haskell stuff outside of NixOS any more; and obviously also because we weren't good at making frequent releases) so it's all right that if a small number of people are affected by a breaking change, we tell them to patch it locally.

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

No branches or pull requests

3 participants