GNOME Bugzilla – Bug 766922
Implement the redesigned shell
Last modified: 2016-06-03 15:37:19 UTC
We have new mockups lying around for quite some time now. The patches that will be attached to this bug report are a tentative to move things forward. Basically, it changes the window from a icon grid to a side list. In this patchset, I added a new non-installable binary (gnome-control-center-alt) to test the panels both with the current and the next layouts in parallel.
Created attachment 328568 [details] [review] window: add a horizontal box before the stack The horizontal box will be used in the future commits to separate the sidelist from the stack.
Created attachment 328571 [details] [review] window: turn into a final class This commit updates the code to use the recently introduced API. The new functions improve the legibility and maintainability of the code, and makes it easier to work on new features.
Created attachment 328572 [details] [review] window: add a horizontal box before the stack The horizontal box will be used in the future commits to separate the sidelist from the stack.
Created attachment 328573 [details] [review] window: separate the panel selector To progressively achieve the sidelist, let's start by moving the current iconview selector to the side and then turning it into a GtkListBox. This commit, then, moves the iconviews' list to the start of the horizontal box added in the previous commit.
Created attachment 328574 [details] [review] window: make the panels' stack expand This way, panels can fill the most of the screen, which is the desired behavior proposed by the mockups.
Created attachment 328575 [details] [review] window: remove the fixed width request code Having a fixed width is bad for a various reasons. First, it's harder to deal with low resolution screens, which was fixed by adding an entire new mode just to deal with it. Second, because it may not scale well for big resolutions as well. Fix that by removing the code that handles the fixed width. The next commit will completely remove the code that manages small screens, in the hope that the new layout will handle both cases well by design.
Created attachment 328576 [details] [review] window: remove the small screen hack Because the old layout didn't scale well on low resolution displays, we had to introduce code that adapts the window to be usable on low res screens. The problem with this code is that it still doesn't scale down very well for really low resolution screens. Partially because of the layout itself, partially because the panels request a size bigger than e.g. 720x480. Because I want to make the new sidelist layout work on all kinds of displays, I'm removing the small screen hack and, if needed, adapt the panels so that they behave better.
Created attachment 328577 [details] [review] window: make it a template class In order to prepare ourselves for the future changes, having the window as a template class is hugely advantageous for we'll be able to modify the interface much more quickly and cleanly. This commit makes the window a template class, and only that. No behavioral changes, nor new features were introduced here.
Created attachment 328579 [details] [review] window: keep searchbar visible when panel changes The sidebar width is determined by the search entry, and now it changes whenever we open a panel. This would also break the desired behavior of keeping the search visible - something impossible before. Fix that by not hiding the search bar when the visible panel changes.
Created attachment 328580 [details] [review] window: remove the previous button The previous button is not present on the new sidelist layout mockups, and doesn't really fit the current behavior anymore. This commit removes the previous button.
Created attachment 328581 [details] [review] window: keep search button always visible Since we're not changing the entire page when selecting a panel anymore, the search can stay visible all the time. This commit, then, removes the code that hides the search button when a panel is selected.
Created attachment 328582 [details] [review] window: put the search button in the start of the headerbar Since the search bar has been moved to above the side list, it makes sense to have the search button on the start of the headerbar, strengtening the relation between both widgets. To make it happen, remove the code that sets the search button at the end of the headerbar.
Created attachment 328583 [details] [review] window: pack the headerbar inside a box In the future commits, we'll use two headerbars to visually separate the panel area and the sidelist.
Created attachment 328584 [details] [review] window: introduce the second headerbar This commit introduces the second headerbar, where the panel title and the panel widgets are displayed. It also adapts the code to use the second headerbar when needed.
Created attachment 328585 [details] [review] window: make the sidelist and first headerbar horizontally equal The first headerbar and the sidelist should stay synchronized, and this commit does so by setting the width request of the headerbar to the allocated width of the sidelist.
Created attachment 328586 [details] [review] window: set the default width and height Since the window can shrink down too much, it makes sense for us to have a sane default width and height. I arbitrarily choose 720x480 because that's the default resolution for CRT TVs.
Created attachment 328587 [details] [review] window: add a nice placeholder view
Created attachment 328588 [details] [review] window: use a listbox The latest mockups use a list instead of icon grid. This commit, after all the work porting the current UI to use a side list, replacing the weird-looking icon grid.
Created attachment 328589 [details] [review] window: reimplement search We previously had a dedicate view for handling search, based on model filtering and a custom panel to display that differently. After moving to GtkListBox, search can be trivially done by using a filtering function, and widgets can be fine-tuned to display extra information. This patch, then, reimplements the search using a filtering function over the panels' list.
Created attachment 328590 [details] [review] build: create another executable for the new layout This way, we can test both layouts in parallel and, in case something goes wrong, we always have to old layout as a fallback.
Created attachment 328605 [details] [review] build: create another executable for the new layout This way, we can test both layouts in parallel and, in case something goes wrong, we always have to old layout as a fallback.
Created attachment 328606 [details] [review] window: turn into a final class This commit updates the code to use the recently introduced API. The new functions improve the legibility and maintainability of the code, and makes it easier to work on new features.
Created attachment 328607 [details] [review] window: add a horizontal box before the stack The horizontal box will be used in the future commits to separate the sidelist from the stack.
Created attachment 328608 [details] [review] window: separate the panel selector To progressively achieve the sidelist, let's start by moving the current iconview selector to the side and then turning it into a GtkListBox. This commit, then, moves the iconviews' list to the start of the horizontal box added in the previous commit.
Created attachment 328609 [details] [review] window: make the panels' stack expand This way, panels can fill the most of the screen, which is the desired behavior proposed by the mockups.
Created attachment 328610 [details] [review] window: remove the fixed width request code Having a fixed width is bad for a various reasons. First, it's harder to deal with low resolution screens, which was fixed by adding an entire new mode just to deal with it. Second, because it may not scale well for big resolutions as well. Fix that by removing the code that handles the fixed width. The next commit will completely remove the code that manages small screens, in the hope that the new layout will handle both cases well by design.
Created attachment 328611 [details] [review] window: remove the small screen hack Because the old layout didn't scale well on low resolution displays, we had to introduce code that adapts the window to be usable on low res screens. The problem with this code is that it still doesn't scale down very well for really low resolution screens. Partially because of the layout itself, partially because the panels request a size bigger than e.g. 720x480. Because I want to make the new sidelist layout work on all kinds of displays, I'm removing the small screen hack and, if needed, adapt the panels so that they behave better.
Created attachment 328612 [details] [review] window: make it a template class In order to prepare ourselves for the future changes, having the window as a template class is hugely advantageous for we'll be able to modify the interface much more quickly and cleanly. This commit makes the window a template class, and only that. No behavioral changes, nor new features were introduced here.
Created attachment 328613 [details] [review] window: keep searchbar visible when panel changes The sidebar width is determined by the search entry, and now it changes whenever we open a panel. This would also break the desired behavior of keeping the search visible - something impossible before. Fix that by not hiding the search bar when the visible panel changes.
Created attachment 328614 [details] [review] window: remove the previous button The previous button is not present on the new sidelist layout mockups, and doesn't really fit the current behavior anymore. This commit removes the previous button.
Created attachment 328615 [details] [review] window: keep search button always visible Since we're not changing the entire page when selecting a panel anymore, the search can stay visible all the time. This commit, then, removes the code that hides the search button when a panel is selected.
Created attachment 328616 [details] [review] window: put the search button in the start of the headerbar Since the search bar has been moved to above the side list, it makes sense to have the search button on the start of the headerbar, strengtening the relation between both widgets. To make it happen, remove the code that sets the search button at the end of the headerbar.
Created attachment 328617 [details] [review] window: pack the headerbar inside a box In the future commits, we'll use two headerbars to visually separate the panel area and the sidelist.
Created attachment 328618 [details] [review] window: introduce the second headerbar This commit introduces the second headerbar, where the panel title and the panel widgets are displayed. It also adapts the code to use the second headerbar when needed.
Created attachment 328619 [details] [review] window: make the sidelist and first headerbar horizontally equal The first headerbar and the sidelist should stay synchronized, and this commit does so by setting the width request of the headerbar to the allocated width of the sidelist.
Created attachment 328620 [details] [review] window: set the default width and height Since the window can shrink down too much, it makes sense for us to have a sane default width and height. I arbitrarily choose 720x480 because that's the default resolution for CRT TVs.
Created attachment 328621 [details] [review] window: add a nice placeholder view
Created attachment 328622 [details] [review] window: use a listbox The latest mockups use a list instead of icon grid. This commit, after all the work porting the current UI to use a side list, replacing the weird-looking icon grid.
Created attachment 328623 [details] [review] window: reimplement search We previously had a dedicate view for handling search, based on model filtering and a custom panel to display that differently. After moving to GtkListBox, search can be trivially done by using a filtering function, and widgets can be fine-tuned to display extra information. This patch, then, reimplements the search using a filtering function over the panels' list.
Review of attachment 328605 [details] [review]: Copy the cc-window.[ch] files as a separate commit, and mention that they are exact copies. ::: shell/Makefile.am @@ +21,3 @@ bin_PROGRAMS = gnome-control-center +noinst_bindir = $(top_srcdir)/shell/alt There's no bindir for a noinst. @@ +58,3 @@ +gnome_control_center_alt_SOURCES = \ + $(common_sources) \ + alt/cc-window.c \ No, add a Makefile.am in alt/ and compile a library there, which you can link against the alt binary. @@ +106,3 @@ +gnome_control_center_LDADD = $(common_ldadd) +gnome_control_center_alt_LDADD = $(common_ldadd) how about gnome_control_center_alt_LDADD = $(gnome_control_center_LDADD) and not any of the changes above?
Review of attachment 328606 [details] [review]: That looks fine, but I would introduce this *before* the patch introducing the alt binary.
Review of attachment 328607 [details] [review]: Ok. Pending future changes.
Review of attachment 328608 [details] [review]: Ok.
Review of attachment 328609 [details] [review]: Sure.
Review of attachment 328610 [details] [review]: > for a various reasons. for various reasons. > Second, because it may not scale well for big resolutions > as well. "it may not scale as well for big resolutions"
Review of attachment 328611 [details] [review]: > Partially because of the layout itself, partially > because the panels request a size bigger than e.g. > 720x480. It was never made to work well with 720x480 because the minimum resolution we were targeting 720x480. You should mention that we never targeted 720x480, but that we want to support it now. Rather than imply that it was bad code :P > Because I want to make the new sidelist layout work > on all kinds of displays, I'm removing the small screen > hack and, if needed, adapt the panels so that they > behave better. Don't use first person here. Use declarative language. The whole commit message needs more work.
Review of attachment 328612 [details] [review]: That would have been nicer to do as one of the first commits, rather than in the middle of the changes. Explain the reason why that is in the commit message.
Review of attachment 328613 [details] [review]: That's not how it's been designed: https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/shell/settings-search.png The sidebar's width needs to be the same width as the longest label + padding.
Review of attachment 328614 [details] [review]: It will be used when we go into the "devices" or "details" sub-category: https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/shell/settings-organization.png
Review of attachment 328615 [details] [review]: That's fine for now.
Review of attachment 328616 [details] [review]: Yep. > side list Make sure you use the same word (or words) in all the commits. It's been "sidelist", "side list", "sidebar". > strengtening Missing letters here. > To make it happen, remove the code that sets the search button > at the end of the headerbar. This is not needed, we're pretty stupid but not that much ;)
Review of attachment 328617 [details] [review]: Ok.
Review of attachment 328618 [details] [review]: ::: shell/alt/cc-window.c @@ +54,3 @@ GtkWidget *stack; GtkWidget *header; + GtkWidget *header2; Needs a better name. panel_headerbar?
Review of attachment 328619 [details] [review]: Sure.
Review of attachment 328620 [details] [review]: That's too big on a 720x480, which will include a system toolbar of some kind (so it will get maximised), and that's probably a bit small for larger screens. If you went with 1024x768 as a "default" size, then it will maximise the window, and look big enough on larger screens. Feel free to test.
Review of attachment 328620 [details] [review]: And before I forget, we might want the default size to be a 16:9 ratio, so that the panel itself is 4:3. You can discuss the exact size to use with #gnome-design.
Review of attachment 328621 [details] [review]: Do we have mockups for that? I'd like to have the language checked.
Review of attachment 328622 [details] [review]: ::: shell/alt/cc-window.c @@ +622,3 @@ + gtk_tree_model_get_iter_first (model, &iter); + + while (gtk_tree_model_iter_next (model, &iter)) Pretty sure you're skipping the first iter. I prefer: cont = gtk_tree_model_get_iter_first (model, &iter); while (cont) { cont = _iter_next(); } @@ +1129,3 @@ + g_str_equal, + g_free, + (GDestroyNotify) row_data_free); I'd rather you made the RowData belong to the ListBox widget, with g_object_set_data_full()
Review of attachment 328623 [details] [review]: > panels' list. A list of panels that belong to multiple ones? "panels list" is fine. This will need work. You removed the whole filtering code that was there before without reimplementing it, that's no good, and your code assumes that the search is always visible, which isn't something we want.
Created attachment 328732 [details] [review] window: turn into a final class Done.
Created attachment 328733 [details] [review] build: copy CcWindow in alt/ directory The copied files are exact copies of shell/cc-window.c and shell/cc-window.h, and they'll be used to implement the restyled shell as an alternative binary.
Review of attachment 328732 [details] [review]: Sure.
Created attachment 328735 [details] [review] build: create another executable for the new layout Done. Split the patch in 2, and this is the second one.
Created attachment 328736 [details] [review] window: add a horizontal box before the stack
Created attachment 328737 [details] [review] window: separate the panel selector To progressively achieve the sidelist, let's start by moving the current iconview selector to the side and then turning it into a GtkListBox. This commit, then, moves the iconviews' list to the start of the horizontal box added in the previous commit.
Created attachment 328738 [details] [review] window: make the panels' stack expand
Created attachment 328739 [details] [review] window: remove the fixed width request code Fixed the typos.
Created attachment 328740 [details] [review] window: remove the small screen code Updated the commit message.
Created attachment 328741 [details] [review] window: make it a template class I didn't made it before because I didn't even think about it. Are you OK in accepting this patch only for the sidelist CcWindow?
Created attachment 328742 [details] [review] window: put the search button in the start of the headerbar Updated the commit message.
Created attachment 328743 [details] [review] window: pack the headerbar inside a box
Created attachment 328744 [details] [review] window: introduce the second headerbar (In reply to Bastien Nocera from comment #53) > Review of attachment 328618 [details] [review] [review]: > > ::: shell/alt/cc-window.c > @@ +54,3 @@ > GtkWidget *stack; > GtkWidget *header; > + GtkWidget *header2; > > Needs a better name. panel_headerbar? Done.
Created attachment 328745 [details] [review] window: make the sidelist and first headerbar horizontally equal
Created attachment 328746 [details] [review] window: set the default width and height I still have to discuss this one.
Review of attachment 328733 [details] [review]: Sure.
Created attachment 328747 [details] [review] window: add a nice placeholder view Also have to discuss this one.
Created attachment 328748 [details] [review] window: use a listbox Removed the hash table, now it uses g_object_set_data_full(). Also fixed the iterator issue. Thanks for pointing that out.
Created attachment 328750 [details] [review] window: reimplement search Is the filtering code relevant still?
Review of attachment 328735 [details] [review]: Looks good apart from that. ::: configure.ac @@ +7,3 @@ AC_CONFIG_MACRO_DIR([m4]) +AM_INIT_AUTOMAKE([1.11.2 no-dist-gzip dist-xz tar-ustar check-news subdir-objects]) Pretty sure you don't need subdir-objects with a Makefile.am in the alt/ directory.
Review of attachment 328736 [details] [review]: Sure.
Review of attachment 328737 [details] [review]: Yep.
Review of attachment 328738 [details] [review]: Yep.
Review of attachment 328739 [details] [review]: Sure.
Review of attachment 328740 [details] [review]: Sure.
Review of attachment 328741 [details] [review]: Looks good. (as you said, you could have done that with the old code, but it's really not worth it right now, as we're unlikely to change the UI of the old shell...)
Review of attachment 328742 [details] [review]: Yep.
Review of attachment 328743 [details] [review]: Yep.
Review of attachment 328744 [details] [review]: Looks good.
Review of attachment 328745 [details] [review]: Sure.
Review of attachment 328746 [details] [review]: > I arbitrarily choose 720x480 because that's the default > resolution for CRT TVs. Better without "I": 720x480 was chosen because ... Looks good otherwise.
Review of attachment 328747 [details] [review]: > window: add a nice placeholder view You can remove the "nice"
Review of attachment 328748 [details] [review]: > replacing the weird-looking icon grid. replacing the temporary icon grid. We can make more changes later on. ::: shell/alt/cc-window.c @@ +169,3 @@ + + /* Icon */ + image = gtk_image_new_from_gicon (icon, GTK_ICON_SIZE_BUTTON); No icons, thanks.
Review of attachment 328750 [details] [review]: ::: shell/alt/cc-window.c @@ +357,2 @@ static gboolean +filter_func (GtkListBoxRow *row, Your filter func doesn't casefold or unaccent, doesn't split by term. Look at the contents of search_entry_changed_cb(). The code is already there, you just need not to remove it ("network" doesn't match "Network"...)
Created attachment 329050 [details] [review] window: set the default width and height Since the window can shrink down too much, it makes sense for us to have a sane default width and height. 720x480 was chosen because that's the default resolution for CRT TVs.
Created attachment 329051 [details] [review] window: set the default width and height Fixed the commit message
Created attachment 329052 [details] [review] window: use a listbox Removed the icons.
Created attachment 329053 [details] [review] window: reimplement search Added unaccent, casefolding and trimming support.
Review of attachment 329052 [details] [review]: Looks fine otherwise. ::: shell/alt/cc-window.c @@ +611,3 @@ + shell->store = (GtkListStore *) cc_shell_model_new (); + model = GTK_TREE_MODEL (shell->store); + valid = TRUE; Remove this. @@ +616,3 @@ + + /* Create a row for each panel */ + gtk_tree_model_get_iter_first (model, &iter); valid = ...
Created attachment 329057 [details] [review] window: use a listbox Fixed the valid var.
Done. I'll open separate bug reports for individual issues and features. Attachment 328732 [details] pushed as 71d39a4 - window: turn into a final class Attachment 328733 [details] pushed as fe3a237 - build: copy CcWindow in alt/ directory Attachment 328735 [details] pushed as 2bb6d44 - build: create another executable for the new layout Attachment 328736 [details] pushed as c006198 - window: add a horizontal box before the stack Attachment 328737 [details] pushed as c47bc08 - window: separate the panel selector Attachment 328738 [details] pushed as 2f844e2 - window: make the panels' stack expand Attachment 328739 [details] pushed as 77ef6c4 - window: remove the fixed width request code Attachment 328740 [details] pushed as 6f38ca8 - window: remove the small screen code Attachment 328741 [details] pushed as 55cc4c3 - window: make it a template class Attachment 328742 [details] pushed as df086d4 - window: put the search button in the start of the headerbar Attachment 328743 [details] pushed as fa1a6b3 - window: pack the headerbar inside a box Attachment 328744 [details] pushed as c0bdc06 - window: introduce the second headerbar Attachment 328745 [details] pushed as a267ebd - window: make the sidelist and first headerbar horizontally equal Attachment 329051 [details] pushed as 2590e38 - window: set the default width and height Attachment 329053 [details] pushed as 57ba49a - window: reimplement search Attachment 329057 [details] pushed as b6ab2a4 - window: use a listbox