GNOME Bugzilla – Bug 731618
Handle 'reverse' shortcuts in the keyboard panel
Last modified: 2014-08-17 18:21:51 UTC
Some shortcuts (alt-tab, switch-input-source, ...) have implicit 'reverse' actions to move the selector in the opposite direction (alt-shift-tab for example). At the moment this tends to be hardcoded/hidden in other places. This series add some metadata to the .xml files used by the keyboard panel so that it knows that such 'reverse' shortcuts are related, and so that it can suggest to change the associated binding when changing alt-tab (for example). The last patches in this series enable this for the 'switch-input-source' shortcut, but this can be changed for more shortcuts. For this to work with swit-input-source, additional patches in other places in the stack are needed.
Created attachment 278401 [details] [review] keyboard: Split accel_edit_callback This function is getting a bit big, and the next commits will add more code to it.
Created attachment 278402 [details] [review] keyboard: Add 'reverse' helpers to CcKeyboardItem In order to handle shortcuts which can be reversed (for example, super-space and shift-super-space to switch input methods forward/backward), we are going to add new attributes to the xml files describing the keyboard shortcuts to show in the panel. This commit is a first step towards that and adds the notion of 'reverse' items to CcKeyboardItem. We will then indicate in the xml description files that 'switch-input-source' is reversed by 'switch-input-source-backward' and that 'switch-input-source-backward' reverses 'switch-input-source'.
Created attachment 278403 [details] [review] keyboard: Parse 'reverse' and 'is-reversed' This commit adds support for 'reverse' and 'is-reversed' attributes when parsing the KeyListEntry XML node.
Created attachment 278404 [details] [review] keyboard: Suggest to automatically set reverse bindings Since we now know when a binding has a 'reverse' binding, we can now suggest to update the 'reverse' shortcut when the user set a shortcut for one of them.
Created attachment 278405 [details] [review] keyboard: Add 'reverse' metadata to switch-input-source shortcuts Now that the keyboard panel knows how to handle reverse shortcuts, we can start annotating the existing ones with the needed XML attributes. This commit does that with switch-input-source{-backward}. Note that some changes in some modules are needed for this to work correctly. In this case, a default value needs to be set for switch-input-source-backward in gsettings-desktop-schemas, and the Meta.KeyBindingFlags.REVERSED flag needs to be removed from the shortcuts defined in gnome-shell source. Instead of having mutter handle the reversion with 'shift' by itself, it's now handled explicitly through gsettings keys.
Created attachment 278406 [details] [review] region: Remove "Shift" hack from region panel The region panel hardcodes that to switch input source backward, one uses the shift modfier with the shortcut to switch input source forward.
Created attachment 278407 [details] [review] For reference, gsettings-desktop-schemas patch Add default binding for switch-input-source-backward Currently mutter/gnome-shell make sure switch-input-source-backward shortcut is set to <Shift> + switch-input-source shortcut, but this is being moved to gnome-control-center, which needs the default value to be set.
Created attachment 278408 [details] [review] For reference, mutter patch which will be used to let gnome-control-center manage switch-input-source-backwards Add meta_key_binding_is_reversed() MetaKeyBinding can be marked as being reversed (META_KEY_BINDING_IS_REVERSED), but MetaKeyHandlerFunc callbacks cannot check whether this flag was set or not on the MetaKeyBinding which triggered the callback.
Created attachment 278409 [details] [review] For reference, gnome-shell patches to let gnome-control-center handle switch-input-source-backwards shortcut
Review of attachment 278405 [details] [review]: ok
Review of attachment 278404 [details] [review]: other than the style nits ::: panels/keyboard/keyboard-shortcuts.c @@ +1553,3 @@ + if (reverse_item != NULL) + { + handle_reverse_item(item, reverse_item, keyval, mask, keycode, view); no braces, space before parentheses
Review of attachment 278403 [details] [review]: Mostly fine ::: panels/keyboard/keyboard-shortcuts.c @@ +429,3 @@ context = *attr_values; + } else if (g_str_equal (*attr_names, "reverse-entry")) { + reverse_entry = *attr_values; This lets zero length strings through. Any reason for that? @@ +431,3 @@ + reverse_entry = *attr_values; + } else if (g_str_equal (*attr_names, "is-reversed")) { + if (g_str_equal (*attr_values, "true")) { no curlies
Review of attachment 278402 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +40,3 @@ /* internal */ + char *reverse_item_name; + gboolean is_reversed; Couldn't we just link to another CcKeyboardItem directly with a pointer? @@ +473,3 @@ } +static GHashTable *reverse_items = NULL; ... would allow us to avoid this table that never gets freed. @@ +485,3 @@ + { + reverse_items = g_hash_table_new_full (g_str_hash, g_str_equal, + NULL, g_object_unref); strange indentation @@ +493,3 @@ + + g_hash_table_insert(reverse_items, item->key, + g_object_ref(G_OBJECT(item))); lack of spaces before opening parentheses @@ +496,3 @@ +} + +CcKeyboardItem * cc_keyboard_item_get_reverse_item (CcKeyboardItem *item) the return type should go in a different line @@ +498,3 @@ +CcKeyboardItem * cc_keyboard_item_get_reverse_item (CcKeyboardItem *item) +{ + if (item->priv->reverse_item_name == NULL) { no need for curly braces here, and please never on the same line as the condition
Review of attachment 278401 [details] [review]: Nice
Review of attachment 278406 [details] [review]: yep
I generally like this. Do you plan to submit patches for gsettings-desktop-schemas, mutter and gnome-shell porting all the keybindinds? If you do please mark those bugs as blocking this one. Thanks!
(In reply to comment #16) > I generally like this. Do you plan to submit patches for > gsettings-desktop-schemas, mutter and gnome-shell porting all the keybindinds? > If you do please mark those bugs as blocking this one. Thanks! I definitely will open bugs for the other patches now that I got an ok on the sanity of this series. I did not open them at the same time as this bug to avoid bug-spamming multiple components if a different approach was suggested during the review of these control center patches.
Review of attachment 278403 [details] [review]: ::: panels/keyboard/keyboard-shortcuts.c @@ +429,3 @@ context = *attr_values; + } else if (g_str_equal (*attr_names, "reverse-entry")) { + reverse_entry = *attr_values; No particular reason, changed.
Review of attachment 278402 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +40,3 @@ /* internal */ + char *reverse_item_name; + gboolean is_reversed; Hmm yes that's doable, however this requires a bit more smartness in the parser if we don't want to require that one item and its reversed item always come in a given order. This also forces one item and its reversed item to be defined in the same xml file, but that's probably a reasonable assumption to make.
2 minor things which don't work perfectly yet: - setting switch-im-source to 'shift-space' suggests setting the reverse action to 'space' which is definitely not what we want ;) - if switch-im-source-backwards is set to 'shift-alt-space', trying to set switch-im-source to this shortcut will result in gnome-control-center reporting conflicting shortcuts rather than suggesting to change switch-im-source-backwards to 'alt-space'
Created attachment 278595 [details] [review] keyboard: Add 'reverse' helpers to CcKeyboardItem In order to handle shortcuts which can be reversed (for example, super-space and shift-super-space to switch input methods forward/backward), we are going to add new attributes to the xml files describing the keyboard shortcuts to show in the panel. This commit is a first step towards that and adds the notion of 'reverse' items to CcKeyboardItem. We will then indicate in the xml description files that 'switch-input-source' is reversed by 'switch-input-source-backward' and that 'switch-input-source-backward' reverses 'switch-input-source'.
Created attachment 278596 [details] [review] keyboard: Parse 'reverse' and 'is-reversed' This commit adds support for 'reverse' and 'is-reversed' attributes when parsing the KeyListEntry XML node.
(In reply to comment #20) > 2 minor things which don't work perfectly yet: Right, both can be handled in handle_reverse_item() I think: > - setting switch-im-source to 'shift-space' suggests setting the reverse action > to 'space' which is definitely not what we want ;) We should call is_valid_binding() with the reversed mask at the beginning of handle_reverse_item() and bail out if it's not allowed. > - if switch-im-source-backwards is set to 'shift-alt-space', trying to set > switch-im-source to this shortcut will result in gnome-control-center reporting > conflicting shortcuts rather than suggesting to change > switch-im-source-backwards to 'alt-space' Should be a matter of checking if (reverse_conflict_item == item) .
Comment on attachment 278404 [details] [review] keyboard: Suggest to automatically set reverse bindings According to the previous comment
Review of attachment 278595 [details] [review]: ok
Review of attachment 278596 [details] [review]: looks fine
Review of attachment 278595 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +481,3 @@ + item->priv->reverse_item = reverse_item; + if (reverse_item->priv->reverse_item == NULL) + reverse_item->priv->reverse_item = item; We should be setting reverse_item->priv->is_reversed here as well.
Created attachment 279225 [details] [review] keyboard: Add 'reverse' helpers to CcKeyboardItem In order to handle shortcuts which can be reversed (for example, super-space and shift-super-space to switch input methods forward/backward), we are going to add new attributes to the xml files describing the keyboard shortcuts to show in the panel. This commit is a first step towards that and adds the notion of 'reverse' items to CcKeyboardItem. We will then indicate in the xml description files that 'switch-input-source' is reversed by 'switch-input-source-backward' and that 'switch-input-source-backward' reverses 'switch-input-source'.
Created attachment 279226 [details] [review] fixup! keyboard: Suggest to automatically set reverse bindings This goes on top of the patch of the same name and should fix the pecularities in it
Created attachment 279227 [details] [review] keyboard: Suggest to automatically set reverse bindings Since we now know when a binding has a 'reverse' binding, we can now suggest to update the 'reverse' shortcut when the user set a shortcut for one of them.
Review of attachment 279225 [details] [review]: looks fine
Review of attachment 279227 [details] [review]: ok
(In reply to comment #16) > I generally like this. Do you plan to submit patches for > gsettings-desktop-schemas, mutter and gnome-shell porting all the keybindinds? > If you do please mark those bugs as blocking this one. Thanks! For the record, switch-input-source-backward is the only '-backward' keybinding that we currently show in gnome-control-center UI. In order to handle the other keybindings, a little bit more work will be needed here so that we can mark some KeyListEntry as not showing up in the UI. We need the KeyListEntry for all the -backward keybindings so that gnome-control-center can detect the duplicates, and so that it has a human-readable name for the binding.
Created attachment 279469 [details] [review] keyboard: Suggest to automatically set reverse bindings Since we now know when a binding has a 'reverse' binding, we can now suggest to update the 'reverse' shortcut when the user set a shortcut for one of them. -- Yet another iteration to fix another bug in a corner case: - set switch-input-source to ctrl + space - try to set switch-input-source-backward to ctrl + space -> a dialog is displayed asking if you want to set switch-input-source to shift + ctrl + space - press 'cancel' -> both switch-input-source and switch-input-source-backward are set to ctrl + space I've added a final check for duplicate at the end of show_reverse_item_dialog, and silently unset the conflicting binding. This seemed better than popping another dialog notifying of the conflict.
(In reply to comment #34) > > I've added a final check for duplicate at the end of show_reverse_item_dialog, s/show_reverse_item_dialog/handle_reverse_item > and silently unset the conflicting binding. This seemed better than popping > another dialog notifying of the conflict.
Hrm, still one more thing which is missing - unset switch-input-source and switch-input-source-backward - set any other keybinding to super+space - set switch-input-source to super+space -> the 'conflict dialog' shows up but there is no suggestion/attempt to set switch-input-source-backward to super+shift+space
Created attachment 279477 [details] [review] keyboard: Add support for hidden keybinding XML data If a KeyListEntry has a hidden="true" attribute, then the corresponding binding information will be loaded as usual, but the binding won't be displayed in the user interface. This is useful as the keyboard panel will take into account hidden keybindings when detecting conflicting shortcuts, or to suggest to set a reverse shortcut. For now, this will be used for the various reverse mutter keybindings ({switch,cycle}.*-backward) as they should not be shown in the UI, but we still want the keyboard panel to know about them.
Review of attachment 279477 [details] [review]: Code looks fine but why is the CcKeyboardItem variable and methods called "visible" instead of "hidden" ? I think this is just added cognitive overhead for no good reason.
(In reply to comment #36) > Hrm, still one more thing which is missing > - unset switch-input-source and switch-input-source-backward > - set any other keybinding to super+space > - set switch-input-source to super+space > -> the 'conflict dialog' shows up but there is no suggestion/attempt to set > switch-input-source-backward to super+shift+space Ugh. Will you fix that and attach a new patch?
Review of attachment 279477 [details] [review]: No very good reason, just some remnants of how this patch evolved during development, I'll change it.
Comment on attachment 278407 [details] [review] For reference, gsettings-desktop-schemas patch Marking as review, as this is handled in bug 732293.
Comment on attachment 278408 [details] [review] For reference, mutter patch which will be used to let gnome-control-center manage switch-input-source-backwards Marking as reviewed, handled in bug 732295
Comment on attachment 278409 [details] [review] For reference, gnome-shell patches to let gnome-control-center handle switch-input-source-backwards shortcut Marking as reviewed, handled in bug 732295
Created attachment 281920 [details] [review] keyboard: Add support for hidden keybinding XML data If a KeyListEntry has a hidden="true" attribute, then the corresponding binding information will be loaded as usual, but the binding won't be displayed in the user interface. This is useful as the keyboard panel will take into account hidden keybindings when detecting conflicting shortcuts, or to suggest to set a reverse shortcut. For now, this will be used for the various reverse mutter keybindings ({switch,cycle}.*-backward) as they should not be shown in the UI, but we still want the keyboard panel to know about them.
(In reply to comment #39) > (In reply to comment #36) > > Hrm, still one more thing which is missing > > - unset switch-input-source and switch-input-source-backward > > - set any other keybinding to super+space > > - set switch-input-source to super+space > > -> the 'conflict dialog' shows up but there is no suggestion/attempt to set > > switch-input-source-backward to super+shift+space > > Ugh. Will you fix that and attach a new patch? Should be fixed with the patch below added to "keyboard: Suggest to automatically set reverse bindings". This causes the 'conflict' dialog to show first, and if the user agrees to move forward and disable the conflicting shortcut, then a second dialog will popup and suggest setting the 'reverse' shortcut. diff --git a/panels/keyboard/keyboard-shortcuts.c b/panels/keyboard/keyboard-sho index 3820893..1023bf0 100644 --- a/panels/keyboard/keyboard-shortcuts.c +++ b/panels/keyboard/keyboard-shortcuts.c @@ -1564,6 +1564,8 @@ accel_edited_callback (GtkCellRendererText *cell, g_object_set (G_OBJECT (item), "binding", str, NULL); g_free (str); + if (reverse_item == NULL) + return; } else { @@ -1573,9 +1575,9 @@ accel_edited_callback (GtkCellRendererText *cell, "keycode", item->keycode, "accel-mods", item->mask, NULL); + return; } - return; } str = binding_name (keyval, keycode, mask, FALSE);
Created attachment 281921 [details] [review] keyboard: Suggest to automatically set reverse bindings Since we now know when a binding has a 'reverse' binding, we can now suggest to update the 'reverse' shortcut when the user set a shortcut for one of them.
Review of attachment 281920 [details] [review]: looks fine
Review of attachment 281921 [details] [review]: Please push this.
Attachment 278401 [details] pushed as 10fd199 - keyboard: Split accel_edit_callback Attachment 278405 [details] pushed as c77d164 - keyboard: Add 'reverse' metadata to switch-input-source shortcuts Attachment 278406 [details] pushed as 5572fb9 - region: Remove "Shift" hack from region panel Attachment 278596 [details] pushed as f7c095b - keyboard: Parse 'reverse' and 'is-reversed' Attachment 279225 [details] pushed as f059f42 - keyboard: Add 'reverse' helpers to CcKeyboardItem Attachment 281920 [details] pushed as ba9a8bc - keyboard: Add support for hidden keybinding XML data Attachment 281921 [details] pushed as 5147cf2 - keyboard: Suggest to automatically set reverse bindings