GNOME Bugzilla – Bug 662755
Keycode support in GtkCellRendererAccel broken
Last modified: 2013-09-17 02:17:12 UTC
Which means that we would be able to use it to assign keys without keysyms. This is especially useful as used in gnome-control-center and gnome-settings-daemon, to provide shortcuts for keys unknown to X.org. To reproduce: - Find a key that's physically on your keyboard in "xmodmap -pke" output. Eg.: keycode 234 = XF86AudioMedia NoSymbol XF86AudioMedia (you can double-check that it's the case with xev) - Disable all the keysyms for that key: xmodmap -e "keycode 234 = " - Verify: $ xmodmap -pke | grep 234 keycode 234 = In testaccel (after changing the mode to GTK_CELL_RENDERER_ACCEL_MODE_OTHER), I now get "Tools" as the label instead of "0xea".
There is code to handle that, but it seems to be broken.
It works fine here, now that you've added the keycode property - do you still have issues ?
Yep, it works now (and I've nuked the old egg renderer from gnome-control-center). My media key was generating Tools, for some reason (thanks Dell). So the last reason why we have custom code in gnome-settings-daemon and gnome-control-center is because gtk_accelerator_parse() and gtk_accelerator_name() don't support keycodes. For example, "<Primary>0xb3" should be supported.
Note that I don't think we want to have those as general purpose functions (we don't want to use keycodes in menu items for example), but we could add those as helper functions to GtkCellRendererAccel, as otherwise you need to use other code to feed/use the keyboard shortcuts.
Sounds fine to me
Created attachment 200618 [details] [review] gtk: Add fuller keybindings parsing functions Which handle keycodes as well as keyvals, so we can use it in applications that use GtkCellRendererAccel's "Other" mode of operations (namely gnome-control-center and gnome-settings-daemon).
Found the little problem that '<Primary><Alt>z' would give me 'Ctrl+Alt+Meta+Z'. I'm guessing that this should read 'Ctrl+Alt+Z' instead. Will need some bitmask juggling with gdk_keymap_add_virtual_modifiers() in gtk_accelerator_get_label_full() and gtk_accelerator_name_full().
About the Alt+Meta problem, see: https://mail.gnome.org/archives/gtk-devel-list/2011-October/msg00089.html
Created attachment 200634 [details] [review] The patch from above mail archive link
Created attachment 200636 [details] [review] gtk: Add test program for keycode parsing
I could verify that Mitch's patch fixes the above problem with the new test program.
Created attachment 200637 [details] [review] updated patch Better keycode parsing as used in gnome-control-center/gnome-settings-daemon.
I think _full() doesn't say much, what about _with_keycode()? Also, why is there both _gtk_accelerator_parse_full() and gtk_accelerator_parse_full()?
Review of attachment 200634 [details] [review]: We've taken this to the list, and there was no outcry. So, lets go with this. This needs to be mentioned in the 3.4 section in README.in, though. Ideally, we'd also have some other place in the docs where we explain basics of accel handling...
Review of attachment 200636 [details] [review]: Looks good, even though it is not a very extensive test suite...
Review of attachment 200637 [details] [review]: I agree with mitch that _full is not a great name for this. _with_keycode is long, but a bit better. ::: gtk/gtkcellrendereraccel.c @@ +768,3 @@ + * @accelerator: string representing an accelerator + * @accelerator_key: (out) (allow-none): return location for accelerator + * keyval, or %NULL These functions should just be moved to gtkaccelgroup.c and live next to their lesser siblings. That will also allow us to ditch that _gtk_accelerator_parse_full business. ::: gtk/gtkcellrendereraccel.h @@ +88,3 @@ + guint accelerator_key, + guint keycode, + GdkModifierType accelerator_mods); And to match, we should also move these to the header where their lesser siblings live.
Created attachment 200684 [details] [review] gtk: Add accel with keycode parsing functions Which handle accelerators with keycodes as well as keyvals, so we can use it in applications that use GtkCellRendererAccel's "Other" mode of operations (namely gnome-control-center and gnome-settings-daemon).
Created attachment 200685 [details] [review] gtk: Add test program for keycode parsing
Review of attachment 200684 [details] [review]: Ok. I don't love the amount of code, but since it is needed...
Review of attachment 200685 [details] [review]: sure
Waiting on Mitch's patch to be documented before closing this bug. Attachment 200684 [details] pushed as 06b55b2 - gtk: Add accel with keycode parsing functions Attachment 200685 [details] pushed as 780a92b - gtk: Add test program for keycode parsing
I'm guessing this can be closed now (either it's been documented, or it's fixed, right?).