GNOME Bugzilla – Bug 726252
Refactor topbar's children into separate classes/modules
Last modified: 2016-03-31 13:22:07 UTC
The topbar's role should be limited to switching between the different toolbars at the right time. Actually it also implements some of the toolbars it manages: they should have their own types and implementations.
Created attachment 271755 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 271757 [details] [review] Use properties-toolbar Replaces the current implementation of the properties toolbar in topbar by a separate on to avoid cluttering the topbar to answer part of bug #726252.
Review of attachment 271755 [details] [review]: I don't think this should be a separate patch from the following one. Having them together will also make it clear that we are not exactly introducing a completely new code but rather only moving existing code to new file/class.
Created attachment 271791 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 271792 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252.
I found an error in 271792: Boxes.CollectionToolbar.setup_ui should contain "assert (App.window.searchbar != null);".
Review of attachment 271792 [details] [review]: Boxes.CollectionToolbar.setup_ui should contain "assert (App.window.searchbar != null);".
Created attachment 271804 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252.
Created attachment 271805 [details] [review] Add selection-toolbar Gives the selection toolbar a proper implementation to answer part of bug #726252.
Review of attachment 271791 [details] [review]: Looks good. The commit log details needs something to be desired (not more info but better description) but I can handle that when merging but please do read the commit logs after I push.
Review of attachment 271757 [details] [review]: Your forgot to obsolete this one.
Review of attachment 271804 [details] [review]: same comment about the commit log details.
Review of attachment 271805 [details] [review]: ::: data/ui/selection-toolbar.ui @@ +8,3 @@ + + <child> + <object class="GtkHeaderBar" id="selection_toolbar"> Well now you can remove the "selection_" prefix from ids in here.
I take it that patches for display-toolbar and wizard-toolbar are on the way? Please keep in mind the comment about no need for ID prefixes anymore.
Created attachment 272054 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252.
Created attachment 272055 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252.
Review of attachment 272054 [details] [review]: ack
Review of attachment 272054 [details] [review]: actually found a minor issue. ::: src/topbar.vala @@ +91,2 @@ App.app.notify["selected-items"].connect (() => { + selection_toolbar.update_selection_label (); Pretty sure we can move this to SelectionToolbar now.
Review of attachment 272055 [details] [review]: Looks good otherwise. ::: src/wizard-toolbar.vala @@ +13,3 @@ + public Gtk.Button create_btn; + + public void back () { Lets keep them consistent with Topbar naming for similar methods: click_back_button and click_forward_button. Besides its more descriptive what it really does.
Created attachment 272061 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252.
Created attachment 272062 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252.
Some issues here: 1. Your patches contain trailing whitespace, please remove those. 2. The last patch (wizard toolbar) doesn't apply here. Please make sure all patches here are recent version. 3. I get this on console with collection toolbar patch: (gnome-boxes:32100): Boxes-CRITICAL **: boxes_collection_toolbar_set_title: assertion 'title != NULL' failed Please investigate that.
Review of attachment 271791 [details] [review]: ::: src/Makefile.am @@ +118,3 @@ os-database.vala \ properties.vala \ + properties-toolbar.vala \ Please align the '\' here with others above it. Unlike in sources, tabs are used in Makefile(.am)s.
Review of attachment 271804 [details] [review]: ::: src/topbar.vala @@ +124,2 @@ private void update_search_btn () { + collection_toolbar.update_search_btn (); I don't think we need to expose update_search_btn, since CollectionToobar can hook to items_added/removed itself. @@ +129,2 @@ private void update_select_btn () { + collection_toolbar.update_select_btn (); same here and hence update_select_btn becomes completely redundant here.
Review of attachment 272061 [details] [review]: ::: src/topbar.vala @@ +29,3 @@ private CollectionToolbar collection_toolbar; [GtkChild] + private SelectionToolbar selection_toolbar; This becomes redundant too due to changes suggested in comments below. @@ +102,3 @@ App.app.collection.item_added.connect (update_select_btn); App.app.collection.item_removed.connect (update_select_btn); + selection_toolbar.update_selection_label (); SelectionToolbar.update_selection_label doesn't need to be exposed I think as it has its own setup_ui method anyway where i can call this from. @@ +118,3 @@ private void update_search_btn () { collection_toolbar.update_search_btn (); + selection_toolbar.update_search_btn (); Same thing here as with CollectionToobar.update_search_btn just above it.
Review of attachment 272062 [details] [review]: Looks good, just ensure that it applies cleanly on top of the previous patches.
Created attachment 277743 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 277744 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: src/topbar.vala
Created attachment 277745 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui src/topbar.vala
Created attachment 277746 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui
Trailing whitespaces are still there in at least the first patch. Please make sure all points in comment#22 are addressed by each patch.
Created attachment 277877 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 277878 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: src/topbar.vala
Created attachment 277879 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui src/topbar.vala
Created attachment 277880 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui
Trailing whitespace still there! :(
Created attachment 277928 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 277929 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: src/topbar.vala
Created attachment 277930 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui src/topbar.vala
Created attachment 277931 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui
I was sure it was OK this time but I missed a serie of trailing spaces… it should be ok now.
Review of attachment 277928 [details] [review]: ::: src/properties-toolbar.vala @@ +1,2 @@ +// This file is part of GNOME Boxes. License: LGPLv2+ +using Gtk; This is redundant here. We should either remove this or 'Gtk.' everywhere below.
Review of attachment 277929 [details] [review]: git-bz points out: Applying: Add collection-toolbar /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:444: space before tab in indent. collection_toolbar.update_search_btn (); /home/zeenix/checkout/gnome/gnome-boxes/.git/rebase-apply/patch:450: space before tab in indent. collection_toolbar.update_select_btn (); warning: 2 lines add whitespace errors. ::: src/Makefile.am @@ +98,3 @@ collection-view.vala \ collection.vala \ + collection-toolbar.vala \ Please align the '\'. Seems other patches need this too. I have pointed this out before. Can you please go through all my comments and ensure that your patches are addressing those? ::: src/topbar.vala @@ +137,2 @@ private void update_search_btn () { + collection_toolbar.update_search_btn (); Sorry but did you actually care to go through my comments at all? I understand that you have been busy with other stuff and could easily forget them but can you please go through them all now to refresh your memory and address them *all* in your next version of patches? comment#24 in this case.
Apart for address my comments, please ensure that you don't introduce any runtime warnings (at least one of your patches does, when you start Boxes).
Created attachment 278007 [details] [review] Add properties-toolbar Gives the properties toolbar a proper implementation to answer part of bug #726252.
Created attachment 278008 [details] [review] Add collection-toolbar Gives the collection toolbar a proper implementation to answer part of bug #726252.
Created attachment 278009 [details] [review] Add selection-toolbar The selection toolbar is moved out of the topbar to its own files to answer part of bug #726252.
Created attachment 278010 [details] [review] Add wizard-toolbar The wizard toolbar is moved out of the topbar to its own files to answer part of bug #726252. https://bugzilla.gnome.org/show_bug.cgi?id=726252 Conflicts: data/ui/topbar.ui
Review of attachment 278007 [details] [review]: Looks good.
Review of attachment 278008 [details] [review]: Looks good otherwise. ::: src/collection-toolbar.vala @@ +38,3 @@ + } + + public void setup_ui () { No biggie but I prefer public methods coming before private. I'll correct it before merging..
Review of attachment 278009 [details] [review]: Looks good.
Review of attachment 278010 [details] [review]: Looks good