[Image Wallpaper Slideshow] Allow setting of different sorting orders POC
Needs ReviewPublic

Authored by msdobrescu on Jul 3 2019, 11:01 PM.

Details

Reviewers
ngraham
davidre
Group Reviewers
Plasma
Summary

Another take of bug 186181, as POC.
The idea is to use filesystem sorting in order to achieve the goal.
Sort by Name, Time, Size, Type or Unsorted implemented for the moment, chosen in a combo, as only one can be selected at a time.
Reversed, DirsFirst/DirsLast, IgnoreCase, LocaleAware options are available as checkboxes.

Code is not cleaned-up yet, debug logs present.

Test Plan


Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
msdobrescu created this revision.Jul 3 2019, 11:01 PM
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptJul 3 2019, 11:01 PM
msdobrescu requested review of this revision.Jul 3 2019, 11:01 PM
msdobrescu edited the summary of this revision. (Show Details)Jul 3 2019, 11:04 PM

This is awkward timing.

See D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

Sorry

This is awkward timing.

See D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

Sorry

It is a different approach. Why not considering?

msdobrescu updated this revision to Diff 61191.Jul 5 2019, 7:10 AM
msdobrescu edited the summary of this revision. (Show Details)

Added Reversed, DirsFirst/DirsLast, IgnoreCase, LocaleAware options.

msdobrescu updated this revision to Diff 61201.Jul 5 2019, 10:27 AM
msdobrescu edited the test plan for this revision. (Show Details)Jul 7 2019, 4:52 PM
msdobrescu edited the test plan for this revision. (Show Details)

The fundamental problem with this approach is you only sort through the dirs correctly, but not the total across multiple paths.

i.e
If I have two paths dirA and dirB and want things sorted by date this patch will have all the images in dirA sorted by date, followed by all the images in dirB sorted by date.

So for those reasons I think the other patch addresses works a bit better handling it dynamically, also allowing us to get rid of one of the model instances.

The fundamental problem with this approach is you only sort through the dirs correctly, but not the total across multiple paths.

i.e
If I have two paths dirA and dirB and want things sorted by date this patch will have all the images in dirA sorted by date, followed by all the images in dirB sorted by date.

So for those reasons I think the other patch addresses works a bit better handling it dynamically, also allowing us to get rid of one of the model instances.

Thank you! I agree with you, although it is not intended so.
The other approach is more elegant, but I need also sorting as you have described. Perhaps some combined solution would be better.
Still, being a QT newbie, I have no clear idea of the complexity of sorting for the other solution. Would it sort each time it looks for the next image? Would that cost? I'm running a big number of images, around 6000. What do you think?

I guess one drawback is that you have to reindex all the files when the sort mode is changed.

I guess one drawback is that you have to reindex all the files when the sort mode is changed.

I expect to simply reload the list, from the filesystem. That should be done when the folder list is changed too.
But this should be done in your case too, the sorting proxy class needs a remap too, if I understand it correctly. Otherwise it would search in the list each time the next wallpaper is needed, right?
Regardless the solution, I would expect to have the filesystem sorting methods too, for this implementation, generally speaking, yours or mine, meaning, be able to serve according to the file system hierarchy point of view too.
For me it's not some contest, I just need to achieve something and I think your approach is more elegant, but needs more methods and I'd optimize it a bit, as I have commented already, because remapping the collection would be better to avoid the switch for each comparison somehow.
Maybe using templates or lambdas, if possible?