GNOME Bugzilla – Bug 765259
Save Windows scancode inside GdkEvent
Last modified: 2017-06-22 16:20:09 UTC
Created attachment 326326 [details] [review] Patch to save that information Some application requiring low level information on keys pressed. On Windows the hardware_keycode saved is too high level and the low level scancode is lost. The patch attached save the scancode in order to make possible for these program to use it.
The traditional way to get at such low level information is to use an event filter, see gdk_window_add_filter.
This patch also breaks ABI compatibility.
Unfortunately however the filter solution will make events handle differently so we'll loose the portability which gtk provides.
(In reply to Paul Davis from comment #2) > This patch also breaks ABI compatibility. No, this was tested. The added field is in a specific position in order to not break the ABI.
(In reply to Frediano Ziglio from comment #3) > Unfortunately however the filter solution will make events handle > differently so we'll loose the portability which gtk provides. This needs some more explanation. I don't understand what you mean here. In any case, putting backend-specific fields into the public GdkEvent structs is pretty much out of the question.
You changed the size of the event object and the offsets to several existing members. I don't see how this cannot break ABI.
it depends on the compiler. Here is what pahole says on my system: struct _GdkEventKey { GdkEventType type; /* 0 4 */ /* XXX 4 bytes hole, try to pack */ GdkWindow * window; /* 8 8 */ gint8 send_event; /* 16 1 */ /* XXX 3 bytes hole, try to pack */ guint32 time; /* 20 4 */ guint state; /* 24 4 */ guint keyval; /* 28 4 */ gint length; /* 32 4 */ /* XXX 4 bytes hole, try to pack */ gchar * string; /* 40 8 */ guint16 hardware_keycode; /* 48 2 */ guint8 group; /* 50 1 */ guint is_modifier:1; /* 51:31 4 */ /* size: 56, cachelines: 1, members: 11 */ /* sum members: 44, holes: 3, sum holes: 11 */ /* padding: 1 */ /* bit_padding: 31 bits */ /* last cacheline: 56 bytes */ };
it totally depends on the compiler, and the compile-time flags. which just doubles down on the reasons not to include this in a public event structure.
To point this in a more constructive direction, look at _gdk_event_set_pointer_emulated _gdk_event_get_pointer_emulated That is essentially x11-backend-specific information that we attach to events. In this case there is no public api. In your case, I would expect that you need a int gdk_win32_event_get_scancode (GdkEvent *event) or somesuch
After talking this through at some length on irc, the consensus seems to be to a) store this in the GdkEventPrivate struct b) make it not backend-specific, and instead just set it to the same value as hardware_keycode in all backends, except for Windows c) Add a gdk_event_get_scancode API to read the value
Oh, and probably: d) Document what value can be found in hardware_keycode on various platforms
Created attachment 326393 [details] [review] New version of the patch This version contain the change as suggested. Tested and the scancode was returned correctly to Windows application.
Review of attachment 326393 [details] [review]: Looks good, but you should add a private `gdk_event_set_scancode()` function that does the `GdkEventPrivate*` cast and checks that you're trying to set the scancode to a key event. ::: gdk/gdkevents.h @@ +1453,3 @@ +GDK_AVAILABLE_IN_3_22 +int gdk_event_get_scancode (GdkEvent *event); Coding style: you should align the declaration with the rest of the header.
Created attachment 326399 [details] [review] New version of the patch (2) This version for indentation and add a private gdk_event_set_scancode so set the field.
Review of attachment 326399 [details] [review]: Looking good overall. Some small fixups left. ::: gdk/broadway/gdkeventsource.c @@ +296,3 @@ event->key.state = message->key.state; event->key.hardware_keycode = message->key.key; + _gdk_event_set_scancode(event, message->key.key); Please put a space before ( ::: gdk/gdkevents.c @@ +2481,3 @@ + * Gets the keyboard low level scancode. + * This is usually hardware_keycode. + * On Windows this is the high work of WM_KEY{DOWN,UP} lParam Typo here: s/work/word/ ::: gdk/mir/gdkmireventsource.c @@ +134,3 @@ event->key.keyval = keyval; event->key.hardware_keycode = keycode + 8; + _gdk_event_set_scancode(event, keycode + 8); Style nit: please put a space before ( There's several more occurrences of this ::: gtk/gtkplug.c @@ +969,3 @@ event->key.state = (GdkModifierType) xevent->xkey.state; event->key.hardware_keycode = xevent->xkey.keycode; + _gdk_event_set_scancode(event, xevent->xkey.keycode); This can't work, since the function is internal to libgdk and not exported. If we need to do this here, we should add the function to the private call mechanism we have in gdk/gdk-private.h. I note that we don't do this for any of the other private event fields.
Created attachment 326411 [details] [review] Proposed patch Fixed typo and syntax. I tried to fix the export problem but I don't know if this way is acceptable. Basically I return the hardware_keycode if scancode is 0 (but not on Windows).
Review of attachment 326411 [details] [review]: ::: gdk/gdkevents.c @@ +2501,3 @@ + if (!private->key_scancode) + return event->key.hardware_keycode; +#endif No need for an ifdef here, I think. The only question with this approach: can 0 be a valid scancode ? if not, this seems a good solution, and would even let you avoid setting the scancode at all in the other backends. ::: gdk/gdkinternals.h @@ +191,3 @@ GdkSeat *seat; GdkDeviceTool *tool; + guint16 key_scancode; Same here: just use a guint @@ +400,3 @@ +void _gdk_event_set_scancode (GdkEvent *event, + guint16 scancode); + I would just make the parameter an guint here.
No, 0 is not valid (I think is reserved for a comment to the keyboard). Anyways, scancodes start from 1. The #ifdef was needed as in Windows keycode and scancode are different so you can't return keycode if scancode is not valid. Why guint instead of guint16 ? hardware_keycode is also guint16. I'll post an update with scancode returning 0 is not set.
Created attachment 326419 [details] [review] New proposed patch Returns 0 if not set.
Review of attachment 326419 [details] [review]: ::: gdk/gdkevents.c @@ +2479,3 @@ + * @event: a #GdkEvent + * + * Gets the keyboard low level scancode. low-level, please @@ +2496,3 @@ + + private = (GdkEventPrivate *) event; + return private->key_scancode; This is fine with me, but then we should put the set_scancode calls back in the other backends, so this function works regardless of backend.
(In reply to Matthias Clasen from comment #17) > The only question with this approach: can 0 be a valid scancode ? if not, > this seems a good solution, and would even let you avoid setting the > scancode at all in the other backends. (In reply to Matthias Clasen from comment #20) > This is fine with me, but then we should put the set_scancode calls back in > the other backends, so this function works regardless of backend. Is not clear. Should I set on all backends or not?
(In reply to Frediano Ziglio from comment #21) > Is not clear. Should I set on all backends or not? Yes. gdk_event_get_scancode() should work on all backends. It will return the same as event_key.hardware_keycode on systems that don't have the scancode/vk distinction (ie all backends other than Windows)
Created attachment 326491 [details] [review] New version Changed: - fixed style (low-level); - update comment; - set scancode for all backends.
Review of attachment 326491 [details] [review]: looks good now, thanks
Looks like i was mistaken. This indeed works as you said. At first i thought that my explanation was essentially correct, but that it the situation was screwed by GDK using virtual key codes (instead of scancodes) as "hardware keycodes" on Windows. So i've tried to remedy that. Then it turned out that this doesn't work even with scancodes. The last part of _gtk_key_hash_lookup() specifically derails this by checking that the "wrong" keyval being reacted to is part of the group. If it is, it bails out and returns NULL. This means that "Undo" works as Ctrl+Z in US layout, as Ctrl+Z in RU layout (because it's impossible to type 'z' in RU layout, so the last test passes; and the initial test passes because virtual keycodes are positionally the same between US and RU layouts), but only as Ctrl+Y in DE layout (because DE layout does have Z character typed by the key that is 'Y' in US layout, so even if i fudge the code to pass through the first lookup by using scancodes (which do remain the same) instead of virtual keycodes, the last check notes that DE layout is capable of producing 'Z' normally, and bails). After that it dawned on me that i should check to see how other applications work. Turns out, Firefox and Notepad work exactly the same way - Ctrl+Z in US and RU, Ctrl+Y in DE. This is unexpectedly bad. I wonder, how shortcut i18n works in these applications then...Maybe they just explicitly define different shortcut maps for different UI languages...? Tomorrow i'll check to see how Gnome is dealing with it.
Sorry, commented the wrong bug. The above was intended for bug 768722.