Fix columns sorting.

Authored by kkasper on Jan 26 2018, 11:17 PM.



QTreeWidgetItem in Qt5 requires overloading < operator to implement
custom sorting. Use existing compare methods to do that.

Additionally, natural sorting was implemented for strings.
This allows to sort by filenames like file_<number>.


Test Plan

Click on columns to sort them.
Columns are sorted by artist, album and track number.

Diff Detail

R344 Juk
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
kkasper requested review of this revision.Jan 26 2018, 11:17 PM
kkasper created this revision.
kkasper added inline comments.Jan 26 2018, 11:19 PM

I'm not sure about this one. Perhaps it would be better to use private member and initialize once in constructor?

mpyne requested changes to this revision.Jan 27 2018, 5:47 PM

Once the placement of the naturalCompare function is corrected, this is ready to commit. I'll leave the handling of the QCollator to your opinion but I think it's fine to be in the method itself, especially once it is made a "free" function at file scope.


Actually I'm not as worried about this as I would have used to be. With C++11 this should be safe at least (and this is a GUI thread method here so we should expect this to be called without concurrent anyways).

If I were to move it, I would probably make it a global static inside of playlistitem.cpp instead of moving to the PlaylistItem type.


There doesn't appear to be anything about this method specific to PlaylistItem as a type.

Please make it a static function in playlistitem.cpp at file scope (not class scope) instead, which can also potentially provide the compiler with better opportunity to optimize since the function will not need to be public to other users of the PlaylistItem type.

This revision now requires changes to proceed.Jan 27 2018, 5:47 PM
kkasper updated this revision to Diff 26127.Jan 28 2018, 2:10 PM

naturalCompare is now a local function.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 28 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.