Make mapping between sorting enum and model columns explicit
ClosedPublic

Authored by rkflx on Jun 21 2018, 11:23 PM.

Details

Summary

In Gwenview sorting is implemented in SortedDirModel::lessThan, which
relies on setting the correct sort column to sort by different criteria
when calling sort in BrowseMainPage::updateSortOrder. In
gwenviewrc and in ViewSort By the Sorting enum is used,
while SortedDirModel has KDirModel::ModelColumns.

Previously mapping between both enums was done implicitly, which
worked because both order and position of the entries matched. However,
this had some caveats:

  • Changes to KDirModel could invalidate Gwenview's configuration file, since the mapping might fail afterwards.
  • Adding another sort criterion like Rating in D13344 would result in it mapping to the KDirModel::Permissions column, which could lead to surprising sorting results.

By making the mapping explicit and also providing a way to set a sort
role, both potential issues can be avoided.

Test Plan

All options in ViewSort By should work as before.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rkflx requested review of this revision.Jun 21 2018, 11:23 PM
rkflx created this revision.
muhlenpfordt accepted this revision.Jun 22 2018, 7:47 AM

Looks good to me and adding the Rating sort option should be easy.

Btw... ;) The Date sort does not distinguish between folders and archives. They are both sorted in front of the images, but not folders first and archives after them as Name / Size sorting does.

This revision is now accepted and ready to land.Jun 22 2018, 7:47 AM
rkflx added a comment.EditedJun 22 2018, 5:41 PM

Thanks for testing (which apparently always bears the risk of finding more issues…). As my test plan is saying: Full bug compatibility ;)

Btw... ;) The Date sort does not distinguish between folders and archives. They are both sorted in front of the images, but not folders first and archives after them as Name / Size sorting does.

That's quite interesting, because the problem should also happen for sorting by Rating. It's working fine though, because we are lucky and Gwenview does not pass on the rating of folders, so both folders being compared have a 0 rating and we fall back to KDirSortFilterProxyModel::lessThan.

That's also how it can be fixed for Date: We should do the special handling for ModifiedTime and RatingRole only for items which are not directories or archives, and let KDE Frameworks handle the rest (which it does just fine).

I'll do some more testing and send a patch in a bit. Hopefully you won't find any further bugs, otherwise I'll never get to your open Diffs (or rather to the context manager issue)…

This revision was automatically updated to reflect the committed changes.