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

Layout xkb options are not cleared during propagation #8230

Closed
ya-solnyshko opened this issue May 26, 2023 · 17 comments
Closed

Layout xkb options are not cleared during propagation #8230

ya-solnyshko opened this issue May 26, 2023 · 17 comments
Labels
affects-4.1 This issue affects Qubes OS 4.1. affects-4.2 This issue affects Qubes OS 4.2. C: gui-virtualization diagnosed Technical diagnosis has been performed (see issue comments). P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.
Milestone

Comments

@ya-solnyshko
Copy link

Qubes OS release

4.1.2 (R4.1)

Brief summary

Keyboard switching pollutes setxbmap options, eventually messing up the key mappings.

Steps to reproduce

  1. Configure multiple keyboard layouts in dom0, e.g., I use the following settings in /etc/X11/xorg.conf.d/00-keyboard.conf:
Section "InputClass"
        Identifier "system-keyboard"
        MatchIsKeyboard "on"
        Option "XkbLayout" "us,ru"
        Option "XkbVariant" "altgr-intl,winkeys"
        Option "XkbOptions" "nodeadkeys,grp:alt_shift_toggle"
EndSection
  1. Start any VM. The state of setxkbmap -print -query is:
user@disp1372:~$ setxkbmap -print -query
xkb_keymap {
	xkb_keycodes  { include "evdev+aliases(qwerty)"	};
	xkb_types     { include "complete"	};
	xkb_compat    { include "complete"	};
	xkb_symbols   { include "pc+us+inet(evdev)+group(alt_shift_toggle)"	};
	xkb_geometry  { include "pc(pc105)"	};
};
rules:      evdev
model:      pc105
layout:     us
options:    nodeadkeys,,grp:alt_shift_toggle
  1. Switch keyboard layout with Alt+Shift:
user@disp1372:~$ setxkbmap -print -query
....
rules:      evdev
model:      pc105
layout:     ru,us
variant:    ,
options:    nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle
  1. Switch a couple of times more:
user@disp1372:~$ setxkbmap -print -query
...
rules:      evdev
model:      pc105
layout:     us
options:    nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle,nodeadkeys,,grp:alt_shift_toggle

Expected behavior

Normal layout switching.

Actual behavior

  1. First, options get more and more polluted after each layout switch.
  2. Second, after many layout switches, the keycode mapping breaks (perhaps some overflow of options?): the arrow buttons, delete button, and some others get remapped to something random. For example, according to xev, the key Up turns into Print: keycode 111 (keysym 0xff61, Print), same_screen YES
  3. Fixing the layout with xmodmap -e "keycode 111 = Up" fixes the mapping until the next Alt+Shift, then it is messed up again. Resetting the options with setxkbmap -option doesn't help.

It seems that there is some competition between dom0 and the vm itself for handling the keycodes

@ya-solnyshko ya-solnyshko added P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists. labels May 26, 2023
@jamke
Copy link

jamke commented May 26, 2023

The keyboard layout switching is broken since R4.1. It is not working properly for me too.
But I do not see the same behavior that you have with options getting longer and polluted.

  1. What do you have in xfce: Settings Manager -> Keyboard -> Layout tab?
    You probably should see Use system defaults being checked and other controls disabled, is it your case?

  2. Can you try the same without nodeadkeys option in the first place?

@ya-solnyshko
Copy link
Author

Hi,

  1. Yes, it is set to Use system defaults.
  2. It indeed helps, Alt+Shift now toggles between
user@private:~$ setxkbmap -print -query
xkb_keymap {
	xkb_keycodes  { include "evdev+aliases(qwerty)"	};
	xkb_types     { include "complete"	};
	xkb_compat    { include "complete"	};
	xkb_symbols   { include "pc+ru(winkeys)+us:2+inet(evdev)+group(alt_shift_toggle)"	};
	xkb_geometry  { include "pc(pc105)"	};
};
rules:      evdev
model:      pc105
layout:     ru,us
variant:    winkeys,
options:    grp:alt_shift_toggle

