search

xmonad / xmonad-contrib

Contributed modules for xmonad
https://xmonad.org
BSD 3-Clause "New" or "Revised" License
589 stars 274 forks source link

Bring XMonad.Actions.GridSelect.makeXEventhandler closer core keypress handler #590

Closed lierdakil closed 2 years ago

lierdakil commented 3 years ago

Description

This changes makeXEventhandler to behave much closer to how xmonad itself handles keypresses. The primary difference lies in that xmonad reads raw keycode and then converts it to unmodified keysym, while makeXEventhandler was reading actual keysyms. As a consequence, key definitions like (shiftMap, xK_Tab) didn't work with makeXEventhandler on many layouts because an actual keysym for 'Shift+Tab' is commonly ISO_LEFT_TAB, and not Tab.

Additionally, the mask is stripped of its high bits, because apparently X likes to encode current layout group information in bits 13 and up, and we don't really want our control keys to depend on the current layout group. Please let me know if there is a better way to handle this.

Not updating the changelog just yet, awaiting feedback.

Checklist

liskin commented 3 years ago

Additionally, the mask is stripped of its high bits, because apparently X likes to encode current layout group information in bits 13 and up, and we don't really want our control keys to depend on the current layout group.

Looks like xmonad core doesn't do this, though. I am puzzled why it'd be needed here and not there.

Otherwise this looks good, although I must admit I know very little about the intricacies of X11 KeySyms and KeyCodes. Oh and it seems like XMonad.Prompt and XMonad.Actions.TreeSelect probably suffer from the same problem?

lierdakil commented 3 years ago

Looks like xmonad core doesn't do this, though. I am puzzled why it'd be needed here and not there.

Honestly, likewise. Couldn't figure out why core isn't affected. Hence the question on how to handle this better.

FWIW, Prompt also filters high bits out (has been for a while):

https://github.com/xmonad/xmonad-contrib/blob/b4a13e6b1b146f051762998fa8e30868dd8e6152/XMonad/Prompt.hs#L597-L604

but apparently it overfilters, since Button5Mask is (1<<12) -- pretty sure the mask should be 1 << 13 - 1 aka 0x1fff; also the docstring is misleading here -- this isn't really a duplicate of core cleanMask anymore.

seems like XMonad.Prompt and XMonad.Actions.TreeSelect probably suffer from the same problem?

I don't use TreeSelect, so can't tell you from the top of my head, but looking at the source code it seems very likely. As for Prompt, I didn't notice it being affected. However, there isn't many opportunities to notice, keysym is only used to bind the completion key there. Again, looking at the source, it is also likely. I will try to test and fix as needed over the weekend.

liskin commented 3 years ago

Honestly, likewise. Couldn't figure out why core isn't affected.

Do you have a simple reproducer (xkb layouts, xmonad config and steps to take) so that I can try to figure this out?

lierdakil commented 3 years ago

Not so sure about simple, but here goes...

XKB layout:

setxkbmap us,de winkeys 'grp:caps_toggle,grp_led:caps,terminate:ctrl_alt_bksp'

this will set layout to US+German with caps lock as layout group toggle key (regular caps lock is on shift+caps). Fair warning, it also enables X termination via ctrl+alt+backspace, which I find useful when debugging X-related stuff, but YMMV.

XMonad config can be almost arbitrary, with GridSelect bound to something. For example:

import XMonad
import qualified XMonad.Actions.GridSelect as GS
import XMonad.Util.EZConfig (additionalKeys)

main = xmonad $ def `additionalKeys`
    [ ((controlMask, xK_g), GS.goToSelected def)]

Now, ctrl+g should bring up GridSelect's "go to window" regardless of the currently active layout group (which, reminder, are switched with caps lock key). However, navigation keys (arrows, hjkl, escape, enter) won't work on the grid select when the second layout group is active. So, the steps:

  1. Bring up grid select with ctrl+g
  2. Verify that navigation keys are working
  3. Close grid select (with escape key for instance)
  4. Switch layout group with caps lock key
  5. Bring up grid select again with ctrl+g
  6. Navigation keys now shouldn't work
  7. Switch the layout group back with caps lock key
  8. Verify that navigation keys are now working.

The question is, why ctrl+g in this example works for either layout group.

liskin commented 3 years ago

^^ This is an excellent writeup! 👍

I'll be off for a week or two now so please excuse me for not trying it right away, but I (or perhaps someone else) will get to it later.

lierdakil commented 3 years ago

FWIW, I've verified that both tree select and prompt have the same issue (with the notable exception of prompt already stripping high bits from the key mask). So I've also fixed those.

Also, with my initial patch, the event handler triggered on key release in addition to key press, while before the patch it only triggered on key press. I've amended that in https://github.com/xmonad/xmonad-contrib/pull/590/commits/68b58694c9abc0024a1d3a593f521389a28bf4ac ; changes to Prompt and TreeSelect also include this additional check.

slotThe commented 3 years ago

The question is, why ctrl+g in this example works for either layout group.

I think that the reason is that xmonad core already "does the right thing" here; i.e., we correctly respond to MappingNotify events. According to the Xlib docs this indicates that the keyboard mapping changed and hence we need to refresh our grab.

liskin commented 3 years ago

I think that the reason is that xmonad core already "does the right thing" here; […] refresh our grab.

Hm, so does refreshing the grab mean that subsequent KeyPress events no longer need cleaning the mask? Sounds a bit suspicious.

(Oh and I'm so sorry I totally forgot about this. I mean, not exactly forgot, it's still fairly up in my taskwarrior "soon" list, I'm just really good at inventing other stuff that needs to be done even sooner. 😿)

slotThe commented 3 years ago

Hm, so does refreshing the grab mean that subsequent KeyPress events no longer need cleaning the mask? Sounds a bit suspicious.

Mh you're right, refreshing the grab is mainly for KeySym's, but the problem in this case is masks

liskin commented 2 years ago

Finally got to this.

Additionally, the mask is stripped of its high bits, because apparently X likes to encode current layout group information in bits 13 and up, and we don't really want our control keys to depend on the current layout group.

Looks like xmonad core doesn't do this, though. I am puzzled why it'd be needed here and not there.

Turns out this is an XKB compatibility feature. A nice summary of it is here: https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Delivering_a_Key_or_Button_Event_to_a_Client Additional information: https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Setting_a_Passive_Grab_for_an_XKB_State, https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Group_Compatibility_Map, https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Compatibility_Components_of_Keyboard_State, https://www.x.org/releases/X11R7.7/doc/kbproto/xkbproto.html#Computing_A_State_Field_from_an_XKB_State

So I guess that answers all the questions and we can merge this at last. (Actually we should've merged it right away and let me satisfy my curiosity later…) Need to log off now but I'll rebase and merge tomorrow. Sorry again for the delay!

liskin commented 2 years ago

I decided to clean this up a bit and tackle some other related issues, so I opened https://github.com/xmonad/xmonad-contrib/pull/686 instead. Merging that one will close this one.