I would suggest to always use the filename as the only secondary criterion

When the ratings are the same, the files are sorted using the default sort method of QSortFilterProxyModel which seems to use the filename by default. (Dolphin uses custom code for the sorting which falls back to the filename as a default sorting column). Commit: eb8adc49ab0b.

When toggling Descending, the order resulting from the secondary criterion does not invert, while it should. IOW, toggling should invert everything from the first to the last element.

This was fixed by commit eb8adc49ab0b.

Gwenview should also build when Baloo is not available (as it is an optional dependency).

I changed the code to only include 'Sort By Rating' when Gwenview is built with Baloo. Commit: 7cba2107db93.

We need to think about what to do in reaction to the rating changing, both externally (in case this is easy to detect) and when the user assigns a specific rating. Currently, you are not re-sorting at all, not even pressing F5 will update the order. For Name and Size we re-sort immediately, so we could do the same for Rating, I guess. Opinions?

With commit c7410f96207e, the files are re-sorted whenever the rating is updated internally. However, if the rating is changed through the Properties it seems that the changes are not applied immediately, thus the files are not re-sorted.

In regards to external changes, I will try to investigate that.

Thanks for the update. Don't have time to look at this right now, but here's one more tip (which should come in handy for your next patches, of which I secretly hope there are many ;)

Commit: eb8adc49ab0b.
Commit: 7cba2107db93
commit c7410f96207e

It's great that you split this up into multiple commits in your local branch (keep it that way), but on Phab's Revision Contents → History I can only see what you changed between invocations of arc diff (for now, maybe upstream changes this in the future, as there already is the Revision Contents → Commits tab). It's no problem really, just wanted to let you know that reviews as well as the final landing on Phab work on a patch level, not on single commits, so I don't see what you see when you are referring to single commits…

(What I normally do is that I look at the patch on Phab, e.g. everything or just what you changed since your last update, and also pull the patch down locally with arc patch D13344 for testing).

Works good for me.

Just found a little crazy thing (at least I got crazy searching the cause...) ;)
KDirSortFilterProxyModel::lessThan() seems to sort by filemode here:

• Set sort order to Rating
• Show folder with files a.jpg, b.jpg, c.jpg (same rating)
• Set filemode 664 for a.jpg and c.jpg, 644 for b.jpg
• -> b.jpg moves at the end

In D13344#275576, @faridb wrote:

However, if the rating is changed through the Properties it seems that the changes are not applied immediately, thus the files are not re-sorted.

I don't see any rating related info in the Properties dialog? Is there anything I have to enable for this?

I think I found the problem.
The enum value Sorting::Rating equals ModelColumns::Permissions, see here.
In BrowseMainPage::updateSortOrder() the sort order is set based on the enum value.
Maybe we should add a special case for rating sort order here.

@muhlenpfordt Thanks for testing, I think you found an important flaw.

In D13344#279953, @muhlenpfordt wrote:

I think I found the problem.
The enum value Sorting::Rating equals ModelColumns::Permissions, see here.
In BrowseMainPage::updateSortOrder() the sort order is set based on the enum value.
Maybe we should add a special case for rating sort order here.

Ah, so we thought it worked, because KCategorizedSortFilterProxyModel::lessThan (called from SortedDirModel::lessThan after we determined equal rating) sorted by filename as the secondary criterion after seeing the same Rating (aka Permissions) as the primary criterion in its own equality check. Obviously the mapping between ModelColumns and Sorting::Enum breaks as soon as files don't have the same permissions. I guess my comment in D13344#inline-71076 was not entirely accurate.

In D13344#279939, @muhlenpfordt wrote:

I don't see any rating related info in the Properties dialog? Is there anything I have to enable for this?

According to the commit in D4614, you need at least baloo-widgets 18.04.

In D13344#280376, @rkflx wrote:

In D13344#280067, @faridb wrote:

• Import header file only when needed
• Use if instead of else if when using return
• Avoid multiple conversions from QVarient to int

@faridb LGTM, but we should still solve the issue Peter discovered, i.e. try to always sort by name for the secondary criterion (not only accidentally ;)

I started with the simple changes and then went to find a solution to the secondary criterion problem. I added a temporary fix but here's what I found:

• Dolphin uses the filename as a fallback when no other criterion was sufficient for sorting (see KFileItemModel::sortRoleCompare in Dolphin source code)
• When setting Sorting::Rating to a value greater than KDirModel::ModelColumns::ColumnCount it breaks the sorting for some reason.
• Gwenview uses SortedDirModel which inherits from KDirSortFilterProxyModel but unlike Dolphin it checks for each case if left == right and then explicitly checks the file name as an alternative criterion (see KDirSortFilterProxyModel::subSortLessThan)