and

user@private:~$ setxkbmap -print -query
xkb_keymap {
	xkb_keycodes  { include "evdev+aliases(qwerty)"	};
	xkb_types     { include "complete"	};
	xkb_compat    { include "complete"	};
	xkb_symbols   { include "pc+us(altgr-intl)+inet(evdev)+group(alt_shift_toggle)"	};
	xkb_geometry  { include "pc(pc105)"	};
};
rules:      evdev
model:      pc105
layout:     us
variant:    altgr-intl
options:    grp:alt_shift_toggle

Even "variant" is set correctly now. I am still wondering though, why it toggles between "ru,us" and "us", this doesn't happen in dom0

@andrewdavidwong andrewdavidwong added C: other needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. C: desktop-linux and removed C: other labels May 26, 2023
@andrewdavidwong andrewdavidwong added this to the Release 4.1 updates milestone May 26, 2023
@jamke
Copy link

jamke commented May 27, 2023

It indeed helps, Alt+Shift now toggles between

Glad it helped.
It's probably a bug in the script that parses options, that's why uncommon nodeadkeys breaks it, real bug.

About your question of layout toggling: starting from at least R4.1 the toogle works in dom0 as is (not changing xkb options) while on any change the event of changing is caught and the current layout is passed (propagated) to the qubes (VMs) manually by script. So, I think, it does not matter much what layout list is set, only the current layout should be correct, because on each toggle xkb settings will be changed any way. Something like that.

More info on this topic:
#8035
#6517
#8035 (comment)

@solardiz
Copy link

solardiz commented Jul 9, 2023

options get more and more polluted after each layout switch.

This was happening for me too, when the options on dom0 include more than one option (comma-separated). The below appears to fix it. More testing is needed, but at least this change is correct per the man page and it shouldn't hurt.

+++ /usr/lib/qubes/qubes-keymap.sh	2023-07-09 15:00:38.085000000 +0200
@@ -21,7 +21,7 @@
     fi
 
     if [ -n "$KEYMAP_OPTIONS" ]; then
-        KEYMAP_OPTIONS="-option $KEYMAP_OPTIONS"
+        KEYMAP_OPTIONS="-option -option $KEYMAP_OPTIONS"
     fi
 
     # Set layout on all DISPLAY
       -option name
               Specifies the name of an option  to  determine  the  components
               which  make  up the keyboard description;  multiple options may
               be specified, one per -option flag. Note  that  setxkbmap  adds
               options  specified in the command line to the options that were
               set before (as saved in root window properties). If you want to
               replace  all previously specified options, use the -option flag
               with an empty argument first.

@solardiz
Copy link

solardiz commented Jul 9, 2023

I am still wondering though, why it toggles between "ru,us" and "us", this doesn't happen in dom0

This is explained in QubesOS/qubes-gui-agent-linux#139 - it doesn't appear to hurt in your case, does it?

@marmarek
Copy link
Member

marmarek commented Jul 9, 2023

Thanks @solardiz ! So, together with #8035 (comment), I think now we know how to fix layout switching finally :)

@solardiz
Copy link

solardiz commented Jul 9, 2023

together with #8035 (comment), I think now we know how to fix layout switching finally :)

That other discussion looks like an alternative way to fix the issue. In this script, we can try simply omitting KEYMAP_OPTIONS.

@marmarek
Copy link
Member

marmarek commented Jul 9, 2023

That other discussion looks like an alternative way to fix the issue. In this script, we can try simply omitting KEYMAP_OPTIONS.

Omitting completely is a bad idea, since some options do remap keys (things like compose key for example).

@jamke
Copy link

jamke commented Jul 10, 2023

If you want to replace all previously specified options, use the -option flag with an empty argument first.

Should not the first -option command be followed by empty argument though?
Like:
KEYMAP_OPTIONS="-option '' -option $KEYMAP_OPTIONS"
I have not tried it, but seems that your quoted part of man says it should be called like that, right?

