| rkflx |
| Gwenview |
Gwenview only allows sorting by ascending order. This patch adds the possibility to switch the sorting order to descending.
It is implemented similarly to the sorting menu in Dolphin.
The sorting order is also saved into the Gwenview configuration file.
Before:
After:
FEATURE: 293333
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
@faridb Thanks for your patches, I'll have a look on this one in the next days.
Please make sure to install and use Arcanist (see https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist_to_post_patches) to submit and update your patches in the future, as it makes reviewing and finally landing on your behalf much easier for us.
Thanks again for the patch, works great for folders with only images in them and even title/summary/test plan are looking excellent.
One thing users surely will complain about is that in case you have folders and images in the same directory and check Descending, the folders move to the end. This is a bit unexpected, because by default Dolphin shows them first. This also breaks how the Home and End keys as well as the corresponding entries in the Go menu are supposed to work, i.e. move between the first and last image while skipping directories.
I'm not saying you should implement a separate checkbox like in Dolphin (you could of course do that in addition, e.g. Folders First), but at least by default folders should be displayed at the top and be sorted separately. I think this can be done with only little changes, but I don't want to spoil the fun in figuring it out ;) Try if you can find the trick, otherwise I'll work on a follow-up patch myself.
There are a couple of smaller problems (but nothing you could've known):
Let me know if you are stuck, then I can try to have a more detailed look.
| app/browsemainpage.cpp | ||
|---|---|---|
| 82 | How about mSortDescendingAction? | |
| 167 | There is a superfluous space before mSortOrderDesc. | |
| 170 | Please use the C++11 for ( : ) here. | |
| lib/gwenviewconfig.kcfg | ||
| 221 | I would prefer naming this SortDescending. | |
Thanks for your detailed feedback! I have made the changes you have requested in your inline comments.
I have also added the ability to sort folders first but I will create a separate revision for that.
See D13283 for 'Folders First' sort.
Great, LGTM now except for two minor comments.
I'll leave it open until the dependent patch is ready and in case other members of the Gwenview project have more comments, and then land it for you.
| app/browsemainpage.cpp | ||
|---|---|---|
| 80 | You could make this variable local to setupActions (and drop the m), now that you have QActionGroup* mSortAction. | |
| 169 | I think by default isChecked() of the action is already false, and you are setting the correct value later anyway, so that line is probably not necessary. | |
I made the changes you requested in your inline comments.
I also fixed the issue where folders are sorted after files when the sort order is set to descending (as you suggested in your comment in D13283).
Excellent. I tested everything again, and the issue with the folders not appearing first anymore is now solved.
@muhlenpfordt I'll wait until Monday evening with landing, just in case you also want to try/review the patch first.
Thanks. I guess you can now use Add Action → Abandon Revision for D13283.
@rkflx Thanks for reviewing and accepting the patch! This is my second patch and your feedback and your advices are encouraging me to keep contributing :-).
In regards to D13283, there is still one idea I want to investigate before abandoning the revision. Have a look at my last comment in D13283 and let me know if it's worth trying.