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 771075 - List/grid toggle works not as expected
List/grid toggle works not as expected
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Main Toolbar
3.21.x
Other Linux
: Normal normal
: 3.22
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-08 17:49 UTC by Peter Sonntag
Modified: 2016-12-12 20:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
view: make icon getter static (18.25 KB, patch)
2016-11-14 14:01 UTC, Carlos Soriano
none Details | Review
window-slot: reverse the view icon action (1.53 KB, patch)
2016-11-14 14:01 UTC, Carlos Soriano
none Details | Review
view: make icon getter static (20.01 KB, patch)
2016-11-21 15:23 UTC, Carlos Soriano
committed Details | Review
window-slot: reverse the view icon action (1.89 KB, patch)
2016-11-21 15:23 UTC, Carlos Soriano
committed Details | Review

Description Peter Sonntag 2016-09-08 17:49:06 UTC
For me an icon on a button should visualize want happens when one clicks on it.
The grid/list toggle displays the mode which is right now activated.

If the view is in grid mode the icon displays the grid mode. I would expect the list view. Because this is what gets activated when clicking on it.
Comment 1 Allan Day 2016-10-12 09:31:54 UTC
I'm inclined to agree with this; it's what we do in some other applications, like Boxes. (I'm also a bit nervous about it though - it's the kind of issue where it would be good to be able to do some user testing.)
Comment 2 Carlos Soriano 2016-11-14 14:01:05 UTC
Created attachment 339802 [details] [review]
view: make icon getter static

We were relying on the current view to return the icon to put in the
toolbar, however that requires a view instance and also we cannot
control really what icon we want to show in which circumstances.

We want more control about that, so we need a single entry point where
we can get the icon to show depending on the known view types we
have.

So this patch converts the view property to a static method.
Comment 3 Carlos Soriano 2016-11-14 14:01:10 UTC
Created attachment 339803 [details] [review]
window-slot: reverse the view icon action

That means, now we will show the grid icon when the current view is
the list view and the opposite.

This goes in similarity with gnome-boxes.

The main question to decide this is, is the button an action, and
therefore the icon should be the "target" of the action, or is more
about an information of the current state, and therefore the icon should
be about the current view type?

This patch decides the icon represents an action, although would be good
to have some user testing.
Comment 4 Ernestas Kulik 2016-11-14 17:46:22 UTC
Review of attachment 339803 [details] [review]:

Straightforward enough, LGTM. :)
Comment 5 Ernestas Kulik 2016-11-14 18:07:25 UTC
Review of attachment 339802 [details] [review]:

Neeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeds wooooooooooooooooooooooooooooooooooooooooooooork.

::: src/nautilus-files-view.h
@@ +316,3 @@
 NautilusWindow *    nautilus_files_view_get_window               (NautilusFilesView      *view);
 
+guint               nautilus_files_view_get_view_id                (NautilusView      *view);

This introduces many a warning. Needs some casts in places where the original was used. :|

::: src/nautilus-window-slot.c
@@ +3090,2 @@
+    current_view_id = nautilus_view_get_view_id (NAUTILUS_VIEW (priv->content_view));
+    switch (current_view_id)

Why not just call nautilus_view_get_icon () here?
Comment 6 Carlos Soriano 2016-11-21 15:23:06 UTC
Created attachment 340430 [details] [review]
view: make icon getter static

We were relying on the current view to return the icon to put in the
toolbar, however that requires a view instance and also we cannot
control really what icon we want to show in which circumstances.

We want more control about that, so we need a single entry point where
we can get the icon to show depending on the known view types we
have.

So this patch converts the view property to a static method.
Comment 7 Carlos Soriano 2016-11-21 15:23:11 UTC
Created attachment 340431 [details] [review]
window-slot: reverse the view icon action

That means, now we will show the grid icon when the current view is
the list view and the opposite.

This goes in similarity with gnome-boxes.

The main question to decide this is, is the button an action, and
therefore the icon should be the "target" of the action, or is more
about an information of the current state, and therefore the icon should
be about the current view type?

This patch decides the icon represents an action, although would be good
to have some user testing.
Comment 8 Carlos Soriano 2016-11-21 15:33:26 UTC
sorry for the warnings... the depredations warnings always hit me hard. I'm pondering to disable them upstream :/ so we can say to new contributors that no new warnings should happen, otherwise is too hard to differentiate them, or we need to tell them to disable them in every make.
Comment 9 Carlos Soriano 2016-11-23 12:03:13 UTC
Pushed as we want this in the stable release. Thanks Ernestas for the feedback! Feel free to say something more with the latest patches.

Attachment 340430 [details] pushed as 88cee39 - view: make icon getter static
Attachment 340431 [details] pushed as 9a261eb - window-slot: reverse the view icon action
Comment 10 Jeremy Bicha 2016-12-12 20:29:09 UTC
In case you wanted feedback, I appreciate this change. Now that the view button is just a button and not a menu, I think the new behavior makes more sense.