Also do not forget proper quoting for bash/shell, I think options cannot have spaces, but better safe than sorry:
KEYMAP_OPTIONS="-option '' -option '${KEYMAP_OPTIONS}'"
Note that I have not tried it.

@jamke
Copy link

jamke commented Jul 10, 2023

Omitting completely is a bad idea, since some options do remap keys (things like compose key for example).

I completely agree, options string should be stripped only of options that allow layout changes.

In my case I do not have other options, so, I temporary modified qvm_start_daemon.py a week ago to simply omit all options before propagation. After a week of tests: the layout switch is working much better.

I think now we know how to fix layout switching finally :)

One more thing - I investigated another (independent) problem that causes minor issues with layout:
https://forum.qubes-os.org/t/why-xev-root-event-property-shows-duplicate-events-it-breaks-keyboard-layout/19656

Long story short:

  • XKLAVIER_ALLOW_SECONDARY comes twice for a single layout switch,
  • and between these 2 calls there is a time gap, sometimes up to a second.
  • if user changes active window (of another qube) in this time gap it breaks layout, because event is coming with the same arguments, but actually layout of dom0 can be different due to settings in Keyboard Layouts applet of XFCE.
  • after that the next switch is not working properly. It is a bigger problem for cases like shift_caps_switch mode.

Can we somehow fix doubling of most events like that? Or is it by xorg design? I could not found any info why there are 2 calls for almost any action:

[dom0 ~]$ xev -root -event property

PropertyNotify event, serial 18, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3435915, state PropertyNewValue

PropertyNotify event, serial 19, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3435916, state PropertyNewValue

    
PropertyNotify event, serial 19, synthetic NO, window 0x544,
    atom 0x1a3 (_NET_CLIENT_LIST_STACKING), time 3442978, state PropertyNewValue

    
PropertyNotify event, serial 20, synthetic NO, window 0x544,
    atom 0x154 (_NET_ACTIVE_WINDOW), time 3443069, state PropertyNewValue

PropertyNotify event, serial 20, synthetic NO, window 0x544,
    atom 0x154 (_NET_ACTIVE_WINDOW), time 3443069, state PropertyNewValue

    
PropertyNotify event, serial 20, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3443081, state PropertyNewValue

PropertyNotify event, serial 21, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3443108, state PropertyNewValue

    
PropertyNotify event, serial 21, synthetic NO, window 0x544,
    atom 0x1a3 (_NET_CLIENT_LIST_STACKING), time 3443542, state PropertyNewValue

    
PropertyNotify event, serial 21, synthetic NO, window 0x544,
    atom 0x154 (_NET_ACTIVE_WINDOW), time 3443550, state PropertyNewValue

PropertyNotify event, serial 21, synthetic NO, window 0x544,
    atom 0x154 (_NET_ACTIVE_WINDOW), time 3443550, state PropertyNewValue

    
PropertyNotify event, serial 24, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3932489, state PropertyNewValue

PropertyNotify event, serial 24, synthetic NO, window 0x544,
    atom 0x1ee (XKLAVIER_ALLOW_SECONDARY), time 3932491, state PropertyNewValue

@jamke
Copy link

jamke commented Jul 10, 2023

That other discussion looks like an alternative way to fix the issue

No, it is different. Your modification clears options before passing them to qube. But it still passes options like shift_caps_switch that allows qube to change the layout itself, which can break things. Nonetheless, your modification is for sure right, because clearing options must be done anyway.

So, the way to fix layout issue as I see it:

  1. Clear previous options with -option ''
    Layout xkb options are not cleared during propagation #8230 (comment)
    Layout xkb options are not cleared during propagation #8230 (comment)

  2. Strip all options that are responsible for shortcuts to switch layout before propagation:
    Change of keyboard layouts with Capslock/Shift+Capslock (shift_caps_switch in setxkbmap) breaks all the time #8035 (comment)

  3. Somehow avoid second calls of propagation due to double XKLAVIER_ALLOW_SECONDARY property changes.
    Layout xkb options are not cleared during propagation #8230 (comment)
    https://forum.qubes-os.org/t/why-xev-root-event-property-shows-duplicate-events-it-breaks-keyboard-layout/19656

