Igalia / cog

WPE launcher and webapp container
MIT License
235 stars 61 forks source link

x11: Further improvements to keyboard handling #446

Open aperezdc opened 2 years ago

aperezdc commented 2 years ago

In https://github.com/Igalia/cog/pull/445 we got some of the key modifiers working on X11 thanks to @ubercomp but quoting from the PR description, there are some other improvements that could be still done in the future:

ubercomp commented 2 years ago

One thing I forgot to mention was that I believe it's possible to replace the call to obtain the keysym with a call to xcb_key_symbols_get_keysym from the libxcb-keysyms library, which theoretically should help handle all edge cases. However, I'm not 100% sure of that, and, furthermore, it would add a new dependency (.so is about 15 kb), so I thought I should ask first: are we willing to add this new dependency if it significantly improves keyboard handling?

aperezdc commented 2 years ago

@ubercomp I think depending on libxcb-keysyms is very reasonable, I expect most users who might use the X11 backend will not mind.

Plus, 15 KiB is almost nothing these days, and I expect using it will handle some tricky corner cases that we may get wrong with out own implementation... at least my experience with input handling code is “there is always one more corner case to handle”, so it seems smart to offload the work to battle-tested components.

One more data point: I see in Debian's popcon that 25% of installs have it installed, which is a lot. Most people who have X11 installed will likely have libxcb-keysyms already installed.

ubercomp commented 2 years ago

@aperezdc that's what I think as well. basically, I can only guarantee the current implementation for the "pc105" keyboard with US layout / keymap. I'll play with libxcb-keysyms when I have some spare time.

psaavedra commented 2 years ago

this commit https://github.com/Igalia/cog/commit/48dfac2ba637e223eeea1b289526d0f51e39ab88 deserves a dependency with libxkbcommon to be reflected in the CMake rules:

/home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:50:9: error: unknown type name 'xkb_mod_mask_t'
|    50 |         xkb_mod_mask_t shift_mask;
|       |         ^~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:51:9: error: unknown type name 'xkb_mod_mask_t'
|    51 |         xkb_mod_mask_t alt_mask;
|       |         ^~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:52:9: error: unknown type name 'xkb_mod_mask_t'
|    52 |         xkb_mod_mask_t control_mask;
|       |         ^~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:262:8: error: unknown type name 'xkb_mod_mask_t'
|   262 | static xkb_mod_mask_t
|       |        ^~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c: In function 'keysym_modifiers_get_mask':
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:265:5: error: unknown type name 'xkb_mod_index_t'
|   265 |     xkb_mod_index_t index = 0;
|       |     ^~~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:276:12: error: 'XKB_MOD_INVALID' undeclared (first use in this function)
|   276 |     return XKB_MOD_INVALID;
|       |            ^~~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:276:12: note: each undeclared identifier is reported only once for each function it appears in
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c: In function 'text_input_modifiers_map':
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:288:65: error: 'XKB_MOD_NAME_SHIFT' undeclared (first use in this function)
|   288 |     priv->modifiers.shift_mask = keysym_modifiers_get_mask(map, XKB_MOD_NAME_SHIFT);
|       |                                                                 ^~~~~~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:289:63: error: 'XKB_MOD_NAME_ALT' undeclared (first use in this function)
|   289 |     priv->modifiers.alt_mask = keysym_modifiers_get_mask(map, XKB_MOD_NAME_ALT);
|       |                                                               ^~~~~~~~~~~~~~~~
| /home/bot/yocto-browsers/builds/wandboard-nightly/tmp/work/cortexa9t2hf-neon-poky-linux-gnueabi/cog/trunk+httpsAUTOINC+286db0e633-r0/git/platform/wayland/cog-im-context-wl-v1.c:290:67: error: 'XKB_MOD_NAME_CTRL' undeclared (first use in this function)
|   290 |     priv->modifiers.control_mask = keysym_modifiers_get_mask(map, XKB_MOD_NAME_CTRL);
|       |                                                                   ^~~~~~~~~~~~~~~~~
aperezdc commented 9 months ago

For the record, we have merged support for using xcb-keysyms in https://github.com/Igalia/cog/pull/679 — but leaving this issue open as there is still work to do.