Fix zero key codes for non-Latin keyboard layouts on Linux/BSD platforms (PR #23410)
unxed
Fixes #23379
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/23410
Commit Summary
- 0307fb3 set key_code to Latin equivalent of pressed key as wx on Windows do
- fb5b061 minor fixes
- c17c2ca performance optimization
- 0c7c8e5 added xkbcommon requirement for GTK target on linux/bsd systems
- 1e2bdf1 moved deinitialization to wxModule, fixed unicode char being lost (as key_code is never zero now)
- 48026eb add required changes for xkbcommon lib
File Changes
(4 files)
- M configure (93)
- M configure.in (19)
- M setup.h.in (3)
- M src/gtk/window.cpp (75)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/23410.patch
- https://github.com/wxWidgets/wxWidgets/pull/23410.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for the PR!
Aside of the test issue, it looks good and needs just a couple of minor changes. But the tests must be fixed, of course...
BTW, please don't hesitate to add a test which doesn't run by default (it must have [.] tag for this) that could be executed to check that things work correctly when Cyrillic layout is used. It's nice to have even a semi-automatic test that can be run manually to confirm that things work instead of having to check it in the keyboard sample. TIA!
> @@ -54,6 +55,10 @@ using namespace wxGTKImpl; typedef guint KeySym; #endif +#if defined (GDK_WINDOWING_X11) || defined (GDK_WINDOWING_WAYLAND)
You need to test for HAVE_XKBCOMMON too here to avoid compilation problems with GTK 2 where libxkbcommon might not be available.
Moreover, to avoid repeating the same complicated preprocessor test condition again below, I suggest adding
#define wxHAS_XKB
here and then just doing #ifdef wxHAS_XKB later.
In configure.in:
> @@ -5013,6 +5013,25 @@ if test "$wxUSE_FSWATCHER" = "yes"; then
fi
fi
+dnl ---------------------------------------------------------------------------
+dnl xkbcommon library for key code translations on Linux/BSD
+dnl ---------------------------------------------------------------------------
+if test "$wxUSE_GTK" = 1; then
+ if test "$USE_WIN32" != 1 ; then
+ if test "$USE_DARWIN" != 1 ; then
+ PKG_CHECK_MODULES(XKBCOMMON, [xkbcommon],
+ [
+ CFLAGS="$XKBCOMMON_CFLAGS $CFLAGS"
+ CXXFLAGS="$XKBCOMMON_CFLAGS $CXXFLAGS"
+ GUI_TK_LIBRARY="$GUI_TK_LIBRARY $XKBCOMMON_LIBS"
+ AC_DEFINE(HAVE_XKBCOMMON, 1, [Define if libxkbcommon is available]) have_xkbcommon=yes
I think this could be just
⬇️ Suggested change- AC_DEFINE(HAVE_XKBCOMMON, 1, [Define if libxkbcommon is available]) have_xkbcommon=yes + AC_DEFINE(HAVE_XKBCOMMON)
because we don't care about the comment (this is used with autoheader which we don't use) and have_xkbcommon just doesn't seem to be used at all.
> @@ -1025,7 +1035,39 @@ wxTranslateGTKKeyEventToWx(wxKeyEvent& event,
: wxT("press"),
static_cast<unsigned long>(keysym));
- long key_code = wxTranslateKeySymToWXKey(keysym, false /* !isChar */);
+ long key_code = 0;
+ bool force_uni = 0;
+
+#if defined (GDK_WINDOWING_X11) || defined (GDK_WINDOWING_WAYLAND)
+ if (gdk_keyval_to_unicode(gdk_event->keyval))
+ {
+ // is event has corresponding unicode char, this is probably
+ // char key press. let us set key_code to character
+ // that is generated by that key in Latin kb layout
+ // (just as wx on Windows do)
+
+ if (!state) {
Nitpick but
⬇️ Suggested change- if (!state) {
+ if (!state)
+ {
> +xkb_context *ctx = nullptr; +xkb_keymap *keymap = nullptr; +xkb_state *state = nullptr;
We really should use g_ prefix for them. Or, perhaps, put them all in some g_xkbData struct and then use the names without prefix for its fields.
> @@ -6518,3 +6561,31 @@ void wxWindowGTK::DoThaw()
if (m_wxwindow && m_wxwindow != m_widget)
GTKThawWidget(m_wxwindow);
}
+
+// ----------------------------------------------------------------------------
+// wxWinModule
Let's rename it to wxXKBModule please to make it more clear what it is used for.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 587bb9d minor code style changes
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- c713588 adopt test logic to new KeyCode/UnicodeKey behavoir
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 51d8711 KeyCode for Latin letter keys should be in upper register to match wx behavior on MSW
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 5ddb103 KeyCode for Latin letter keys should be in upper register, try 2
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 4cb2c8e adopt kb test to new KeyCode/UnicodeKey behavior, try 2
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- e69dbf5 fixed a typo
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 8b1fcd2 trying to fix build under ubuntu 18.04
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 20c1d13 temporary revert test changes to inspect CI test failures more accurately
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- e775602 removed redundancy
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 27956ce attempting to try to fix kb test
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- fe42df6 attempting to try to fix kb test, try 2
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- d077e11 fix #17643
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 6a256e4 reverted comment back
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 2af73b0 trying to fix #17643 better
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 291c649 fix #17643 even better
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 159e410 cosmetic
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@unxed pushed 1 commit.
- 5bfb034 fixed key_code value
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I've applied this with some minor tweaks:
- Don't make xkbcommon a hard dependency, i.e. allow building without it which is probably useful at least for wxGTK2.
- Add a struct to contain all XKB-related stuff together as IMHO it looks a bit cleaner.
- Some minor comments/style changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #23410.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Thank you! I hope I checked everything enough and nothing breaks anywhere. At least I compared the behavior of both character and function keys (in combination with modifiers in various combinations) in the Russian and English keyboard layouts in the "keyboard" demo application under Windows and in my branch under GTK. I did not find any discrepancies (except for the use of the ANSI character code for KeyCode value of character key events in Windows).
The current content logic for keyboard event fields may not be perfect, but at least it is now more consistent between Windows and GTK. We should probably take some time to document the contents of these fields more accurately, as the current documentation doesn't exactly reflect the reality.
As for keyboard tests for non-Latin characters, I realized that it is too early to write them. I don't know how the macOS version behaves in these cases. Alas, I can not check, there is no macOS device at hand. So let's put it off until we find a macOS developer willing to help with it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks a lot for finally fixing it, it's great that this at last behaves correctly in wxGTK.
I can test under macOS and, in fact, already did run the keyboard sample there and it seems that it behaves in the same way as under MSW, although I could have missed something. So as long as wxGTK behaves in the same way as wxMSW too, all 3 ports should do the same thing now.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
CMake should get the libxkbcommon check too. I'll try to have a look at it at some time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
CMake should get the
libxkbcommoncheck too. I'll try to have a look at it at some time.
Oops, sorry, I actually wanted to add this but forgot.
Looking at it now, it seems that the convention is to add build/cmake/modules/FindXKBCOMMON.cmake, even if we really just need pkg_check_modules()?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Yes, you add a Find[PACKAGENAME].cmake to the modules folder. Either find one on the internet that we can use, or make one yourself (using maybe FindFONTCONFIG.cmake as example).
Then you can use find_package([PACKAGENAME]), probably in init.cmake.
If the package is found ([PACKAGENAME]_FOUND), then set(HAVE_XKBCOMMON ON) and add the include dirs and libraries to the wxTOOLKIT_ variables. Similar to GSPELL.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Please note that for systems there we do not have libxkbcommon (like ubuntu 18.04 with gtk 2) we can still make such translations using pure x11 api:
#ifdef GDK_WINDOWING_X11
int gtk_hardware_key_code = '12'; // whatsever
char keycodes[] = "evdev";
char types[] = "complete";
char compat[] = "complete";
char symbols[] = "pc+us+inet(evdev)";
XkbComponentNamesRec component_names = {
.keycodes = keycodes,
.types = types,
.compat = compat,
.symbols = symbols
};
Display *display = (Display *)wxGetDisplay();
XkbDescPtr xkb = XkbGetKeyboardByName(
display, XkbUseCoreKbd, &component_names, XkbGBN_AllComponentsMask, XkbGBN_AllComponentsMask, False);
XkbGetControls(display, XkbGroupsWrapMask, xkb);
XkbGetNames(display, XkbGroupNamesMask, xkb);
KeySym ks;
unsigned int mods;
XkbTranslateKeyCode(xkb, gtk_hardware_key_code, 0, &mods, &ks);
// ks now contains keysym in Latin keyboard layout
#endif
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
FWIW I don't think it's worth having this code, libxkbcommon looks like a common (sic) dependency, so it shouldn't be a problem to rely on it. But if anybody feels strongly enough about using GTK 2 without it, I'd merge a PR adding it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
I am also not sure that solving the problem under GTK2 is worth further complication of already non-trivial code and efforts to further maintain it. Developers of applications that will benefit from our improvement will probably simply prompt users to upgrade to a more recent version of the distribution. I myself do not use GTK2, so it does not matter to me whether the problem under it will be solved or not. If there is someone who needs it, the sample code is left here, PR is easy to do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Wouter van Oortmerssen
@unxed This fix appears to break the behavior of the DEL key on (at least) Polish keyboards, see e.g. aardappel/treesheets#422
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
This fix appears to break the behavior of the DEL key on (at least) Polish keyboards, see e.g. aardappel/treesheets#422
@aardappel still relevant? We recently changed behavior to more conservative and compatible.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Wouter van Oortmerssen
@unxed not sure, will ping the person with the Polish keyboard :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Wouter van Oortmerssen
@unxed apparently not fixed, see discussion here: aardappel/treesheets#422
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@aardappel Can this be reproduced in the keyboard sample? I don't understand what is going on in the video at the link and all I can see is that Del key generates the expected codes for me in the sample.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Wouter van Oortmerssen
@vadz sorry, I do not have time to try and reproduce it outside my application.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.