@solardiz
Copy link

Should not the first -option command be followed by empty argument though?

I've just tried this on the command line, and it seems to work just as well as having it without an argument.

Like: KEYMAP_OPTIONS="-option '' -option $KEYMAP_OPTIONS"
I have not tried it, but seems that your quoted part of man says it should be called like that, right?

I also haven't yet tried patching the script that way. Yes, the man page wording suggests that having an empty argument is more correct.

Also do not forget proper quoting for bash/shell, I think options cannot have spaces, but better safe than sorry: KEYMAP_OPTIONS="-option '' -option '${KEYMAP_OPTIONS}'"

This won't help - we're just building a string here, and any distinction on where the space characters came from is lost anyway. Further, the string is meant to contain multiple options/arguments separated by spaces, and is thus used without quoting on the command line later. If we want to support spaces inside options and pass them to setxkbmap verbatim, we'll need to make more invasive changes to the script.

@andrewdavidwong andrewdavidwong added diagnosed Technical diagnosis has been performed (see issue comments). and removed needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. labels Jul 10, 2023
@DemiMarie
Copy link

How should this be mapped to libxkbcommon? The Wayland compositor I am working on does all of its keymap handling in C and uses libxkbcommon directly, not via a command-line tool.

@solardiz
Copy link

Like: KEYMAP_OPTIONS="-option '' -option $KEYMAP_OPTIONS"
I have not tried it

Unfortunately, the above line doesn't work as desired - it results in literal '' (two apostrophes) getting into the options. We'd need to revise the setxkbmap command in the script in order to pass an empty argument. While that would be more correct per the man page, in practice my previous way of doing things (omitting the argument) just works, so we can use it for now.

solardiz added a commit to solardiz/qubes-gui-agent-linux that referenced this issue Jul 12, 2023
@andrewdavidwong andrewdavidwong added the pr submitted A pull request has been submitted for this issue. label Jul 13, 2023
@jamke
Copy link

jamke commented Jul 14, 2023

Unfortunately, the above line doesn't work as desired - it results in literal '' (two apostrophes) getting into the options. We'd need to revise the setxkbmap command in the script in order to pass an empty argument. While that would be more correct per the man page, in practice my previous way of doing things (omitting the argument) just works, so we can use it for now.

I also checked, both ways of calling setxkbmap work in the system. I also searched internet and found that it is common to skip the empty argument, so, maybe it was allowed by design at some point. So, I agree, we can use two -option in a row instead of adding empty argument if it requires any additional work.

@andrewdavidwong andrewdavidwong added the affects-4.1 This issue affects Qubes OS 4.1. label Aug 8, 2023
@andrewdavidwong andrewdavidwong removed this from the Release 4.1 updates milestone Aug 13, 2023
@jamke
Copy link

jamke commented Aug 20, 2023

I propose this issue:

  1. Be renamed to "Layout xkb options are not cleared during propagation" to limit the original problem and reflect it better.
  2. Closed as solved due to QubesOS/qubes-gui-agent-linux@c2cf6ae
  3. Add affects-4.2 (not tried, but why should it not affect 4.2)

@andrewdavidwong andrewdavidwong changed the title Layout switching eventually breaks key mapping Layout xkb options are not cleared during propagation Aug 20, 2023
@andrewdavidwong andrewdavidwong added the affects-4.2 This issue affects Qubes OS 4.2. label Aug 20, 2023
@andrewdavidwong
Copy link
Member

Closing as resolved. If anyone believes this issue is not yet resolved, or if anyone is still affected by this issue, please leave a comment, and we'll be happy to reopen it. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-4.1 This issue affects Qubes OS 4.1. affects-4.2 This issue affects Qubes OS 4.2. C: gui-virtualization diagnosed Technical diagnosis has been performed (see issue comments). P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.
Projects
None yet
Development

No branches or pull requests

6 participants