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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  Could it be some threading issue?

REPOSITORY
  R120 Plasma Workspace

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

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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  Thanks, selecting from any image is useful, to avoid starting always from the 
top one. But should not enable the button until a check is done.

REPOSITORY
  R120 Plasma Workspace

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

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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  I'd say it is some half-implemented feature, probably. Could it be useful to 
navigate by keyboard and check/uncheck images, by using space, for example?

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma, davidedmundson
Cc: davidedmundson, msdobrescu, ngraham, filipf, 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#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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  Is it more efficient from the performance point of view to have a 
QSortFilterProxyModel class per sorting method? I have a 6000 images case, 
would that be consuming compared to setting the sort class once according to 
the sorting method?

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma, davidedmundson
Cc: davidedmundson, msdobrescu, ngraham, filipf, 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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  IMHO, for lines like these, 'm_currentSlide = 
m_slideFilterModel->indexOf(path) - 1;', checks must be added because it is 
transparent to the QML code and let room for failure.
  
  Besides that, did you know that if there are no wallpapers, could be added 
one or more by drag and drop?
  Image files could be added by drag and drop, but never appear, although they 
are in the list, where should be folders only IMHO.
  They stay there, nothing happens, no background change on the desktop.
  Also, if a folder is added, 'Apply' button pressed, then removed, after 
'Apply', the background remains set to the last image, although I would have 
expected to have no image set as wallpaper.

INLINE COMMENTS

> davidre wrote in image.cpp:600
> Currently the slideshow would restart. I guess we could check here if the 
> index is -1. Or alternatively don't trigger this from the qml side if the 
> image is unchecked. In my mind this even better because then we enable the 
> apply button, too.

IMHO, here must be checked because it is transparent to the QML code and lets 
room for failure.

REPOSITORY
  R120 Plasma Workspace

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

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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  Indeed, at should crash, but when is that string empty?
  I can't crash mine, although inherits the same code.

REPOSITORY
  R120 Plasma Workspace

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

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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  Ha Ha Ha! I know! I have asked you about, remember?
  Sometimes the actual build goes bananas and needs a reset.

REPOSITORY
  R120 Plasma Workspace

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

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

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

[Mihai Sorin Dobrescu]

msdobrescu added a comment.


  In D22121#491369 , @ngraham wrote:
  
  > Huh, in a VM everything works for me too. I wonder if there's some subtle 
issue with the fact that I'm running a custom-compiled Plasma at a different 
path on bare metal.
  
  
  Hi, I have followed this one, the new version, step by step: 
https://community.kde.org/Get_Involved/development#Plasma
  Before, some custom script was necessary, but did not work and all my 
crashes, I have experienced and told you about, were due to that. Could be some 
environment variable that pointed to something from the main install.
  I recommend to perform a full cleanup, follow again the testing setup, 
rebuild all the deps (kdesrc-build plasma-desktop plasma-workspace 
plasma-framework plasma-nm plasma-pa plasma-thunderbolt plasma-vault 
plasma-workspace-wallpapers kdeplasma-addons kwin systemsettings kscreen breeze 
discover kinfocenter --include-dependencies ~ and maybe add ~ --refresh-build), 
then test and see if it works. This was the way I've got rid of the errors that 
should not come in the first place, before developing and correcting it for 
real.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: davidedmundson, msdobrescu, ngraham, filipf, 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

[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