In my temporary fix, Sorting::Rating is still equal to ModelColumns::Permissions and the fallback to sorting by file name is incomplete because I only do a simple QString::compare whereas KDirSortFilterProxyModel uses a private function (see KDirSortFilterProxyModel::KDirSortFilterProxyModelPrivate::compare

I am not sure if this is the right way to implement a new sorting criterion. Any thoughts? Was the Sorting enum in sorting.h intended to be used in a specific way?

Thanks for investigating!

In D13344#280404, @faridb wrote:

• Dolphin uses the filename as a fallback when no other criterion was sufficient for sorting (see KFileItemModel::sortRoleCompare in Dolphin source code)

That's what we should arrive at in Gwenview too (not sure how yet, though).

• When setting Sorting::Rating to a value greater than KDirModel::ModelColumns::ColumnCount it breaks the sorting for some reason.

Ah, I tried that too before, but does not work as you found. I guess that's because QSortFilterProxyModelPrivate::update_source_sort_column() sets the column to -1.

• Gwenview uses SortedDirModel which inherits from KDirSortFilterProxyModel but unlike Dolphin it checks for each case if left == right and then explicitly checks the file name as an alternative criterion (see KDirSortFilterProxyModel::subSortLessThan)

Both approaches seem reasonable to me.

In my temporary fix, Sorting::Rating is still equal to ModelColumns::Permissions and the fallback to sorting by file name is incomplete because I only do a simple QString::compare whereas KDirSortFilterProxyModel uses a private function (see KDirSortFilterProxyModel::KDirSortFilterProxyModelPrivate::compare

I am not sure if this is the right way to implement a new sorting criterion. Any thoughts?

Hm, I think we should find a better way, and I already have an idea where the problem might be.

Was the Sorting enum in sorting.h intended to be used in a specific way?

As far as I can see it was intended to be mainly used for Gwenview, e.g. in the config file and in the sort menu. The mapping to KDirModel::ModelColumns was more of a lucky coincidence (aka "hack").

Let's see if I have something for you tomorrow (sorry for the cliffhanger ;)

Continuing where I left off:

After a lot of debugging, code reading and head-scratching I realized that Qt's models provide different ways to access data. There are indexes (defined by columns and rows) as well as "roles", which allow to return specialized data for different purposes when accessing an index. I'd encourage you to read Qt's docs for more information. To get an intuition how that works out for Gwenview's SortedDirModel, you can look at it in GammaRay's module for models, which will display live content for columns, rows and roles.

The issues we have is that in general sorting works with columns, but semantic information like ratings are added as roles (see GvCore::setRating). To solve this, we can either add ratings as a new column, or adapt lessThan to use the correct columns (which your patch currently does not) while accessing ratings via roles (which your patch already does).

Trying the first option I got stuck, until I found that KDirModel::insertColumns is a stub returning false. Therefore I turned to the second option, and got it working beautifully. Please see D13669 and rebase on that, where I'm essentially removing the hack which assumed a direct mapping between Sorting::Enum and KDirModel::ModelColumns.

Adding Rating to the explicit mapping should now be straightforward in your patch, as well as checking for SemanticInfoDirModel::RatingRole instead of the column when sorting. Let me know if I should give more hints ;)

@rkflx Thanks for your work! I rebased on D13669.

I followed your suggestions and I changed SortedDirModel::lessThan to use SemanticInfoDirModel::RatingRole instead of KDirModel::Permissions.

In D13344#281881, @faridb wrote:

@rkflx Thanks for your work! I rebased on D13669.

Great to hear!

You ran into a caveat with Arcanist/Phabricator: Adding Depends on from the Web UI won't affect the dependent revisions, this will only work when submitting with arc diff (not sure whether only for the initial submission or always). In your case you'll also have to add the dependent revision from the sidebar on the right, i.e. Edit Related Revisions → Edit Parent Revisions.

I followed your suggestions and I changed SortedDirModel::lessThan to use SemanticInfoDirModel::RatingRole instead of KDirModel::Permissions.

LGTM.

I removed the code which explicitly triggers sorting when the file's data is changed because it seems like using the rating role solved that problem. I did some tests and the sorting refreshes automatically when the rating of an image is changed without manually calling invalidate().

@faridb Are you still around?

While proofreading the release notes I had the idea to add "Sort by image size" (Bug 263059 actually, which will probably mean comparing the image's area), but wanted to give you the opportunity first if you are interested in becoming a regular contributor. Let me know ;)

In D13344#306903, @rkflx wrote:

@faridb Are you still around?

While proofreading the release notes I had the idea to add "Sort by image size" (Bug 263059 actually, which will probably mean comparing the image's area), but wanted to give you the opportunity first if you are interested in becoming a regular contributor. Let me know ;)

Actually, I started an implementation for that a few weeks ago but I didn't have the time to finish it.

I haven't found the correct way to get the image size in SortedDirModel::lessThan. I used DocumentFactory::instance()->load() but then sorting by image size only works after the document's info have been loaded (i.e you have to explicitly click on each photo to load its info and then the sorting by image size would work).

Also, I am not sure where I should add the new role for sorting by image size; I created an additional namespace in sorting.h for that.

I used DocumentFactory::instance()->load() **but then sorting by image size only works after the document's info have been loaded (i.e you have to explicitly click on each photo to load its info and then the sorting by image size would work).
**

I've got it to work by using Document::waitUntilLoaded() but it waits until the full image has been loaded which renders the sorting by image size quite slow. Any suggestions?

In D13344#308027, @faridb wrote:

I've got it to work by using Document::waitUntilLoaded() but it waits until the full image has been loaded which renders the sorting by image size quite slow. Any suggestions?

Document::size() should be available in state MetaInfoLoaded, it's not necessary to load the whole image.
If the meta info is not retrieved automatically for all images in the current directory maybe we need a way to trigger this.
Sorting by Date actually works with EXIF info (if available), so I think the meta info should be loaded already.

In D13344#308039, @muhlenpfordt wrote:

In D13344#308027, @faridb wrote:

I've got it to work by using Document::waitUntilLoaded() but it waits until the full image has been loaded which renders the sorting by image size quite slow. Any suggestions?

Document::size() should be available in state MetaInfoLoaded, it's not necessary to load the whole image.
If the meta info is not retrieved automatically for all images in the current directory maybe we need a way to trigger this.
Sorting by Date actually works with EXIF info (if available), so I think the meta info should be loaded already.

The value given by Document::loadingState() is 1 (KindDetermined) (for all images except for the one currently selected) and it changes to 2 (MetaInfoLoaded) when I click on the images.