GNOME Bugzilla – Bug 764632
Merge view controls into the hamburger menu
Last modified: 2016-10-23 13:03:47 UTC
There are some issues with the current view controls: • You can't switch between list and grid mode with a single click. • In tests [1], users have struggled to find how to switch between list and grid modes. • In tests [1], the zoom slider hasn't performed well - there are large gaps between the slider "ticks", which results in a lack of feedback when dragging the slider handle. One way to deal with these issues would be to merge the view controls into the hamburger popover menu, and then turn the view button into a toggle. A mockup for this is attached. [1] https://ginadobrescu.wordpress.com/outreachy-program/week-10-gnome-usability-test-results-part-1/
Created attachment 325465 [details] mockup Actually attach the mockup.
Created attachment 326625 [details] [review] toolbar: Convert action menu from GMenu into GtkPopoverMenu This is in preparation for merging the view menu into the action menu. The action menu is currently a GMenu, which doesn't support the controls we have on the view menu, e.g. the zoom slider and the button rows. Converting to a GtkPopoverMenu will allow us to merge the menus. This is part of the work to convert the list/icon view into a toggle button on the toolbar, and move the other view menu items into the action menu.
Will split the work for this up into multiple smaller patches. That's assuming it's ok for this to be worked on, and that no one else is working on it.
Hey Neil, thanks to work on this! The background work you are doing for achieve this looks good to me. It's true that having everything with GMenu is nice, but it's time to do something more flexible. Be aware that the design of this is highly likely to change since it depends on other things we have in mind for 3.22, but that should be details to change on the UI. Also expect few iterations with designers.
> The background work you are doing for achieve this looks good to me. > It's true that having everything with GMenu is nice, but it's time to do > something more flexible. > Let me clarify this, I think I didn't explain myself well and can be misinterpreted. I meant that your approach of using GtkModelButton is more flexible than the current implementation, so this is a change that will be useful even if the design in the mockup changes or we decide to not change the current design at all. So please go ahead with the changes you planned :)
(In reply to Allan Day from comment #1) > Created attachment 325465 [details] > mockup > > Actually attach the mockup. This is pretty big. With large fonts, I can't see it fitting into most portrait screen resolutions.
Was planning on getting something working quickly on a basic level, so we can see how it looks/works. Then if it needs to change or we decide not to make the changes, not much effort has been wasted. If it gets binned or redesigned, I wouldn't be upset. But if it's not worth considering until some of the 3.22 changes have settled, then I'll hold off. Don't mind either way.
(In reply to Bastien Nocera from comment #6) ... > > Actually attach the mockup. > > This is pretty big. With large fonts, I can't see it fitting into most > portrait screen resolutions. It's not that much bigger than the existing view popover (roughly speaking 12 items vs 10), and I think it's just within the limits of what we should have in such a popover (as a rule of thumb, 12 items is about the maximum number for a menu). But I agree - we should test this aspect of the design.
Created attachment 327178 [details] [review] Current changes Here's a patch of changes so far - can apply with: patch -p1 < squashed-changes.patch Visually the changes are complete, but there are a couple of minor issues I need to fix (e.g. view toggle button doesn't grey out for other locations, in some cases). Will leave fixing those until later. A couple of things we need to think about: - How should the hamburger menu behave for Other Locations? At the moment I'm just disabling it. Previously the view menu was disabled, and the hamburger menu was enabled (with many items in it disabled) - What should the button that shows the zoom % do? Currently it resets to the default zoom level when it's clicked (doesn't work atm as there's a bug in master) Haven't made the changes to the sort yet, so there's still a 'Reverse Order' option Have just squashed this into a single patch, but I've got the changes as separate, smaller commits locally. Will submit proper patches when needed.
Review of attachment 327178 [details] [review]: Thanks for working on it! A first review: ::: src/nautilus-files-view.c @@ +269,3 @@ GtkWidget *stop; GtkWidget *reload; + GtkBox *zoom_controls_box; By convention we always use the parent, in all these cases, GtkWidget. @@ +6800,3 @@ gboolean show_sort_trash, show_sort_search, show_sort_access, show_sort_modification, sort_available; const gchar *hint; + gchar *zoom_level_percent; use g_autofree. @@ +6819,3 @@ !nautilus_files_view_is_empty (view)); + extra line @@ +7911,3 @@ + gchar *redo_label) +{ + set_string_property (G_OBJECT (view->details->undo_button), "text", undo_label); I think you can use gtk_button_set_label here no? @@ +7916,3 @@ + +static void +undo_manager_changed (NautilusFilesView *view) I'm unsure about this. Undo and redo are window actions, so here you are modifying the labels and action status for every view, for the same window. What is the reason to move it to the view? I would move it to the toolbar in any case. @@ +7950,3 @@ + redo_label = redo_active ? redo_label : g_strdup (_("Redo")); + undo_label = undo_label == NULL ? g_strdup (_("Undo")) : undo_label; + redo_label = redo_label == NULL ? g_strdup (_("Redo")) : redo_label; Nmemonics missing ::: src/nautilus-files-view.h @@ +193,3 @@ + /* The current zoom level as a percentage of the default (0, 1] */ + gfloat (*get_zoom_level_percentage) (NautilusFilesView *view); (* get_zoom...) ::: src/nautilus-icon-info.h @@ -86,3 @@ -guint nautilus_get_list_icon_size_for_zoom_level (NautilusListZoomLevel zoom_level); -guint nautilus_get_canvas_icon_size_for_zoom_level (NautilusCanvasZoomLevel zoom_level); - I like this change, was kinda strange to use types of views in this class. ::: src/nautilus-window-slot.c @@ +745,3 @@ + self = NAUTILUS_WINDOW_SLOT (user_data); + priv = nautilus_window_slot_get_instance_private (self); + if (priv->content_view == NULL || !NAUTILUS_IS_FILES_VIEW (priv->content_view)) { this check shouldn't be necesary now (I know there are more, something to clean up after all the improvements with the desktop split...), as any special view has it's special slot, and either the action is not available for those, or this needs to be subclassed. In this case, action is disabled for both desktop slot and other locations slot. @@ +752,3 @@ + if (current_view_id == NAUTILUS_VIEW_GRID_ID) { + change_files_view_mode (self, NAUTILUS_VIEW_LIST_ID); + } bad formatting @@ +753,3 @@ + change_files_view_mode (self, NAUTILUS_VIEW_LIST_ID); + } + else if (current_view_id == NAUTILUS_VIEW_LIST_ID) { Is there a reason to check this? ::: src/resources/ui/nautilus-toolbar-view-menu.ui @@ +194,3 @@ + <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="text" translatable="yes">Redo</property> nmemonics missing in some labels ::: src/resources/ui/nautilus-toolbar.ui @@ +114,3 @@ <child internal-child="accessible"> <object class="AtkObject"> + <property name="accessible-name" translatable="yes">Grid/list toggle</property> I don't think you should us "/" in accessible names. Imagine Orca reading this: "grid slash list toggle" I would use "view mode toggle button"
(In reply to Neil Herald from comment #9) > Created attachment 327178 [details] [review] [review] > Current changes > > Here's a patch of changes so far - can apply with: patch -p1 < > squashed-changes.patch > > Visually the changes are complete, but there are a couple of minor issues I > need to fix (e.g. view toggle button doesn't grey out for other locations, > in some cases). Will leave fixing those until later. I cannot see where the files-mode-toggle action is disabled for the other locations slot, so I guess that's the problem. > > A couple of things we need to think about: > - How should the hamburger menu behave for Other Locations? At the moment > I'm just disabling it. Previously the view menu was disabled, and the > hamburger menu was enabled (with many items in it disabled) The first row of actions are still important, so I would show it. > - What should the button that shows the zoom % do? Currently it resets to > the default zoom level when it's clicked (doesn't work atm as there's a bug > in master) I would say reset to 100%, since "default" is actually set by the user when changing the zoom level. > > Haven't made the changes to the sort yet, so there's still a 'Reverse Order' > option > > Have just squashed this into a single patch, but I've got the changes as > separate, smaller commits locally. Will submit proper patches when needed. For review is actually better to have them separated, so attach in that way.
Allan, after trying the patch I have some feedback: - The "sort" heading looks confusing now, since there are quite a bit more items now and some of them are deactivated (therefore using the same styling as "sort"). Looks to me that we can get rid of it. - I have the visual feeling of too many separators. Maybe remove the one between the first two rows of buttons and apply a 6px bottom margin on the first row? Apart of that, this feels like an improvement!
(In reply to Carlos Soriano from comment #10) > @@ +7916,3 @@ > + > +static void > +undo_manager_changed (NautilusFilesView *view) > > I'm unsure about this. Undo and redo are window actions, so here you are > modifying the labels and action status for every view, for the same window. > What is the reason to move it to the view? > > I would move it to the toolbar in any case. I don't like it either. Sadly the files-view creates the view menu popover, so it owns the references to the undo/redo menu items (each files-view instance has it's own popover...). Could move the logic to update the labels back into the toolbar, and add getters to the files-view to return the undo/redo widgets, but that's horrible. Can you think of a better way? Really the toolbar should create the view menu popover, so there would be only one popover per window - it is a widget shown on the toolbar, afterall. I spent a silly amount of time debugging an issue, and one of the reasons it took so long was because of the complexity with how the current view menu works. But that's a much bigger change, and I wasn't sure if there was a reason why it's the way it is? Was going to suggest that to you later on though. I agree with your other comments, I'll make those changes. Most of your comments are about code I'd moved from elsewhere (e.g. missing mnemonics), or code where I'd copied the approach.
(In reply to Neil Herald from comment #13) > (In reply to Carlos Soriano from comment #10) > > @@ +7916,3 @@ > > + > > +static void > > +undo_manager_changed (NautilusFilesView *view) > > > > I'm unsure about this. Undo and redo are window actions, so here you are > > modifying the labels and action status for every view, for the same window. > > What is the reason to move it to the view? > > > > I would move it to the toolbar in any case. > > I don't like it either. Sadly the files-view creates the view menu popover, > so it owns the references to the undo/redo menu items (each files-view > instance has it's own popover...). Could move the logic to update the labels > back into the toolbar, and add getters to the files-view to return the > undo/redo widgets, but that's horrible. Can you think of a better way? > > Really the toolbar should create the view menu popover, so there would be > only one popover per window - it is a widget shown on the toolbar, afterall. > I spent a silly amount of time debugging an issue, and one of the reasons it > took so long was because of the complexity with how the current view menu > works. But that's a much bigger change, and I wasn't sure if there was a > reason why it's the way it is? Was going to suggest that to you later on > though. > > I agree with your other comments, I'll make those changes. Most of your > comments are about code I'd moved from elsewhere (e.g. missing mnemonics), > or code where I'd copied the approach. You are right. The problem moving those items to the toolbar is that the view has controls over most of what should be shown... i.e. other locations doesn't have reload/etc, canvas view have the sort menu, list view doesn't have the sort menu and have the "visible columns" part. Not only that, but sometimes we want to make the buttons insensitive (folder emtpy) and sometimes we want them to be hidden (when in recent location). So it's really a mix of responsibilities, so it's hard to come with a code design that works here. So what we can say is: - Shared actions are bookmark this location, new tab, redo and undo, which are also window actions rather than view actions. - Zoom, hidden files and reload are shared between files views So either we go with the solution you folow on your patch, moving everything to the view, or maybe we can create a more abstract way of handling this? maybe we can have different ui files? One for the toolbar, containing the Popover with the first row, zoom section, undo redo, and custom section. ----------- first row ---------- zoom section with buttons, hidden by default, and with a setter/binding for percent/action? ---------- undo redo --------- custom section with a setter/binding. I cannot think of a better solution for this interaction between the current view and the toolbar, since the hierarchy is: window -> window-slot -> view -> toolbar Georges, any though?
I'm doing this in sections. --- About Allan's mockup: In the new hamburger menu, I think we can add a "Reverse order" check instead of two radiobuttons for the same actions reversed. That'd save us 1 precious entry from the menu. Besides that, it's definitely an improvement. --- About the current way we handle View menus: By the time we moved the view menu to the view, the intention was to make the code more decoupled so we could have different views with different menus. Up to now, it served well for this purpose. The window-slot acts as a layer to expose the current view's menu to the toolbar. With that, we don't need to expose the toolbar to the view (and vice-versa). It'd be trivial to actually add the only popover in the toolbar (thus, 1 popover per window) and instead, expose the ~widget~ to be added inside the popover, against exposing 1 popover per view. That wouldn't change the bigger picture, though - the view must have access and control over some of the widgets displayed in the menu. I can further investigate if the view really needs to access (i.e. can't we use bindings instead? Since NautilusFilesView setups the toplevel's actiongroup, we could use some bindings between actions and the ::visible property of the widgets). --- About Carlos' idea: I very much enjoy the idea of separating Window-related stuff from View-related stuff. Having the menu mostly handled by the toolbar, however, will limit us wrt the control over the menu. For example, if we're moving to a new Search view (something I personally want to see happening), the menu certainly would be completely different. Also, I wonder if splitting the menu like that right now wouldn't be an overenginnering action. I'm cautiously positive about going with Carlos' proposal. Are we actually exposing the View menu for, say, extensions? If not, this is a good chance to do so.
(In reply to Carlos Soriano from comment #12) > Allan, after trying the patch I have some feedback: > - The "sort" heading looks confusing now, since there are quite a bit more > items now and some of them are deactivated (therefore using the same styling > as "sort"). Looks to me that we can get rid of it. It needs to say "sort" though, doesn't it? Otherwise how will people know what the radio buttons do? My main issue with it is the style: it is indented when it shouldn't be, and it should be bold (I suspect that these are generic issues and not related to this bug). > - I have the visual feeling of too many separators. Maybe remove the one > between the first two rows of buttons and apply a 6px bottom margin on the > first row? Sure, let's give it a go.
(In reply to Georges Basile Stavracas Neto from comment #15) ... > In the new hamburger menu, I think we can add a "Reverse order" check > instead of two radiobuttons for the same actions reversed. That'd save us 1 > precious entry from the menu. Besides that, it's definitely an improvement. "Reverse order" is terrible! It gives no clue about what the default order is. I mean, what does "sort by date, reverse order" mean?
I've tried the patch and really like it! Aside from the issues that have already been raised, checking/unchecking "show hidden files" shouldn't close the popover, and pressing the percentage zoom button should return to 100%.
Thanks for the feedback folks. (In reply to Georges Basile Stavracas Neto from comment #15) > I can further investigate if the view really needs to access (i.e. can't we > use bindings instead? Since NautilusFilesView setups the toplevel's > actiongroup, we could use some bindings between actions and the ::visible > property of the widgets). I don't mind doing that. Though I suspect the answer is that the view doesn't need access to the toolbar. Currently anyway, but given we're talking about flexibility (e.g. for the new search view / extensions), I think moving all of the control to the toolbar - as per my suggestion - will limit our flexibility. I'll try what you've suggested Carlos, with a view of keeping it flexible, and see what I can come up with.
About the patch, I'd also add to Carlos' review: - Be very careful to use Tabs instead of Spaces. I saw some Tabs in the code, please remove them. - You could've used gtk_button_set_label() instead of this custom set_string_property() function. - In undo_manager_changed (), the labels are not being initialized (which was an issue with the previous code as well). Also, it wouldn't hurt to add some comments explainig what's being done here.
(In reply to Georges Basile Stavracas Neto from comment #20) > - Be very careful to use Tabs instead of Spaces. I saw some Tabs in the > code, please remove them. Meh, I painstakingly added those back in to be consistent with surrounding code! Which is pretty much what the coding guidelines say https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en What would be lovely is if there was a page with the guidelines - even if it was just a Nautilus specific one, it would save you guys repeating yourself
(In reply to Neil Herald from comment #21) > (In reply to Georges Basile Stavracas Neto from comment #20) > > - Be very careful to use Tabs instead of Spaces. I saw some Tabs in the > > code, please remove them. > > Meh, I painstakingly added those back in to be consistent with surrounding > code! Which is pretty much what the coding guidelines say > https://developer.gnome.org/programming-guidelines/stable/c-coding-style. > html.en > > What would be lovely is if there was a page with the guidelines - even if it > was just a Nautilus specific one, it would save you guys repeating yourself Hey Neil, Sorry for the confusion about styling. Nautilus cannot have a code style guide in its current state, because it's a mix of styles. Georges proposal is something I came up to slowly start stopping using tabs, but it's not working out since we are not going to touch all the code at once. So I'm actually planning to run uncrustify soon, actually as soon as Geroges gives me a green light (he is working on some code that will have conflicts if not). So soon we will have a consistent style, and the guideline will be the configuration file itself of uncrustify. You can read it here if you are curious: https://git.gnome.org/browse/nautilus/commit/?h=wip/csoriano/uncrustify&id=01685ff47ad3f83b0c48ad36410cdc844d262694 So, don't worry much about the style right now. Also regarding the code, if you find out a good way to apply what me and Georges propose, it will be lovely. But if the code design cannot be great, I prefer to have everything in files-view as you did in your patch. So I would say, make a decision based on the feedback we have provide, and I will happy with whatever the outcome is.
Allan, "It needs to say "sort" though, doesn't it? Otherwise how will people know what the radio buttons do? My main issue with it is the style: it is indented when it shouldn't be, and it should be bold (I suspect that these are generic issues and not related to this bug)." Right, I was expecting maybe it's clear enough to see the A-Z etc to guess it's order. But I guess those expectations are not good UX design. It is indented? You mean the "sort" should be more to the left than the items? Also the style needs to be bold? That's not in the mockup right? (so double checking this is what you mean) "Reverse order" is terrible! It gives no clue about what the default order is. I mean, what does "sort by date, reverse order" mean?" I agree. However I find the "size" item cannot be reversed now based on the mockups, is it an oversight? fwiw it's actually the only thing I revert :) "show hidden files" shouldn't close the popover" This is a technical limitation :(, let's see if we can fix it now at the same time. Neil, I would say the popover should be permanent to the toolbar, and only offer the content, in this way I think the popover can remain opened. Thanks all for the UI and code work, is looking great.
(In reply to Carlos Soriano from comment #23) > Allan, > > "It needs to say "sort" though, doesn't it? Otherwise how will people know > what the radio buttons do? My main issue with it is the style: it is > indented when it shouldn't be, and it should be bold (I suspect that these > are generic issues and not related to this bug)." > > Right, I was expecting maybe it's clear enough to see the A-Z etc to guess > it's order. But I guess those expectations are not good UX design. It will often be OK without the heading. I'm a bit concerned that that won't always be the case though. > It is indented? You mean the "sort" should be more to the left than the > items? Yes, so it's lined up with the other menu items. There's not enough padding above and below, either. > Also the style needs to be bold? That's not in the mockup right? (so > double checking this is what you mean) Actually, now that I look at it again, I'm not sure bold is needed. :) > "Reverse order" is terrible! It gives no clue about what the default order > is. I mean, what does "sort by date, reverse order" mean?" > I agree. > However I find the "size" item cannot be reversed now based on the mockups, > is it an oversight? It's intentional. The idea was that for size, you can only sort biggest first. > fwiw it's actually the only thing I revert :) Why?
> > However I find the "size" item cannot be reversed now based on the mockups, > > is it an oversight? > > It's intentional. The idea was that for size, you can only sort biggest > first. > > > fwiw it's actually the only thing I revert :) > > Why? Currently I revert it because reverted means from big to small, which sounds like a bug now that I think of it. So I agree, only allow bigger to smaller is fine.
(In reply to Carlos Soriano from comment #22) > So I'm actually planning to run uncrustify soon, actually as soon as Geroges > gives me a green light (he is working on some code that will have conflicts No need to apologise. Uncrustify is a great idea, it'll make all our jobs much easier :)
Created attachment 328019 [details] [review] toolbar: convert action menu from GMenu into GtkPopoverMenu This is in preparation for merging the view menu into the action menu. The action menu is currently a GMenu, which doesn't support the controls we have on the view menu, e.g. the zoom slider and the button rows. Converting to a GtkPopoverMenu will allow us to merge the menus. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 328020 [details] [review] files-view: merge the action and view menus, and impl. view toggle Usability tests conducted by Gina Dobrescu have highlighted a number of issues with the toolbar menus. Users can't switch between list and grid mode with a single click, and they have struggled to find the switch between list and grid mode. Allan Day has come up with a design to address these problems. The view and action menus have been combined into a single menu, and we have added a new button to the toolbar which toggles the view mode between list and grid mode.
Created attachment 328021 [details] [review] toolbar-view-menu: move the undo/redo items to higher up in the menu Moved them to above the sort options, as part of the toolbar menu redesign.
Created attachment 328022 [details] [review] toolbar-view-menu: remove the Select All and Enter Loc. items
Created attachment 328023 [details] [review] toolbar-view-menu: convert new folder/tab/bookmark items to button list Converted these from three separate menu items into a horizontal button list, and moved them to the top of the menu, as part of the menu redesign.
Created attachment 328024 [details] [review] icon-info: remove some unused function prototypes
Created attachment 328025 [details] [review] toolbar-view-menu: replace zoom slider with buttons Convert the zoom slider into a horizontal button list, with a button in the middle to show the current zoom level percentage. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 328026 [details] [review] toolbar-view-menu: small tweaks to the appearance of the Sort heading The "Sort" heading in the toolbar menu doesn't look right; it has less vertical padding than the surrounding menu items, and is indented one pixel more. It also doesn't need to be bold - this commit addresses these issues.
Created attachment 328027 [details] [review] toolbar-view-menu: remove the Search Relevance sort option This sort option is never shown in the menu and therefore doesn't need to be in the menu xml. The view is sorted on search relevance when the user is searching, however all of the sort options are hidden in this case.
Created attachment 328028 [details] [review] canvas-view: improve usability of sort options The sort options shown on the toolbar menu aren't very intuitive. For example, 'last modified, reverse order' can be interpreted a number of ways. They also require the user to set both the sort order and whether the sort should be reversed. These issues have been addressed by replacing the sort order items that are often sorted in either ascending and descending order (e.g. name) with options that are better to use (e.g. A-Z and Z-A). Options that are generally only sorted one way (e.g. type) have been changed to sort in ascending/descending as appropriate for the option. This is part of the toolbar menu redesign to improve the usability of the menus.
I've made the changes you suggested in the review, and fixed the issues I pointed out. Have also reworked the sort options, which turned out to be fairly involved in the end. Thought I'd resubmit as it's a decent chunk of code, and it might be a while before I refactor the menus (if I go ahead with it). Some things to point out about the implementation (apologies if this is super-wordy): Sort: - I've favoured the approach that respects the current file metadata and global gsettings. So e.g. if the user had previously (in an older verion of Nautilus) set a folder to be sorted by name, descending, then the Z-A sort option will be selected in the new version. Obviously we've limited the available sort options (e.g. 'size, ascending' doesn't have a menu option). If the user had set a folder to sort via one of these combinations that we don't have a menu item for, the files will still be sorted as the user wants, but no sort menu item will be selected. There's one exception, which is 'Trashed time, ascending' isn't supported. If this order has been set for the trash folder, it will revert to sorting by name, ascending (can be easily be changed though) - Previously the sort order and whether the sort is reversed were stored as separate properties in the code (SortCriterion and sort_reversed). I tried to keep it this way, via a couple of different approaches, but it got horrible pretty quickly. The SortCriterion objects are tied to the menu items (each has an associated action) so I needed to duplicate these (e.g. one for name-ascending, and another for name-descending). Meaning that it made sense to add a property to SortCriterion to indicate whether asc/desc, and then remove the separate sort_reversed property from the view. Another reason why this is good is that changes to the sort order/reversed flag are always applied at the same time now - applying them individually is inefficient as would mean sorting twice. So having both things in SortCriterion means we can do that. - The semantics of nautilus_canvas_view_clean_up_by_name have changed: it doesn't revert the 'sort reversed' property to the previous value. This function is only called to sort the icons on the desktop by name, so is the reversed property ever anything but false in this case? I'm sure you can come up with a counter-argument, so thought I'd point it out. Other - We can't use gtk_button_set_label to set the undo/redo text. This sets the label property, which is a center aligned, greyed label. For GtkModelButton, we need to set the text property, but it doesn't have a function for that. I raised a bug against gtk+ to add gtk_model_button_set_text, but Mathias didn't want to change it. I'll find out why - if it's just a time/effort thing then I can submit a patch (if you think it's worth me following up on it, that is).
Ignore the comment about sort combinations we don't have a menu item for. If folder metadata has: - size, ascending -> reverts view to name, asc - type, descending -> reverts to name, asc - trash time, ascending -> reverts to name, asc - atime, asc/desc -> view is sorted by atime asc/desc. No sort item selected on menu It's a bit inconsistent at the moment. To handle the first 3 the same way as the last, that's just a case of adding the appropriate SortCriterion objects. Should we be worrying about handling these cases at all? Would you prefer different logic?
Review of attachment 328028 [details] [review]: ::: src/nautilus-canvas-view.c @@ +75,3 @@ const char *metadata_text; const char *action_target_name; + const gboolean is_reverse_order; reverse_order. is_reverse_order would be a function to know whether is reverse order or not. @@ +303,3 @@ } else if (sort != NULL && canvas_view->details->sort != sort) { overrided_sort_criterion = sort; + spurious line @@ +539,3 @@ + sort->metadata_text); + nautilus_file_set_boolean_metadata + (file, first parameter in the same line @@ +1797,3 @@ default_sort_order_changed_callback, canvas_view); + spurious line I think (maybe bugzilla is confusing me though) ::: src/nautilus-file.c @@ +372,3 @@ for (i = 0; attrs[i] != NULL; i++) { id = nautilus_metadata_get_id (attrs[i] + strlen ("metadata::")); + spurious line
Thanks Neil for the hard work on it! The solution and the code is of high quality, and helps that you splat the patches properly, making the review work really easy. Thanks for that. Overall all patches looks good to me, will take a line by line look once designers agree (which I'm sure they will, it feels like a great improvement). One thing I would change is the 100% size for the canvas view. Should be the 96px one. Otherwise, I don't think any change is needed.
One thing I wanted to note/ask. It doesn't matter if not all the sorting options are available in the view menu, as long as it's possible to switch to the list view, choose the sorting using the column headers, and switch back to the icon view.
(In reply to Bastien Nocera from comment #41) > One thing I wanted to note/ask. It doesn't matter if not all the sorting > options are available in the view menu, as long as it's possible to switch > to the list view, choose the sorting using the column headers, and switch > back to the icon view. I don't think that's a great experience. Also note that some options are not present by default on list view, like the type, so it would add another difficulty.
(In reply to Carlos Soriano from comment #42) > (In reply to Bastien Nocera from comment #41) > > One thing I wanted to note/ask. It doesn't matter if not all the sorting > > options are available in the view menu, as long as it's possible to switch > > to the list view, choose the sorting using the column headers, and switch > > back to the icon view. > > I don't think that's a great experience. It's better than relegating some of the sorting preferences to gsettings or gnome-tweak-tool. > Also note that some options are not > present by default on list view, like the type, so it would add another > difficulty. Which you're removing from the sort menu? If not, why is that relevant?
Nevermind, I think I misunderstood you. If you are talking about the reversed order for size and type, yeah, I agree it's not a big deal.
I've tried the latest patches and they it looks great. Two small issues I spotted: * Checking/unchecking "Show hidden files" closes the popover - it should remain open. * The popover should close after the user has pressed the new folder button.
(In reply to Carlos Soriano from comment #40) > Thanks Neil for the hard work on it! The solution and the code is of high > quality, and helps that you splat the patches properly, making the review > work really easy. Thanks for that. Cheers Carlos, much appreciated (In reply to Bastien Nocera from comment #41) > One thing I wanted to note/ask. It doesn't matter if not all the sorting > options are available in the view menu, as long as it's possible to switch > to the list view, choose the sorting using the column headers, and switch > back to the icon view. I think I read this the same as Carlos originally. I thought you were talking about switching to list to set a particular sort order, and then switching to canvas view and expecting that sort order to remain. But I don't think that's what you're getting at - you can always switch to list view to sort in a more specific way than the canvas view allows (I think that's your point anyway!). (In reply to Allan Day from comment #45) > I've tried the latest patches and they it looks great. Two small issues I > spotted: > > * Checking/unchecking "Show hidden files" closes the popover - it should > remain open. Thanks :) We thought that creating a singular popover in the toolbar would fix this, but I tried it and actually it's more complicated than that. I'm going to create another bug for it with the details - it's not something I've introduced (happens in older versions), and it's a bigger bit of work that can be done separately (which I'll happily look into). > * The popover should close after the user has pressed the new folder button. I'll sort that out, I found it annoying too.
> (In reply to Bastien Nocera from comment #41) > > One thing I wanted to note/ask. It doesn't matter if not all the sorting > > options are available in the view menu, as long as it's possible to switch > > to the list view, choose the sorting using the column headers, and switch > > back to the icon view. > > I think I read this the same as Carlos originally. I thought you were > talking about switching to list to set a particular sort order, and then > switching to canvas view and expecting that sort order to remain. That's exactly what I was talking about because it looks like there might be some sort orders that don't make it to the view menu, but which could still be useful to people who want to use the bigger icons for the icon view.
Bastien, I think the ordering should be tied to the view set, I won't like what you're proposing, for every possibile sorting option in icon view, if really needed, I think it would be better to go for some sort of easter egg
(In reply to Lapo Calamandrei from comment #48) > Bastien, I think the ordering should be tied to the view set, I won't like > what you're proposing, for every possibile sorting option in icon view, if > really needed, I think it would be better to go for some sort of easter egg I agree with this. If we're confident of the new reduced set of sort options, then we should stick to our guns and not give the user a way to get the ones that aren't on the menu. As Lapo points out, the sort is tied to the view - so you can set a different sort order for the icon view than canvas view. We'd have to break this and allow the selected sort to be respected when switching view, which I don't think would be a good idea.
Hey Neil, I hope the exams went well :) We were thinking about having this feature for 3.22 and give some user testing with a student in Outreachy during summer. Do you think you will have some time so we can work on it together and merge it? Cheers
Hey Carlos Sure - there's only a small amount of coding left, I was aiming to get that done this weekend. Hah, I'm long past exams :) I'm a lead dev at a s/w consultancy, I've just been a bit busy with work recently so haven't been able to finish this off.
(In reply to Neil Herald from comment #51) > Hah, I'm long past exams :) I'm a lead dev at a s/w consultancy, I've just > been a bit busy with work recently so haven't been able to finish this off. That explains quite a few things! Anyway, keep up the good work :)
Created attachment 330018 [details] [review] toolbar: convert action menu from GMenu into GtkPopoverMenu This is in preparation for merging the view menu into the action menu. The action menu is currently a GMenu, which doesn't support the controls we have on the view menu, e.g. the zoom slider and the button rows. Converting to a GtkPopoverMenu will allow us to merge the menus. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 330019 [details] [review] files-view: merge the action and view menus, and impl. view toggle Usability tests conducted by Gina Dobrescu have highlighted a number of issues with the toolbar menus. Users can't switch between list and grid mode with a single click, and they have struggled to find the switch between list and grid mode. Allan Day has come up with a design to address these problems. The view and action menus have been combined into a single menu, and we have added a new button to the toolbar which toggles the view mode between list and grid mode.
Created attachment 330020 [details] [review] toolbar-view-menu: move the undo/redo items to higher up in the menu Moved them to above the sort options, as part of the toolbar menu redesign.
Created attachment 330021 [details] [review] toolbar-view-menu: remove the Select All and Enter Loc. items
Created attachment 330022 [details] [review] toolbar-view-menu: convert new folder/tab/bookmark items to button list Converted these from three separate menu items into a horizontal button list, and moved them to the top of the menu, as part of the menu redesign.
Created attachment 330023 [details] [review] icon-info: remove some unused function prototypes
Created attachment 330024 [details] [review] toolbar-view-menu: replace zoom slider with buttons Convert the zoom slider into a horizontal button list, with a button in the middle to show the current zoom level percentage. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 330025 [details] [review] toolbar-view-menu: small tweaks to the appearance of the Sort heading The "Sort" heading in the toolbar menu doesn't look right; it has less vertical padding than the surrounding menu items, and is indented one pixel more. It also doesn't need to be bold - this commit addresses these issues.
Created attachment 330026 [details] [review] toolbar-view-menu: remove the Search Relevance sort option This sort option is never shown in the menu and therefore doesn't need to be in the menu xml. The view is sorted on search relevance when the user is searching, however all of the sort options are hidden in this case.
Created attachment 330027 [details] [review] canvas-view: improve usability of sort options The sort options shown on the toolbar menu aren't very intuitive. For example, 'last modified, reverse order' can be interpreted a number of ways. They also require the user to set both the sort order and whether the sort should be reversed. These issues have been addressed by replacing the sort order items that are often sorted in either ascending and descending order (e.g. name) with options that are better to use (e.g. A-Z and Z-A). Options that are generally only sorted one way (e.g. type) have been changed to sort in ascending/descending as appropriate for the option. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 330028 [details] [review] toolbar: change ownership of menu popover from the view to the toolbar Prior changes to merge the view and action menus essentially moved the action menu items into the view menu. This was the path of least resistance; the view has a lot of hooks on items in the view menu, whereas the there are very few hooks on items in the action menu, meaning the latter could be moved more easily. However, previously the view menu was disabled for Other Locations and the action menu wasn't. So the side effect of the changes is the remaining menu is now disabled completely on Other Locations. There are a couple of items that could be shown for Other Locations (e.g. New Tab), so we still want to show a menu, but this involves some refactoring so has been deferred until now. This commit is the first part of the refactoring; the files view owns the menu popover, meaning that the Other Locations view doesn't have access to it. This commit moves the ownership of the menu popover to the toolbar. Future commits will move the common items into the popover so all views will show them.
Created attachment 330029 [details] [review] toolbar: move the new folder/tab/bookmark items into the shared popover The files view creates and owns these buttons currently, meaning they aren't shown on the menu when the Other Locations view is active. This change moves the menu items from the view's toolbar menu ui file and into the ui file for the shared popover, so that the buttons are also shown for Other Locations.
Created attachment 330030 [details] [review] toolbar: move the toolbar menu ui definition into its own file The popover ui definition was in the toolbars definition file which was getting pretty big. Have moved the popover definition into its own file, to separate things out a bit.
Created attachment 330031 [details] [review] files-view: close menu when new folder prompt appears The menu previously remained open when the New Folder prompt appeared, if New Folder was selected from the menu. This change closes the menu when New Folder is selected.
Created attachment 330032 [details] [review] view: allow view to have more control over the toolbar menu Currently we have this menu structure: ------------------------------ 1. New Folder/New Tab/Bookmark ------------------------------ 2. Zoom controls ------------------------------ 3. Undo/Redo ------------------------------ 4. Sort options ------------------------------ 5. Other view related controls ------------------------------ The view creates 2-5, contained in a single GtkWidget - which is then passed to the toolbar via the enclosing window slot. The problem is that 3 shouldn't be created or managed by the view as the controls in that section of the menu are not related to the view. We'd like to move this responsibility back to the toolbar, but that would mean the view must pass multiple menu sections back to the toolbar (as 3 is in the middle of the other view controls). This change allows the view to pass multiple sections back to the toolbar, using the new NautilusToolbarMenuSections structure. The files view now passes 2 as a separate section to 3-5 (3 will be moved out of the view in a future commit).
Created attachment 330033 [details] [review] toolbar: move undo/redo toolbar menu code into toolbar Due to the toolbar menu reorganisation work, the code to create and manage the undo/redo items on the menu ended up in files-view.c. This isn't the correct place as they don't have much to do with the files view. Some refactoring was needed before the code for these items could be moved back into the toolbar, which has now been done in a previous commit. This commit moves the undo/redo creation and management code back into the toolbar (and partially into nautilus-window.c).
(In reply to Neil Herald from comment #51) > Hey Carlos > > Sure - there's only a small amount of coding left, I was aiming to get that > done this weekend. Cool! Yeah, I would say 99% was done. Just the last push :) > > Hah, I'm long past exams :) I'm a lead dev at a s/w consultancy, I've just > been a bit busy with work recently so haven't been able to finish this off. Oh sorry man, but as Georges said, that explains few things :) Going to review now
Review of attachment 330018 [details] [review]: ::: src/nautilus-toolbar.c @@ +75,3 @@ GtkWidget *view_icon; + GtkModelButton *undo_button; + GtkModelButton *redo_button; We always use the parent, in this case, GtkWidget. Even I don't like this (why GtkWidget and not GObject? And why should be care about the parent and not the actual specific type?)... but I guess better to keep with the guidelines. @@ +923,3 @@ } +static void I remember you filed a bug against gtk+ for this right? Can you put a comment here pointing to the bug or at least explaining why this is necessary? It is kinda confusing. ::: src/nautilus-window.c @@ +463,3 @@ + undo_label = undo_label == NULL ? g_strdup (_("Undo")) : undo_label; + redo_label = redo_label == NULL ? g_strdup (_("Redo")) : redo_label; + nautilus_toolbar_update_undo_redo_labels (toolbar, undo_label, redo_label); I wonder if we should connect to the undo manager in the toolbar itself and avoid this API. After all is the toolbar changing because of this, and I don't see any reason why it cannot be in the toolbar itself. What do you think?
Review of attachment 330019 [details] [review]: For the commit message, don't use abreviations like "impl." Also, the title having an "and" is usually a red flag. I think you can do something like "files-view: merge action and view menus" as it is. Otherwise, looks really good. ::: src/nautilus-files-view.c @@ +7979,3 @@ static void +update_menu_item (GtkWidget *menu_item, + NautilusWindow *window, Ok this invalidates one of my comments in the previous patch :) @@ +8001,3 @@ +} + + g_simple_action_set_enabled (G_SIMPLE_ACTION (action), enabled); And this invalidates another comment more @@ +8025,3 @@ + undo_active = redo_active = FALSE; + if (info != NULL && undo_state > NAUTILUS_FILE_UNDO_MANAGER_STATE_NONE) { +static void Not need for parenthesis
Review of attachment 330020 [details] [review]: LGTM
Review of attachment 330021 [details] [review]: Don't use abreviations on the commit message. You can go with: "toolbar-view-menu: remove Select All and Enter Location" If the title surpass slightly the limit is fine if there is no other way around.
Review of attachment 330022 [details] [review]: ::: src/resources/ui/nautilus-toolbar-view-menu.ui @@ +53,3 @@ + <child internal-child="accessible"> + <object class="AtkObject"> + </object> This nmemonic does nothing here afaik, just remove it. @@ +76,3 @@ + <child internal-child="accessible"> + <object class="AtkObject"> + <property name="visible">True</property> This nmemonic doesn't do anything afaik. In case it would, it collides with sorting by type, so that would be incorrect too. I would just remove it.
Review of attachment 330023 [details] [review]: yep
Review of attachment 330024 [details] [review]:
Review of attachment 330025 [details] [review]: Why not use a GtkSeparator?
Review of attachment 330026 [details] [review]: Oh yeah, thanks
(In reply to Carlos Soriano from comment #70) > I wonder if we should connect to the undo manager in the toolbar itself and > avoid this API. After all is the toolbar changing because of this, and I > don't see any reason why it cannot be in the toolbar itself. > What do you think? I could have done it either way. Went with this because the actions are scoped on the window (and this was where this logic was before my changes). Will move it to the toolbar - I agree this is a much better place for it
(In reply to Carlos Soriano from comment #77) > Review of attachment 330025 [details] [review] [review]: > > Why not use a GtkSeparator? Despite the class name, it's not a separator (yeah, I know!). The original class name was "separator", so I just went with that when I added my more specific class. I'll rename to something better, e.g. "nautilus-menu-sort-heading"
Thanks for reviewing so far btw (it can't be much fun!) Once I've made the changes, is it better for me to upload all patches to bugzilla again in the correct order, or just the ones that have changed? Might sound like a stupid question, but I don't know how bugzilla works in terms of ordering the patches etc.
Review of attachment 330027 [details] [review]: Good you managed to keep previous behaviour and new one with a good result. You can commit this one as it is ignoring the next comment. ::: src/nautilus-canvas-view.c @@ +616,3 @@ for (i = 0; i < G_N_ELEMENTS (sort_criteria); i++) { + if (g_strcmp0 (sort_criteria[i].metadata_text, metadata_text) == 0 + && reversed == sort_criteria[i].reverse_order) { Problem here is that users using an old nautilus can have a sort order like Type - reversed which is not contemplated here and you will return by Name. But since this is used in a way that after this the directory is sorted by this, there is no inconsistency between the UI and the real sort, apart of the user finding the sorting to be different than what he put last time, but that should be fine for once...
Review of attachment 330028 [details] [review]: the title was too long, how about something along the lines of "toolbar: change ownership of popover to the toolbar" Otherwise, looks good!
Review of attachment 330029 [details] [review]:
Review of attachment 330030 [details] [review]: much better
Review of attachment 330031 [details] [review]: This looks slightly fishy. I wonder if this should be work for the toolkit, dismissing any other modal window/dialog if some other modal window/dialog is shown. Ignoring that and reviewing the current code: The GAction API is done in a way that modifying an action from a top level window or application (win. or app.) is perfectly fine. However I kinda agree with you on the idea of not accessing what belongs to another layer without the use of a explicit API of that layer. In any case, I would name it nautilus_window_hide_all_menus or nautilus_window_hide_view_menu. ::: src/nautilus-window.c @@ +780,3 @@ + + action_group = gtk_widget_get_action_group (GTK_WIDGET (window), "win"); + menu_action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "view-menu"); g_action_map_lookup_action
Review of attachment 330032 [details] [review]: ::: src/nautilus-files-view.c @@ +2947,3 @@ + g_clear_object (&view->details->toolbar_menu_sections->zoom); + g_clear_object (&view->details->toolbar_menu_sections->custom); + nautilus_toolbar_menu_sections_destroy (view->details->toolbar_menu_sections); hm this looks slightly fishy. Either we use the generic GObject management and convert the menu section struct to a GObject or just use g_free directly. ::: src/nautilus-toolbar-menu-sections.h @@ +4,3 @@ + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or Nautilus uses GPL2, including this file as it is will change the licence of Nautilus as a whole. @@ +26,3 @@ +struct _NautilusToolbarMenuSections { + GtkWidget *zoom; + GtkWidget *custom; custom is misleading. The zoom widget is also custom, isn't it? I would name it extended_section or so. @@ +30,3 @@ + +NautilusToolbarMenuSections *nautilus_toolbar_menu_sections_new (void); +void nautilus_toolbar_menu_sections_destroy (NautilusToolbarMenuSections *queue); queue? ::: src/nautilus-view.h @@ +26,2 @@ #include "nautilus-query.h" +#include "nautilus-toolbar-menu-sections.h" Why all the changes in this file?
Review of attachment 330033 [details] [review]: ::: src/nautilus-toolbar-menu-sections.h @@ +27,3 @@ GtkWidget *zoom; GtkWidget *custom; + gboolean show_undo; is there someone setting this as FALSE somewhere? ::: src/nautilus-toolbar.c @@ +69,3 @@ GtkWidget *view_button; GtkWidget *view_menu_zoom_section; + GtkWidget *view_menu_undo_section; undo_redo_section. We do this in other parts of Nautilus, and I feel is more correct as well. If you don't mind would be good to have this change throughout this patch @@ +1073,3 @@ + /* Set the text of the menu item. Can't use gtk_button_set_label here + * as we need to set the text property, not the label. There's no equivalent + GAction *action; can you put the gtk+ bug number here? ::: src/nautilus-window.c @@ +472,3 @@ + g_free (undo_label); + g_free (redo_label); + if (info != NULL && undo_state > NAUTILUS_FILE_UNDO_MANAGER_STATE_NONE) { Why moved again to the windows and not let it in the toolbar?
(In reply to Neil Herald from comment #81) > Thanks for reviewing so far btw (it can't be much fun!) Phew, almost 5h of review :) It's actually fun when reviewing code that has high quality or elegant because I learn on the process. > > Once I've made the changes, is it better for me to upload all patches to > bugzilla again in the correct order, or just the ones that have changed? > > Might sound like a stupid question, but I don't know how bugzilla works in > terms of ordering the patches etc. Actually that's a question all of us asked at some point.. Attach only the patches that need changes. Only problem is that git-bz stop working in this way, so for that I recommend to have a public wip/ branch in nautilus repo that I can use for testing.
Wow, that's a long review :) Cheers, I'll make those changes. * License: I don't mean to be the bearer of bad news, but many files in Nautilus have GPL3 - I wouldn't just introduce a new license without asking :) $ grep -r "Foundation, either version 3" => 19 files * Many changes in src/nautilus-view.h => that was just to keep the function names and parameters aligned (which I have to admit, I'm not particularly keen on, as it means changing lines that aren't related to the change).
(In reply to Neil Herald from comment #90) > Wow, that's a long review :) Cheers, I'll make those changes. Cool. Indeed, it's hard to imagine how much time it takes to review until you do it... > > * License: I don't mean to be the bearer of bad news, but many files in > Nautilus have GPL3 - I wouldn't just introduce a new license without asking > :) > > $ grep -r "Foundation, either version 3" > > => 19 files > ugh I need to sit down one day and email the committers to change it back... so far I think it's just Georges and me that created new files. In any case, please keep it GPL2. > > * Many changes in src/nautilus-view.h => that was just to keep the function > names and parameters aligned (which I have to admit, I'm not particularly > keen on, as it means changing lines that aren't related to the change). I see. I don't mind either choice.
(In reply to Neil Herald from comment #79) > (In reply to Carlos Soriano from comment #70) > > I wonder if we should connect to the undo manager in the toolbar itself and > > avoid this API. After all is the toolbar changing because of this, and I > > don't see any reason why it cannot be in the toolbar itself. > > What do you think? > > I could have done it either way. Went with this because the actions are > scoped on the window (and this was where this logic was before my changes). > Will move it to the toolbar - I agree this is a much better place for it Right. In the toolbar looks better I think.
Created attachment 330181 [details] [review] toolbar: move undo/redo toolbar menu code into toolbar Due to the toolbar menu reorganisation work, the code to create and manage the undo/redo items on the menu ended up in files-view.c. This isn't the correct place as they don't have much to do with the files view. Some refactoring was needed before the code for these items could be moved back into the toolbar, which has now been done in a previous commit. This commit moves the undo/redo creation and management code into the toolbar.
Created attachment 330182 [details] [review] view: allow view to have more control over the toolbar menu Currently we have this menu structure: ------------------------------ 1. New Folder/New Tab/Bookmark ------------------------------ 2. Zoom controls ------------------------------ 3. Undo/Redo ------------------------------ 4. Sort options ------------------------------ 5. Other view related controls ------------------------------ The view creates 2-5, contained in a single GtkWidget - which is then passed to the toolbar via the enclosing window slot. The problem is that 3 shouldn't be created or managed by the view as the controls in that section of the menu are not related to the view. We'd like to move this responsibility back to the toolbar, but that would mean the view must pass multiple menu sections back to the toolbar (as 3 is in the middle of the other view controls). This change allows the view to pass multiple sections back to the toolbar, using the new NautilusToolbarMenuSections structure. The files view now passes 2 as a separate section to 3-5 (3 will be moved out of the view in a future commit).
Created attachment 330183 [details] [review] view: allow view to have more control over the toolbar menu Currently we have this menu structure: ------------------------------ 1. New Folder/New Tab/Bookmark ------------------------------ 2. Zoom controls ------------------------------ 3. Undo/Redo ------------------------------ 4. Sort options ------------------------------ 5. Other view related controls ------------------------------ The view creates 2-5, contained in a single GtkWidget - which is then passed to the toolbar via the enclosing window slot. The problem is that 3 shouldn't be created or managed by the view as the controls in that section of the menu are not related to the view. We'd like to move this responsibility back to the toolbar, but that would mean the view must pass multiple menu sections back to the toolbar (as 3 is in the middle of the other view controls). This change allows the view to pass multiple sections back to the toolbar, using the new NautilusToolbarMenuSections structure. The files view now passes 2 as a separate section to 3-5 (3 will be moved out of the view in a future commit).
Created attachment 330224 [details] [review] toolbar: convert action menu from GMenu into GtkPopoverMenu This is in preparation for merging the view menu into the action menu. The action menu is currently a GMenu, which doesn't support the controls we have on the view menu, e.g. the zoom slider and the button rows. Converting to a GtkPopoverMenu will allow us to merge the menus. This is part of the toolbar menu redesign to improve the usability of the menus.
Created attachment 330225 [details] [review] files-view: merge action and view menus Usability tests conducted by Gina Dobrescu have highlighted a number of issues with the toolbar menus. Users can't switch between list and grid mode with a single click, and they have struggled to find the switch between list and grid mode. Allan Day has come up with a design to address these problems. The view and action menus have been combined into a single menu, and we have added a new button to the toolbar which toggles the view mode between list and grid mode.
Created attachment 330226 [details] [review] toolbar-view-menu: remove Select All and Enter Location
Created attachment 330227 [details] [review] toolbar-view-menu: convert new folder/tab/bookmark items to button list Converted these from three separate menu items into a horizontal button list, and moved them to the top of the menu, as part of the menu redesign.
Created attachment 330228 [details] [review] toolbar-view-menu: small tweaks to the appearance of the Sort heading The "Sort" heading in the toolbar menu doesn't look right; it has less vertical padding than the surrounding menu items, and is indented one pixel more. It also doesn't need to be bold - this commit addresses these issues.
Created attachment 330229 [details] [review] files-view: close menu when new folder prompt appears The menu previously remained open when the New Folder prompt appeared, if New Folder was selected from the menu. This change closes the menu when New Folder is selected.
Have pushed changes to wip/neilh/toolbar-reorg
thanks Neil! Going to review now
Review of attachment 330024 [details] [review]: I'm not sure why I didn't mark this as accept_commit_now. Marking it.
Review of attachment 330181 [details] [review]: ::: src/nautilus-toolbar-menu-sections.c @@ +23,2 @@ NautilusToolbarMenuSections * +nautilus_toolbar_menu_sections_new (gboolean show_undo_redo_section) sorry I was not clear before... I think trying to do this wrapper functions around a struct is kinda misleading. Looks like an object but it's not. Either use directly the struct "API" or create a GObject. for instance do directly the g_new() and the ->show_undo_redo_section in the client. I would say there is not need to create a whole GObject, so I would go to the direct manipulation of structs. Also instead of calling it show_undo_redo_section we can call it supports_undo_redo if you think that makes sense. I guess this should be a change in both a previous patch I marked as accept_commit_now and here. ::: src/nautilus-toolbar.c @@ +775,3 @@ + /* Set the text of the menu item. Can't use gtk_button_set_label here as + * we need to set the text property, not the label. There's no equivalent + GValue val = G_VALUE_INIT; don't use this kind of abbreviations like "+" in comments please. You can see "for context see bug #whatever" because we don't know if the decision is going to be changed to "we want it" @@ +814,3 @@ + nautilus_file_undo_info_get_strings (info, &undo_label, &undo_description, + &redo_label, &redo_description); + NautilusFileUndoInfo *info; btw, it's not necessary to change it, but you might be interested about g_autofree for this situations
Review of attachment 330183 [details] [review]: ::: src/nautilus-toolbar-menu-sections.h @@ +1,1 @@ +/* Oh yeah this is the patch I was talking about before. I would say just use the struct manipulation directly. You can keep the file for the type declaration.
Review of attachment 330181 [details] [review]: ::: src/nautilus-view.h @@ +42,3 @@ /* * Returns the menu sections that should be shown in the toolbar menu + * when this view is active. Implementations must not return %NULL But then in the upcoming patch in windows-slot you have a condition that allows it. Is this changed unintended?
Review of attachment 330224 [details] [review]: LGTM
Review of attachment 330225 [details] [review]: LGTM
Review of attachment 330226 [details] [review]: LGTM
Review of attachment 330227 [details] [review]: LGTM
Review of attachment 330228 [details] [review]: ah now I get it. LGTM
Review of attachment 330229 [details] [review]: ::: src/nautilus-window.c @@ +780,3 @@ + + action_group = gtk_widget_get_action_group (GTK_WIDGET (window), "win"); + menu_action = g_action_map_lookup_action (G_ACTION_MAP (action_group), "view-menu"); I think I pointed out this before, but why not use directly g_action_map_lookup_action?
Another round of review, I think we are close to merging it :)
(In reply to Carlos Soriano from comment #113) > I think I pointed out this before, but why not use directly > g_action_map_lookup_action? I think I'm missing something here - g_action_map_lookup_action takes a GActionMap as the first param, so I need to look that up with gtk_widget_get_action_group. Is the action map already stored somewhere or is there a way to pass "win" in? (Or am I missing the point completely?) (In reply to Carlos Soriano from comment #107) > + * when this view is active. Implementations must not return %NULL > > But then in the upcoming patch in windows-slot you have a condition that > allows it. Is this changed unintended? Yes it's intended. At some point in the object lifecycle the view isn't set, therefore we can't call view->xyz_get_toolbar_menu_sections. Meaning the view slot has to pass NULL back as the menu sections object, so we have to handle that case. If the view always passes non-null back, it simplifies the logic elsewhere. I.e. we don't have to assume what the default value of show_undo_redo_section is, everywhere that uses the property. (In reply to Carlos Soriano from comment #105) > NautilusToolbarMenuSections * > +nautilus_toolbar_menu_sections_new (gboolean show_undo_redo_section) > > ... > > I would say there is not need to create a whole GObject, so I would go to > the direct manipulation of structs. Cool, I hadn't realised it was a GObject-ism to have a _new method, will change that. Yep, didn't want to go for a whole GObject when all we need is to pass a few properties around.
(In reply to Neil Herald from comment #115) > (In reply to Carlos Soriano from comment #113) > > I think I pointed out this before, but why not use directly > > g_action_map_lookup_action? > > I think I'm missing something here - g_action_map_lookup_action takes a > GActionMap as the first param, so I need to look that up with > gtk_widget_get_action_group. Is the action map already stored somewhere or > is there a way to pass "win" in? (Or am I missing the point completely?) oh, any GtkApplicationWindow or GtkApplication *is* a GActionMap. so G_ACTION_MAP (window) will work. Look how the undo_manager_changed code you modified does it. OTHO, the Gtk+ docs are maybe not clear enough about this. > > > (In reply to Carlos Soriano from comment #107) > > + * when this view is active. Implementations must not return %NULL > > > > But then in the upcoming patch in windows-slot you have a condition that > > allows it. Is this changed unintended? > > Yes it's intended. At some point in the object lifecycle the view isn't set, > therefore we can't call view->xyz_get_toolbar_menu_sections. Meaning the > view slot has to pass NULL back as the menu sections object, so we have to > handle that case. > > If the view always passes non-null back, it simplifies the logic elsewhere. > I.e. we don't have to assume what the default value of > show_undo_redo_section is, everywhere that uses the property. Sorry I misread the code. It makes perfect sense. The window slot has to be prepared for no view at all. > > > (In reply to Carlos Soriano from comment #105) > > > NautilusToolbarMenuSections * > > +nautilus_toolbar_menu_sections_new (gboolean show_undo_redo_section) > > > > ... > > > > I would say there is not need to create a whole GObject, so I would go to > > the direct manipulation of structs. > > Cool, I hadn't realised it was a GObject-ism to have a _new method, will > change that. Yep, didn't want to go for a whole GObject when all we need is > to pass a few properties around. Even if it wasn't, I don't like much to have "half-being" objects, like a struct in steroids. Either it's a struct, or it's complex enough to deserve being a Object (being GObject on C or if we had c++, a class). I guess my Java background shines here :)
Created attachment 330282 [details] [review] files-view: close menu when new folder prompt appears The menu previously remained open when the New Folder prompt appeared, if New Folder was selected from the menu. This change closes the menu when New Folder is selected.
Created attachment 330283 [details] [review] view: allow view to have more control over the toolbar menu Currently we have this menu structure: ------------------------------ 1. New Folder/New Tab/Bookmark ------------------------------ 2. Zoom controls ------------------------------ 3. Undo/Redo ------------------------------ 4. Sort options ------------------------------ 5. Other view related controls ------------------------------ The view creates 2-5, contained in a single GtkWidget - which is then passed to the toolbar via the enclosing window slot. The problem is that 3 shouldn't be created or managed by the view as the controls in that section of the menu are not related to the view. We'd like to move this responsibility back to the toolbar, but that would mean the view must pass multiple menu sections back to the toolbar (as 3 is in the middle of the other view controls). This change allows the view to pass multiple sections back to the toolbar, using the new NautilusToolbarMenuSections structure. The files view now passes 2 as a separate section to 3-5 (3 will be moved out of the view in a future commit).
Created attachment 330284 [details] [review] toolbar: move undo/redo toolbar menu code into toolbar Due to the toolbar menu reorganisation work, the code to create and manage the undo/redo items on the menu ended up in files-view.c. This isn't the correct place as they don't have much to do with the files view. Some refactoring was needed before the code for these items could be moved back into the toolbar, which has now been done in a previous commit. This commit moves the undo/redo creation and management code into the toolbar.
Review of attachment 330282 [details] [review]: Excellent
Review of attachment 330283 [details] [review]: good!
Review of attachment 330284 [details] [review]: ::: src/nautilus-toolbar.c @@ +881,3 @@ +void +nautilus_toolbar_on_window_constructed (NautilusToolbar *self) sorry I missed this before. Couldn't we assume that if the toolbar is constructed the window is already too (or in the middle of it)? In this case, there is not need for a public API but instead override the toolbar constructed. What do you think?
(In reply to Carlos Soriano from comment #122) > In this case, there is not need for a public API but instead override the > toolbar constructed. > > What do you think? undo_manager_changed needs a few things before it can be called: 1. toolbar->window to be set 2. actions to be initialised 3. toolbar/undo items created 1 and 2 aren't set by the time toolbar.init and toolbar.constructed are called - toolbar.constructed was the first place I tried to call undo_manager_changed, but that's too early. I think we'd need to do a fair bit of investigation to try to move 1 and 2 to earlier on in the process, but even that's risky. E.g. as something could be relying on toolbar->window not being set until later. Another option is to drop this commit and pass it onto the gtk+ guys; as you say it's potentially something gtk should handle.
Oh I don't think I said this is gtk+ fault, that was related to a different commit about modal dialogs. Here it's in our side completely. So based on your investigation this has to be done in this way, to going to mark the patch as accept_commit_now. Thanks a lot Neil for the work!
Review of attachment 330284 [details] [review]: based on previous comments, this looks good to me
You're right - I'm not sure how I got the two confused Thanks, I'll push the changes when I get in
Attachment 330020 [details] pushed as 1f4f09f - toolbar-view-menu: move the undo/redo items to higher up in the menu Attachment 330023 [details] pushed as cb7134c - icon-info: remove some unused function prototypes Attachment 330024 [details] pushed as 06029b1 - toolbar-view-menu: replace zoom slider with buttons Attachment 330026 [details] pushed as 3fd3bf7 - toolbar-view-menu: remove the Search Relevance sort option Attachment 330027 [details] pushed as 6435180 - canvas-view: improve usability of sort options Attachment 330029 [details] pushed as d605243 - toolbar: move the new folder/tab/bookmark items into the shared popover Attachment 330030 [details] pushed as 2b7b890 - toolbar: move the toolbar menu ui definition into its own file Attachment 330224 [details] pushed as 6ab063d - toolbar: convert action menu from GMenu into GtkPopoverMenu Attachment 330225 [details] pushed as bdcdbe6 - files-view: merge action and view menus Attachment 330226 [details] pushed as a3613c0 - toolbar-view-menu: remove Select All and Enter Location Attachment 330227 [details] pushed as 142326c - toolbar-view-menu: convert new folder/tab/bookmark items to button list Attachment 330228 [details] pushed as b18989c - toolbar-view-menu: small tweaks to the appearance of the Sort heading Attachment 330282 [details] pushed as 0453216 - files-view: close menu when new folder prompt appears Attachment 330283 [details] pushed as 35f1014 - view: allow view to have more control over the toolbar menu Attachment 330284 [details] pushed as a312f56 - toolbar: move undo/redo toolbar menu code into toolbar
Yay! Fantastic! Thanks for working on this Neil, it's great!
> toolbar-view-menu: remove Select All and Enter Location Why "Enter Location" removed from menu? I often use this feature. Now, i can do this only with hotkey "Ctrl+L". It's possible return this menu item back?