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

Auto-Type: Support multiple Xkb layouts #6247

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

hifi
Copy link
Member

@hifi hifi commented Mar 8, 2021

This is a fairly naive attempt at fixing multiple Xkb layout issues. There are more than one open issue for this but I'm not linking them unless this ends up being mergeable.

Testing strategy

setxkbmap fi,us -option grp:alt_space_toggle

Ensure there's an entry with username that has characters that use different modifiers on each layout, @ and " both use Shift-2 for example and changing layouts on-the-fly should keep things working. With old code it should consistently fail and produce the same key always. You can also use any special characters like umlauts.

I haven't tested this a lot yet but figured I'd push it out here.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@droidmonkey
Copy link
Member

That nested for-for-for-for loop is nuts. I'll clean that up.

@hifi
Copy link
Member Author

hifi commented Mar 13, 2021

Linked a bunch of issues now. I feel confident enough this should fix at least the linked ones.

@stefan123t
Copy link

As mentioned auto-type works for multiple keyboard layouts. I tested three us, de and fi positively!

Maybe #1915 is related to the Alt-Shift key combo for layout switching which is sometimes hanging.
I also experience this as an issue from time to time and would like to understand that if I press any of the Modifiers too early the code in the loop has to send some different keycode(s) to achieve the required results.
Eventually the X Input server could get tricked by two "keyboards" (the physical one and the Keepass Auto-type function) pressing and releasing Shift in a somewhat wrong sequence ?

@hifi
Copy link
Member Author

hifi commented Mar 14, 2021

Thank you for testing!

I'm not exactly sure what causes the layout toggle switch from not working after performing Auto-Type. As I said in the other issue I've had it persistently only once where restarting the X server was the only solution but at that time I didn't debug it as I planned to get back to that later but unfortunately I couldn't reproduce it at all when I got to it.

I'm pretty sure it isn't related to modifiers because I was able to reproduce it consistently after every X restart that it failed to switch after a single Auto-Type sequence.

Though, if you occasionally have this issue and don't mind using pre-release software it would be interesting to know if with this PR in daily use if you encounter it again. Just make sure you have backups of your database. This PR makes multiple changes to how the input is managed so it may avoid that completely.

@hifi hifi added this to the v2.7.0 milestone Mar 14, 2021
@hifi
Copy link
Member Author

hifi commented Mar 14, 2021

I've built an AppImage for testing purposes. Keep in mind that this is an unsigned and unofficial build of KeePassXC.

https://github.com/hifi/keepassxc/releases/download/2.7.0-snapshot-pr6247/KeePassXC-2.7.0-snapshot-pr6247-x86_64.AppImage

@stefan123t
Copy link

@hifi I will test the version I built from your PR in daily use throughout the week and update you in case I spot any errors or the Alt-Shift Problem reoccurs. If you would like to have an earlier status update, just ping me.

@stefan123t
Copy link

stefan123t commented Mar 15, 2021

BTW I have been thinking about adding custom ANSI-C/UTF-8 keyboard layout to the XServer and switch to it for Auto-Type. That way it is not necessary to search for the corresponding keycode in the active/available keyboard layouts and would do away with the nested for loops.
Dunno if this is feasible as it would require adding such a custom layout to the X11 system somehow and adding/removing it to/from the Xinput stack of layouts on the fly (before auto-type and afterwards). I am not aware if adding a custom UTF-8/ANSI-C keyboard layout is possible programmatically or if that would require additional care when packaging to be available for selection using setxkbmap or related system calls ?

@hifi
Copy link
Member Author

hifi commented Mar 15, 2021

It's fairly straightforward how searching for a keycode is managed in this PR so I don't see much benefit in adding a custom keymap. The old method of adding a symbol on-the-fly to the current layout would be more feasible to keep.

There are a lot of loops but it's not that much data it needs to go through anyway that it would make a meaningful difference in performance. Even if it takes a few milliseconds to build the full keymap and a millisecond or two to find a keycode it wouldn't feel much different having a pre-built complete keymap either.

From non-scientific testing the code is fast enough on a modern CPU it types almost instantly if you disable typing delay.

@stefan123t
Copy link

@hifi, thanks for explanation. I do assume then, that the "old merhod" of adding a symbol to the current keymap would only be performed in case the symbol does not yet exist on the primary/presumed active keymap. Hence in case of @,z,y etc it would first reuse a presumed keycode which in the truly active keymap might have a different symbol.
Hence the error using mingled characters.
@droidmonkey so making the "old method" the default for every symbol/char/keycode would also solve the problem, isnt it ?
Anyway thanks hifi for fixing the problem and please implement it either way in master.

@stefan123t
Copy link

@hifi I have been using the new version for one week productively and did not notice any mistypings whatsoever.

@stefan123t
Copy link

@hifi I have the Alt+Shift not working for keyboard layout switching problem again at the moment.
Using xev I have been able to verify that it still issues a ISO_Next_Group when I press Alt_L and Shift_L together.
I also added and removed Shift+CapsLock to the keyboard switching options with no results.

Do you have anything I should do to further analyse the LAlt+LShift problem or would you prefer to log a separate issue for it ?

Though typing in the correct keyboard layout (manual switching via the Panel applet in Cinnamon is still possible) works flawless with your patch. I do use Citrix Receiver and Zoom client from time to time. And Zoom client freezes my mouse, which goes away after I close the Zoom session or killall zoom on the command line. So I do not think this has anything to do with KeepassXC to be honest.

