GNOME Bugzilla – Bug 561275
shortcuts don't work if keysym bound to multiple keycodes
Last modified: 2008-11-24 20:51:47 UTC
Please describe the problem: It appears that machines that have a software eject key are not able to eject disks from gnome. In evaluating the cause of the problem, a lot of possible indicators show up. At a first glance it seems to be an X bug because pressing the key mapped to XF86Eject shows this output in xev: <jcristau> state 0x0, keycode 170 (keysym 0x1008ff2c, XF86Eject), same_screen YES, <jcristau> XKeysymToKeycode returns keycode: 169 This is actually the intended behavior though because there are indeed two keysyms that match: <jcristau> key <I169> { [ XF86Eject ] }; <jcristau> key <I170> { [ XF86Eject, XF86Eject ] }; There are two keysyms that match because the kernel supports two different eject keys that act very similarly: <jcristau> correspond to the following in the kernel: <jcristau> #define KEY_EJECTCD 161 <jcristau> #define KEY_EJECTCLOSECD 162 Gnome-settings-daemon gets confused then because the keycode you press in X is not the keycode it is expecting since it uses XKeysymToKeycode. Steps to reproduce: 1. Run xev and press your XF86Eject key 2. Verify it mapped to two different possible keycodes 3. Try to eject a mounted disk by pressing XF86Eject w/ gsd running Actual results: The disk doesn't eject Expected results: The disk ejects Does this happen every time? Yes Other information: This is with Ubuntu 8.10 gold
Created attachment 122915 [details] [review] Patch to resolve the issue This also helps a variety of other keys including Play and WWW.
Thanks. I haven't had time for an in-depth review, yet, but the patch has serious indentation issues. Please try to follow the coding style of the surrounding code.
Created attachment 122973 [details] [review] cleaned up indentations in the patch This patch should try to match indentation in the different files for each section.
Coding style also includes things like using "ret = func (a, b);" instead of "ret=func(a,b);" and stuff like that. + keycode = &tmp_keycode; + *keycodes=g_slist_append(*keycodes,GINT_TO_POINTER(keycode)); You want to store the keycode here. So why do you store the pointer to the keycode (and try to turn it into a pointer)? That means you're reading from invalid memory later on. + /* we need to look through the entire keymap in case there + * are multiple matches */ + gint min,max; + XDisplayKeycodes(GDK_DISPLAY(),&min,&max); + for (i=min; i< max; i++) + { + if (XKeycodeToKeysym(GDK_DISPLAY(), i, 0) == keyval) + { + *keycodes=g_slist_append(*keycodes,GINT_TO_POINTER(i)); + } + } You should use gdk_keymap_get_entries_for_keycode instead. That gives you an array instead of a list which also makes handling a bit simpler. +++ gnome-settings-daemon-2.24.0.new/plugins/keybindings/gsd-keybindings-manager.c 2008-11-18 14:29:21.000000000 -0500 @@ -126,7 +126,7 @@ if (egg_accelerator_parse_virtual (binding->binding_str, &binding->key.keysym, - &binding->key.keycode, + NULL, &binding->key.state) == FALSE) { This is wrong. If you don't parse the keycode we lose the ability to define a shortcut by keycode only (e.g. if the keymap isn't configured correctly). Just put the keycodes array into the Key struct instead of a single keycode and you should be ok. As a bonus task, I'd be interested in what kind of a difference going through the entire keymap for each key makes for the plugin startup sequence, performance-wise.
Created attachment 123082 [details] [review] Updated patch w/ array changes This patch should meet all of your requests from the previous one. It is now using that array that gdk_keymap_get_entries_for_keyval is returning and passing it around instead. For your bonus task, I added in local patch a gettimeofday call at the start and end of init_kbd. I did 10 runs with the patch and 10 runs without. Surprisingly, both came to the exact same average of 0.03264 seconds.
Created attachment 123213 [details] [review] patch against trunk I've changed a few more things, and fixed some memory issues with your patch. This one also applies against trunk instead of 2.24. If this works for you, too, I'll apply this one.
Created attachment 123293 [details] [review] Patch against 2.24 Hi Jens: I made a few minor changes to make it apply against 2.24, but yes it appears to work correctly, so go ahead and apply yours to trunk. Any distros that want to pick this up using 2.24 can use the patch i'll attach for 2.24 Thanks!
Actually, I spoke too soon. You've got some memory issues that have developed with your implementation. Too many keys are getting grabbed. I think the problem is an assumption you made that these arrays are NULL terminated, which I don't think they are. With some extra debug output turned on: ** (test-media-keys:10094): DEBUG: Key name: XF86AudioPlay ** (test-media-keys:10094): DEBUG: number of key codes: 3 ** (test-media-keys:10094): DEBUG: adding in key index j of 0 ** (test-media-keys:10094): DEBUG: adding in key index i of 0 ** (test-media-keys:10094): DEBUG: adding in key code of 172 ** (test-media-keys:10094): DEBUG: adding in key index j of 1 ** (test-media-keys:10094): DEBUG: adding in key index i of 1 ** (test-media-keys:10094): DEBUG: adding in key code of 208 ** (test-media-keys:10094): DEBUG: adding in key index j of 2 ** (test-media-keys:10094): DEBUG: adding in key index i of 2 ** (test-media-keys:10094): DEBUG: adding in key code of 215 ** (test-media-keys:10094): DEBUG: Key sym: 0x1008ff14 ** (test-media-keys:10094): DEBUG: Grabbing keycode: 172 ** (test-media-keys:10094): DEBUG: Grabbing keycode: 208 ** (test-media-keys:10094): DEBUG: Grabbing keycode: 215 ** (test-media-keys:10094): DEBUG: Grabbing keycode: 17 ** (test-media-keys:10094): DEBUG: Grabbing keycode: 269025044
Created attachment 123300 [details] [review] Updated patch against trunk Given your assumption of checking for a zero value, make the array at least one element larger that is preset to zero.
Created attachment 123301 [details] [review] Updated patch against 2.24
(In reply to comment #9) > Given your assumption of checking for a zero value, make the array at least one > element larger that is preset to zero. Good catch. I don't know why I did that correctly for the special but failed to do so for the normal one... Note that my patch had a few problems with memory leaks, too. Should all be fixed in trunk. 2008-11-24 Jens Granseuer <...> When multiple keys (keycodes) were mapped to the same keysym, g-s-d would only accept the first of those keycodes in the keymap as a valid shortcut. To fix this, instead of checking against a single keycode, we need to grab all keycodes that match the respective keysym (bug #561275). With thanks to Mario Limonciello <...> * plugins/common/eggaccelerators.c: (egg_accelerator_parse_virtual): * plugins/common/eggaccelerators.h: possibly return multiple keycodes * plugins/common/gsd-keygrab.c: (grab_key_unsafe), (key_uses_keycode), (match_key): grab all matching keys * plugins/common/gsd-keygrab.h: * plugins/keybindings/gsd-keybindings-manager.c: (parse_binding), (bindings_get_entry), (same_keycode), (same_key), (key_already_used), (binding_register_keys), (gsd_keybindings_manager_stop): * plugins/media-keys/gsd-media-keys-manager.c: (update_kbd_cb), (init_kbd), (gsd_media_keys_manager_stop): update to handle changes in data structures