D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  In D22256#492127 , @davidre wrote:
  
  > 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?

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[David Redondo]

davidre added a comment.


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

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  In D22256#491942 , @davidedmundson 
wrote:
  
  > 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?

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[David Edmundson]

davidedmundson added a comment.


  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.

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu updated this revision to Diff 61201.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22256?vs=61191=61201

REVISION DETAIL
  https://phabricator.kde.org/D22256

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/config/main.xml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu updated this revision to Diff 61191.
msdobrescu edited the summary of this revision.
msdobrescu added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22256?vs=6=61191

REVISION DETAIL
  https://phabricator.kde.org/D22256

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/config/main.xml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  In D22256#490558 , @davidedmundson 
wrote:
  
  > This is awkward timing.
  >
  > See D22121 : [Image Wallpaper 
Slideshow] Allow setting of different sorting orders
  >
  > Sorry
  
  
  It is a different approach. Why not considering?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[David Edmundson]

davidedmundson added a comment.


  This is awkward timing.
  
  See D22121 : [Image Wallpaper Slideshow] 
Allow setting of different sorting orders
  
  Sorry

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22256

To: msdobrescu, ngraham, davidre, #plasma
Cc: davidedmundson, davidre, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22256: [Image Wallpaper Slideshow] Allow setting of different sorting orders POC

[Mihai Sorin Dobrescu]

msdobrescu created this revision.
msdobrescu added reviewers: ngraham, davidre.
msdobrescu added a project: Plasma.
Herald added a subscriber: plasma-devel.
msdobrescu requested review of this revision.

REVISION 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.
  Reversed, DirsFirst/DirsLast, IgnoreCase, LocaleAware to be added as 
checkboxes.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D22256

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/config/main.xml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: msdobrescu, ngraham, davidre
Cc: davidre, ngraham, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart