Differential D12317

implement and enable sorting in the views
ClosedPublic
Actions

Authored by astippich on Apr 18 2018, 5:29 PM.

Details

Reviewers

mgallien

Group Reviewers

Elisa

Commits

R255:b36a9268072b: implement and enable sorting in the views

Summary

implements the option to sort ascending or descending for all views except the album view.

currently there is only one possibility to sort for each view, e.g. album name for the all album view

Diff Detail

Repository

R255 Elisa

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

astippich requested review of this revision.Apr 18 2018, 5:29 PM

astippich created this revision.

Screenshot of the all tracks view:

Looks like we'll ideally need more icons, because that one currently implies alphabetical sorting with the A and the Z in it. Also those icons don't exactly localize well...

There are alphabetically sorted, it's just in the all tracks view that the track number is additionally taken into account. But yes, localization is a valid point. I could just use an arrow up and down instead

FWIW, Here are my votes for sensible defaults for the other views

Artist: sort artists alphabetically, then sort their albums by year
Album: sort alphabetically

Also, ascending and descending sorting are mutually exclusive, and one is always used. So I would recommend making the buttons into toggles rather than pushbuttons, and have one always be visually in the checked state.

Thanks Alex for your work.

It works fine. This is nice.

Could you try to have one single button with three states ?

first state: no sort
second state: sorted in ascending order
third state: sorted in descending order

Another solution would be to follow dolphin or cantata and have a menu with the choice of data to sort. The button would just indicate if it is ascending or descending.

In D12317#249323, @mgallien wrote:

Could you try to have one single button with three states ?

first state: no sort

second state: sorted in ascending order

third state: sorted in descending order

A single multi-mode button would be nice, yes, but what does "unsorted" actually mean for a collection of music? A random order?

implement a toggle button for sorting

unsorted would be the order in which the tracks were added to the library, which is not useful in my opinion. I would chose to default to ascending.

In D12317#249323, @mgallien wrote:

Thanks Alex for your work.

It works fine. This is nice.

Could you try to have one single button with three states ?

first state: no sort

second state: sorted in ascending order

third state: sorted in descending order

Another solution would be to follow dolphin or cantata and have a menu with the choice of data to sort. The button would just indicate if it is ascending or descending.

Choice of data to sort is definitely on my list, but one step at a time :) Should be fairly easy to implement it for the artist and album views, and a little bit more complicated for the all tracks view.

implementation with a single toggle button

I sticked to the current icon until we get a better one. An arrow alone is also not very meaningful

In D12317#250397, @astippich wrote:

I sticked to the current icon until we get a better one. An arrow alone is also not very meaningful

Looks really good. I am reviewing the track sort method because through testing, things are not clear to me.
The first time I press the sort button there is a noticeable delay during which the application is blocked.

I would really prefer to have another default for the tracks proxy model.

src/models/alltracksproxymodel.cpp
76–98	The track sorting is not easy to understand for me. If we add a way to choose the way to sort, we may no longer need a good default but in the meantime, I would suggest to sort by title then album and then artist. Currently, without looking at the code I did not understood the sort being used.

This revision now requires changes to proceed.Apr 23 2018, 8:06 PM

mgallien mentioned this in D12470: Don't take case into account when sorting albums.Apr 23 2018, 8:41 PM

In D12317#252505, @mgallien wrote:

Looks really good. I am reviewing the track sort method because through testing, things are not clear to me.
The first time I press the sort button there is a noticeable delay during which the application is blocked.

I haven't experienced a delay, but I think you have a larger library.
That actually points out to a major flaw with the current design. Since the sorting is done in the proxy model, it will always be blocking the ui thread. Reading some sources on the internet, this cannot be changed.
Most recommend to do the sorting in the "real" model anyways, and with D11167 this would then be done in a non-blocking way. Is it okay to port the sorting to the real models for this patch and merge? That would still impose blocking currently. Or do you prefer to postpone this completely until the models are moved to a different thread?

src/models/alltracksproxymodel.cpp
76–98	Think of it as a prioritized sorting with 4 columns, e.g. it is first sorted by artist, then by album, then by track. Please try yourself the default of sorting with title. I found the experience to be horrible, both visually and behavior-wise. This makes the all tracks view pretty useless for me. You just have to comment out the "lessThan" operater and add "this->setSortRole(AllTracksModel::TitleRole);" to the constructor

In D12317#254048, @astippich wrote:

In D12317#252505, @mgallien wrote:

Looks really good. I am reviewing the track sort method because through testing, things are not clear to me.
The first time I press the sort button there is a noticeable delay during which the application is blocked.

I haven't experienced a delay, but I think you have a larger library.
That actually points out to a major flaw with the current design. Since the sorting is done in the proxy model, it will always be blocking the ui thread. Reading some sources on the internet, this cannot be changed.

Well, I believe that your patch is already a very big improvement. Let's integrate it and we can always improve later without impact on the interface. This is filling a big gap and people are waiting for it.

Most recommend to do the sorting in the "real" model anyways, and with D11167 this would then be done in a non-blocking way. Is it okay to port the sorting to the real models for this patch and merge? That would still impose blocking currently. Or do you prefer to postpone this completely until the models are moved to a different thread?

I am still not sure that the added complexity of D11167 is worth it. Let's not block progress because of it.

remove custom sort operator for alltracksview

Thanks and good work

This revision is now accepted and ready to land.May 1 2018, 2:50 PM

Closed by commit R255:b36a9268072b: implement and enable sorting in the views (authored by astippich). · Explain WhyMay 1 2018, 4:01 PM

This revision was automatically updated to reflect the committed changes.

astippich mentioned this in D12639: remove sorting from sql queries.May 1 2018, 5:18 PM

Revision Contents
Changeset List

			Path	Packages
M			src/models/abstractmediaproxymodel.h (10 lines)
M			src/models/abstractmediaproxymodel.cpp (11 lines)
M			src/models/allalbumsproxymodel.cpp (3 lines)
M			src/models/allartistsproxymodel.cpp (3 lines)
M			src/models/alltracksproxymodel.cpp (3 lines)
M			src/models/singleartistproxymodel.cpp (3 lines)
M			src/qml/GridBrowserView.qml (3 lines)
M			src/qml/ListBrowserView.qml (4 lines)
M			src/qml/NavigationActionBar.qml (24 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	32490	17edcbf		Apr 18 2018, 5:29 PM	★	★
Diff 2	32645	ba2601f	- implement a toggle button for sorting	Apr 20 2018, 1:25 PM	★	★
Diff 3	33378	602a686	- remove custom sort operator for alltracksview	May 1 2018, 10:25 AM	★	★
Diff 4	33403	52e5fb0	R255:b36a9268072b4b0ffc68bfade4230bd4b2a1b60f	May 1 2018, 4:01 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
dc3debdf6d21	02fdf866c5ac	fff06408db40	Alexander Stippich	remove custom sort operator for alltracksview	May 1 2018, 10:23 AM
fff06408db40	abf6f4b27b8d	dd3ab2da6e41	Alexander Stippich	implement a toggle button for sorting	Apr 20 2018, 1:25 PM
dd3ab2da6e41	443cff78f3fa	a2eefd9180b6	Alexander Stippich	implement and enable sorting in the views (Show More…)	Apr 17 2018, 6:44 AM
a2eefd9180b6	a093286939a3	602a686e1335	Alexander Stippich	implement and enable sorting in the views	Apr 17 2018, 6:39 AM