GNOME Bugzilla – Bug 697002
Grab and emit a signal when XK_ISO_Next_Group is pressed
Last modified: 2013-05-24 23:24:38 UTC
This is what I finally ended up doing to tackle the modifiers-only keybinding problem. Capturing XK_ISO_Next_Group was Ray's suggestion some time ago but it couldn't be done outside mutter/gnome-shell because then the capturing wouldn't work in the overview but the actual XKB group index switch would be performed on the X server. For the setting itself I'm using the xkb-options key we already have in org.gnome.desktop.input-sources.
Created attachment 240262 [details] [review] Grab and emit a signal when XK_ISO_Next_Group is pressed This will make it possible to implement input source switching in gnome-shell using the popular modifiers-only keybinding that's implemented on the X server through an XKB option.
*** Bug 693747 has been marked as a duplicate of this bug. ***
*** Bug 695238 has been marked as a duplicate of this bug. ***
*** Bug 692017 has been marked as a duplicate of this bug. ***
Created attachment 240576 [details] [review] Grab and emit a signal when XK_ISO_Next_Group is pressed -- * Added "shifts_toggle" for both shifts * Fixed disabling the option
I realized I should perhaps post the pros and cons of using XK_ISO_Next_Group instead of defining our own combos. Pros: a) being the compositor we can actually now grab that everywhere unlike a 3rd process like the g-s-d helper in 3.6; b) we don't have to play tricks for the CapsLock key (having to undo the caps lock...) because the X server handles it already; c) the users wanting this feature are already used to that specific set of options and the way it works; d) since I'm targeting 3.8.1, I didn't want to add a gsettings dependency on the org.gnome.settings-daemon.peripherals.keyboard schema where the 3.6 setting lives (and I always intended that one to be temporary); e) it just works if people have it configured in their xorg.conf since g-s-d already has code to set the gsettings key from that. Cons: a) no flexibility in which combos exist; b) less future proof, i.e. will have to be totally re-done for wayland, but then, so many other things are.
Created attachment 241136 [details] [review] display: Add signal 'modifiers-accelerator-activated' We'll trigger this on a special modifiers-only keybinding.
Created attachment 241137 [details] [review] prefs: Track the XKB 'grp:' option in gsettings as a keybinding pref We'll use the value of this option to establish a passive grab on the keycode/modifier combos generating XK_ISO_Next_Group.
Created attachment 241138 [details] [review] keybindings: Grab and emit a signal when XK_ISO_Next_Group is pressed -- I've splitted this patch.
*** Bug 698185 has been marked as a duplicate of this bug. ***
Will this be in 3.8?
Created attachment 243309 [details] [review] display: Add signal 'modifiers-accelerator-activated' -- Florian pointed on IRC that freezing the keyboard by default breaks stand-alone mutter and other libmutter users so I've changed the signal to have a return value so that handlers can request freezing explicitly but by default we unfreeze.
Created attachment 243310 [details] [review] prefs: Track the XKB 'grp:' option in gsettings as a keybinding pref -- Rebased.
Created attachment 243311 [details] [review] keybindings: Grab and emit a signal when XK_ISO_Next_Group is pressed -- Rebased.
Review of attachment 243309 [details] [review]: I'm not sure the patch makes a lot of sense on its own (signal return value?), maybe just squash it with the main patch ... ::: src/core/display.c @@ +290,3 @@ + G_SIGNAL_RUN_LAST, + 0, + meta_display_boolean_accumulator, NULL, NULL, Is there really a use case for more than one signal connection? I think g_signal_accumulator_first_wins() should work just fine here ...
Review of attachment 243310 [details] [review]: ::: src/core/prefs.c @@ +1054,3 @@ handle_preference_update_int (settings, key); + else if (g_variant_type_equal (type, G_VARIANT_TYPE_STRING) || + g_variant_type_equal (type, G_VARIANT_TYPE_STRING_ARRAY)) No. handle_preference_update_string() happens to work for other types when using a handler, but that's no excuse for ignoring that it will fail horribly when using the target branch. Either add proper support for string arrays (basically copy the handler branch of string properties and generalize the workspace-names stuff for the target branch) or handle KEY_XKB_OPTIONS explicitly in settings_changed (as is currently the case for workspace-names). I'll file a bug for the former ... @@ +1574,3 @@ +static void +set_iso_next_group_option (const char *option) This is an odd function to have - I'd prefer this in iso_next_group_handler(), along the lines of: char *value = NULL; ... for (p = xkb_options; p && *p; ++p) if (g_str_has_prefix (*p, "grp:")) { value = g_strdup (*p + 4); break } changed = g_strcmp0 (iso_next_group_option, value) != 0; if (iso_next_group_option) g_free (iso_next_group_option); iso_next_group_option = value; if (changed) queue_changed (META_PREF_KEYBINDINGS); @@ +2158,3 @@ +meta_prefs_get_iso_next_group_option (void) +{ + return g_strdup (iso_next_group_option); This is the first and only preference that transfers ownership; please don't, unless there's a very good reason for that which I missed.
Review of attachment 243311 [details] [review]: ::: src/core/keybindings.c @@ +155,3 @@ static GHashTable *external_grabs; +static char *iso_next_group_option; Odd to cache this both in prefs.c and here - without the strdup (see earlier review), you can just use the prefs getter directly ... @@ +157,3 @@ +static char *iso_next_group_option; +static MetaKeyCombo *iso_next_group_combos; +static int n_iso_next_group_combos; Maybe put those two into MetaDisplay (as overlay_combo)? @@ +308,3 @@ +static int +get_keycodes_for_keysym (MetaDisplay *display, This is basically gdk_keymap_get_entries_for_keyval(), no?
Created attachment 244112 [details] [review] prefs: Track the XKB 'grp:' option in gsettings as a keybinding pref -- (In reply to comment #16) > ::: src/core/prefs.c > @@ +1054,3 @@ > handle_preference_update_int (settings, key); > + else if (g_variant_type_equal (type, G_VARIANT_TYPE_STRING) || > + g_variant_type_equal (type, G_VARIANT_TYPE_STRING_ARRAY)) > > No. handle_preference_update_string() happens to work for other types when > using a handler, but that's no excuse for ignoring that it will fail horribly > when using the target branch. > Either add proper support for string arrays (basically copy the handler branch > of string properties and generalize the workspace-names stuff for the target > branch) or handle KEY_XKB_OPTIONS explicitly in settings_changed (as is > currently the case for workspace-names). > I'll file a bug for the former ... Yes, I feared you'd say that :-) Thanks for doing that, this is rebased on your patch. > @@ +1574,3 @@ > > +static void > +set_iso_next_group_option (const char *option) > > This is an odd function to have - I'd prefer this in iso_next_group_handler(), > along the lines of: [snip] Agreed. I did something like that. > @@ +2158,3 @@ > +meta_prefs_get_iso_next_group_option (void) > +{ > + return g_strdup (iso_next_group_option); > > This is the first and only preference that transfers ownership; please don't, > unless there's a very good reason for that which I missed. No good reason, just overlooked the fact we didn't need the copy. Fixed
Created attachment 244113 [details] [review] keybindings: Grab and emit a signal when XK_ISO_Next_Group is pressed -- (In reply to comment #15) > I'm not sure the patch makes a lot of sense on its own (signal return value?), > maybe just squash it with the main patch ... Ok, squashed it. > ::: src/core/display.c > @@ +290,3 @@ > + G_SIGNAL_RUN_LAST, > + 0, > + meta_display_boolean_accumulator, NULL, NULL, > > Is there really a use case for more than one signal connection? I think > g_signal_accumulator_first_wins() should work just fine here ... No, that's fine. I didn't know about this predefined accumulator! (In reply to comment #17) > @@ +157,3 @@ > +static char *iso_next_group_option; > +static MetaKeyCombo *iso_next_group_combos; > +static int n_iso_next_group_combos; > > Maybe put those two into MetaDisplay (as overlay_combo)? Ok. > @@ +308,3 @@ > > +static int > +get_keycodes_for_keysym (MetaDisplay *display, > > This is basically gdk_keymap_get_entries_for_keyval(), no? Yes, it didn't feel right to access GDK here since mutter tracks the X keymap itself already. I'm not even sure it would work, I didn't investigate if mutter is passing the relevant events to GDK for it to update its view of the keymap. So I basically copied the non XKB version of gdk_x11_keymap_get_entries_for_keyval() since it's simpler and we don't really need to care about XKB groups for our purposes here. I added a comment about where the code came from.
Review of attachment 244112 [details] [review]: OK
Review of attachment 244113 [details] [review]: (In reply to comment #19) > Yes, it didn't feel right to access GDK here since mutter tracks the X > keymap itself already. Yes, it's not immediately suitable - also the hacking guide still mentions that gdk/gtk+ shouldn't be used outside ui/ (though some symbols have been sneaked in). This was just a drive-by comment to see if I got the source right :-) > I added a comment about where the code came from. That's definitively a good idea, yeah.
Attachment 244112 [details] pushed as d664cfc - prefs: Track the XKB 'grp:' option in gsettings as a keybinding pref Attachment 244113 [details] pushed as 7186ddf - keybindings: Grab and emit a signal when XK_ISO_Next_Group is pressed
Why aren't those patches on master?
(In reply to comment #23) > Why aren't those patches on master? Basically just because I haven't been running master branches for a while and I want to test this stuff there before pushing. And as it takes a while to switch everything I'm thinking of doing it tomorrow instead of now :-)
OK - I was just wondering :-)