GNOME Bugzilla – Bug 779216
split up Details panel
Last modified: 2017-07-25 03:19:17 UTC
As for the new gnome-control-center design, each page of details page has to be split up to fit to the new design.
Created attachment 346705 [details] [review] split up Details panel This patch splits only the Overview panel. Please review the patch, and let me know if I'm doing things right. :)
Created attachment 346708 [details] [review] split up Details panel
Review of attachment 346708 [details] [review]: Overall, the patch is pretty simple and looks good. There are a couple of issues though: - It'd be better to call the new class CcInfoOverViewPanel (will avoid renaming it in the future) - The commit message needs much more improvement... See the log of Control Center's git repo to see how they should be formatted. ::: panels/info/Makefile.am @@ +23,3 @@ cc-info-panel.h \ + cc-info-overview.c \ + cc-info-overview.h \ Misalingnment of '\'
Created attachment 346825 [details] [review] details: Split overview as a separate panel The new shell design requires each panel to be separate. This commit splits the overview page from details panel as a seperate panel.
Review of attachment 346825 [details] [review]: Looks good
Comment on attachment 346825 [details] [review] details: Split overview as a separate panel Thanks for the patch. Now that we're entering GNOME 3.26 cycle, I'll push the patch. Attachment 346825 [details] pushed as fd5c0e8 - details: Split overview as a separate panel
Because the redesign is a priority for 3.26, I'm increasing the priority of this bug.
Created attachment 352255 [details] [review] info: Remove unused code
Review of attachment 352255 [details] [review]: Looks good!
Comment on attachment 352255 [details] [review] info: Remove unused code Pushed with a minor addition to the commit message. Attachment 352255 [details] pushed as 7bebbc0 - info: Remove unused code
Created attachment 352405 [details] [review] details: Split default apps as a separate panel
The above patch is almost complete. The mnemonic actions are not configured.
Created attachment 352494 [details] [review] details: Split default apps as a separate panel
Review of attachment 352494 [details] [review]: Per IRC discussion, the mnemonic issue should be handled like Calendar using struct offsets: https://git.gnome.org//browse/gnome-calendar/tree/src/gcal-edit-dialog.c#n758 ::: panels/info/cc-info-default-apps-panel.c @@ +76,3 @@ + + /*< private >*/ + CcInfoDefaultAppsPanelPrivate *priv; You don't need a private struct when on final classes. Just use this struct directly. @@ +80,3 @@ + + +G_DEFINE_TYPE_WITH_PRIVATE (CcInfoDefaultAppsPanel, cc_info_default_apps_panel, GTK_TYPE_BOX) G_DEFINE_TYPE() only, without private @@ +86,3 @@ +{ + G_OBJECT_CLASS (cc_info_default_apps_panel_parent_class)->finalize (object); +} Since finalize() is empty, you can just not override it. @@ +186,3 @@ + +static void +info_panel_setup_default_apps (CcInfoDefaultAppsPanel *self) Use only one space in function parameter. @@ +190,3 @@ + int i; + + for (i = 0; i < G_N_ELEMENTS(preferred_app_infos); i++) Space between 'G_N_ELEMENTS' and '('
Created attachment 352496 [details] [review] details: Split default apps as a separate panel
Comment on attachment 352496 [details] [review] details: Split default apps as a separate panel LGTM
Comment on attachment 352496 [details] [review] details: Split default apps as a separate panel Attachment 352496 [details] pushed as 1154658 - details: Split default apps as a separate panel
Created attachment 352546 [details] [review] info: Remove unused struct members These struct members has been moved to overview panel and is no longer used here.
Created attachment 352562 [details] [review] details: Split removable media as a separate panel
Review of attachment 352546 [details] [review]: ++
Review of attachment 352562 [details] [review]: That CcInfoMediaDialog class is an overkill. Having the dialog in 'info-removable-media.ui' is just fine. Some other comments below. ::: panels/info/cc-info-panel.c @@ +211,3 @@ info_panel_setup_selector (self); info_panel_setup_overview (self); + /* info_panel_setup_media (self); */ You should completely remove it rather than commenting it out. ::: panels/info/cc-info-removable-media-panel.c @@ +80,3 @@ + GtkWidget *media_software_combobox; + + GtkWidget *media_dialog; The struct fields should be visually aligned. @@ +571,3 @@ +{ + G_OBJECT_CLASS (cc_info_removable_media_panel_parent_class)->finalize (object); +} No need to override it finalize() it's empty. @@ +608,3 @@ + +GtkWidget * +cc_info_removable_media_panel_new (void) Not needed. ::: panels/info/cc-info-removable-media-panel.h @@ +29,3 @@ +G_DECLARE_FINAL_TYPE (CcInfoRemovableMediaPanel, cc_info_removable_media_panel, CC, INFO_REMOVABLE_MEDIA_PANEL, GtkBox) + +GtkWidget *cc_info_removable_media_panel_new (void); Not needed.
Comment on attachment 352546 [details] [review] info: Remove unused struct members Attachment 352546 [details] pushed as 84ccb36 - info: Remove unused struct members
Created attachment 352691 [details] [review] details: Split removable media as a separate panel
(In reply to Georges Basile Stavracas Neto from comment #21) > Review of attachment 352562 [details] [review] [review]: > > That CcInfoMediaDialog class is an overkill. Having the dialog in > 'info-removable-media.ui' is just fine. Some other comments below. We moved the ui to templates. So I thought it would be easy to use another template (ie, another class) for CcInfoMediaDialog rather than adding hacks to the template and use builder again.
Created attachment 352699 [details] [review] details: Split removable media as a separate panel
Created attachment 352700 [details] [review] details: Split removable media as a separate panel
Review of attachment 352700 [details] [review]: LGTM
Review of attachment 352700 [details] [review]: Actually, there's a runtime error.
Created attachment 352701 [details] [review] details: Split removable media as a separate panel The runtime error was also occuring in master. So this was introduced long time ago. Anyway, this has been fixed by checking for NULL. Error from sanitizer: cc-info-removable-media-panel.c:324:52: runtime error: null pointer passed as argument 1, which is declared to never be null
Review of attachment 352701 [details] [review]: Looks good!
Comment on attachment 352701 [details] [review] details: Split removable media as a separate panel Attachment 352701 [details] pushed as 8713eac - details: Split removable media as a separate panel
Removable Media doesn't look like a separate panel with gnome-control-center-alt in git master. Maybe there's more work to be done?
(In reply to Jeremy Bicha from comment #32) > Removable Media doesn't look like a separate panel with > gnome-control-center-alt in git master. Maybe there's more work to be done? Yeah. The work is done only in gnome-control-center not in alternate gnome-control-center-alt. It has to be done.
(In reply to Jeremy Bicha from comment #32) > Removable Media doesn't look like a separate panel with > gnome-control-center-alt in git master. Maybe there's more work to be done? It is not meant to be seen as split panels ~yet~. Sadiq is splitting the code in various subclasses but, until we're not 100% sure we'll move to the new Shell, it'll be one Details panel. When the Network and Sound panels redesigns land, then we drop the bomb and make the various Details subpanels different panels.
Created attachment 353290 [details] [review] info: Remove dead ui code
Created attachment 353291 [details] [review] info-overview: replace GtkTable with GtkGrid GtkTable has been deprecated. So let it be replaced with GtkGrid.
Created attachment 353292 [details] [review] removable-media: Replace GtkTable with GtkGrid
Review of attachment 353290 [details] [review]: Looks good. I'm sure you could write better commit messages...
Review of attachment 353291 [details] [review]: Looks good too.
Review of attachment 353292 [details] [review]: Looks good.
Attachment 353290 [details] pushed as d6db504 - info: Remove dead ui code Attachment 353291 [details] pushed as 695887c - info-overview: replace GtkTable with GtkGrid Attachment 353292 [details] pushed as 4a06cff - removable-media: Replace GtkTable with GtkGrid
Created attachment 353498 [details] [review] info-overview: Simplify UI code Increasing width of a column makes column in each row to have the same width. So it is unnecessary to set 3rd column of each row separately to have the same width. Let's do it just once.
Created attachment 353508 [details] [review] removable-media: Avoid use of GtkAlignment GtkAlignment has been deprecated since Gtk 3.14. So Let's replace it with properties like "valign", "halign", "margin", and so on. Sorry, it's a bit noisy. What I have done is replace usage of <property name="left_padding">12</property> with <property name="margin-left">12</property> in the child (also remove GtkAlignment) and the whole source code is indented.
Created attachment 353509 [details] [review] removable-media: Avoid use of GtkAlignment GtkAlignment has been deprecated since Gtk 3.14. So Let's replace it with properties like "valign", "halign", "margin", and so on. replaced margin-left from last patch with margin-start.
Review of attachment 353498 [details] [review]: Nice.
Review of attachment 353509 [details] [review]: Noisy, but good enough.
Attachment 353498 [details] pushed as 572f9fe - info-overview: Simplify UI code Attachment 353509 [details] pushed as f82af13 - removable-media: Avoid use of GtkAlignment
Created attachment 353519 [details] [review] info: Derive subpanels from CcPanel This is required for adding these subpanels as panels for the new g-c-c shell design.
Created attachment 353520 [details] [review] info: Trivial, fix indentation
Review of attachment 353519 [details] [review]: Looks good.
Review of attachment 353520 [details] [review]: Looks good
Attachment 353519 [details] pushed as 0e01c7d - info: Derive subpanels from CcPanel Attachment 353520 [details] pushed as bab859a - info: Trivial, fix indentation
Created attachment 353527 [details] [review] info: Show the panels as separate in shell/alt Show only the Details panel in current design. In alternate shell design, hide the Details panel, and show subpanels as panels. A few things are yet to be done: * Add a separate symbolic icon for each panel * Set the right padding from margin * Add some command line arguments that will load the corresponding panel
Created attachment 353528 [details] [review] shell-model: Add hidden settings Some panels shall be shown only in current design, And some other panels shall be shown only in new Shell design. So let's have a code that would help us do that
Created attachment 353529 [details] [review] info: Add desktop files for the split panels This commit shall show the panels separate. Some panels are hidden in current design, while some other panels are hidden in new shell design.
Review of attachment 353528 [details] [review]: ::: shell/cc-panel-loader.c @@ +196,3 @@ +#else +#define CC_HIDE_CATEGORY CC_CATEGORY_HIDDEN +#endif By using only one category, this can be removed (and below, just check for "category != CC_CATEGORY_HIDDEN") ::: shell/cc-shell-model.h @@ +66,3 @@ CC_CATEGORY_SYSTEM, CC_CATEGORY_HARDWARE, + CC_CATEGORY_HIDDEN, This can be simplified by adding only one "CC_CATEGORY_HIDDEN" outside the #ifdefs.
Review of attachment 353529 [details] [review]: Looks good.
Created attachment 356329 [details] [review] shell-model: Add hidden settings Implement the single CC_CATEGORY_HIDDEN thing mentioned before.
Created attachment 356330 [details] [review] info: Make margins consistent with other panels All the new panels have a standard 24px margin now, so since we're already splitting the info pages into separate panels, also fix this minor annoyance.
Created attachment 356331 [details] [review] panel-loader: Cosmetic changes This commit only fixes some very minor cosmetic changes like int → gint, simplifies the code by using g_autofoo, et cetera.
Attachment 353529 [details] pushed as 6cf52ad - info: Add desktop files for the split panels Attachment 356329 [details] pushed as 1485b50 - shell-model: Add hidden settings Attachment 356330 [details] pushed as 5bc84ae - info: Make margins consistent with other panels Attachment 356331 [details] pushed as b419404 - panel-loader: Cosmetic changes
Created attachment 356332 [details] [review] info-panel: Remove unused headers