QubesOS / qubes-issues

The Qubes OS Project issue tracker
https://www.qubes-os.org/doc/issue-tracking/
536 stars 47 forks source link

`XKLAVIER_ALLOW_SECONDARY` property changed twice with a time gap, that breaks layout switching sometimes #8441

Open jamke opened 1 year ago

jamke commented 1 year ago

How to file a helpful issue

Qubes OS release

R4.1 (probably R4.2, too)

Brief summary

Keyboard layout switch shortcut causes property change of XKLAVIER_ALLOW_SECONDARY to be fired twice, and not in one moment, but sometimes even with time gap up to a second. It breaks keyboard layout, at least in case the user switches between active windows in this time gap.

Steps to reproduce

Currently in R4.1 (and R4.2) keyboard layout switch is applied and propagated by reacting on XKLAVIER_ALLOW_SECONDARY property being changed in dom0, these changes can be monitored with command in dom0: $ xev -root -event property

Keyboard layout switch shortcut causes property change of XKLAVIER_ALLOW_SECONDARY appear in the output of this command. That is expected. But it happens twice! And not in one moment, but sometimes even with time gap up to a second. It breaks keyboard layout switch sometimes, if user switches between active windows at that moment. Because the mechanism of propagation also happens twice and enforces possibly wrong (outdated) layout to a possibly wrong qube.

Expected behavior

XKLAVIER_ALLOW_SECONDARY property change event fired once, code in dom0instantly finds the active window, active qube and passes the correct layout there.

Actual behavior

XKLAVIER_ALLOW_SECONDARY property change event fired twice, with a time gap and sometimes sets wrong layout in the qube.

More:

Topic on forum if useful https://forum.qubes-os.org/t/why-xev-root-event-property-shows-duplicate-events-it-breaks-keyboard-layout/19656

Log

The example of output from xev I provided in the next message, as github has no collapsible spoilers.

jamke commented 1 year ago

Log example:

[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
andrewdavidwong commented 1 year ago

as github has no collapsible spoilers.

You can make them like this: https://gist.github.com/scmx/eca72d44afee0113ceb0349dd54a84a2

jamke commented 1 year ago

Another problem with XKLAVIER_ALLOW_SECONDARY event: it is fired only when layout inside dom0 is changed, it prevents syncing layout of the qube for layout switching modes like shift_caps_switch, when user does not toggle between layouts, but uses shortcut for targeting needed, the propagation calls are not coming. Should be even more drastic in case of 3 layouts with shortcuts.

Example of this sub-problem with 2 layouts:

  1. User uses shift_caps_switch option, meaning capslock - set first layout, shift+capslock - set second.
  2. Layout in qube lost sync due to something (like #8441 bug). In dom0 it is 1st, in qube it is 2nd.
  3. User wants 1st layout in the qube and, thus, presses capslock once or even many times.
  4. No XKLAVIER_ALLOW_SECONDARY is fired at all, propagation does not start, layout inside qube stays unsynced. Because in dom0 it is not changing, it is correct there, but not in qube.

Idea: maybe it would be better to use something that fires on EVERY keyboard shortcut switching. Maybe not even windows/xev event. It will be more reliable and avoid this problem. If such even exists.

andrewdavidwong commented 1 year ago

Added regression label because @jamke wrote in a forum post that this is a regression from 4.0.

@jamke, you also mentioned in that post that you've already diagnosed this issue. Could you please provide the technical diagnosis here (in as brief and easy-to-read form as possible) to make it more likely that a developer can quickly understand the solution and implement it?

jamke commented 1 year ago

@andrewdavidwong maybe I understand "diagnosed" differently, but what I meant is that everything that causes the problem is already clear and shown (check the first 2 posts).

In a nutshell: the problem is caused by 2 events fired with a time gap as xev output shows. Everyone can easily reproduce and measure time gap in the log. The second event causes the unnecessary layout propagation by qvm_start_daemon.py for the second time. And it breaks layout in some situations because active window, or qube, or layout can be changed during this time gap.

The possible solutions that I see:

UndeadDevel commented 9 months ago

I think I'm encountering this bug, too; I'm able to 100% reproduce it with the following steps:

  1. Set up at least two different keyboard layouts and use the "per-window-layout" mode
  2. Launch one window each on 3 adjacent work spaces (W0, W1, W2)
  3. Set the keyboard layout of W0 and W2 to A and of W1 to B
  4. Start on the workspace of W0, test keyboard layout there (should be A), then quickly switch to W2 via your keyboard shortcut for "next workspace" (pressed twice, which means it will pass over W1 briefly)
  5. Observe that the keyboard layout of W2 is now B even though A was set

Switching work spaces more slowly will not trigger the bug, which is why I think it's this issue.

jamke commented 9 months ago

Switching work spaces more slowly will not trigger the bug, which is why I think it's this issue.

Quite possible. I was told that having double events is a result of having layout indicator on lxpanel (XFCE is once again one to blame). So, please, try to remove layout indicator called Keyboard Layouts from the panel (if you have one) and try to reproduce.

UndeadDevel commented 9 months ago

Quite possible. I was told that having double events is a result of having layout indicator on lxpanel (XFCE is once again one to blame). So, please, try to remove layout indicator called Keyboard Layouts from the panel (if you have one) and try to reproduce.

Removing the widget from the panel will break the "per-window" layout functionality completely (it will then activate "per application" mode, which means "per application" in dom0, but seems to treat all domUs as the same application), so while that may "fix" this issue it actually makes things worse; when I keep the widget and set layout to global mode then I don't have much of an issue (rarely it will still mess up, but it's hard to reproduce); and I like having an indicator that shows me the current layout as well as if caps lock is active so I'm keeping the widget.

jamke commented 9 months ago

Removing the widget from the panel will break the "per-window" layout functionality completely (it will then activate "per application" mode, which means "per application" in dom0, but seems to treat all domUs as the same application), so while that may "fix" this issue it actually makes things worse

I see, yes. I still do not know the reason why the second event is fired by the widget.