Key events do not return key code when non-latin keyboard layout is active (Issue #23379)
unxed
Under GTK, the problem can be worked around by calling GetRawKeyFlags and then using XGetKeyboardMapping to get all possible key symbols associated with this raw x11 key code. So if a button with a Latin letter is pressed while a non-Latin keyboard layout is enabled, we can still recognize the Latin letter. This forces us to additionally link against the x11 libraries, but at least it works.
Under other platforms, there doesn't seem to be a solution.
Related issues in other projects:
wxWidgets/Phoenix#1391
elfmz/far2l#1562
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
Did you try using GetUnicodeKey() instead of GetKeyCode()? The docs for GetKeyCode() explicetely say that it doesn't work for non-latin keyboard layouts and that GetUnicodeKey() should be used instead (https://docs.wxwidgets.org/stable/classwx_key_event.html#a8214333c112432c4c9df114d3bd9122c).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Of course GetUnicodeKey() is working. But it can not give me any information that identify pressed key, not char. But I want to know that key was pressed exactly. Only GetRawKeyFlags() can identify key independently of current selected keyboard layout.
I know it is a documented behavior, so maybe this should be called not a bug report, but a "feature request", like "provide wx key codes in key events for non-latin keyboard layouts also"?
Usage example: I want to use accelerator Ctrl+> and want it to work in any keyboard layout. Classical wx accelerators only support Latin keys, so I can not use them for it and need to process keydown events by myself. But in, for example, Russian keyboard layout Ctrl+> is Ctrl+ю, so keydown event gives ю as unicode char and no way to match it to > key except for
GetRawKeyFlags() hack.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
This seems to be very similar, if not quite the same, as #17643, and has also been discussed on the mailing list quite a few times. The current situation is that wx API indeed doesn't provide a way to uniquely identify a keyboard key portably, i.e. we don't have any portable scan code equivalent.
Historically this was very difficult to do, as there was a great variety of keyboards and layouts. Now that everybody uses more or less the same hardware, it might be doable, and if anybody would like to contribute support for this, it would be very welcome (but please let's discuss it in details first!). But unfortunately I don't think I'm going to have time to work on this myself in the observable future.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
The problem (at least with GDK backend) is that GDK keyboard event does not contain any portable key code. The only thing that can be used for it is hardware_keycode field, and to translate it to some portable key code we need to link directly with x11 libs (to use something like XGetKeyboardMapping), and I am not sure if this is architecturally correct solution. Also not sure it would work under Wayland.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
if anybody would like to contribute support for this
For GTK backend I have required code already:
if (key_code == 0) {
Display *display;
display = XOpenDisplay(NULL);
KeySym *keymap;
int keysyms_per_keycode;
char* keysym;
keymap = XGetKeyboardMapping(display, gdk_event->hardware_keycode, 1, &keysyms_per_keycode);
for (int i = 0; i < keysyms_per_keycode; i++) {
if (keymap[i] && (keymap[i] != NoSymbol)) {
keysym = XKeysymToString(keymap[i]);
if (strlen(keysym) == 1) {
key_code = toupper(*keysym);
}
switch (keymap[i]) {
case XK_minus: key_code = '-'; break;
case XK_equal: key_code = '+'; break;
case XK_bracketleft: key_code = '['; break;
case XK_bracketright: key_code = ']'; break;
case XK_semicolon: key_code = ';'; break;
case XK_apostrophe: key_code = '\''; break;
case XK_grave: key_code = '`'; break;
case XK_backslash: key_code = '\\'; break;
case XK_comma: key_code = ','; break;
case XK_period: key_code = '.'; break;
case XK_slash: key_code = '/'; break;
}
}
if (key_code)
break;
}
}
XFree(keymap);
XCloseDisplay(display);
}
Should I make a PR, or should support be implemented in all backends first?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I'm not sure what will be the API/semantics here? I don't think we want to return something from GetKeyCode() when some non-Latin letter is entered, as this would confuse the existing code and make it handle it as this letter, i.e. I believe we need yet another function in wxKeyEvent -- but please let me know if I'm missing something.
Also, it's not a problem to have this function implemented for GTK/X only initially. But it would be really nice to define it in such a way that it could be implemented for the other platforms too later, and at the very least for GTK/Wayland, i.e. at least check Wayland docs to see if it provides anything similar.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Okay, will look how to implement this under Wayland.
As for incompatibility: as far as I can understand the code, key_code values for Alt/Ctrl+non_latin_letters are set at least under Windows platform. So we will not break anything, just make the behavior consistent.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Here is Wayland-compatible implementation:
if (key_code == 0) {
xkb_context *ctx = xkb_context_new(XKB_CONTEXT_NO_FLAGS);
xkb_keymap *keymap = xkb_keymap_new_from_names(ctx, NULL, XKB_KEYMAP_COMPILE_NO_FLAGS);
xkb_state *state = xkb_state_new(keymap);
xkb_keycode_t keycode = gdk_event->hardware_keycode;
char key_code_str[64];
int len = xkb_state_key_get_utf8(state, keycode, key_code_str, sizeof(key_code_str));
if (len == 1) {
key_code = toupper(key_code_str[0]);;
} else {
switch (keycode) {
case XKB_KEY_minus: key_code = '-'; break;
case XKB_KEY_equal: key_code = '+'; break;
case XKB_KEY_bracketleft: key_code = '['; break;
case XKB_KEY_bracketright: key_code = ']'; break;
case XKB_KEY_semicolon: key_code = ';'; break;
case XKB_KEY_apostrophe: key_code = '\''; break;
case XKB_KEY_grave: key_code = '`'; break;
case XKB_KEY_backslash: key_code = '\\'; break;
case XKB_KEY_comma: key_code = ','; break;
case XKB_KEY_period: key_code = '.'; break;
case XKB_KEY_slash: key_code = '/'; break;
}
}
xkb_state_unref(state);
xkb_keymap_unref(keymap);
xkb_context_unref(ctx);
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Here is Wayland-compatible implementation. Will work on both X11 and Wayland systems.
#include <xkbcommon/xkbcommon.h>
// ...
if (key_code == 0) {
xkb_context *ctx = xkb_context_new(XKB_CONTEXT_NO_FLAGS);
xkb_keymap *keymap = xkb_keymap_new_from_names(ctx, NULL, XKB_KEYMAP_COMPILE_NO_FLAGS);
xkb_state *state = xkb_state_new(keymap);
xkb_keycode_t keycode = gdk_event->hardware_keycode;
char key_code_str[64];
xkb_state_key_get_utf8(state, keycode, key_code_str, sizeof(key_code_str));
key_code = toupper(key_code_str[0]);
xkb_state_unref(state);
xkb_keymap_unref(keymap);
xkb_context_unref(ctx);
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Here is Wayland-compatible implementation. Will work on both X11 and Wayland systems.
#include <xkbcommon/xkbcommon.h>
// ...
if (key_code == 0) {
xkb_context *ctx = xkb_context_new(XKB_CONTEXT_NO_FLAGS);
struct xkb_rule_names names = {
.rules = NULL,
.model = "pc105",
.layout = "us",
.variant = NULL,
.options = NULL
};
xkb_keymap *keymap = xkb_keymap_new_from_names(ctx, &names, XKB_KEYMAP_COMPILE_NO_FLAGS);
xkb_state *state = xkb_state_new(keymap);
xkb_keycode_t keycode = gdk_event->hardware_keycode;
char key_code_str[64];
xkb_state_key_get_utf8(state, keycode, key_code_str, sizeof(key_code_str));
key_code = toupper(key_code_str[0]);
xkb_state_unref(state);
xkb_keymap_unref(keymap);
xkb_context_unref(ctx);
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Here is Wayland-compatible implementation. Will work on both X11 and Wayland systems.
#include <xkbcommon/xkbcommon.h>
// ...
if (key_code == 0) {
xkb_context *ctx = xkb_context_new(XKB_CONTEXT_NO_FLAGS);
struct xkb_rule_names names = {
.rules = NULL,
.model = "pc",
.layout = "us",
.variant = NULL,
.options = NULL
};
xkb_keymap *keymap = xkb_keymap_new_from_names(ctx, &names, XKB_KEYMAP_COMPILE_NO_FLAGS);
xkb_state *state = xkb_state_new(keymap);
xkb_keycode_t keycode = gdk_event->hardware_keycode;
char key_code_str[64];
xkb_state_key_get_utf8(state, keycode, key_code_str, sizeof(key_code_str));
key_code = toupper(key_code_str[0]);
xkb_state_unref(state);
xkb_keymap_unref(keymap);
xkb_context_unref(ctx);
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Is there anything else I can do to help resolve this issue?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
.model = "pc105",
It's better to leave .model here as NULL for xkb to use system model.
Btw, is there anything else I can do to help resolve this issue?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
If you could please make a PR with your changes, this would be great. We're probably going to need some changes to the build system (configure and, ideally, also CMake) to link with libxkb, and at least a brief update to the docs explaining what does GetKeyCode() return in wxGTK exactly.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
at least a brief update to the docs explaining what does GetKeyCode() return in wxGTK exactly
First of all I made some experiments to make sure my assumption about wx behavior on Windows is correct. Looks like it does. Windows wx version behave for char keys as follows:
Then Latin keyboard layout is selected, OnKeyUp/Down events contain uppercase Latin letter both in KeyCode and UnicodeKey fields, and OnChar events contain lowercase Latin letter both in KeyCode and UnicodeKey fields.
Then (for example) Russian keyboard layout is selected, OnKeyUp/Down behaves the same way: both KeyCode and UnicodeKey has uppercase Latin letter! Only OnChar has Russian letter, its code is stored using Unicode in UnicodeKey field and using windows ansi (in my case it is windows-1251) charset in KeyCode filed.
I do not think we should touch KeyCode filed of OnChar events anyway, as UnicodeKey should be used instead on all platforms.
As for OnKeyUp/Down, to make behavior consistent across platforms, I suggest the following logic under GTK:
if UnicodeKey is present (so it is a char key), we should replace KeyCode value (which is currently keyboard layout dependent) by Latin letter corresponding to pressed key, as detected in xkb code sample above. If we do so, meaning of KeyCode field of OnKeyUp/Down events for char keys will be consistent across Windows and GTK backends: it will contain character that is placed on pressed key in Latin layout.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I agree that this makes sense. Under MSW KeyUp/Down events use VK_XXX, i.e. virtual key constants, which just happen to coincide with the letters for the keys producing letters, and it would be nice if we could make them work in the same way under the other platforms, especially as it seems that there is something very close to VKs (evdev-based constants).
So the first conclusion is that we should definitely do this in master/3.3 and a PR doing this would be very welcome.
But the change to the key code of KeyUp/Down messages is clearly a compatibility break and even if it could be argued that the old behaviour is useless, we can't change 3.2 like this. If we (you?) need this to work in 3.2 too, I think we're going to add some way of opting in the new behaviour, e.g. add some static wxKeyEvent::UseLayoutIndependentKeyCodes(bool).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Let it be 3.3. There is no need to rush. We in the far2l project have been living with this problem since 2016, and a little more waiting is not catastrophic. Especially since I've already made a workaround (using GetRawKeyFlags() for getting Latin equivalents for keys on our side) that solves the problem for me personally :)
Also, this will be my first PR to wx, and also it will introduce a new dependency, so I need to take my time and learn everything properly so as not to break something. I also need to do a few more experiments first to make sure that the solution I proposed works reliably enough.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Actually modification itself is ready and tested (keyboard sample app works like expected), but I am not very familiar with configure scripts syntax and not sure how to add xkbcommon presence check.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I think you just need a PKG_CHECK_MODULES() for xkbcommon in configure.
I do have a couple of other questions about the code which I hoped to leave as comments in the PR, but let me do it here:
- Minor: I think it would be better to initialize
nameswith{0}, this is standard C++ and not a gcc extension (even if this code is very unlikely to have to be compiled with not a gcc compatible compiler, of course...) and it would take care of initializing any fields added to this struct in the future. - Shouldn't we check that
key_code_strhas exactly one character? I don't know if it can have more, but just in case it does, we shouldn't misrepresent it by returning just the first character. - Similar question about whether we really want to apply
toupper()unconditionally here, I'd check forislower()first just to be sure we're not doing something wrong. - But, in fact,
xkb_state_key_get_utf8()is documented as "applying keysym transformations" and it's not obvious whether we really want this here. Could we just usexkb_keysym_to_utf8()instead? Or maybexkb_keymap_key_get_name()? I haven't had time to actually test anything, but it seems like we shouldn't apply any transformations here. - Is
xkb_keymap_new_from_names()so cheap that it's not a problem to call it on every key press? I suspect it might be worth to store it in some global variable (in which case we'd need to usewxModuleto clean it up before the GUI shutdown). - Last and really least: please format wx code according to our style conventions, i.e. put
{s on their own lines.
Sorry for all these questions, but I'd like to be sure we're not doing something stupid here.
And thanks again for looking at this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
I think you just need a PKG_CHECK_MODULES() for xkbcommon in configure.
Done. See unxed@0c7c8e5
Also,
1, 2, 3, 6. fixed. See unxed@fb5b061
-
keysym transformations is described here. They are applied if Ctrl or Caps Lock keys are pressed. In our case, we're creating an "empty" keyboard state object, and passing only a character key into it, with no modifiers, so no transformations will be applied. xkb_keysym_to_utf8() takes keysyms, not evdev codes that we need to translate to chars. xkb_keymap_key_get_name() gives symbolic names for chars, its ok to use it for
A-Z, but not ok for keys like;or[. xkb_state_key_get_utf8() will work for all of them. -
As I am not very familiar with wxModule concept I just put [de]initialization in wxWindowGTK constructor/destructor. See unxed@c17c2ca Is it ok?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Could you please create a PR with your changes? This would make it simpler to comment on them, and you can always update it later if necessary, it's just a branch in your repository.
But, to keep commenting here:
- The changes to configure are not quite enough, you need to:
- Add
HAVE_XKBCOMMONtosetup.h.in. - Add
XKBCOMMON_LIBStoGUI_TK_LIBRARY. - Ideally add
XKBCOMMON_CFLAGStoCXXFLAGS, even if this library doesn't typically need any special CFLAGS.
- I do agree that using an empty state should result in the correct results, but it just seems a bit weird to go to the trouble of creating an empty state just to not really use it. I don't know xkb API at all, but are you sure there is no function for just getting what we need directly from the keymap?
- Doing it on a per-window basis really doesn't seem like a good idea because this increases size (and adds overhead to the creation of) all windows, even if 99% of them never process any keys. You can see a simple example of using
wxModuleinsrc/gtk/settings.cppfor example: basically, you just define a class inheriting from it, do nothing in itsOnInit()and free the resources (which should be created on demand) in itsOnExit().
Please let me know if you have any questions!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
- Done, please review.
- Could not find an easier way to do it. Probably we could parse xkb_keymap_key_get_name() results, but in this case we should write a complex switch-case for every non-latin-letter key hoping we do not forget some. Do not sounds very reliable.
- Switched to wxModule, it really makes more sense.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
are you sure there is no function for just getting what we need directly from the keymap?
The only way to avoid using xkb_state_key_get_utf8() is to use pre-generated translation table, as mapping of x11 scan codes to Latin chars will hardly change in future. Looks a bit weird, but allows to avoid xkbcommon dependency.
static const int x11_to_latin[] =
{
0, 0, 0, 0, 0, 0, 0, 0,
0, 27, 49, 50, 51, 52, 53, 54,
55, 56, 57, 48, 45, 61, 8, 9,
81, 87, 69, 82, 84, 89, 85, 73,
79, 80, 91, 93, 13, 0, 65, 83,
68, 70, 71, 72, 74, 75, 76, 59,
39, 96, 0, 92, 90, 88, 67, 86,
66, 78, 77, 44, 46, 47, 0, 42,
0, 32, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 45, 0, 0, 0, 43, 0,
0, 0, 0, 0, 0, 0, 60, 0,
0, 0, 0, 0, 0, 0, 0, 0,
13, 0, 47, 0, 0, 10, 0, 0,
0, 0, 0, 0, 0, 0, 0, 127,
0, 0, 0, 0, 0, 61, 194, 0,
0, 46, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 40, 41, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 36, 226
}
int x11_scan_code_to_latin_char(int raw_key_code) {
if (raw_key_code < 444) { return x11_to_latin[raw_key_code]; }
else return 0;
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I don't think it's worth hardcoding the keys, dependency on libxkbcommon should be fine as libgtk-3-dev depends on libxkbcommon-dev anyhow, so it should be always available.
So I think it should be fine as is, but please make a PR so that the CI jobs could run and confirm that this doesn't break anything. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
please make a PR so that the CI jobs could run and confirm that this doesn't break anything
#23410 :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Ubuntu 18.04 wxGTK 2 fails with
./src/gtk/window.cpp:59:10: fatal error: xkbcommon/xkbcommon.h: No such file or directory
probably we missed something
On other systems build goes ok, but one Xvfb test fails:
2023-03-31T16:32:01.5005540Z -------------------------------------------------------------------------------
2023-03-31T16:32:01.5005971Z KeyboardEventTestCase
2023-03-31T16:32:01.5006226Z NormalLetter
2023-03-31T16:32:01.5006664Z -------------------------------------------------------------------------------
2023-03-31T16:32:01.5007028Z ./events/keyboard.cpp:204
2023-03-31T16:32:01.5007312Z ...............................................................................
2023-03-31T16:32:01.5007490Z
2023-03-31T16:32:01.5007619Z ./events/keyboard.cpp:167: FAILED:
2023-03-31T16:32:01.5007988Z REQUIRE( (char)desc.m_keycode == (char)ev.GetUnicodeKey() )
2023-03-31T16:32:01.5008291Z with expansion:
2023-03-31T16:32:01.5008550Z 'A' == 'a'
2023-03-31T16:32:01.5008777Z with message:
2023-03-31T16:32:01.5009040Z wrong Unicode key in key down event at line 256
2023-03-31T16:32:01.5009228Z
2023-03-31T16:32:01.5037700Z 0.106 s: NormalLetter
2023-03-31T16:32:01.5038009Z 0.000 s: KeyboardEventTestCase
2023-03-31T16:32:01.6072961Z 0.103 s: NormalSpecial
2023-03-31T16:32:01.6073343Z 0.103 s: KeyboardEventTestCase
2023-03-31T16:32:01.7551670Z -------------------------------------------------------------------------------
2023-03-31T16:32:01.7552184Z KeyboardEventTestCase
2023-03-31T16:32:01.7552509Z CtrlLetter
2023-03-31T16:32:01.7553209Z -------------------------------------------------------------------------------
2023-03-31T16:32:01.7553669Z ./events/keyboard.cpp:206
2023-03-31T16:32:01.7554080Z ...............................................................................
2023-03-31T16:32:01.7554693Z
2023-03-31T16:32:01.7554865Z ./events/keyboard.cpp:167: FAILED:
2023-03-31T16:32:01.7555281Z REQUIRE( (char)desc.m_keycode == (char)ev.GetUnicodeKey() )
2023-03-31T16:32:01.7555674Z with expansion:
2023-03-31T16:32:01.7556009Z 'Z' == 'z'
2023-03-31T16:32:01.7556286Z with message:
2023-03-31T16:32:01.7556639Z wrong Unicode key in key down event at line 290
2023-03-31T16:32:01.7556881Z
I guess the problem is in test here: it requires KeyCode value to be equal to UnicodeKey value for Latin letters. But as we are trying to mimic Windows behavior where KeyCode is always uppercase for Latin letters, we can not guarantee this now. Of course we can make UnicodeKey uppercase, but I am not sure it is a good idea.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
The suspicious thing here is that the tests currently pass under MSW, so I don't understand how can mimicking MSW behaviour make the test fail. I can't debug this myself right now but something seems fishy here.
I've addressed GTK 2 issue and a couple of other ones in a comment on the PR itself.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
I don't understand how can mimicking MSW behaviour make the test fail
Digged into that issue. On Windows, for KeyUp/Down events, the UnicodeKey field really contains the same value as the Keycode field (strange, but true). And the real unmodified Unicode character comes in the UnicodeKey field of OnChar event. But if we copy this behavior, then in Unix systems there will be no way to get the original unmodified Unicode character since onChar event does not fire on *nixes (#17643).
As for other minor issues, all of them are fixed now. As for Russian layout keypress test I still not sure how to write it correctly.
For Ubuntu 18.04 I now get configure error:
configure: error: Package requirements (xkbcommon) were not met:
No package 'xkbcommon' found
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Fixed #17643 (trying to mimic Windows version behavior as close as possible). Now we can set UnicodeKey field in KeyUp/Down events to the same value as KeyCode without losing a Unicode character as it can be fetched in OnChar handler. Just as it works on Windows.
Keyboard test is now passing without modifications.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #23379 as completed via 2c0f6a2.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Looks like 2c0f6a2 also fixes #23421. An old bug, forgot to create an issue about it earlier. And now it looks like it is eliminated along with other bug we fixed by this change.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Also #22055 is not reproducing after 2c0f6a2, closed it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
@vadz my colleagues from the Internet have noticed that the value of the wx key_code does not necessarily fit into the range of an unsigned char expected by isalpha(). So it would be better to change this line:
if ( event.ControlDown() && isalpha(event.m_keyCode) )
to something like
if ( event.ControlDown() && (event.m_keyCode >= 0 && event.m_keyCode <= UCHAR_MAX && isalpha(event.m_keyCode)) )
to avoid undefined behavior. Doesn't seem to affect anything, as I've done experiments and isalpha() under glibc works correctly for all valid wx keycodes. But it's better to be safe. At least new codes can be added in future.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks, I agree that using isalpha() without checking the input is wrong, but looking at this code, I think that even using it at all is not really correct neither, so I'd rather apply this:
diff --git a/src/gtk/window.cpp b/src/gtk/window.cpp index 4a892098b7..87e15041a5 100644 --- a/src/gtk/window.cpp +++ b/src/gtk/window.cpp @@ -1413,9 +1413,12 @@ gtk_window_key_press_callback( GtkWidget *WXUNUSED(widget), { wxKeyEvent eventChar(wxEVT_CHAR, event); - if ( event.ControlDown() && isalpha(event.m_keyCode) ) + // Check for the special case of Ctrl+letter, see comment before + // AdjustCharEventKeyCodes(). + if ( event.ControlDown() && + ((event.m_keyCode >= 'a' && event.m_keyCode <= 'z') || + (event.m_keyCode >= 'A' && event.m_keyCode <= 'Z')) ) { - // Ctrl+letter is handled specially by AdjustCharEventKeyCodes(). eventChar.m_keyCode = event.m_keyCode; eventChar.m_uniChar = event.m_uniChar; }
Do you see any problems with this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Looks ok!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
unxed
Oh, sorry, but we suddenly broke things up! Retested on fresh master: OnChar contains zero key codes for Alt+RussianLetter once again! Need to apply a minor fix:
diff --git a/src/gtk/window.cpp b/src/gtk/window.cpp
--- a/src/gtk/window.cpp
+++ b/src/gtk/window.cpp
@@ -1425,7 +1425,7 @@ gtk_window_key_press_callback( GtkWidget *WXUNUSED(widget),
else
{
// use Unicode values
- eventChar.m_keyCode = key_code;
+ eventChar.m_keyCode = key_code ? key_code : event.m_keyCode;
eventChar.m_uniChar = uniChar;
}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.