GNOME Bugzilla – Bug 771075
List/grid toggle works not as expected
Last modified: 2016-12-12 20:29:09 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.
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.)
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.
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.
Review of attachment 339803 [details] [review]: Straightforward enough, LGTM. :)
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?
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.
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.
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.
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
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.