GNOME Bugzilla – Bug 769063
Implement the new Keyboard panel
Last modified: 2016-09-08 12:51:46 UTC
Per new mockups, the following patches implement the new keyboard panel.
Created attachment 331924 [details] [review] shell: update sidelist when active panel is set externally When the active panel is set by external agents, e.g. calling the Control Center through command line and asking to open a specific panel, the panel is correctly opened but the sidelist is not updated to reflect that. Fix that by selecting the externally set panel row, and eventually moving to the correct list.
Created attachment 331925 [details] [review] keyboard: remove deprecated GtkHBox and GtkVBox These classes are deprecated by Gtk+ and should be replaces by vertical and/or horizontal GtkBoxes. This commit replaces the usage of the deprecated box classes.
Created attachment 331926 [details] [review] panel: add a autocleanup function CcPanel uses the old boilerplate code from GLib, which does not set an autocleanup function. The lack of a cleanup function implies that panels cannot use G_DECLARE_{FINAL,DERIVABLE}_TYPE, making the code stick to the old boilerplate. This patch adds a cleanup function to CcPanel. It doesn't move CcPanel to G_DECLARE_DERIVABLE_TYPE() because it'd break the CcPanel's subclasses.
Created attachment 331927 [details] [review] keyboard: remove boilerplate code After introducing the autocleanup function to CcPanel, it is now possible to remove a lot of boilerplate code from the panels. This commit ports CcKeyboardPanel to be a final type, removing all the old boilerplate code in the proccess.
Created attachment 331928 [details] [review] keyboard: include the needed header
Created attachment 331929 [details] [review] keyboard: make it a template class The current keyboard panel is not a real class, and delegates all the loading of keyboard shortcuts to another file (which is also not a real class). This code organization is confusing and limits the possibility to add new features and work on the code. To fix that, and to allow a much easier porting to the new layout, the keyboard panel is now a template class. That has various implications on the code organization: - The keyboard-shortcuts.c was responsible for filling the shortcuts. Because it relied on the GtkBuilder of the panel, most of its code was moved to the CcKeyboardPanel class. - The unused code from the keyboard panel class had to be removed in order to make it work again. - All the unit-allocated hash tables and widgets are now part of the CcKeyboardPanel structure. - The interface elements have a single entry point. While moving the code into CcKeyboardPanel, all the tabs were removed and indentation issues were also fixed.
Created attachment 331930 [details] [review] keyboard: show all shortcuts in a single treeview To move away from the old sections -> shortcuts UI, we have to merge all the shortcuts in the treeview and remove the sections treeview.
Created attachment 331931 [details] [review] keyboard: add a group field to CcKeyboardItem This group field will be consumed by the next patches in order to provide the correct ordering of elements in the listbox.
Created attachment 331932 [details] [review] keyboard: show sortcuts in a listbox The new listbox will replace the current treeview. This patch simply the listbox and add the initial add() and remove() functions to manage the rows, and does not remove the treeview yet.
Created attachment 331933 [details] [review] keyboard: remove the shortcuts treeview After porting the shortcuts management entirely to GtkListBox, the current treeview is not necessary anymore. This patch removes the shortcuts treeview and all the related functions.
Created attachment 331934 [details] [review] keyboard: add CcShortcutLabel In order to properly implement the mockups, the shortcut accelerators must match the visuals of Gtk+. To make that happen, copy GtkShortcutLabel to the tree and adapt whatever is needed to make it work here.
Created attachment 331935 [details] [review] keyboard: factor out shortcut editor dialog Instead of overloading CcKeyboardPanel class by making it handle the shortcut editing routine, move this to a new class that handles that.
Created attachment 331936 [details] [review] keyboard: move keyboard management code to custom class Instead of having CcKeyboardPanel managing both UI and backend code, factor the backend code to a new CcKeyboardManager class and drop backend management from the panel itself. The next patch will move the current code to use the manager class.
Created attachment 331937 [details] [review] keyboard: use CcKeyboardManager After introducing the manager object to handle the backend code, this patch ports both CcKeyboardShortcutEditor and CcKeyboardPanel to use the new class instead of manually handling backend code.
Created attachment 331938 [details] [review] keyboard: bring back uniqueness check The collision detection code was removed as the cleanup was happening, and this patch readd the feature in a much cleaner and saner code.
Created attachment 331939 [details] [review] keyboard: add API to track whether a shortcut is modified The current keyboard item API does not track whether the keyboard shortcut is modified or not. In order to properly implement the Reset operation, the keyboard item must receive this API and ideally handle it internally. This patch adds the necessary API to CcKeyboardItem to track whether the shortcut is modified.
Created attachment 331940 [details] [review] keyboard: support resetting keys from the list Following the proposed mockups, the shortcut list must have the ability to reset modified right away. After adding the necessary API in CcKeyboardItem, adding the user-visible elements to enable that is easy. To make that happen, add a button that resets the keyboard shortcut.
Created attachment 331941 [details] [review] keyboard: remove unused code CcKeyboardItem overrides GObject:constructor with an empty function that only hooks up with the parent constructor. Lets simplify the code by removing this blank function.
Created attachment 331942 [details] [review] keyboard: bring back reverse item management In order to simplify the porting to the new UI, the reverse item functionality was removed. Now that all the necessary code landed, it is time to bring it back. This patch readds the ability to manage reverse items.
Created attachment 331946 [details] [review] keyboard: add search support Based on the latest mockups, the Keyboard panel is highly benefitted by adding a search functionality. This patch add all the necessary UI and functions to make the search work.
Review of attachment 331924 [details] [review]: s/sidelist/sidebar/ everywhere. > When the active panel is set by external agents "When the active panel is not changed through sidebar navigation" ::: shell/alt/cc-panel-list.c @@ +809,3 @@ gtk_container_add (GTK_CONTAINER (self->search_listbox), search_data->row); + + self->panels = g_list_prepend (self->panels, data); That's not a list of panels, that's a list of data associated with each ListBoxRow. @@ +827,3 @@ + g_return_if_fail (CC_IS_PANEL_LIST (self)); + + for (l = self->panels; l != NULL; l = l->next) Using a hashtable would be much quicker.
Review of attachment 331925 [details] [review]: ::: panels/keyboard/gnome-keyboard-panel.ui @@ +64,3 @@ <property name="orientation">vertical</property> <child> + <object class="GtkBox" id="vbox4"> You're not setting the orientation property for this one, and it defaults to horizontal. If that doesn't matter because there's just one element, please rename the widget.
Review of attachment 331926 [details] [review]: Sure. Note that the prefix is "shell: " for everything in the shell/ subdirectory.
Review of attachment 331927 [details] [review]: Major win. Can you please file a bug for doing this to the other panels as well?
Review of attachment 331928 [details] [review]: Why is it needed now and wasn't before? It's not like that panel hasn't built for years...
Review of attachment 331929 [details] [review]: > The current keyboard panel is not a real class, and delegates > all the loading of keyboard shortcuts to another file (which > is also not a real class). This code organization is confusing > and limits the possibility to add new features and work on the > code. This isn't necessary at all. The panels that just delegate all the work to a separate file were panels ported from the stand-alone GNOME 2 settings dialogues. "gnome-mouse-properties.c" anyone? > - All the unit-allocated hash tables and widgets are now part of the > CcKeyboardPanel structure. unit-allocated? > While moving the code into CcKeyboardPanel, all the tabs were removed and > indentation issues were also fixed. No, don't "fix" indentation issues when moving code. You can change the indentation as you go along and modifying the majority of a function. You could also move the code in smaller steps if you want, first making the structs public in the .h file, then moving the code. ::: panels/keyboard/gnome-keyboard-panel.ui @@ +9,3 @@ <property name="page_increment">200</property> </object> + <object class="GtkDialog" id="custom_shortcut_dialog"> I'd rather renaming the widgets happened in a separate patch. And I'm pretty sure it's not necessary for the Builder templates to work either.
Review of attachment 331930 [details] [review]: > To move away from the old sections -> shortcuts UI, we > have to merge all the Move away from the old sections sidebar, by merging all the ... ::: panels/keyboard/cc-keyboard-panel.c @@ +44,3 @@ /* Treeviews */ + GtkListStore *sections_store; + GtkTreeModel *sections_model; Where are those new items destroyed?
Review of attachment 331931 [details] [review]: Sure.
Review of attachment 331932 [details] [review]: > Subject: [PATCH] keyboard: show sortcuts in a listbox shortcuts > The new listbox will replace the current treeview. This Replace the current treeview with listbox. > patch simply the listbox and add the initial add() and > remove() functions to manage the rows, and does not remove > the treeview yet. To many "ands" (I also don't understand what this does from the description) ::: panels/keyboard/cc-keyboard-panel.c @@ +39,3 @@ +typedef struct +{ brace on the line above. @@ +87,3 @@ +/* + * RowData functions One line comment is probably enough. @@ +89,3 @@ + * RowData functions + */ +static RowData* Space before "*" @@ +129,3 @@ + accelerator = convert_keysym_state_to_string (item->keyval, item->mask, item->keycode); + + g_value_set_string (to_value, accelerator); Use g_value_take_string() directly, no need to free afterwards. @@ +265,3 @@ + add_header = FALSE; + + /* The + row always have a separator */ always has ::: panels/keyboard/keyboard-shortcuts.c @@ +358,3 @@ } +/* Stealed from GtkCellRendererAccel */ Stolen. Please add a full git web URL as a reference. ::: panels/keyboard/keyboard-shortcuts.h @@ +97,3 @@ GError **error); +gchar* convert_keysym_state_to_string (guint keysym, You can probably add that helper in a separate patch.
Review of attachment 331933 [details] [review]: > After porting the shortcuts management entirely to > GtkListBox, the current treeview is not necessary > anymore. But you're doing some of the porting in this patch, still, no? I'd mention that you're removing the shortcuts treeview, so also separating its tree model from the view. ::: panels/keyboard/cc-keyboard-panel.c @@ +50,3 @@ + /* Shortcut view */ + GtkListStore *shortcuts_model; Where is that destroyed?
Review of attachment 331934 [details] [review]: How about exporting GtkShortcutLabel instead?
Review of attachment 331935 [details] [review]: > Instead of overloading CcKeyboardPanel class by making it handle This really isn't "overloading a class". It's C. It's just that the code is in the same file. Your translation aren't going to work, you forgot to edit po/POTFILES.in ::: panels/keyboard/cc-keyboard-panel.c @@ +636,3 @@ title = _(keylist->name); } + Whitespace changes. @@ +642,3 @@ group = BINDING_GROUP_APPS; + Ditto. @@ +962,2 @@ + /* Shortcut not found or not a custom shortcut */ + g_assert (valid); I'd prefer a separate "shortcut_found" or something, so that the assertion's message is a little self-explanatory. Or: if (!valid) g_error ("Tried to remove a non-existent shortcut"); @@ +1188,3 @@ cc_keyboard_panel_init (CcKeyboardPanel *self) { + Whitespace change. @@ +1193,3 @@ gtk_widget_init_template (GTK_WIDGET (self)); + /* Bindings */ Is that really useful? ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ +1,3 @@ +/* cc-keyboard-shortcut-editor.h + * + * Copyright (C) 2016 Endless, Inc Please keep the original copyright you copied most of this code from as well. @@ +16,3 @@ + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Authors: Georges Basile Stavracas Neto <georges.stavracas@gmail.com> Ditto the authors. @@ +29,3 @@ + * Workaround to stop receiving a stray Meta modifier. + */ +#ifdef __APPLE__ That wasn't in the original code. @@ +93,3 @@ + +/* + * Auxiliary methods That's really not necessary. (and we call those "helpers") @@ +148,3 @@ + +static gboolean +custom_shortcut (CcKeyboardShortcutEditor *self) is_custom_shortcut() @@ +507,3 @@ + self->custom_mask = real_mask; + + /* Ignore CapsLock */ Could you try keeping the original comments as much as possible, when splitting code? @@ +577,3 @@ + * The current keyboard panel. + */ + properties[PROP_PANEL] = g_param_spec_object ("panel", Make that a pointer please, so we don't end up with circular dependencies. Given that the dialogue cannot exist without the panel existing...
Review of attachment 331936 [details] [review]: I don't understand what the backend/front-end split could be here. What do you manipulate in the backend? Please also remove the existing (and similar) code in the same patch so we can see where the code comes from and goes. ::: panels/keyboard/cc-keyboard-manager.c @@ +1,2 @@ +/* + * Copyright (C) 2016 Endless, Inc Again with the copyright and author.
Review of attachment 331937 [details] [review]: To be merged into the previous patch.
Review of attachment 331938 [details] [review]: When doing the final commit, please make sure to reference the commit ID in which the code was removed. I would have preferred if that code was not removed, but moved even if ineffective along with a TODO comment. Not a requirement, but if it's doable with little efforts...
Review of attachment 331939 [details] [review]: > Subject: [PATCH] keyboard: add API to track whether a shortcut is modified Modified compared to what? ::: panels/keyboard/cc-keyboard-item.c @@ +313,3 @@ + g_object_class_install_property (object_class, + PROP_IS_MODIFIED, + g_param_spec_boolean ("is-modified", modified compared to what? To the default value? Then "is-value-default" To an empty value? Then "is-shortcut-set" etc. @@ +540,3 @@ + g_return_val_if_fail (CC_IS_KEYBOARD_ITEM (self), FALSE); + + /* When the shortcut is custom, we don't treat it as modified */ Why not?
Review of attachment 331940 [details] [review]: > Subject: [PATCH] keyboard: support resetting keys from the list This is computer techie speak ;) "Add button to reset shortcut to default" > have the ability to reset modified right away. reset shortcuts set to non-default values. ::: panels/keyboard/cc-keyboard-manager.c @@ +984,3 @@ + * + * Resets the keyboard shortcut managed by @item, and eventually + * disables any collision detected. "Disables a collision"? Or "disables shortcuts that conflict with the new shortcut's value"? @@ +999,3 @@ + default_binding = get_binding_from_variant (default_value); + + /* Disables any eventual collision we find */ Ditto. @@ +1000,3 @@ + + /* Disables any eventual collision we find */ + if (default_binding && default_binding[0] != '\0') I prefer: *default_binding != '\0' @@ +1009,3 @@ + gtk_accelerator_parse_with_keycode (default_binding, &keyval, &keycodes, &mask); + + g_message ("default value for '%s' is '%s'", item->description, default_binding); g_debug or remove. ::: panels/keyboard/cc-keyboard-panel.c @@ +109,3 @@ item = CC_KEYBOARD_ITEM (g_binding_get_source (binding)); + /* Bold the label when the shortcut is modified */ Embolden.
Review of attachment 331941 [details] [review]: > Lets simplify the code by removing this blank function. Would be "let's". And it's not "blank" (or empty) But: Simplify the code by removing this function. Or even nothing, you already say it's removed in the suject.
Review of attachment 331942 [details] [review]: Same thing as with the conflict resolution, I'd rather have seen the code moved even if ineffective rather than removed then re-added. > readds adds back needs-work for the comment that I don't understand. ::: panels/keyboard/cc-keyboard-item.c @@ +162,3 @@ + /* + * Always treat the pair (item, reverse) as a unit: setting one also I don't understand this. Should it read "Setting one sets the other, disabling one disables the other"? ::: panels/keyboard/cc-keyboard-manager.c @@ +107,3 @@ + } + else if (item->keyval != 0 || data->new_keycode != item->keycode) + { Don't need braces for a one-liner.
Review of attachment 331946 [details] [review]: > Based on the latest mockups, the Keyboard panel > is highly benefitted by adding a search functionality. would highly benefit from a search functionality. Looks fine, would need to test the actual code. What happens when I click to hide the search bar and there's still text in it? (it should reset) Could you also hook up search-as-you-type? See gtk_search_bar_handle_event(). You can do that in a separate patch if you want.
The delete button for resetting shortcuts just doesn't look right to me, it looks like permanently deleting a shortcut, hence kind of scary, it should be something like edit-undo or edit-clear
Created attachment 332055 [details] [review] shell: update sidebar when active panel is set externally (In reply to Bastien Nocera from comment #21) > s/sidelist/sidebar/ everywhere. Done. > "When the active panel is not changed through sidebar navigation" Done. > Using a hashtable would be much quicker. Indeed. Fixed.
Created attachment 332056 [details] [review] shell: add a autocleanup function to CcPanel Update the commit message. Reattaching to keep patch ordering in Bugzilla.
Created attachment 332057 [details] [review] keyboard: remove boilerplate code No changes, reattaching to keep Bugzilla patch ordering.
Created attachment 332058 [details] [review] keyboard: remove deprecated GtkHBox and GtkVBox (In reply to Bastien Nocera from comment #22) > If that doesn't matter because there's just one element, please rename the > widget. Done.
Created attachment 332059 [details] [review] keyboard: expose structures in header (In reply to Bastien Nocera from comment #26) > You could also move the code in smaller steps if you want, first making the > structs public in the .h file, then moving the code. Done.
Created attachment 332060 [details] [review] keyboard: make it a template class (In reply to Bastien Nocera from comment #26) > This isn't necessary at all. The panels that just delegate all the work to a > separate file were panels ported from the stand-alone GNOME 2 settings > dialogues. "gnome-mouse-properties.c" anyone? Removed. > unit-allocated? Clarified. > No, don't "fix" indentation issues when moving code. You can change the > indentation as you go along and modifying the majority of a function. Sure. > I'd rather renaming the widgets happened in a separate patch. And I'm pretty > sure it's not necessary for the Builder templates to work either. Unfortunately, I can't make it work with template classes when using the "-" char.
Created attachment 332061 [details] [review] keyboard: show all shortcuts in a single treeview (In reply to Bastien Nocera from comment #27) > Move away from the old sections sidebar, by merging all the ... Done. > Where are those new items destroyed? Done. Now they are properly destroyed in finalize().
Created attachment 332062 [details] [review] keyboard: add a group field to CcKeyboardItem No changes. Reattaching to keep Bugzilla patch order.
Created attachment 332063 [details] [review] keyboard: add helper method for user-friendly accelerators (In reply to Bastien Nocera from comment #29) > You can probably add that helper in a separate patch. Done.
Created attachment 332064 [details] [review] keyboard: show shortcuts in a listbox (In reply to Bastien Nocera from comment #29) > > Subject: [PATCH] keyboard: show sortcuts in a listbox > > shortcuts Done. > > The new listbox will replace the current treeview. This > > Replace the current treeview with listbox. Done > To many "ands" (I also don't understand what this does from the description) Does the new message makes it clear enough? > ::: panels/keyboard/cc-keyboard-panel.c > @@ +39,3 @@ > > +typedef struct > +{ > > brace on the line above. Done. > +/* > + * RowData functions > > One line comment is probably enough. Sure. Done. > > @@ +89,3 @@ > + * RowData functions > + */ > +static RowData* > > Space before "*" Done. > @@ +129,3 @@ > + accelerator = convert_keysym_state_to_string (item->keyval, item->mask, > item->keycode); > + > + g_value_set_string (to_value, accelerator); > > Use g_value_take_string() directly, no need to free afterwards. Thanks for the tip. Fixed. > + /* The + row always have a separator */ > > always has Corrected. > +/* Stealed from GtkCellRendererAccel */ > > Stolen. > > Please add a full git web URL as a reference. Done.
Created attachment 332065 [details] [review] keyboard: remove the shortcuts treeview (In reply to Bastien Nocera from comment #30) > But you're doing some of the porting in this patch, still, no? > > I'd mention that you're removing the shortcuts treeview, so also separating > its tree model from the view. Done. > > ::: panels/keyboard/cc-keyboard-panel.c > @@ +50,3 @@ > > + /* Shortcut view */ > + GtkListStore *shortcuts_model; > > Where is that destroyed? In finalize().
Created attachment 332066 [details] [review] keyboard: add CcShortcutLabel (In reply to Bastien Nocera from comment #31) > How about exporting GtkShortcutLabel instead? Working on that. I'll keep the patches here until it's public.
Created attachment 332067 [details] [review] keyboard: introduce CcKeyboardShortcutEditor (In reply to Bastien Nocera from comment #32) > > This really isn't "overloading a class". It's C. It's just that the code is > in the same file. Rewrote the commit message (see my comment below). > Your translation aren't going to work, you forgot to edit po/POTFILES.in Fixed. > Whitespace changes. Removed. > @@ +962,2 @@ > + /* Shortcut not found or not a custom shortcut */ > + g_assert (valid); > > I'd prefer a separate "shortcut_found" or something, so that the assertion's > message is a little self-explanatory. > > Or: > if (!valid) > g_error ("Tried to remove a non-existent shortcut"); I went with the second form. > Is that really useful? Removed comment. > ::: panels/keyboard/cc-keyboard-shortcut-editor.c > @@ +1,3 @@ > +/* cc-keyboard-shortcut-editor.h > + * > + * Copyright (C) 2016 Endless, Inc > > Please keep the original copyright you copied most of this code from as well. > [...] > Ditto the authors. > [...] > That wasn't in the original code. I think the commit message was completely misleading. This dialog is entirely a creation of mine, and does not share any code with other components. > That's really not necessary. > > (and we call those "helpers") Removed. > +static gboolean > +custom_shortcut (CcKeyboardShortcutEditor *self) > > is_custom_shortcut() Done. > Could you try keeping the original comments as much as possible, when > splitting code? This is new code. > > @@ +577,3 @@ > + * The current keyboard panel. > + */ > + properties[PROP_PANEL] = g_param_spec_object ("panel", > > Make that a pointer please, so we don't end up with circular dependencies. > > Given that the dialogue cannot exist without the panel existing... Indeed, thanks for pointing that out.
Created attachment 332068 [details] [review] keyboard: move keyboard management code to custom class (In reply to Bastien Nocera from comment #33) > I don't understand what the backend/front-end split could be here. What do > you manipulate in the backend? It handles the loading, creation and removal and search of keyboard shortcuts. > Please also remove the existing (and similar) code in the same patch so we > can see where the code comes from and goes. Done. > ::: panels/keyboard/cc-keyboard-manager.c > @@ +1,2 @@ > +/* > + * Copyright (C) 2016 Endless, Inc > > Again with the copyright and author. Done.
Created attachment 332069 [details] [review] keyboard: bring back uniqueness check (In reply to Bastien Nocera from comment #35) > I would have preferred if that code was not removed, but moved even if > ineffective along with a TODO comment. Not a requirement, but if it's doable > with little efforts... I tried to rollback my changes and try to work with the unused code, but it didn't go quite well. Mostly because only a small part of the old code applies to the new one.
Created attachment 332070 [details] [review] keyboard: add API to track whether a shortcut is default (In reply to Bastien Nocera from comment #36) > > modified compared to what? > > To the default value? Then "is-value-default" > To an empty value? Then "is-shortcut-set" > etc. Done. > + /* When the shortcut is custom, we don't treat it as modified */ > > Why not? Per mockups and IRC discussion: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/keyboard/keyboard-wires.png
Created attachment 332071 [details] [review] keyboard: add support to reset shortcuts to their default values (In reply to Bastien Nocera from comment #37) > This is computer techie speak ;) > > "Add button to reset shortcut to default" Done. > > > have the ability to reset modified right away. > > reset shortcuts set to non-default values. Sure. > "Disables a collision"? Or "disables shortcuts that conflict with the new > shortcut's value"? > [...] > Ditto. Fixed. > I prefer: > *default_binding != '\0' Done. > g_debug or remove. Removed. > + /* Bold the label when the shortcut is modified */ > > Embolden. Thanks for the correction.
Created attachment 332072 [details] [review] keyboard: remove unused code (In reply to Bastien Nocera from comment #38) > Would be "let's". And it's not "blank" (or empty) > > But: > Simplify the code by removing this function. > > Or even nothing, you already say it's removed in the suject. Simply removed that.
Created attachment 332073 [details] [review] keyboard: bring back reverse item management (In reply to Bastien Nocera from comment #39) > Same thing as with the conflict resolution, I'd rather have seen the code > moved even if ineffective rather than removed then re-added. Not possible, sorry. > > readds > > adds back Sure. > I don't understand this. > > Should it read "Setting one sets the other, disabling one disables the > other"? Corrected. Is it clearer now? > ::: panels/keyboard/cc-keyboard-manager.c > @@ +107,3 @@ > + } > + else if (item->keyval != 0 || data->new_keycode != item->keycode) > + { > > Don't need braces for a one-liner. This was part of the old code that I added back, and it was like that before. Shall I procceed and remove the braces anyway?
Created attachment 332074 [details] [review] keyboard: add search support (In reply to Bastien Nocera from comment #40) > > Based on the latest mockups, the Keyboard panel > > is highly benefitted by adding a search functionality. > > would highly benefit from a search functionality. Fixed. > Looks fine, would need to test the actual code. What happens when I click to > hide the search bar and there's still text in it? (it should reset) That is the current behavior. > Could you also hook up search-as-you-type? See gtk_search_bar_handle_event(). This is already done in the UI file.
Created attachment 332112 [details] [review] keyboard: add API to track whether a shortcut is default Fixed a small issue with custom shortcuts. Since I'm officially screwing up the patch order in Bugzilla - and because I'm not really into the burden of keeping this order - I ask to please test these patches through my branch: wip/gbsneto/new-keyboard-panel
Review of attachment 332055 [details] [review]: Sure. ::: shell/alt/cc-panel-list.c @@ +829,3 @@ + g_return_if_fail (CC_IS_PANEL_LIST (self)); + + data = g_hash_table_lookup (self->id_to_data, id); g_assert (data);
Review of attachment 332058 [details] [review]: Sure.
Review of attachment 332059 [details] [review]: ::: panels/keyboard/keyboard-shortcuts.c @@ -34,3 @@ #endif -#define BINDINGS_SCHEMA "org.gnome.settings-daemon.plugins.media-keys" You're removing a number of lines that aren't actually getting added to the header file. If you don't need them later, remove them when you don't need them, or move them to the header file... Please try to keep the code compiling in between patches.
Review of attachment 332060 [details] [review]: The XML-ish parser needs to be broken out in a different way. Maybe you could pass all the bits of data from the parser to a helper function, or do all the XML parsing in keyboard-shortcuts.c with a callback so you add it to the model in the file that knows about the model. The constant definitions need to be fixed as well. ::: panels/keyboard/cc-keyboard-panel.c @@ +35,3 @@ +#endif + +#define BINDINGS_SCHEMA "org.gnome.settings-daemon.plugins.media-keys" Here are the lines that disappeared in the previous patch! ::: panels/keyboard/keyboard-shortcuts.h @@ +90,3 @@ + guint keycode); + +void parse_start_tag (GMarkupParseContext *ctx, Urgh. No. You'll need to break this out in a different way.
Review of attachment 332060 [details] [review]: ::: panels/keyboard/gnome-keyboard-panel.ui @@ +9,3 @@ <property name="page_increment">200</property> </object> + <object class="GtkDialog" id="custom_shortcut_dialog"> Mentioned in a previous review: I'd rather renaming the widgets happened in a separate patch. And I'm pretty sure it's not necessary for the Builder templates to work either.
Review of attachment 332061 [details] [review]: Looks good.
Attachment 332055 [details] pushed as 42a360e - shell: update sidebar when active panel is set externally Attachment 332056 [details] pushed as 61d7abe - shell: add a autocleanup function to CcPanel Attachment 332057 [details] pushed as bca7c59 - keyboard: remove boilerplate code Attachment 332058 [details] pushed as ce08134 - keyboard: remove deprecated GtkHBox and GtkVBox
Review of attachment 332063 [details] [review]: Sure.
Review of attachment 332064 [details] [review]: Looks fine otherwise. ::: panels/keyboard/cc-keyboard-panel.c @@ +111,3 @@ + +/* + * Listbox-related functions Remember what I said about one-line comments? :)
Review of attachment 332065 [details] [review]: ::: panels/keyboard/cc-keyboard-panel.c @@ +48,3 @@ CcPanel parent; + /* Shortcut view */ That's not a view, those are models. @@ +49,3 @@ + /* Shortcut view */ + GtkListStore *shortcuts_model; Where is that "finalized"?
Review of attachment 332066 [details] [review]: Add a "See http://bugzilla..." with the link to the bug you filed about making this widget public API. Looks fine otherwise.
Review of attachment 332067 [details] [review]: > Readd "Re-add" ::: panels/keyboard/cc-keyboard-panel.c @@ +639,1 @@ + We don't need 2 linefeeds here. ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ +29,3 @@ + * Workaround to stop receiving a stray Meta modifier. + */ +#ifdef __APPLE__ This still wasn't in the original code. Remove that, and add it in a separate patch that explains why it's necessary (my guess is that it's not). @@ +245,3 @@ + gtk_widget_set_sensitive (self->add_button, valid); + + if (valid) Deep nesting, see below. @@ +436,3 @@ + + case PROP_PANEL: + g_set_object (&self->panel, g_value_get_pointer (value)); It's not an object, but a pointer, so you can't use g_set_object(). @@ +456,3 @@ + gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->edit_button)); + + if (editing) Deep nesting, see below. @@ +503,3 @@ + self->custom_mask = real_mask; + + /* Ignore CapsLock */ Can you try to keep the original comments, as mentioned in a previous review? @@ +509,3 @@ + grab_seat (self, (GdkEvent*) event); + + validate_custom_shortcut (self); Not checking the return value? Maybe validate_custom_shortcut() needs to be renamed, as it doesn't just validate the shortcut. @@ +683,3 @@ + g_return_if_fail (CC_IS_KEYBOARD_SHORTCUT_EDITOR (self)); + + if (g_set_object (&self->item, item)) if (!g_set_object (...)) return; @@ +705,3 @@ + g_return_if_fail (CC_IS_KEYBOARD_SHORTCUT_EDITOR (self)); + + if (self->mode != mode) if (self->mode == mode) return; etc. Avoids the deep nesting.
Review of attachment 332068 [details] [review]: From the previous review: > I don't understand what the backend/front-end split could be here. What do you manipulate in the backend?
Review of attachment 332069 [details] [review]: Remember: > When doing the final commit, please make sure to reference the commit ID in which the code was removed. I guess you didn't want to do that: > I would have preferred if that code was not removed, but moved even if ineffective along with a TODO comment. Not a requirement, but if it's doable with little efforts...
Review of attachment 332071 [details] [review]: Looks good.
Review of attachment 332072 [details] [review]: Sure.
Review of attachment 332073 [details] [review]: Looks fine otherwise. ::: panels/keyboard/cc-keyboard-manager.c @@ +962,3 @@ data.conflict_item = NULL; /* Any number of shortcuts can be disabled */ This is the comment for the loop actually, not the conditional.
Review of attachment 332074 [details] [review]: Looks fine. Please add a note that it doesn't do search-as-you-type (and file a separate bug that depends on this one to implement it).
Review of attachment 332112 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +540,3 @@ + g_return_val_if_fail (CC_IS_KEYBOARD_ITEM (self), FALSE); + + /* When the shortcut is custom, we don't treat it as modified */ Please, expand this comment to explain why, as mentioned in the previous review.
Created attachment 332148 [details] [review] keyboard: expose structures in header (In reply to Bastien Nocera from comment #65) > Please try to keep the code compiling in between patches. Ugh, some nastyness from my side. Sorry about that, it's fixed.
Created attachment 332164 [details] [review] keyboard: make it a template class (In reply to Bastien Nocera from comment #66) > The XML-ish parser needs to be broken out in a different way. Maybe you > could pass all the bits of data from the parser to a helper function, or do > all the XML parsing in keyboard-shortcuts.c with a callback so you add it to > the model in the file that knows about the model. I moved all the code to keyboard-shortcuts.c > The constant definitions need to be fixed as well. Done.
Created attachment 332166 [details] [review] keyboard: show shortcuts in a listbox (In reply to Bastien Nocera from comment #71) > Remember what I said about one-line comments? :) Sorry, that one slipped through. Fixed now.
Created attachment 332167 [details] [review] keyboard: remove the shortcuts treeview (In reply to Bastien Nocera from comment #72) > That's not a view, those are models. Renamed. > Where is that "finalized"? Sorry, my fault again. Should be fixed now.
Review of attachment 332148 [details] [review]: Looks good.
Review of attachment 332164 [details] [review]: Looks good.
Review of attachment 332166 [details] [review]: Sure.
Review of attachment 332167 [details] [review]: Yep.
Created attachment 332289 [details] [review] keyboard: introduce CcKeyboardShortcutEditor Applied all the comments before, and now it makes use of GtkShortcutLabel from Gtk+ master.
Created attachment 332290 [details] [review] keyboard: avoid stray Meta modified While using the Keyboard shortcut editor dialog under Wayland, the user receives a <Meta> key even when this key isn't present in the keyboard. This is probably the result of Wayland inheriting from X's xkb. To work around that, simply filter out this modified when outside an Apple OS (which does have this key).
Created attachment 332291 [details] [review] keyboard: introduce CcKeyboardShortcutEditor I actually forgot to update the commit message, and this patch fixes that.
Created attachment 332292 [details] [review] keyboard: avoid stray Meta modified While using the Keyboard shortcut editor dialog under Wayland, the user receives a <Meta> key even when this key isn't present in the keyboard. This is probably the result of Wayland inheriting from X's xkb. To work around that, simply filter out this modified when outside an Apple OS (which does have this key).
Created attachment 332293 [details] [review] keyboard: move keyboard management code to custom class (In reply to Bastien Nocera from comment #75) > From the previous review: > > I don't understand what the backend/front-end split could be here. What do you manipulate in the backend? From my previous comment: it handles the loading, creation and removal and search of keyboard shortcuts. It also resolves reversible shortcuts when searching. It's also a bit of personal taste here: I don't like mixing UI and non-UI operations and, since this code is used both by the panel and the shortcut editor dialog, I believe it's better for it to be shared through a new, non-UI class then delegating it to the panel.
Created attachment 332295 [details] [review] keyboard: bring back uniqueness check (In reply to Bastien Nocera from comment #76) > Remember: > > When doing the final commit, please make sure to reference the commit ID in which the code was removed. Fixed. > I guess you didn't want to do that: > > I would have preferred if that code was not removed, but moved even if ineffective along with a TODO comment. Not a requirement, but if it's doable with little efforts... Although I tried to do that, I didn't want indeed, and it also wasn't feasible with little nor medium efforts.
Created attachment 332296 [details] [review] keyboard: add API to track whether a shortcut is default No changes, update patch to apply on top of the others.
Created attachment 332297 [details] [review] keyboard: add support to reset shortcuts to their default values No changes, update patch to apply on top of the others.
Created attachment 332298 [details] [review] keyboard: bring back reverse item management Updated the comment.
Created attachment 332299 [details] [review] keyboard: add search support > Looks fine. Please add a note that it doesn't do search-as-you-type (and file a separate bug that depends on this one to implement it). It should do that. There seems to be a focus issue going on.
Created attachment 332300 [details] [review] keyboard: add API to track whether a shortcut is default Updated the comment
Review of attachment 332291 [details] [review]: Looks good.
Review of attachment 332292 [details] [review]: > even when this > key isn't present in the keyboard. Present on the keyboard? Or on the keymap? > To work around that, simply filter out this modified when > outside an Apple OS (which does have this key). This code won't run on MacOS, so you can remove that particular case. Could you please also file a bug against GTK+ so that gtk_accelerator_get_default_mod_mask() returns what it should? (and that means that the capture code capture a Meta key, I wonder why it would...)
Review of attachment 332293 [details] [review]: From the 2 previous reviews of this same patch: > I don't understand what the backend/front-end split could be here. What do you manipulate in the backend?
Review of attachment 332295 [details] [review]: Make sure to update the commit ID.
Review of attachment 332297 [details] [review]: From the previous review, acn.
Review of attachment 332298 [details] [review]: Looks good.
Review of attachment 332299 [details] [review]: From the previous review: > Please add a note that it doesn't do search-as-you-type (and file a separate bug that depends on this one to implement it).
Review of attachment 332070 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +545,3 @@ + + user_value = g_settings_get_user_value (self->settings, self->key); + is_value_default = user_value == NULL; That's not quite enough. It's possible that the user value is set to the same as the default value. You'll need to compare with the g_settings_get_default_value() output as well.
Created attachment 332302 [details] [review] keyboard: avoid stray Meta modified Done.
Created attachment 332303 [details] [review] keyboard: move keyboard management code to custom class Updated the commit message.
Created attachment 332304 [details] [review] keyboard: add API to track whether a shortcut is default Indeed. It's fixed now.
Review of attachment 332302 [details] [review]: Please also reference the GTK+ bug in the commit message. ::: panels/keyboard/cc-keyboard-shortcut-editor.c @@ +26,3 @@ +/* + * Workaround to stop receiving a stray Meta modifier. One-line comment, please.
Review of attachment 332303 [details] [review]: > creation and removal and search creation, removal and search Fine otherwise.
Review of attachment 332304 [details] [review]: ::: panels/keyboard/cc-keyboard-item.c @@ +548,3 @@ + + user_value = g_settings_get_user_value (self->settings, self->key); + is_value_default = user_value == NULL; That's the same code as in the previous review, still wrong.
Created attachment 332361 [details] [review] keyboard: add API to track whether a shortcut is default Shit, I squashed the correction with the wrong patch.
Created attachment 332362 [details] [review] keyboard: bring back reverse item management Update to work with the changes.
Review of attachment 332361 [details] [review]: Looks fine otherwise. ::: panels/keyboard/cc-keyboard-item.c @@ +549,3 @@ + user_value = g_settings_get_user_value (self->settings, self->key); + + if (!user_value) is_value_default = TRUE; if (user_value) { .... } No need for the out label. @@ +568,3 @@ + default_binding = ""; + + is_value_default = g_strcmp0 (default_binding, g_variant_get_string (user_value, NULL)) == 0; Put parenthesis around that.
Review of attachment 332362 [details] [review]: Looks good.
Thanks for all the reviews, and sorry for the mistakes I did throughout the proccess. Attachment 332061 [details] pushed as c4e1ca2 - keyboard: show all shortcuts in a single treeview Attachment 332062 [details] pushed as d85047d - keyboard: add a group field to CcKeyboardItem Attachment 332063 [details] pushed as c1529c3 - keyboard: add helper method for user-friendly accelerators Attachment 332072 [details] pushed as d0c32c3 - keyboard: remove unused code Attachment 332148 [details] pushed as dfa01ba - keyboard: expose structures in header Attachment 332164 [details] pushed as d940d7b - keyboard: make it a template class Attachment 332166 [details] pushed as fd30442 - keyboard: show shortcuts in a listbox Attachment 332167 [details] pushed as 847fe44 - keyboard: remove the shortcuts treeview Attachment 332291 [details] pushed as a0a1558 - keyboard: introduce CcKeyboardShortcutEditor Attachment 332295 [details] pushed as 4e50c3c - keyboard: bring back uniqueness check Attachment 332297 [details] pushed as 1c85479 - keyboard: add support to reset shortcuts to their default values Attachment 332299 [details] pushed as ab8721f - keyboard: add search support Attachment 332302 [details] pushed as 70bba2b - keyboard: avoid stray Meta modified Attachment 332303 [details] pushed as 60b2357 - keyboard: move keyboard management code to custom class Attachment 332361 [details] pushed as 376459a - keyboard: add API to track whether a shortcut is default Attachment 332362 [details] pushed as 7b877bc - keyboard: bring back reverse item management
keyboard: show all shortcuts in a single treeview: https://git.gnome.org/browse/gnome-control-center/commit/panels/keyboard?id=c4e1ca2ee0a70fd70dec288f9aa157f140386a2b This commit removed this part: if (g_str_equal (id, "Typing")) fill_xkb_options_shortcuts (shortcut_model); And it seems that it was never added back. Typing group is missing 3 shortcuts.
(In reply to Alberts Muktupāvels from comment #120) > keyboard: show all shortcuts in a single treeview: > https://git.gnome.org/browse/gnome-control-center/commit/panels/ > keyboard?id=c4e1ca2ee0a70fd70dec288f9aa157f140386a2b > > This commit removed this part: > if (g_str_equal (id, "Typing")) > fill_xkb_options_shortcuts (shortcut_model); > > And it seems that it was never added back. Typing group is missing 3 > shortcuts. This was filed in bug 770955, and followed-up in bug 771009.