GNOME Bugzilla – Bug 767431
Use Popover for Move/Label menu in toolbar
Last modified: 2016-06-20 05:41:16 UTC
Created attachment 329431 [details] [review] Early WIP to replace FolderMenu with FolderPopover I made a very quick implementation using a popover. In the end, I would like to make it similar to the popover used in gnome-characters (see https://blogs.gnome.org/aday/files/2014/02/filter-menu.png). That would also make it a bit the same as in GMail. Some things that still need to be done: * Style the labels. I've added the css classes, but I have no idea in what file to put the css itself. * For some unknown reason, the popover is automatically opened. * Add a filter on top. Any thoughts?
Created attachment 329441 [details] [review] [WIP - almost done] Patch: FolderMenu -> FolderPopover Solved most of the todo's (including the search bar). The only thing that still needs to happen is to style it a bit more. Any idea where to put the css?
I'm pretty happy with this after taking it for a quick run. Some comments before I do a proper review: - Patch comment needs a bug number and there is some trailing whitespace introduced on a few lines that should be removed. - The source file should be `git mv`'ed to match the new class name - The Gtk.ScrolledWindow should have an `IN` border - The search box should request focus when the popover is set visible (it isn't focused when the menu button is activated the first time after app startup) - Hitting Enter in the search box should do something - if there's a single match then activate that, else focus the list box - We should make life easy for people for people with a small number of folders and prefer using the mouse - maybe move the search box down the bottom of the popover, or it should only appear if scrolling is necessary - Relatedly it would be good if the popover was shrinkwrapped when scrolling isn't necessary. Currently it can fit ~11 folders, so if a person has less than that it will appear oversized. I don't know if this is possible with GTK 3.14 however.
Also, as I mentioned in the other bug, it I'd like to see if we can address nested folders properly if we're going to switch these menus over to popovers. Some approaches I can think of are: 1. Keep the list flat, but indicate hierarchy using indentation: || INBOX || || > Foo || || > Bar || || > Quux || || > Baz || Here, the '>''s are pan-end-symbolic icons or similar. It might also be possible to keep the list flat and use Gtk.ListBox headers to indicate indentation, but can't see how to make that work without it sucking somewhat. 2. A stack of list boxes that works similarly to popover-based menus (E.g. the GEdit toolbar menu), where folders with children have a navigation arrow and activating them transitions the stack: || INBOX || || File here || || Foo > || || Baz || Activating Foo above yields this: || < Foo || || File here || || Bar || || Quux || And activating Foo there transitions back to INBOX. The "File here" menu items sucks, maybe it could be replaced with a folder icon end-aligned on the first item? 3. Use a Gtk.PathBar at the top, like the file open/save dialogs. I'm not really happy with any of the three above. I like the idea of #2 the most but think #3 is most likely to work best. What do you think? Some other random comments: When doing a search the hierarchy would need to get filled in anyway for context. E.g. searching for "ba" might yield: || INBOX > Foo > Bar || || INBOX > Baz || Also, in all cases for mail servers that only support creating folders under INBOX, when there is only one top-level folder it should probably be special-cased somehow.
Created attachment 329649 [details] [review] [PATCH] replace FolderMenu with FolderPopover Patch with further progress. Most of your points should be solved (I have no idea how to set a max size on the listbox. However, I don't really agree with either hiding the scrollbar or displaying a hierarchy in the popover. This might seem strange --especially the latter--, but that's because we can now make labeling/moving an efficient operation using the keyboard (for example: <l> + <somelabelprefix> + <Enter>). I'm already using this quite happily, since the other option is to either switch to my crappy touchpad, or fiddle around with the arrow-keys trying to find the right label --which would even worsen if it would be a multi-level menu. I agree that showing the folder hierarchy is an important visual feature in bug 746055, but has only very limited use here.
Review of attachment 329649 [details] [review]: Looks good - only issues are needing the search box moved to the bottom and seeing if it is worth using Gtk.ListBox's built in sort and filtering rather than doing both manually. ::: src/client/components/folder-popover.vala @@ +5,3 @@ + */ + +public class FolderPopover : Gtk.Popover { Might want to use a GtkTemplate & UI file for this? @@ +6,3 @@ + +public class FolderPopover : Gtk.Popover { + private Gee.List<Geary.Folder> folder_list = new Gee.ArrayList<Geary.Folder>(); Did you consider setting set sort and filter functions on the Gtk.ListBox rather than maintaining a separate list of folders and sorting & filtering manually? Might be a much simpler implementation. @@ +28,3 @@ + return true; + }); + stdout.printf("Herpaderp\n"); Needs less derp. :) @@ +30,3 @@ + stdout.printf("Herpaderp\n"); + searchEntry.activate.connect(search_activated); + container.add(searchEntry); The search entry should be below the scrolled window - this won't hurt people who are using the keyboard but will help people scanning the list and using the mouse. @@ +34,3 @@ + Gtk.ScrolledWindow scrolled = new Gtk.ScrolledWindow(null, null); + scrolled.shadow_type = Gtk.ShadowType.IN; + scrolled.set_size_request(250, 400); Probably want to use minimum content width/height here instead of a specific size request, then set a horizontal scroll policy of never so that the max width of folders dictates the width of the popovers. I'd suggest minimums of 200/350 might be better defaults. In the future we can use max width to get some better shrink wrapping happening: https://blog.gtk.org/2016/06/08/controlling-content-sizes-in-gtkscrolledwindow/ @@ +88,3 @@ + private Gtk.ListBoxRow build_row(Geary.Folder folder) { + Gtk.ListBoxRow row = new Gtk.ListBoxRow(); + row.get_style_context().add_class("folder-menu-list-row"); This isn't needed if you're not adding CSS to main-window.vala, but if you do need need it, prefix it with "geary-" so it can't clash with internal theme class names. @@ +90,3 @@ + row.get_style_context().add_class("folder-menu-list-row"); + row.set_border_width(1); + row.set_size_request(100, 35); We definitely don't want to set the row's size, and probably don't want the border either. I'd be inclined to follow Character's lead and just 6px of padding on the ListBoxRow via CSS - see the comment above. @@ +98,3 @@ + } + + private void row_selected(Gtk.ListBoxRow? row) { The naming convention for methods that are signal handlers is to use a "on_" prefix, and you might want to group with the one below. @@ +99,3 @@ + + private void row_selected(Gtk.ListBoxRow? row) { + if(row != null) { Needs a space after the if. @@ +117,3 @@ + } + + private void search_activated() { Ditto about the handler method name. @@ +118,3 @@ + + private void search_activated() { + if(filteredFolderCount == 1) { Needs a space after the if. @@ +122,3 @@ + if(row != null) + row_selected(row); + } else if(filteredFolderCount > 0) { Needs a space after the if.
(In reply to Niels De Graef from comment #4) > > However, I don't really agree with either hiding the scrollbar or displaying > a hierarchy in the popover. Oh, I'm not against the scrollbar at all, and definitely want to the current efficiency of the search for keyboard users as-is. However I am concerned about the visual noise. At the moment, with a deep tree like the examples I used above, the folder list looks like this: || INBOX || || INBOX>Foo || || INBOX>Foo>Bar || || INBOX>Foo>Quux || || INBOX>Baz || The redundancy makes it more difficult to visually scan, but also impossible to search for folders with some sub-string of "INBOX" or "Foo" in its name. I don't want to switch to a Gtk.TreeView or a menu-based implementation, but finding some solution for this would be good. I think I'd like to play around with the options above, so maybe we should actually get the initial popover implementation committed first then worry about this later.
Created attachment 329937 [details] [review] [PATCH] Use Popover for Move/Label menu in toolbar Changes with previous patch * Use a composite template (i.e. also use folder-popover.glade) * Use CSS to style the rows in the folder list * Don't use a separate list for the folders (that was how it was done before this bug, but ListBox allows sorting and filtering, which is just what we need) * Use min_content_width/height instead of size_request * Use selection to better explore the list visually (and use activation to effectively choose the folder). * Code conventions and less derp. Most of what you asked should be implemented now. There are still 2 things though: >> However, I don't really agree with either hiding the scrollbar or displaying >> a hierarchy in the popover. >Oh, I'm not against the scrollbar at all, and definitely want to the current >efficiency of the search for keyboard users as-is. That was a brainfart on my part, I meant the search bar. I don't think it should be put at the bottom: it would jump several times while typing, making it hard to follow your own search terms and I don't see the merit for mouse users. And of course I certainly don't agree it should be hidden when there's a small list.
The other thing is about the borders, I do think they're a nice extra.
Created attachment 329979 [details] [review] [PATCH] 0001-Use-GtkPopover-for-Move-Label-menu.patch Urgh, previously attached was incorrect. This should be better.
Review of attachment 329979 [details] [review]: Just a few remaining things to fix up, then this should be good to commit. ::: src/client/components/folder-popover.vala @@ +5,3 @@ + */ + +[GtkTemplate (ui = "/org/gnome/Geary/folder-popover.glade")] Since the docs for Gtk.Builder suggest using ".ui" for its XML files, it's probably worth renaming this XML to match. @@ +9,3 @@ + + [GtkChild] + private Gtk.SearchEntry searchEntry; Property names use_underscores rather than camelCaps, ditto for the three that follow. So the XML file and callback names will need updating as well. @@ +20,3 @@ + + public FolderPopover() { + searchEntry.key_release_event.connect((event) => { Connecting to "search-changed" on the gtk.SearchBox would be a better idea here, and you can hook it up via the Builder XML file as well. @@ +93,3 @@ + } + + searchEntry.set_text(""); Need to invalidate the ListBox filter here as well so the complete list is returned for next use, although hooking up to search-changed per above may make this unnecessary. @@ +100,3 @@ + private void on_searchEntry_activate() { + if (filteredFolderCount == 1) { + Gtk.ListBoxRow? row = listBox.get_row_at_y(0); I guess using ::get_row_at_index(0) here instead doesn't work since the filter just hides rows? @@ +104,3 @@ + on_row_activated(row); + } else if (filteredFolderCount > 0) { + scrolled.grab_focus(); I'm not sure if this is working for me - might need to focus the ListBox instead? ::: src/client/components/main-window.vala @@ +189,3 @@ + row.geary-folder-popover-list-row > label { + color: @theme_text_color; + } You'll need to rebase of master and move this to geary.css since Bug 767814 has landed. ::: ui/CMakeLists.txt @@ +11,3 @@ STRIPBLANKS "edit_alternate_emails.glade" STRIPBLANKS "find_bar.glade" + STRIPBLANKS "folder-popover.glade" Might not need this since there's no translatable strings in the XML file? ::: ui/folder-popover.glade @@ +2,3 @@ +<!-- Generated with glade 3.20.0 --> +<interface> + <requires lib="gtk+" version="3.16"/> Need to drop this down to 3.14 and check there's no warnings as a result. @@ +12,3 @@ + <property name="margin_end">5</property> + <property name="margin_top">5</property> + <property name="margin_bottom">5</property> The HIG says use increments of 6 for visual spacing, so lets do that here. https://developer.gnome.org/hig/stable/visual-layout.html.en @@ +14,3 @@ + <property name="margin_bottom">5</property> + <property name="orientation">vertical</property> + <property name="spacing">2</property> Same here.
(In reply to Niels De Graef from comment #7) > That was a brainfart on my part, I meant the search bar. I don't think it > should be put at the bottom: it would jump several times while typing, > making it hard to follow your own search terms and I don't see the merit for > mouse users. And of course I certainly don't agree it should be hidden when > there's a small list. Yeah, for consistency with the one other popover search Geary now has let's leave the search bar at the top for now. I see you point about leaving it in place even for a small list - for keyboard access, right? Out of curiosity, are you using Geary with a large number of folders that are at the same level of your inbox, or are they children of it? Speaking of brainfarts - ignore the comment about ui/CMakeLists.txt in the review above, I was thinking it was POTFILES.in for some reason.
Created attachment 330015 [details] [review] [PATCH] replace FolderMenu with FolderPopover Should fix all remarks. > I guess using ::get_row_at_index(0) here instead doesn't work since the filter > just hides rows? Yep. Put it in the comments to make sure others who read the code get it as well. > Yeah, for consistency with the one other popover search Geary now has let's > leave the search bar at the top for now. I see you point about leaving it in > place even for a small list - for keyboard access, right? Yep, that was what I intended to say. > Out of curiosity, are you using Geary with a large number of folders that are > at the same level of your inbox, or are they children of it? A large number of folders: ✔. Most of them are at the same level as INBOX, but they do have a hierarchy of up to 3 levels deep.
Review of attachment 330015 [details] [review]: Two last nits to pick — fix them up and I'll get it committed. ::: src/client/components/folder-popover.vala @@ +13,3 @@ + private Gtk.ListBox list_box; + + private int filteredFolderCount = 0; This was missed when converting names to use underscores. I guess it's not a property since it's private? But in any case, needs moar underscores. @@ +106,3 @@ + + [GtkCallback] + private void invalidate_filter() { This is the handler for search-changed, right? Per convention should to be named to reflect that, so on_search_blah_balh.
Created attachment 330041 [details] [review] [PATCH] hopefully final patch Naming of filtered_folder_count should be fixed. > + > + [GtkCallback] > + private void invalidate_filter() { > > This is the handler for search-changed, right? Per convention should to be > named to reflect that, so on_search_blah_balh. Well, the thing is: I'm also calling this method after the popover's hide-signal, or the list would keep the filter after clicking it away (or after selecting a folder). So it's not only a callback of search_changed, which is why I wanted the name to better reflect the purpose of the method.
Oh right. In that case I would keep the method named that, but then call add two separate handlers that simply call it, or else connect to the signals in the constructor using anonymous handlers (i.e. lambdas) and call the method from them. It's a little bit of extra code, but makes it immediately obvious to bystanders how it works.
Created attachment 330043 [details] [review] Fix callback naming Can't argue with readability :) Fixed.
True that. Committed to master as 423b619. Thanks!
Whoops, that's actually 7e446b1 on master.
Oh PPS, we both forgot to check po/POFILES.in - the l10n.gnome.org was complaining about folder-menu.vala being missing after pushing the patch up. I've removed the reference to the old source from POFILES.in, but we should get some pre-commit hooks or a `test` make target to catch this sort of thing.