GNOME Bugzilla – Bug 683563
Add the ability to change the sort order of the view
Last modified: 2015-12-16 18:31:25 UTC
In some occasion, in particular when dealing with sets of documents like course slides, the documents in a collection are in a strict logical order, and the name usually reflects that. On the other hand, in documents by default they're sorted by modified date, so the order is apparently random. Even if the order is consistent, it's very likely to be reversed: in the example case of course slides, documents from the last lesson are shown on the top. I'm not sure what the best UI for this would be. It could be a button like in Files, or a submenu of the app menu, together with View As.
Created attachment 273172 [details] mockup I think we could extend the existing view button to display a popover. This would contain controls for changing between list/grid, and for changing the sort order. A mockup is attached.
Does this require that the sort order to be persistent? Or it should be by modified date by default at startup?
Need not be persistent in the first attempt. We can make it persistent after some iterations, if need be.
I tried working on this by adding a sort-by action like how view-as is implemented. I used GdMainColumns as the enum, which contains more elements than the three sort orders, and added a setting for that. I'll attach the patches that I made.
Created attachment 273511 [details] [review] application: Add new action sort-by Add a new action and setting "sort-by" which changes the column used for sorting the documents in the overview.
Created attachment 273512 [details] [review] mainToolbar: Move "view as" buttons into a popover
Created attachment 273513 [details] [review] mainToolbar: Add sort buttons to view popover Add the buttons for sorting the documents in the overview, and update them when the sort-by setting changes.
Created attachment 273514 [details] [review] view: Update column for sorting when sort-by changes Update the column of the overview's model for sorting when the state of the sort-by action changes.
Created attachment 273518 [details] [review] mainToolbar: Move "view as" buttons into a popover There's a typo in the second patch. Here's the replacement.
Review of attachment 273514 [details] [review]: I think we should also change the ORDER BY clause in the SPARQL query. Otherwise the order in which the items are loaded in batches will still be in the descending order of their mtime.
I'm busy during the GNOME 3.14 cycle, so if someone wants to take over fixing the bug, please do so :)
Created attachment 314677 [details] [review] application: Add new action sort-by Pushed, after Alessandro rebased it on top of current master.
Created attachment 314678 [details] [review] application: Add new action sort-by Sorry, wrong patch.
Created attachment 315241 [details] [review] Update gschema for Books
Created attachment 315242 [details] [review] mainToolbar: Move "view as" buttons into a popover
Created attachment 315243 [details] [review] view: Update column for sorting when sort-by changes Update the column of the overview's model for sorting when the state of the sort-by action changes.
Created attachment 315245 [details] [review] query, view: Change the ORDER BY clause
Created attachment 315246 [details] [review] view-menu: Add sort options in the popover
Review of attachment 315241 [details] [review]: Good catch.
Review of attachment 315242 [details] [review]: Thanks, Alessandro. Please push after making this minor change. ::: src/mainToolbar.js @@ +151,3 @@ + let viewMenu = builder.get_object('viewMenu'); + + this._viewMenuButton = new Gtk.MenuButton({ tooltip_text: _("View items as a list or a grid"), popover: viewMenu }); Wrap the "popover: ..." to the next line, as we do elsewhere.
Review of attachment 315243 [details] [review]: ::: src/view.js @@ +426,3 @@ + Gtk.SortType.ASCENDING : Gtk.SortType.DESCENDING; + + this._model.set_sort_column_id(sortColumn, sortType); While the UI only allows sorting by PRIMARY_TEXT, SECONDARY_TEXT and MTIME, GdMainColumns has values other than those three. Therefore, I think we shouldn't pass those values directly to set_sort_column_id. We should handle them the same as MTIME, like you did in the SPARQL query patch.
Review of attachment 315246 [details] [review]: ::: data/ui/view-menu.ui @@ +69,3 @@ + <property name="can_focus">False</property> + <property name="halign">start</property> + <property name="label" translatable="yes">Sort</property> Once we start adding the sort options here, the MenuButton's tooltip can't just be: "View items as a list or a grid". Nautilus calls it the "View Menu". Similarly the buttons to change the view should be "View items as a list" and "View items as a grid of icons", because just calling them "Grid" or "List" won't be enough.
(In reply to Debarshi Ray from comment #22) > Review of attachment 315246 [details] [review] [review]: > > ::: data/ui/view-menu.ui > @@ +69,3 @@ > + <property name="can_focus">False</property> > + <property name="halign">start</property> > + <property name="label" translatable="yes">Sort</property> > > Once we start adding the sort options here, the MenuButton's tooltip can't > just be: "View items as a list or a grid". Nautilus calls it the "View > Menu". > > Similarly the buttons to change the view should be "View items as a list" > and "View items as a grid of icons", because just calling them "Grid" or > "List" won't be enough. One quick comment. Nautilus is a bit buggy in this regard, since it doesn't have a tool tip for the view menu and directly sets the ATK label/description. According to the HIG, we should have tooltips [1]. [1] https://developer.gnome.org/hig-book/unstable/toolbars-labels-tooltips.html.en
Created attachment 315752 [details] [review] mainToolbar: Move "view as" buttons into a popover
Created attachment 315753 [details] [review] view: Update column for sorting when sort-by changes Update the column of the overview's model for sorting when the state of the sort-by action changes.
Created attachment 315754 [details] [review] query, view: Change the ORDER BY clause
Created attachment 315755 [details] [review] view-menu: Add sort options in the popover
Attachment 315241 [details] pushed as d19b4b6 - Update gschema for Books Attachment 315752 [details] pushed as 90a0466 - mainToolbar: Move "view as" buttons into a popover
Review of attachment 315753 [details] [review]: Thanks, Alessandro. Looks good.
Review of attachment 315753 [details] [review]: One minor issue: ::: src/view.js @@ +423,3 @@ + _updateSortForSettings: function() { + let sortBy = Application.settings.get_enum('sort-by'); + sortType should be defined here, otherwise one gets: JS WARNING: [resource:///org/gnome/Documents/js/view.js 434]: assignment to undeclared variable sortType
Review of attachment 315754 [details] [review]: ::: src/query.js @@ +126,3 @@ }, + _buildQueryInternal: function(global, flags, offsetController, sortBy) { There is one more use of _buildQueryInternal where the sortBy is useless. Maybe consider passing 'null' to make it more obvious? @@ +183,3 @@ }, + buildGlobalQuery: function(flags, offsetController, sortBy) { buildGlobalQuery is also used in shellSearchProvider.js. In that case, it might make sense to actually pass the setting. ::: src/trackerController.js @@ +327,3 @@ return Application.queryBuilder.buildGlobalQuery(flags, + Application.offsetCollectionsController, + this._sortBy); Underscored prefixed names are supposed to be 'private', so I am a little uncomfortable using it as 'protected'. In cases like this, we have just dropped the underscore.
Created attachment 316269 [details] [review] view: Update column for sorting when sort-by changes Update the column of the overview's model for sorting when the state of the sort-by action changes.
Comment on attachment 316269 [details] [review] view: Update column for sorting when sort-by changes Attachment 316269 [details] pushed as b739e1b - view: Update column for sorting when sort-by changes
Created attachment 316274 [details] [review] query, view: Change the ORDER BY clause
Review of attachment 316274 [details] [review]: Looks great. Pushed.
Review of attachment 315755 [details] [review]: Works like a charm! Pushed.
Thanks Arnel & Alessandro for all the patches.