I have done some more deep digging to find out about the problem and came across this interesting answer on Stack Overflow:

Why is the order in which shortcuts keys are executed important?
https://unix.stackexchange.com/questions/391870/why-is-the-order-in-which-shortcuts-keys-are-executed-important/391968#391968

Because non-modifier actions are enacted on the key down event.
...
(The exception to the rule is the Fn key, which is the one modifier that is implemented in hardware. It is implemented entirely in hardware and not seen by software at all. It does not even generate any events over the wire. There's actually another hardware modifier, too. It isn't a key. It's the state of the NumLock LED.)
...

So once I de/re-activated the NumLock key on my keyboard things (keyboard switch via LAlt+LShift) started working again.

I only checked the current state of the keyboard LED mask after the problem disappeared using:

$ xset -q|grep LED
  auto repeat:  on    key click percent:  0    LED mask:  00000002

Would you kindly verify whether this is the case for your problem too ?

$ xset -q
Keyboard Control:
  auto repeat:  on    key click percent:  0    LED mask:  00000002
  XKB indicators:
    00: Caps Lock:   off    01: Num Lock:    on     02: Scroll Lock: off
    03: Compose:     off    04: Kana:        off    05: Sleep:       off
    06: Suspend:     off    07: Mute:        off    08: Misc:        off
    09: Mail:        off    10: Charging:    off    11: Shift Lock:  off
    12: Group 2:     off    13: Mouse Keys:  off
  auto repeat delay:  500    repeat rate:  33
  auto repeating keys:  00ffffffdffffbbf
                        fadfffefffedffff
                        9fffffffffffffff
                        fff7ffffffffffff
  bell percent:  50    bell pitch:  400    bell duration:  100
Pointer Control:
  acceleration:  2/1    threshold:  4
Screen Saver:
  prefer blanking:  yes    allow exposures:  yes
  timeout:  0    cycle:  0
Colors:
  default colormap:  0x20    BlackPixel:  0x0    WhitePixel:  0xffffff
Font Path:
  /usr/share/fonts/X11/misc,/usr/share/fonts/X11/Type1,built-ins
DPMS (Energy Star):
  Standby: 0    Suspend: 0    Off: 0
  DPMS is Enabled
  Monitor is On

@stefan123t
Copy link

When switching the keyboard layout under normal conditions using LAlt+LShift working the change to my secondary keyboard layout (layouts being us,de, ie. when set to de) is reflected in the following two lines (using watch xset -q):

Keyboard Control:
  auto repeat:  on    key click percent:  0    LED mask:  00001002
...
    12: Group 2:     on     13: Mouse Keys:  off

As mentioned in the #324 this change is reflected just the same in case when utilizing a ternary keyboard layout as with my trials adding us,de,fi keyboard layouts to the mix! That is I cannot distinguish de or fi from each other, as they both show the same in xset -q. That is why I suggested the code used in xkblayout-state to debug further. I have to compile it from scratch next time I am stuck with this issue to get further debug output for comparison.
Thanks for listening =^D

@hifi
Copy link
Member Author

hifi commented Mar 24, 2021

So layouts work but switching is still glitching 🤔

Would be nice to know a way to trigger that condition on demand as it's impossible to debug without a consistent repro.

Thanks for putting so much effort into this!

I wonder if it's just some timing issue with modifiers.

@droidmonkey
Copy link
Member

droidmonkey commented Mar 24, 2021

We are very firmly in niche territory with 3 keyboard layouts, we may have to just accept this is as good as it'll get with this PR.

@droidmonkey
Copy link
Member

This is excellent work @hifi, want to merge it?

@hifi
Copy link
Member Author

hifi commented Mar 26, 2021

If we can live without remap support, sure, I think this is good enough and can be improved in develop when issues arise.

Because the layout switch dying is not related to this PR as it's still present we can fix it separately when we understand the issue better.

Completely rewritten XCB Auto-Type keymap system.

 - supports multiple simultaneous layouts
 - prefers current layout if it has all keysyms available
 - removed hardcoded KeySymMap
 - removed clunky custom KeySym emulation

Biggest breaking change is removing KeySym emulation for keys that
do not exist in any of the layouts currently in use. It would be
possible to make it work but if you are trying to type syms that
are not available in any of your layouts you are abusing it. It
also adds unnecessary complexity and opens up timing issues when
the keymap is modified on-the-fly. Now we are just reading it.

This also workarounds a Qt related issue where QX11Info::display()
returns a connection to X server that fails to receive updated
keymap data when client settings change. We use our own connection
now to get it working.
@hifi hifi force-pushed the fix/autotype-keymap branch from c1a998f to 5fb9619 Compare March 26, 2021 05:06
@stefan123t
Copy link

We are very firmly in niche territory with 3 keyboard layouts, we may have to just accept this is as good as it'll get with this PR.

@droidmonkey, layouts work also for three keyboard layouts. But the layout toggle switch is also glitching with two layouts. But it may not be apparent with only one =^P
@hifi i can consistently get it back to work using the num lock key twice. Then also layout toggle switch works again. I just dont know how to provoke it. But that is a workaround I can live with.
Many thanks for your effort with this PR!

@droidmonkey droidmonkey merged commit 4d07507 into keepassxreboot:develop Mar 26, 2021
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants