GNOME Bugzilla – Bug 772565
Cannot search for keybindings, just for names
Last modified: 2017-05-04 08:44:25 UTC
Steps to reproduce: 1. open gnome-control-center, keyboard panel 2. open search bar 3. search for "Alt+Tab" or "Ctrl" or "Super" What happens: Nothing Expected behavior: I want to be able to search for a key combination. This comes in handy if you accidentally pressed a wrong key binding and want to know what it did or should have done. Software versions: gtk3-3.22.1-1.fc25.x86_64 control-center-3.22.0-1.fc25.x86_64 glib2-2.50.0-1.fc25.x86_64
Created attachment 350836 [details] [review] keyboard: Also perform search on accels When managing the keyboard shortcuts, the user might want to check for keybindings based on their accelerators, not only their names. Currently, however, the Keyboard panel only supports searching for the keybinding description. Fix that by also considering the normalized keybinding accelerator when performing the search.
Created attachment 350837 [details] [review] keyboard: Also search shortcut accelerators Improved the commit message.
Review of attachment 350837 [details] [review]: ::: panels/keyboard/cc-keyboard-panel.c @@ +462,2 @@ search = cc_util_normalize_casefold_and_unaccent (gtk_entry_get_text (GTK_ENTRY (self->search_entry))); + accel = convert_keysym_state_to_string (item->keyval, item->mask, item->keycode); I think this is a very naive implementation. This will make "Disabled" a search term, as well as "+". Please check with Allan on how this should be implemented. I think at least exact substring matching should be used, so that typing "a " would look for something with "a" being used in the shortcut, otherwise it would match "Alt". We also probably want synonyms, so that "Win" matches "Super" shortcuts, or "Ctrl" matches "Strg" shortcuts in German.
Created attachment 350865 [details] [review] keyboard: Also search shortcut accelerators When managing the keyboard shortcuts, the user might want to check for keybindings based on their accelerators, not only their names. Currently, however, the Keyboard panel only supports searching for the keybinding description. Fix that by also considering the normalized keybinding accelerator when performing the search.
Created attachment 350869 [details] [review] keyboard: Also search shortcut accelerators Don't try to search on empty keybindings.
Review of attachment 350869 [details] [review]: It's still missing: - Launch the Settings in German (LC_ALL=de_DE.UTF-8) and search for "Ctrl", it should match shortcuts with "Strg" in the string - Search for "Win =" it should match the "Super+=" shortcut ("Zoom in" by default) Special casing in strv_contains_prefix_or_match() might be the easiest way to do this. ::: panels/keyboard/cc-keyboard-panel.c @@ +343,3 @@ + guint i; + + for (i = 0; strv[i]; i++) strv[i] != NULL @@ +376,3 @@ + const gchar *token; + + /* Disconsider leading and trailing whitespaces */ Disconsider? Not really a common or current verb to use :) "Strip leading..."
Created attachment 350981 [details] [review] keyboard: Also search shortcut accelerators When managing the keyboard shortcuts, the user might want to check for keybindings based on their accelerators, not only their names. Currently, however, the Keyboard panel only supports searching for the keybinding description. Fix that by also considering the normalized keybinding accelerator when performing the search.
Review of attachment 350981 [details] [review]: ::: panels/keyboard/cc-keyboard-panel.c @@ +350,3 @@ + const gchar *key; + const gchar *alias; + gboolean translatable; I'd rather you had 2 separate strings here. Takes the same amount of space, and should be a bit clearer that you're either looking for a synonym (Super / Win) or a translation (Ctrl / Strg). @@ +749,3 @@ + + /* Initialize the GTK+ gettext domain, so we can steal their + * accelerator translations */ This shouldn't be necessary, GTK+ already does that when we initialise it.
Created attachment 350992 [details] [review] keyboard: Also search shortcut accelerators When managing the keyboard shortcuts, the user might want to check for keybindings based on their accelerators, not only their names. Currently, however, the Keyboard panel only supports searching for the keybinding description. Fix that by also considering the normalized keybinding accelerator when performing the search.
Review of attachment 350992 [details] [review]: ::: panels/keyboard/cc-keyboard-panel.c @@ +346,3 @@ + const gchar *key; + const gchar *translation; + const gchar *synonim; synonym @@ +350,3 @@ + { + { "ctrl", "Ctrl", "ctrl" }, + { "super", "Super", "win" }, I don't understand why you're doing "super" -> "win" and "win" -> "super" as synonyms. The user can type both "win" or "super", but the shortcut will always contain "Super" never "Win" otherwise it would be invalid. (in a separate bug and/or patch, it would be nice to add "option" as a synonym for "alt", and both "command" and "apple" as additional synonyms for the super key, for when used on Mac keyboards) @@ +374,3 @@ + synonim = key_aliases[i].synonim; + + /* If a translation or synonim of the key is in the accelerator, and we typed ditto.
Created attachment 351004 [details] [review] keyboard: Also search shortcut accelerators When managing the keyboard shortcuts, the user might want to check for keybindings based on their accelerators, not only their names. Currently, however, the Keyboard panel only supports searching for the keybinding description. Fix that by also considering the normalized keybinding accelerator when performing the search.
Created attachment 351005 [details] [review] keyboard: Add more key aliases To improve even more the search feature, add the following new key aliases: Option → Alt Command → Super Apple → Super
Attachment 351004 [details] pushed as 1cb787e - keyboard: Also search shortcut accelerators Attachment 351005 [details] pushed as f981eb5 - keyboard: Add more key aliases I renamed the "translation" struct member to "untranslated" as that seemed more accurate and removed the "Steal" from the comment about the GTK+ translation as that could have implied getting an allocated string from GTK+/gettext, which isn't the case.