GNOME Bugzilla – Bug 140448
Metacity keybindings with hex-values have no affect
Last modified: 2006-08-22 15:51:09 UTC
Distribution: Debian testing/unstable Package: metacity Severity: normal Version: GNOME2.6. unspecified Gnome-Distributor: GNOME.Org Synopsis: Metacity keybindings with hex-values have no affect Bugzilla-Product: metacity Bugzilla-Component: general Bugzilla-Version: unspecified Description: Description of Problem: Assigning a keybind that 'belongs' to Metacity that has no literary value does not work. If I assign i.e. Steps to reproduce the problem: 1. Click 'Toggle shaded state' in the Keyboard shortcuts dialog 2. Choose a key that produces a hex-value (for me i.e. '0xa0') 3. Use that button Actual Results: None Expected Results: A shaded window. ;) Additional Information: If I assign it to another button, i.e. the 'Pause'-key (no hex-value), the button works. This seems to happend to all keybindings under Window Management. For an action under Sound and Desktop they work. I'm running Debian with Metacity 2.8. ------- Bug moved to this database by unknown@bugzilla.gnome.org 2004-04-19 08:40 ------- Unknown platform unknown. Setting to default platform "Other". Unknown milestone "unknown" in product "metacity". Setting to default milestone for this product, '---' The original reporter of this bug does not have an account here. Reassigning to the person who moved it here, unknown@bugzilla.gnome.org. Previous reporter was orange@fobie.net. Setting to default status "UNCONFIRMED". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one.
This is because the eggaccelerators.c in metacity/ is significantly behind the eggaccelerators.c in gnome-settings-daemon/; comparing http://cvs.gnome.org/viewcvs/metacity/src/eggaccelerators.c?rev=1.3&view=auto and http://cvs.gnome.org/viewcvs/gnome-control-center/gnome-settings-daemon/eggaccelerators.c?rev=1.3&view=auto one can see that the eggaccelerators.c in gnome-settings-daemon/ supports 0x## codes where the eggaccelerators.c in metacity/ does not. The relevant code is at and following line 331 in gnome-settings-daemon/eggaccelerators.c: if (keyval == 0) { /* If keyval is 0, than maybe it's a keycode. Check for 0x## */ if (len >= 4 && is_keycode (accelerator)) { ... The best fix would be to update eggaccelerators.c in metacity/ to that in gnome-settings-daemon/, as this will ensure that the behaviour of the two keybinding systems becomes as similar as possible. (Updating to the current libegg version is another option.) The signature of egg_accelerator_parse_virtual() has changed so this will require altering code that calls it. I'll see if I can work up a patch.
Created attachment 33450 [details] [review] metacity-2.9.0-0x00-keycodes.patch Against 2.9.0 tarball.
Created attachment 33451 [details] [review] metacity-2.9.0-0x00-keycodes.patch Corrected for indenting
Note: patch is valid on current CVS HEAD. Sorry for the size, but I couldn't see a way to make it smaller.
Comment on attachment 33451 [details] [review] metacity-2.9.0-0x00-keycodes.patch The changes to metacity proper look reasonable to me. For eggcelerators.[hc] you should be sure to update it by getting latest libegg then doing "make regenerate-built-sources" so we are sure we don't have any manual edits in there. As a side note, I guess we need to figure out how to get eggcelerators in gtk - not sure there's even a bug for it.
Actually, it appears that the changes to enable 0x## codes in the gnome-settings-daemon eggaccelerators.[hc] are not from libegg but are a manual edit. I can't quite see who was responsible - in acme, the eggaccelerators.[hc] are the real thing but by the time acme was merged into gnome-settings-daemon the change had already been made: compare http://cvs.gnome.org/viewcvs/acme/src/eggaccelerators.h?rev=1.1&view=markup and http://cvs.gnome.org/viewcvs/gnome-control-center/gnome-settings-daemon/eggaccelerators.h?rev=1.1&view=markup - the current libegg version is at http://cvs.gnome.org/viewcvs/libegg/libegg/treeviewutils/eggaccelerators.h?rev=1.1&view=auto (it's the function signature egg_accelerator_parse_virtual that's different, btw) However, this means that for things to work as expected (consistency!) either 1. Metacity will need to use the modified eggaccelerators.[hc] (but without Jody Goldberg's use of GDK_DISPLAY() - http://cvs.gnome.org/viewcvs/gnome-control-center/gnome-settings-daemon/eggaccelerators.c?r1=1.1&r2=1.2&makepatch=1&diff_format=u - which breaks in Metacity). 2. Either that or feed back the 0x## support into 'upstream' libegg. 3. Or put gnome-settings-daemon back to the libegg version, which will annoy people who are used to the fancy keys on their fancy keyboards working. I'd prefer 1. or 2., mainly because I'm one of those people with a fancy keyboard.
Argh, someone messed up. We need to feed all changes back into "upstream" libegg, then be sure both apps are using it. Or even better, figure out how we land this in some real library. In Jody's patch, perhaps GDK_KEYMAP(keymap)->display would be a better choice than GDK_DISPLAY(), or if no GdkKeymap or GdkDisplay is passed in to that function, add a new variant of the function that's fixed to take a keymap/display arg.
OK, yep. egg_accelerator_parse_virtual() is and should be DISPLAY-independent, so it should be a function that sets either a keysym or keycode depending on the form of the parse string - no need to make it too intelligent. Then we can add a function egg_accelerator_parse_virtual_to_keycode() which takes a signature (const gchar *, KeyCode *, EggVirtualModifierType *, Display *) - no (KeySym *) argument - and if appropriate uses the (Display *) to convert a keysym to keycode. gnome-settings-daemon can use this, as currently the keysym argument is actually unused (although it is passed around in a structure, it is never referenced). I'll code up patches to metacity, libegg and gnome-settings-daemon.
Created attachment 33573 [details] [review] libegg.patch
Created attachment 33574 [details] [review] metacity.patch
Created attachment 33575 [details] [review] gnome-control-center.patch
Patches as proposed, against CVS HEAD. In the libegg patch: eggaccelerators.[ch]: add argument guint *accelerator_keycode to egg_accelerator_parse_virtual() takes a keycode if the accelerator key is of the form 0x##; add derived function egg_accelerator_parse_virtual_to_keycode() uses GdkKeymap to map keysym in the accelerator string to a keycode; add argument guint accelerator_keycode to egg_virtual_accelerator_name(), output in the returned string as 0x## if nonzero and keyval is not given In the metacity patch: eggaccelerators.[ch]: update from libegg keybindings.c, prefs.[ch], ui.[ch]: handle keycodes as well as keyvals, some cleanup In the gnome-control-center patch: eggaccelerators.[ch]: update from libegg gnome-settings-keybindings.c, gnome-settings-multimedia-keys.c, acme.h: use egg_accelerator_parse_virtual_to_keycode() convenience function, remove redundant keysym code Sorry this took so long.
All looks reasonable to me. You probably need to ping the control center people.
*** Bug 137297 has been marked as a duplicate of this bug. ***
*** Bug 112968 has been marked as a duplicate of this bug. ***
This has been approved on the metacity side. Have the control center people been notified? Seems like there's really no obstacle to this being committed.
Ed, Bastien, Sebastien: Is everything okay from the control center side of things and "upstream" libegg to commit this now? (I'm marking the gnome-control-center.patch as commented-on although an isn't-for-this-module might be more appropriate if it existed)
fine with me for the g-c-c part
The libegg patch is outdated, I already sync'ed both g-c-c and libegg some months ago: 2004-11-30 Bastien Nocera <hadess@hadess.net> * libegg/treeviewutils/eggaccelerators.c: (is_keycode), * libegg/treeviewutils/eggaccelerators.h: * libegg/treeviewutils/eggcellrendererkeys.c: * libegg/treeviewutils/eggcellrendererkeys.h: update from the gnome-control-center (was never updated after the keycode changes) What are the actual changes against libegg nowadays? The copied files (the ones mentioned above) should really be omitted from the patches, apart from the libegg patch if changes are necessary.
*** Bug 144223 has been marked as a duplicate of this bug. ***
The relevant libegg code has been removed from metacity and replaced by the new gtk+ stuff. Did that fix this bug as a side-effect, or is it still relevant?
Uh, that's this[1] commit, right? Came after 2.15.13 release, so I don't have it yet. I'll patch it and check. [1] http://cvs.gnome.org/viewcvs/metacity/src/ui.c?r1=1.61&r2=1.62
Yes, that's right. The change was part of the 2.15.21 release made yesterday.
No, it doesn't work. gtk_accelerator_parse doesn't handle "0x##" syntax, because that's a keycode, not a keyval. The problem is that these extended keys don't have keyvals associated. For the media keys control-center (gnome-settings-daemon) works round this by binding them to XF86AudioNext etc.; however there isn't an appropriate keycode for e.g. "Toggle shaded state", so that won't work here. I guess if gtk_accelerator_parse fails, metacity should check if the string matches "0x##" and if so parse it as a keycode.
doh! Okay, reopening then. :(
Created attachment 70520 [details] [review] meta-keybind.patch OK. This patches metacity as above. It only supports extended keys on their own, not with modifiers. If we want to support that we'll have to reimport from libegg.
I'm not so worried about modifiers with those special keys. Patch looks fine to me in my quick overview. Can I get an additional tester or two (/me tries to volunteer Bjorn and Thomas, plus people on the cc list) and then we can apply this before the next release?
Hmm, that call for testers didn't work too well. Let's try a different way of requesting help with testing. ;-) 2006-08-21 Elijah Newren <newren gmail com> Patch from Ed Catmur to fix keybindings with hex-values (coming from special extended keyboard keys). #140448. * src/keybindings.c (struct _MetaKeyBinding): change keycode from KeyCode to unsigned int (comment from Elijah: why???), (reload_keycodes): only grab keysyms for keybindings that have them, (count_bindings, rebuild_binding_table): bindings can be valid either due to a valid keysym or a valid keycode, (display_get_keybinding_action, meta_change_keygrab, process_tab_grab, process_workspace_switch_grab): handle keycode as well as keysym * src/prefs.[ch] (struct MetaKeyCombo, update_binding, update_list_binding): handle keycode as well as keysym * src/ui.[ch] (meta_ui_accelerator_parse): new function special cases strings of the form "0x[0-9a-fA-F]+" and otherwise calling gtk_accelerator_parse(), (meta_ui_parse_accelerator, meta_ui_parse_modifier): call meta_ui_accelerator_parse instead of gtk_accelerator_parse. Thanks Ed!!
(In reply to comment #28) > * src/keybindings.c (struct _MetaKeyBinding): change keycode from > KeyCode to unsigned int (comment from Elijah: why???), It's the other way round! @@ -232,8 +232,8 @@ struct _MetaKeyBinding { const char *name; KeySym keysym; + KeyCode keycode; unsigned int mask; - unsigned int keycode; MetaVirtualModifier modifiers; const MetaKeyHandler *handler; }; (Perhaps I shouldn't have moved the variable up in the struct?)
(In reply to comment #29) > (In reply to comment #28) > > * src/keybindings.c (struct _MetaKeyBinding): change keycode from > > KeyCode to unsigned int (comment from Elijah: why???), > > It's the other way round! <snip> > (Perhaps I shouldn't have moved the variable up in the struct?) Perhaps I should read more carefully. ;-) I thought it was really odd that I "hadn't noticed the change to unsigned int when I first looked over the patch" and only noticed it when I went to type up the changelog. Now I know why...