Differential D13283

Add possibility to sort folders first
AbandonedPublic
Actions

Authored by faridb on Jun 2 2018, 12:16 PM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Summary

This patch adds the possibility to enable or disable the 'Sort Folders First'
feature.

The function SortedDirModel::lessThan has been removed because it does not respect
the sort order and causes folders to be sorted last when the sort order is set to
descending. I think it was used to sort both folders and archives first but it
does not work properly which causes an inconsistent sorting
behaviour. I do not know whether Gwenview should sort archives as folders or sort
them as any other file. In any case, this issue should be investigated because I only
removed the code which caused problems and there is still code related to the sorting
of archives.

Depends on D13213

Test Plan
  • Open a folder which contains both files and folder in Gwenview
  • Uncheck 'Folders First' in View > Sort By
  • Folders should be sorted among other files
  • Check 'Folders First' in View > Sort By
  • Folders should be sorted before any other file

Diff Detail

Repository
R260 Gwenview
Branch
feature/sort-folders-first
Lint
No Linters Available
Unit
No Unit Test Coverage
faridb requested review of this revision.Jun 2 2018, 12:16 PM
faridb created this revision.
rkflx requested changes to this revision.Jun 2 2018, 6:56 PM
rkflx added a reviewer: Gwenview.

Nice! And thanks for using Arcanist to submit the patch.

I'd also recommend to always set #gwenview as a reviewer, so every member of the Gwenview project will know about the patch (although note that for larger projects like Plasma where reviews are automatically forwarded to a mailing list, you are not supposed to set the project as a reviewer to avoid spamming people's inboxes).

As for the patch, I see you've found SortedDirModel::lessThan I was hinting at. My idea was to return rightIsDirOrArchive instead of leftIsDirOrArchive in case Descending is checked (accessible via sortOrder()).

I think it was used to sort both folders and archives first but it does not work properly […] I do not know whether Gwenview should sort archives as folders or sort them as any other file

Hm, for me archives are sorted first without your patches (right behind folders), so I guess it is working fine? It also makes sense to me: First display "collections" (i.e. folders and archives), and only then present the "documents". I don't think we should simply remove that feature.

However, there is another problem I only noticed now (apologies for that): Not sorting folders first (or archives, for that matter) results in a broken View mode: When you are viewing images and the next item would be a folder, Gwenview thinks you are at the end. Even in Browse mode pressing Space will stop at folders (and it is unclear whether it should select the folder or skip it, as normally this action is supposed to move to the next image). Also the whole concept of a consecutive slideshow breaks down a bit if we allowed folders or archives in between images. I guess this feature makes Gwenview different from Dolphin, it was probably added on purpose for the special use case of an image viewer.

Therefore I would suggest to backtrack a bit and not add Folders First at all (again, sorry for suggesting it, sometimes we notice problems too late…). Instead, you could re-purpose your patch to fix the problem for Descending as outlined above.

there is still code related to the sorting of archives.

Is there any particular part of the code you were referring to? If you mean fileItemIsDirOrArchive, that's needed in a bunch of other places, e.g. the statusbar entry is only counting images, but not folders or archives.

This revision now requires changes to proceed.Jun 2 2018, 6:56 PM
faridb added a comment.Jun 2 2018, 10:38 PM

I think there is a way to make the browsing feature work even if Folders First is disabled. For example, skip the next element if it's a folder and go directly to the next document after it. Would that be an acceptable behaviour?

Do you think that the possibility to disable Folders First would be a nice feature (providing that it works without any bug) or should I just abandon the idea without trying?

rkflx added a comment.Jun 2 2018, 11:04 PM
In D13283#272821, @faridb wrote:

I think there is a way to make the browsing feature work even if Folders First is disabled. For example, skip the next element if it's a folder and go directly to the next document after it. Would that be an acceptable behaviour?

Space and Backspace navigate between images linearly, while ←+→+↑+↓ also allow to move to folders. Currently that's pretty easy to understand for users, because there is a clear separation between both types. However, if you mix folders and archives, this sort of behaviour might become a bit confusing.

Do you think that the possibility to disable Folders First would be a nice feature (providing that it works without any bug) or should I just abandon the idea without trying?

"Without any bug" is a noble goal, but I suspect there are assumptions all over the code base regarding the behaviour, so there might be more subtle bugs we don't even know about yet. In case you need to add a lot of special code, this can also become a maintenance nightmare.

Let me put it this way: What's the actual use case of the feature, i.e. what's the benefit for Gwenview's users to mix folders with images? I don't see any really. Initially my thought was to be consistent with Dolphin, but thinking about it again, this probably was a bad idea.

Still, I don't want to slow down your enthusiasm. IIRC there are more open bugs wrt. sorting, e.g. https://bugs.kde.org/show_bug.cgi?id=340667, or any other bug or feature you find interesting, really (but make sure to let us know before starting to work on something, as some bugs are a bit weird and not actually something we might want to add to Gwenview…).

rkflx added a comment.EditedJun 2 2018, 11:16 PM
In D13283#272823, @rkflx wrote:

Space and Backspace navigate between images linearly, while ←+→+↑+↓ also allow to move to folders.

Speaking of that: In Browse mode, ←+→ currently don't wrap around like in Dolphin, i.e. keeping → pressed down will stop when reaching the end of the row. It would be nice it could move to the next row instead. That's either an easy thing to solve, or more complicated (did not look into it yet, but it's something I wanted to solve for a long time – feel free to work on it ;)

faridb abandoned this revision.Jun 2 2018, 11:23 PM

Yes, you are right. Implementing this feature could probably break other aspects of Gwenview that assume that folders are sorted before files.

(but make sure to let us know before starting to work on something, as some bugs are a bit weird and not actually something we might want to add to Gwenview…).

How should I do that?

Speaking of that: ←+→ currently don't wrap around like in Dolphin, i.e. keeping → pressed down will stop when reaching the end of the row. It would be nice it could move to the next row instead. That's either an easy thing to solve, or more complicated (did not look into it yet, but it's something I wanted to solve for a long time – feel free to work on it ;)

I will have a look at it.

rkflx added a comment.Jun 2 2018, 11:26 PM
In D13283#272827, @faridb wrote:

(but make sure to let us know before starting to work on something, as some bugs are a bit weird and not actually something we might want to add to Gwenview…).

How should I do that?

If it is an existing bug on Bugzilla, just add a comment. If you have an idea for a bigger feature, you can also add as task to Gwenview's workboard for discussion: https://phabricator.kde.org/project/board/263/

Revision Contents
Changeset List

Diff 35399

View Options

app/browsemainpage.h

Loading...
View Options

app/browsemainpage.cpp

Loading...
View Options

lib/gwenviewconfig.kcfg

Loading...
View Options

lib/semanticinfo/sorteddirmodel.h

Loading...
View Options

lib/semanticinfo/sorteddirmodel.cpp

Loading...