GNOME Bugzilla – Bug 775958
Tweak panel UI towards the new design
Last modified: 2021-06-09 16:29:24 UTC
There are new mockups for the Search panel at https://wiki.gnome.org/Design/SystemSettings/Search The following patches gradually tweak the panel towards the new design. I will use this bug to track the other bugs which are required for the completion of the new panel.
Created attachment 341771 [details] [review] search: Place the whole content in a scrolled window A step towards the new panel design is to place all the panel content inside of a scrolled window, instead of just the list box. The new panel mockups are available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 341772 [details] [review] search: Set min-content-height on the scrolled window Set a minimum content height of 490px for the panel when the allocated height is smaller than 490px. 490 is an estimated value for the panels to properly fit on netbook screens. See https://wiki.gnome.org/Design/SystemSettings#Notes
Created attachment 341773 [details] [review] search: Use list box separators between entries
Created attachment 341774 [details] [review] search: Drop undesirable shadow of scrolled Window
Review of attachment 341771 [details] [review]: Sure.
Review of attachment 341772 [details] [review]: That function again. Didn't we put that in a common place?
Review of attachment 341773 [details] [review]: Yep.
Review of attachment 341774 [details] [review]: Merge that into the first patch adding the scrolled window?
(In reply to Bastien Nocera from comment #6) > Review of attachment 341772 [details] [review] [review]: > > That function again. Didn't we put that in a common place? we did but these utilities don't quite apply in this case since we are embedding other content in the scrolled_window, such as the toolbar in the bottom. It will suit us better when we will have drag and drop.
Comment on attachment 341774 [details] [review] search: Drop undesirable shadow of scrolled Window it will be merged in the first patch.
Created attachment 350720 [details] [review] search: Place the whole content in a scrolled window A step towards the new panel design is to place all the panel content inside of a scrolled window, instead of just the list box. The new panel mockups are available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 350721 [details] [review] search: Use list box separators between entries
Created attachment 350722 [details] [review] search: Introduce Drag and Drop These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 350723 [details] [review] search: Move the gear button to the top of the panel Also introduce panel usage instructions. These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 350724 [details] [review] search: Drop move buttons Since we now support Drag & Drop to sort the list. These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 350725 [details] [review] search: Adjust panel height according to num of rows We cannot benefit here from the cc_list_box_setup_scrolling () list-box helper function because we are also embedding other widgets in the scrolled window, such as the box which contains the panel usage instructions and the gear button. In doing so, we manually set the "cc-scrolling-scrolled-window" and "cc-max-row-visible" properties in the GtkListBox so the cc_list_box_adjust_scrolling () helper function can properly calculate the total height of the visible list box rows. MAX_ROWS_VISIBLE is arbitrarily set to 7 instead of 5 because the other children of the scrolled window are not computed in the cc_list_box_adjust_scrolling () function. The two lines GtkLabel height summed to its vertical margins should equal something around the size of two rows (2 + 5 = 7). This commit should be reverted when the new Control Center resizable shell gets introduced.
Attachment 350720 [details] pushed as a0dccaf - search: Place the whole content in a scrolled window Attachment 350721 [details] pushed as f14829c - search: Use list box separators between entries
Created attachment 350726 [details] screencast
(In reply to Felipe Borges from comment #15) > Created attachment 350724 [details] [review] [review] > search: Drop move buttons > > Since we now support Drag & Drop to sort the list. > > These changes are according to the design guidelines available > at https://wiki.gnome.org/Design/SystemSettings/Search I'm pretty sure we don't want to do that, this makes the list reordering unusable with the keyboard, unless there's another, standard, way to start a reorder?
Comment on attachment 350724 [details] [review] search: Drop move buttons (In reply to Bastien Nocera from comment #19) > (In reply to Felipe Borges from comment #15) > > Created attachment 350724 [details] [review] [review] [review] > > search: Drop move buttons > > > > Since we now support Drag & Drop to sort the list. > > > > These changes are according to the design guidelines available > > at https://wiki.gnome.org/Design/SystemSettings/Search > > I'm pretty sure we don't want to do that, this makes the list reordering > unusable with the keyboard, unless there's another, standard, way to start a > reorder? Alright. We don't need to apply this patch. The following ones apply clearly without it too.
The only step back here would be that you just see the arrow buttons by reaching the end of the scrollbar. For that I suggest: 1. Move the toolbar above the list box? 2. Embed the listbox itself on a scrolled window? (like it is now on master) - but makes it harder to drag and drop. 3. Overlay the arrow buttons when a list box row gets selected/activated? I am including Allan in the convo so he can contribute as well.
Created attachment 352099 [details] screenshot @mclasen pointed out at the behavior or ordering a listbox in GNOME Recipes. It has the row handlers just like the implementation of these patches above AND it also supports using Alt+Up/Down to reorder the rows. @hadess thinks the shortcut approach is not very discoverable. I suggested that we could advertise the shortcut in the label that we already have on top of the list. (See screenshot). p.s.: ignore the duplicate entries in the screenshot, it's because of my jhbuild setup.
(In reply to Bastien Nocera from comment #19) > (In reply to Felipe Borges from comment #15) > > Created attachment 350724 [details] [review] [review] [review] > > search: Drop move buttons > > > > Since we now support Drag & Drop to sort the list. > > > > These changes are according to the design guidelines available > > at https://wiki.gnome.org/Design/SystemSettings/Search > > I'm pretty sure we don't want to do that, this makes the list reordering > unusable with the keyboard, unless there's another, standard, way to start a > reorder? We didn't support reordering with the keyboard before either, did we?
(In reply to Felipe Borges from comment #23) > (In reply to Bastien Nocera from comment #19) > > (In reply to Felipe Borges from comment #15) > > > Created attachment 350724 [details] [review] [review] [review] [review] > > > search: Drop move buttons > > > > > > Since we now support Drag & Drop to sort the list. > > > > > > These changes are according to the design guidelines available > > > at https://wiki.gnome.org/Design/SystemSettings/Search > > > > I'm pretty sure we don't want to do that, this makes the list reordering > > unusable with the keyboard, unless there's another, standard, way to start a > > reorder? > > We didn't support reordering with the keyboard before either, did we? You can navigate to the up/down buttons just fine with the keyboard.
HI, I've been asked to report Bug 784301 (enhancement) in this thread. Does it apply here? Should I copy the comment and attachment here and close the previous bug? Sorry for the dumb questions, I'm new to this.
(In reply to Jorge Toledo from comment #25) > HI, I've been asked to report Bug 784301 (enhancement) in this thread. > > Does it apply here? Should I copy the comment and attachment here and close > the previous bug? Sorry for the dumb questions, I'm new to this. Hi, thanks for testing and reporting the bug. Your report is valid and very relevant. I will attach bellow a fix and push the changes to the wip/feborges/new-search-panel branch for testing purposes. Be aware that WIP branches are experimental and in a very early stage of development. Your feedback is very welcomed.
Created attachment 354684 [details] [review] search: Make the list_box the DnD target By making the list_box itself the Drag and Drop target, we are able to use CSS to create a gap between the rows while on hover.
*** Bug 784301 has been marked as a duplicate of this bug. ***
Created attachment 357268 [details] [review] search: Introduce reordering keyboard shortcut Alt + UP/DOWN
Created attachment 357273 [details] [review] search: Add notification advertising the reordering shortcuts When a list row is selected, reveal a notification instructing how to reorder the list with the keyboard shortcuts. "Use Alt+↑ and Alt+↓ to move rows"
Created attachment 357274 [details] screenshot-notification (In reply to Felipe Borges from comment #30) > Created attachment 357273 [details] [review] [review] > search: Add notification advertising the reordering shortcuts The attached screenshot shows how the notification looks like.
Created attachment 357275 [details] Screenshot: proposal with keycaps We could use keycaps in the shortcuts, if desirable (see example in the screenshot attached).
Created attachment 359599 [details] [review] search: Introduce Drag and Drop These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 359600 [details] [review] search: Move the gear button to the top of the panel Also introduce panel usage instructions. These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 359601 [details] [review] search: Drop move buttons Since we now support Drag & Drop to sort the list. These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 359602 [details] [review] search: Adjust panel height according to num of rows We cannot benefit here from the cc_list_box_setup_scrolling () list-box helper function because we are also embedding other widgets in the scrolled window, such as the box which contains the panel usage instructions and the gear button. In doing so, we manually set the "cc-scrolling-scrolled-window" and "cc-max-row-visible" properties in the GtkListBox so the cc_list_box_adjust_scrolling () helper function can properly calculate the total height of the visible list box rows. MAX_ROWS_VISIBLE is arbitrarily set to 7 instead of 5 because the other children of the scrolled window are not computed in the cc_list_box_adjust_scrolling () function. The two lines GtkLabel height summed to its vertical margins should equal something around the size of two rows (2 + 5 = 7). This commit should be reverted when the new Control Center resizable shell gets introduced.
Created attachment 359603 [details] [review] search: Make the list_box the DnD target By making the list_box itself the Drag and Drop target, we are able to use CSS to create a gap between the rows while on hover.
Created attachment 359604 [details] [review] search: Introduce reordering keyboard shortcut Alt + UP/DOWN
Created attachment 359605 [details] [review] search: Add notification advertising the reordering shortcuts When a list row is selected, reveal a notification instructing how to reorder the list with the keyboard shortcuts. "Use Alt+↑ and Alt+↓ to move rows"
Review of attachment 359605 [details] [review]: ::: panels/search/search.ui @@ +2,3 @@ <interface> <!-- interface-requires gtk+ 3.0 --> +<object class="GtkOverlay" id="search_vbox"> not a fan of how this look, overlayed on top of the panel's main (top) label. especially since it doesn't go away after it shows up. perhaps it could go at the bottom, after the list? and perhaps be permanently visible? @@ +13,3 @@ + <object class="GtkLabel"> + <property name="visible">True</property> + <property name="label" translatable="yes">Use Alt+↑ and Alt+↓ to move rows</property> maybe this could use GtkShortcutsShortcut to show the shortcuts?
Review of attachment 359604 [details] [review]: ::: panels/search/cc-search-panel.c @@ +555,3 @@ + + source_parent = gtk_widget_get_parent (source); + target_parent = gtk_widget_get_parent (target); their parent should really be the same instance so we only need one of them, right? could just pass the list box as an argument into the function @@ +585,3 @@ + target = GTK_WIDGET (gtk_list_box_get_row_at_index (GTK_LIST_BOX (self->priv->list_box), index)); + if (target) { + gtk_list_box_set_sort_func (GTK_LIST_BOX (self->priv->list_box), NULL, NULL, NULL); same question as in the dnd patch
Review of attachment 359603 [details] [review]: ::: panels/search/cc-search-panel.c @@ +380,3 @@ + tmp = gtk_list_box_get_row_at_index (list, i); + if (tmp == NULL) + return row; we will probably never have enough rows for this to be an issue but still I think it would be better to just keep track of how many rows we have instead of doing it like this @@ +577,3 @@ gtk_container_add (GTK_CONTAINER (box), handle); + gtk_style_context_add_class (gtk_widget_get_style_context (row), "row"); is this needed? GtkListBoxRow class already sets its css name as "row" @@ +957,3 @@ + G_CALLBACK (drag_leave), NULL); + + provider = gtk_css_provider_new (); needs to be unrefed or use autoptr @@ +960,3 @@ + gtk_css_provider_load_from_data (provider, css, -1, NULL); + gtk_style_context_add_provider_for_screen (gdk_screen_get_default (), + GTK_STYLE_PROVIDER (provider), 800); GTK_STYLE_PROVIDER_PRIORITY_APPLICATION ? but shouldn't this css be in gtk+? it seems quite general to me
Review of attachment 359599 [details] [review]: ::: panels/search/cc-search-panel.c @@ +447,3 @@ +} + +static GtkTargetEntry entries[] = { too generic, maybe drag_target_entries @@ +465,3 @@ + * We don't drop the sorting entirely because you want the list to be + * sorted when there is no previous sorting stored. */ + gtk_list_box_set_sort_func (list_box, NULL, NULL, NULL); shouldn't this be undone in a drag-end handler? if the gsetting changes after we drag a row the listbox won't sync with it. in fact, what happens if the gsetting changes while we have the drag going? @@ +469,3 @@ + row = gtk_widget_get_ancestor (widget, GTK_TYPE_LIST_BOX_ROW); + gtk_widget_get_allocation (row, &alloc); + surface = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, alloc.width, alloc.height); this should probably be gdk_window_create_similar_surface() since we don't really need an image surface (and there's gdk API to get an image one too) @@ +531,3 @@ + gtk_container_foreach (GTK_CONTAINER (self->priv->list_box), update_row_position, self); + + search_panel_propagate_sort_order (self); shouldn't gtk_drag_finish() be called? @@ +564,3 @@ + /* Drag and Drop */ + handle = gtk_event_box_new (); + gtk_container_add (GTK_CONTAINER (handle), gtk_image_new_from_icon_name ("open-menu-symbolic", 1)); instead of 1 we should use a GtkIconSize enum value
Review of attachment 359600 [details] [review]: ok
Review of attachment 359601 [details] [review]: sure
Review of attachment 359602 [details] [review]: we're using the resizable shell now so do we still need this?
(In reply to Rui Matos from comment #40) > Review of attachment 359605 [details] [review] [review]: > > ::: panels/search/search.ui > @@ +2,3 @@ > <interface> > <!-- interface-requires gtk+ 3.0 --> > +<object class="GtkOverlay" id="search_vbox"> > > not a fan of how this look, overlayed on top of the panel's main (top) > label. especially since it doesn't go away after it shows up. > > perhaps it could go at the bottom, after the list? and perhaps be > permanently visible? Not sure whether this helps with the discoverability problem. What if it would go away after the first usage of the shortcuts? > > @@ +13,3 @@ > + <object class="GtkLabel"> > + <property name="visible">True</property> > + <property name="label" translatable="yes">Use Alt+↑ and Alt+↓ to > move rows</property> > > maybe this could use GtkShortcutsShortcut to show the shortcuts? The initial implementation was using GtkShortcutsShortcut but Allan suggested to not use them.
Comment on attachment 359602 [details] [review] search: Adjust panel height according to num of rows (In reply to Rui Matos from comment #46) > Review of attachment 359602 [details] [review] [review]: > > we're using the resizable shell now so do we still need this? No, we don't.
Created attachment 360747 [details] [review] search: Introduce Drag and Drop These changes are according to the design guidelines available at https://wiki.gnome.org/Design/SystemSettings/Search
Created attachment 360748 [details] [review] search: Make the list_box the DnD target By making the list_box itself the Drag and Drop target, we are able to use CSS to create a gap between the rows while on hover.
Created attachment 360749 [details] [review] search: Introduce reordering keyboard shortcut Alt + UP/DOWN
(In reply to Rui Matos from comment #42) > Review of attachment 359603 [details] [review] [review]: > > ::: panels/search/cc-search-panel.c > @@ +577,3 @@ > gtk_container_add (GTK_CONTAINER (box), handle); > > + gtk_style_context_add_class (gtk_widget_get_style_context (row), "row"); > > is this needed? GtkListBoxRow class already sets its css name as "row" The custom css provided in the code doesn't seem to get applied without this. > > @@ +960,3 @@ > + gtk_css_provider_load_from_data (provider, css, -1, NULL); > + gtk_style_context_add_provider_for_screen (gdk_screen_get_default (), > + GTK_STYLE_PROVIDER (provider), > 800); > > GTK_STYLE_PROVIDER_PRIORITY_APPLICATION ? > > but shouldn't this css be in gtk+? it seems quite general to me Eventually GtkListBox will wrap the whole drag and drop machinery in Gtk+, but so far we need to provide our custom css since it is not there yet.
Created attachment 360825 [details] quick mockup (In reply to Felipe Borges from comment #47) > (In reply to Rui Matos from comment #40) ... > > not a fan of how this look, overlayed on top of the panel's main (top) > > label. especially since it doesn't go away after it shows up. Yes, it is quite visually dominant. > > perhaps it could go at the bottom, after the list? Placing the hint at the bottom and making it smaller will help to make it feel less intrusive - see the attached image for how it could look. > > and perhaps be > > permanently visible? I'm not sure why you'd want to make it permanently visible. Moving the rows using the keyboard isn't going to be very common and you have to figure out how to give keyboard focus the list in the first place, which a lot of people won't know how to do. > Not sure whether this helps with the discoverability problem. > > What if it would go away after the first usage of the shortcuts? The problem with hiding after the first usage of the shortcuts is that people might not remember them...
(In reply to Felipe Borges from comment #52) <snip> > Eventually GtkListBox will wrap the whole drag and drop machinery in Gtk+, > but so far we need to provide our custom css since it is not there yet. When? This cycle? Shouldn't we land this in GTK+ ASAP rather than implement it here and wonder why we need to reimplement it in the Notifications, Privacy, or other similar panels?
Marking these patches as reviewed until further action is taken. I agree this should be living in GTK+ side, for G-C-C is not the only module that would benefit from DnD in GtkListBox.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new bug report at https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/ Thank you for your understanding and your help.