After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764632 - Merge view controls into the hamburger menu
Merge view controls into the hamburger menu
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Views: All
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-05 11:10 UTC by Allan Day
Modified: 2016-10-23 13:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mockup (41.54 KB, image/png)
2016-04-06 09:16 UTC, Allan Day
  Details
toolbar: Convert action menu from GMenu into GtkPopoverMenu (12.60 KB, patch)
2016-04-24 09:40 UTC, Neil Herald
none Details | Review
Current changes (44.69 KB, patch)
2016-05-02 18:27 UTC, Neil Herald
needs-work Details | Review
toolbar: convert action menu from GMenu into GtkPopoverMenu (12.43 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
files-view: merge the action and view menus, and impl. view toggle (36.18 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
toolbar-view-menu: move the undo/redo items to higher up in the menu (3.22 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
toolbar-view-menu: remove the Select All and Enter Loc. items (1.94 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
toolbar-view-menu: convert new folder/tab/bookmark items to button list (6.50 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
icon-info: remove some unused function prototypes (1.02 KB, patch)
2016-05-16 23:05 UTC, Neil Herald
none Details | Review
toolbar-view-menu: replace zoom slider with buttons (21.51 KB, patch)
2016-05-16 23:06 UTC, Neil Herald
none Details | Review
toolbar-view-menu: small tweaks to the appearance of the Sort heading (2.09 KB, patch)
2016-05-16 23:06 UTC, Neil Herald
none Details | Review
toolbar-view-menu: remove the Search Relevance sort option (4.67 KB, patch)
2016-05-16 23:06 UTC, Neil Herald
none Details | Review
canvas-view: improve usability of sort options (26.52 KB, patch)
2016-05-16 23:06 UTC, Neil Herald
none Details | Review
toolbar: convert action menu from GMenu into GtkPopoverMenu (12.43 KB, patch)
2016-06-19 18:57 UTC, Neil Herald
none Details | Review
files-view: merge the action and view menus, and impl. view toggle (36.18 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
reviewed Details | Review
toolbar-view-menu: move the undo/redo items to higher up in the menu (3.22 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: remove the Select All and Enter Loc. items (1.94 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
reviewed Details | Review
toolbar-view-menu: convert new folder/tab/bookmark items to button list (6.50 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
none Details | Review
icon-info: remove some unused function prototypes (1.02 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: replace zoom slider with buttons (21.51 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: small tweaks to the appearance of the Sort heading (2.09 KB, patch)
2016-06-19 18:59 UTC, Neil Herald
none Details | Review
toolbar-view-menu: remove the Search Relevance sort option (4.67 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
committed Details | Review
canvas-view: improve usability of sort options (26.21 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
committed Details | Review
toolbar: change ownership of menu popover from the view to the toolbar (36.04 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
committed Details | Review
toolbar: move the new folder/tab/bookmark items into the shared popover (10.60 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
committed Details | Review
toolbar: move the toolbar menu ui definition into its own file (12.97 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
committed Details | Review
files-view: close menu when new folder prompt appears (2.58 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
none Details | Review
view: allow view to have more control over the toolbar menu (31.94 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
none Details | Review
toolbar: move undo/redo toolbar menu code into toolbar (17.81 KB, patch)
2016-06-19 19:00 UTC, Neil Herald
none Details | Review
toolbar: move undo/redo toolbar menu code into toolbar (21.08 KB, patch)
2016-06-22 06:36 UTC, Neil Herald
none Details | Review
view: allow view to have more control over the toolbar menu (31.76 KB, patch)
2016-06-22 06:37 UTC, Neil Herald
none Details | Review
view: allow view to have more control over the toolbar menu (31.77 KB, patch)
2016-06-22 06:52 UTC, Neil Herald
none Details | Review
toolbar: convert action menu from GMenu into GtkPopoverMenu (12.41 KB, patch)
2016-06-22 22:43 UTC, Neil Herald
committed Details | Review
files-view: merge action and view menus (36.13 KB, patch)
2016-06-22 22:44 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: remove Select All and Enter Location (1.93 KB, patch)
2016-06-22 22:44 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: convert new folder/tab/bookmark items to button list (6.50 KB, patch)
2016-06-22 22:45 UTC, Neil Herald
committed Details | Review
toolbar-view-menu: small tweaks to the appearance of the Sort heading (2.10 KB, patch)
2016-06-22 22:45 UTC, Neil Herald
committed Details | Review
files-view: close menu when new folder prompt appears (2.59 KB, patch)
2016-06-22 22:46 UTC, Neil Herald
none Details | Review
files-view: close menu when new folder prompt appears (2.47 KB, patch)
2016-06-23 21:48 UTC, Neil Herald
committed Details | Review
view: allow view to have more control over the toolbar menu (30.35 KB, patch)
2016-06-23 21:49 UTC, Neil Herald
committed Details | Review
toolbar: move undo/redo toolbar menu code into toolbar (20.20 KB, patch)
2016-06-23 21:49 UTC, Neil Herald
committed Details | Review

Description Allan Day 2016-04-05 11:10:45 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/
Comment 1 Allan Day 2016-04-06 09:16:56 UTC
Created attachment 325465 [details]
mockup

Actually attach the mockup.
Comment 2 Neil Herald 2016-04-24 09:40:33 UTC
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.
Comment 3 Neil Herald 2016-04-24 09:43:40 UTC
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.
Comment 4 Carlos Soriano 2016-04-25 07:24:42 UTC
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.
Comment 5 Carlos Soriano 2016-04-25 09:08:40 UTC
> 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 :)
Comment 6 Bastien Nocera 2016-04-25 09:22:57 UTC
(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.
Comment 7 Neil Herald 2016-04-25 12:17:50 UTC
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.
Comment 8 Allan Day 2016-04-25 16:08:12 UTC
(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.
Comment 9 Neil Herald 2016-05-02 18:27:24 UTC
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.
Comment 10 Carlos Soriano 2016-05-03 08:06:16 UTC
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"
Comment 11 Carlos Soriano 2016-05-03 08:19:27 UTC
(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.
Comment 12 Carlos Soriano 2016-05-03 08:37:27 UTC
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!
Comment 13 Neil Herald 2016-05-03 12:25:14 UTC
(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.
Comment 14 Carlos Soriano 2016-05-03 13:03:39 UTC
(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?
Comment 15 Georges Basile Stavracas Neto 2016-05-04 02:39:15 UTC
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.
Comment 16 Allan Day 2016-05-04 10:09:08 UTC
(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.
Comment 17 Allan Day 2016-05-04 10:12:11 UTC
(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?
Comment 18 Allan Day 2016-05-04 10:13:20 UTC
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%.
Comment 19 Neil Herald 2016-05-04 12:28:46 UTC
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.
Comment 20 Georges Basile Stavracas Neto 2016-05-04 12:44:07 UTC
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.
Comment 21 Neil Herald 2016-05-04 19:56:59 UTC
(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
Comment 22 Carlos Soriano 2016-05-05 08:23:14 UTC
(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.
Comment 23 Carlos Soriano 2016-05-05 08:32:25 UTC
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.
Comment 24 Allan Day 2016-05-05 10:07:45 UTC
(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?
Comment 25 Carlos Soriano 2016-05-05 10:19:56 UTC
> > 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.
Comment 26 Neil Herald 2016-05-16 23:03:30 UTC
(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 :)
Comment 27 Neil Herald 2016-05-16 23:05:34 UTC
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.
Comment 28 Neil Herald 2016-05-16 23:05:39 UTC
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.
Comment 29 Neil Herald 2016-05-16 23:05:44 UTC
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.
Comment 30 Neil Herald 2016-05-16 23:05:49 UTC
Created attachment 328022 [details] [review]
toolbar-view-menu: remove the Select All and Enter Loc. items
Comment 31 Neil Herald 2016-05-16 23:05:53 UTC
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.
Comment 32 Neil Herald 2016-05-16 23:05:58 UTC
Created attachment 328024 [details] [review]
icon-info: remove some unused function prototypes
Comment 33 Neil Herald 2016-05-16 23:06:04 UTC
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.
Comment 34 Neil Herald 2016-05-16 23:06:09 UTC
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.
Comment 35 Neil Herald 2016-05-16 23:06:14 UTC
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.
Comment 36 Neil Herald 2016-05-16 23:06:20 UTC
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.
Comment 37 Neil Herald 2016-05-16 23:10:11 UTC
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).
Comment 38 Neil Herald 2016-05-17 06:27:38 UTC
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?
Comment 39 Carlos Soriano 2016-05-24 13:56:36 UTC
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
Comment 40 Carlos Soriano 2016-05-24 14:00:20 UTC
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.
Comment 41 Bastien Nocera 2016-05-24 14:10:31 UTC
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.
Comment 42 Carlos Soriano 2016-05-24 14:16:10 UTC
(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.
Comment 43 Bastien Nocera 2016-05-24 14:24:34 UTC
(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?
Comment 44 Carlos Soriano 2016-05-24 14:31:06 UTC
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.
Comment 45 Allan Day 2016-05-24 15:00:35 UTC
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.
Comment 46 Neil Herald 2016-05-24 21:52:11 UTC
(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.
Comment 47 Bastien Nocera 2016-05-25 13:31:44 UTC
> (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.
Comment 48 Lapo Calamandrei 2016-05-25 16:15:01 UTC
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
Comment 49 Neil Herald 2016-05-26 06:14:01 UTC
(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.
Comment 50 Carlos Soriano 2016-06-17 12:29:16 UTC
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
Comment 51 Neil Herald 2016-06-18 07:05:33 UTC
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.
Comment 52 Georges Basile Stavracas Neto 2016-06-18 20:10:47 UTC
(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 :)
Comment 53 Neil Herald 2016-06-19 18:57:37 UTC
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.
Comment 54 Neil Herald 2016-06-19 18:59:20 UTC
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.
Comment 55 Neil Herald 2016-06-19 18:59:25 UTC
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.
Comment 56 Neil Herald 2016-06-19 18:59:32 UTC
Created attachment 330021 [details] [review]
toolbar-view-menu: remove the Select All and Enter Loc. items
Comment 57 Neil Herald 2016-06-19 18:59:39 UTC
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.
Comment 58 Neil Herald 2016-06-19 18:59:45 UTC
Created attachment 330023 [details] [review]
icon-info: remove some unused function prototypes
Comment 59 Neil Herald 2016-06-19 18:59:51 UTC
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.
Comment 60 Neil Herald 2016-06-19 18:59:56 UTC
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.
Comment 61 Neil Herald 2016-06-19 19:00:02 UTC
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.
Comment 62 Neil Herald 2016-06-19 19:00:08 UTC
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.
Comment 63 Neil Herald 2016-06-19 19:00:14 UTC
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.
Comment 64 Neil Herald 2016-06-19 19:00:20 UTC
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.
Comment 65 Neil Herald 2016-06-19 19:00:26 UTC
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.
Comment 66 Neil Herald 2016-06-19 19:00:32 UTC
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.
Comment 67 Neil Herald 2016-06-19 19:00:40 UTC
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).
Comment 68 Neil Herald 2016-06-19 19:00:50 UTC
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).
Comment 69 Carlos Soriano 2016-06-20 08:19:43 UTC
(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
Comment 70 Carlos Soriano 2016-06-20 08:29:58 UTC
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?
Comment 71 Carlos Soriano 2016-06-20 11:02:09 UTC
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
Comment 72 Carlos Soriano 2016-06-20 11:03:59 UTC
Review of attachment 330020 [details] [review]:

LGTM
Comment 73 Carlos Soriano 2016-06-20 11:07:40 UTC
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.
Comment 74 Carlos Soriano 2016-06-20 11:11:17 UTC
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.
Comment 75 Carlos Soriano 2016-06-20 11:12:41 UTC
Review of attachment 330023 [details] [review]:

yep
Comment 76 Carlos Soriano 2016-06-20 11:22:14 UTC
Review of attachment 330024 [details] [review]:

Comment 77 Carlos Soriano 2016-06-20 11:23:11 UTC
Review of attachment 330025 [details] [review]:

Why not use a GtkSeparator?
Comment 78 Carlos Soriano 2016-06-20 11:23:59 UTC
Review of attachment 330026 [details] [review]:

Oh yeah, thanks
Comment 79 Neil Herald 2016-06-20 11:31:11 UTC
(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
Comment 80 Neil Herald 2016-06-20 11:33:26 UTC
(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"
Comment 81 Neil Herald 2016-06-20 11:42:16 UTC
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.
Comment 82 Carlos Soriano 2016-06-20 11:54:32 UTC
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...
Comment 83 Carlos Soriano 2016-06-20 12:11:44 UTC
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!
Comment 84 Carlos Soriano 2016-06-20 12:12:53 UTC
Review of attachment 330029 [details] [review]:

Comment 85 Carlos Soriano 2016-06-20 12:14:13 UTC
Review of attachment 330030 [details] [review]:

much better
Comment 86 Carlos Soriano 2016-06-20 12:23:21 UTC
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
Comment 87 Carlos Soriano 2016-06-20 14:09:44 UTC
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?
Comment 88 Carlos Soriano 2016-06-20 15:53:47 UTC
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?
Comment 89 Carlos Soriano 2016-06-20 16:00:28 UTC
(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.
Comment 90 Neil Herald 2016-06-20 18:28:01 UTC
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).
Comment 91 Carlos Soriano 2016-06-20 19:57:33 UTC
(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.
Comment 92 Carlos Soriano 2016-06-20 19:58:12 UTC
(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.
Comment 93 Neil Herald 2016-06-22 06:36:34 UTC
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.
Comment 94 Neil Herald 2016-06-22 06:37:04 UTC
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).
Comment 95 Neil Herald 2016-06-22 06:52:27 UTC
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).
Comment 96 Neil Herald 2016-06-22 22:43:51 UTC
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.
Comment 97 Neil Herald 2016-06-22 22:44:16 UTC
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.
Comment 98 Neil Herald 2016-06-22 22:44:52 UTC
Created attachment 330226 [details] [review]
toolbar-view-menu: remove Select All and Enter Location
Comment 99 Neil Herald 2016-06-22 22:45:15 UTC
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.
Comment 100 Neil Herald 2016-06-22 22:45:43 UTC
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.
Comment 101 Neil Herald 2016-06-22 22:46:10 UTC
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.
Comment 102 Neil Herald 2016-06-22 23:05:27 UTC
Have pushed changes to wip/neilh/toolbar-reorg
Comment 103 Carlos Soriano 2016-06-23 08:17:06 UTC
thanks Neil! Going to review now
Comment 104 Carlos Soriano 2016-06-23 08:49:55 UTC
Review of attachment 330024 [details] [review]:

I'm not sure why I didn't mark this as accept_commit_now. Marking it.
Comment 105 Carlos Soriano 2016-06-23 09:08:27 UTC
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
Comment 106 Carlos Soriano 2016-06-23 09:11:44 UTC
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.
Comment 107 Carlos Soriano 2016-06-23 09:15:20 UTC
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?
Comment 108 Carlos Soriano 2016-06-23 09:19:44 UTC
Review of attachment 330224 [details] [review]:

LGTM
Comment 109 Carlos Soriano 2016-06-23 09:21:30 UTC
Review of attachment 330225 [details] [review]:

LGTM
Comment 110 Carlos Soriano 2016-06-23 09:22:10 UTC
Review of attachment 330226 [details] [review]:

LGTM
Comment 111 Carlos Soriano 2016-06-23 09:22:59 UTC
Review of attachment 330227 [details] [review]:

LGTM
Comment 112 Carlos Soriano 2016-06-23 09:23:55 UTC
Review of attachment 330228 [details] [review]:

ah now I get it. LGTM
Comment 113 Carlos Soriano 2016-06-23 09:25:37 UTC
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?
Comment 114 Carlos Soriano 2016-06-23 09:26:26 UTC
Another round of review, I think we are close to merging it :)
Comment 115 Neil Herald 2016-06-23 11:40:46 UTC
(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.
Comment 116 Carlos Soriano 2016-06-23 11:51:48 UTC
(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 :)
Comment 117 Neil Herald 2016-06-23 21:48:58 UTC
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.
Comment 118 Neil Herald 2016-06-23 21:49:30 UTC
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).
Comment 119 Neil Herald 2016-06-23 21:49:57 UTC
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.
Comment 120 Carlos Soriano 2016-06-24 07:39:41 UTC
Review of attachment 330282 [details] [review]:

Excellent
Comment 121 Carlos Soriano 2016-06-24 07:40:55 UTC
Review of attachment 330283 [details] [review]:

good!
Comment 122 Carlos Soriano 2016-06-24 07:47:40 UTC
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?
Comment 123 Neil Herald 2016-06-24 11:12:56 UTC
(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.
Comment 124 Carlos Soriano 2016-06-24 11:18:36 UTC
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!
Comment 125 Carlos Soriano 2016-06-24 11:19:06 UTC
Review of attachment 330284 [details] [review]:

based on previous comments, this looks good to me
Comment 126 Neil Herald 2016-06-24 12:00:45 UTC
You're right - I'm not sure how I got the two confused 

Thanks, I'll push the changes when I get in
Comment 127 Neil Herald 2016-06-24 17:57:08 UTC
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
Comment 128 Allan Day 2016-06-27 10:02:32 UTC
Yay! Fantastic! Thanks for working on this Neil, it's great!
Comment 129 vitalik_p 2016-10-23 13:03:47 UTC
> 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?