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

[David Redondo]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:ea32a7611227: [Image Wallpaper Slideshow] Allow setting 
of different sorting orders (authored by davidre).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D22121?vs=62529=62530#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=62529=62530

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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

[David Redondo]

davidre updated this revision to Diff 62529.
davidre added a comment.


  Last touches
  
  - remove QDebug
  - don't restore image if we are in random mode
  - if we have seen all images generate a new random order

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61648=62529

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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

[David Redondo]

davidre added a comment.


  How can we move this forward? Are there things I overlooked? Is it sill 
crashing sometimes? Code comments?

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

[Nathaniel Graham]

ngraham added a comment.


  I no longer have any crashes with this latest version! \o/

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

[David Redondo]

davidre updated this revision to Diff 61648.
davidre added a comment.


  - Only restore wallpaper on startup
  - Guard against empty path
  
  I couldn't understand why this was being called with an empty string so I 
just guard 
  against it at the call site. Maybe this also helps @ngrahams crash but I 
still don't know
  where it is coming from.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61335=61648

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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.


  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

[David Redondo]

davidre added a comment.


  So I'm crashing when I hit apply and then ok because `addURL` is called from 
qml with an empty string and I don't understand why. In main.qml I have
  
onConfiguredImageChanged: {
  if (modelImage != configuredImage) {
  console.log("configured Image" + wallpaper.configuration.Image);
  console.log(configuredImage + "!=" + modelImage)
  imageWallpaper.addUrl(configuredImage);
  }
  }
  onModelImageChanged:{
  Qt.callLater(loadImage);
  console.log("mic to "+modelImage);
  wallpaper.configuration.Image = modelImage;
  console.log("mic:" + wallpaper.configuration.Image +" configimage:" 
+configuredImage);

  }
  
  On apply :
  
qml: mic to file:///home/david/testimages/hk5lmt9qxfw21.jpg
qml: mic:file:///home/david/testimages/hk5lmt9qxfw21.jpg 
configimage:file:///home/david/testimages/hk5lmt9qxfw21.jpg
  
  Then clicking on OK
  
qml: configured Image
qml: !=file:///home/david/testimages/hk5lmt9qxfw21.jpg
  
  What I dont understand is why this happens. When pressing OK without apply 
everything works as I would expect it.

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

[David Redondo]

davidre planned changes to this revision.
davidre added a comment.


  I may have introduced a similar crash or  made it easier to trigger Nate's 
crash.

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

[David Redondo]

davidre updated this revision to Diff 61335.
davidre added a comment.


  - Remember current slide between starts

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61318=61335

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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

[David Redondo]

davidre added a comment.


  In D22121#492179 , @msdobrescu 
wrote:
  
  > 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.
  
  
  Ideally if you want a specific image you should use a  static wallpaper. But 
I see what you mean, I think it would be useful to remember the current slide 
between restarts.

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

[David Redondo]

davidre added a comment.


  In D22121#492173 , @msdobrescu 
wrote:
  
  > 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?
  
  
  You can still navigate by Keyboard. But selecting an image will not enable 
the apply button anymore and then when clicked set the current image to the one 
selected. 
  I like your idea that you can uncheck/check when the whole delegate is 
selected.

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

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

[David Redondo]

davidre updated this revision to Diff 61318.
davidre added a comment.


  Don't start if we are in config mode
  
  I had another crash here via reload -> processPaths -> endInsertRows.
  Triggered by repeatetly checking and unchecking a checkbox. My guess is that 
because of multiple
  calls to reload and beginInsertRows, endInsertRows there were some 
inconsistencies. With this
  I can't crash it anymore by checking and unchecking a checkbox.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61314=61318

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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

[David Redondo]

davidre updated this revision to Diff 61314.
davidre marked 2 inline comments as done.
davidre added a comment.


  Disable selecting the wallpaper in  slideshow configuration
  
  This was introduced when the gridview from the single image dialog was added 
to the 
  slideshow one. In my mind it makes no sense to have it, by clicking on a 
image the user
  doesn't change any visible setting and needlesly changes changes the state of 
the configuration
  dialog.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61116=61314

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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.


  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

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

[David Redondo]

davidre added a comment.


  In D22121#491833 , @davidedmundson 
wrote:
  
  > Nate's crash is due to data not code.
  >
  >   for (int i = 0; i < m_packages.size(); i++) {
  >   QString package = m_packages[i].path();
  >   if (package.at(package.length() - 1) == QChar::fromLatin1('/')) {   
<--- you know you crash on this line
  >   
  >   
  >
  > we're calling .at(length -1) unconditionally
  >
  > For an empty string our index will be -1.
  >
  > This hints towards an insertion bug rather than saying we should just add a 
guard here.
  
  
  Thanks for the hint David. @ngraham could you maybe try to get the path for 
which the crash happens (if package.length == 0) and how the folder structure 
for that image is added to your slideshow. Also it would be good to know if it 
happens if you start fresh and add it in the same way (then I could reproduce 
it) or not.

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

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  Nate's crash is due to data not code.
  
for (int i = 0; i < m_packages.size(); i++) {
QString package = m_packages[i].path();
if (package.at(package.length() - 1) == QChar::fromLatin1('/')) {   
<--- you know you crash on this line
  
  we're calling .at(length -1) unconditionally
  
  For an empty string our index will be -1.
  
  This hints towards an insertion bug rather than saying we should just add a 
guard here.

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

[Nathaniel Graham]

ngraham added a comment.


  In D22121#491723 , @msdobrescu 
wrote:
  
  > 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.
  
  
  Yes, that's how I do it. In fact, I'm one who wrote  that part of the 
documentation. :)

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

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

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> image.cpp:600
> +m_slideshowModel->addBackground(path);
> +m_currentSlide = m_slideFilterModel->indexOf(path) - 1;
>  nextSlide();

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.

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

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> image.cpp:600
> +m_slideshowModel->addBackground(path);
> +m_currentSlide = m_slideFilterModel->indexOf(path) - 1;
>  nextSlide();

what will happen if you add a background that the slideFilterModel then filters 
out?

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre added a comment.


  In D22121#490983 , @ngraham wrote:
  
  > I still get the same crash. :(
  
  
  Very weird I just tried it in a VM which hadn't seen the patch at all yet and 
it doesn't crash.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[Nathaniel Graham]

ngraham added a comment.


  I still get the same crash. :(

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 61116.
davidre added a comment.


  - Sort case insensitive

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61115=61116

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 61115.
davidre added a comment.


  I couldm't crash it by clicking apply but noticed some type errors because 
the proxymodel 
  din't have all the functionality of the underlying model.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61081=61115

BRANCH
  slideshow (branched from master)

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

AFFECTED FILES
  wallpapers/image/CMakeLists.txt
  wallpapers/image/backgroundlistmodel.h
  wallpapers/image/image.cpp
  wallpapers/image/image.h
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
  wallpapers/image/imagepackage/contents/ui/config.qml
  wallpapers/image/imagepackage/contents/ui/main.qml
  wallpapers/image/slidefiltermodel.cpp
  wallpapers/image/slidefiltermodel.h
  wallpapers/image/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre added a comment.


  Weird. I can't get it to crash at all but I will look at and think about it.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[Nathaniel Graham]

ngraham added a comment.


  Now Plasma crashes when I click Apply no matter which mode is selected:
  
0x74ac682f in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x74ac682f in raise () at /usr/lib/libc.so.6
#1  0x74ab1672 in abort () at /usr/lib/libc.so.6
#2  0x74fc87fc in  () at /usr/lib/libQt5Core.so.5
#3  0x74fc7c28 in qt_assert_x(char const*, char const*, char 
const*, int) ()
at /usr/lib/libQt5Core.so.5
#4  0x7fffdc35fb91 in QString::at(int) const (this=0x7fff26c8, i=-1)
at /usr/include/qt/QtCore/qstring.h:936
#5  0x7fffdc35cb55 in BackgroundListModel::indexOf(QString const&) const
(this=0x55f1d8d0, path=...)
at 
/home/nate/kde/src/plasma-workspace/wallpapers/image/backgroundlistmodel.cpp:238
#6  0x7fffdc354654 in Image::setWallpaper(QString const&)
(this=0x55eff620, path=...)
at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:600
#7  0x7fffdc3543f2 in Image::addUrl(QUrl const&, bool)
(this=0x55eff620, url=..., setAsCurrent=true)
at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:566
#8  0x7fffdc352003 in Image::addUrl(QString const&) 
(this=0x55eff620, url=...)
at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:126
#9  0x7fffdc34d3a6 in Image::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (_o=0x55eff620, 
_c=QMetaObject::InvokeMetaMethod, _id=30, _a=0x7fff2b00)
at 
/home/nate/kde/build/plasma-workspace/wallpapers/image/plasma_wallpaper_imageplugin_autogen/EWIEGA46WW/moc_image.cpp:327
#10 0x7fffdc34df6d in Image::qt_metacall(QMetaObject::Call, int, void**)
(this=0x55eff620, _c=QMetaObject::InvokeMetaMethod, _id=30, 
_a=0x7fff2b00)
at 
/home/nate/kde/build/plasma-workspace/wallpapers/image/plasma_wallpaper_imageplugin_autogen/EWIEGA46WW/moc_image.cpp:525
#11 0x7705a62e in  () at /usr/lib/libQt5Qml.so.5
#12 0x76f5b592 in  () at /usr/lib/libQt5Qml.so.5
#13 0x76f5d0a5 in  () at /usr/lib/libQt5Qml.so.5
#14 0x76f5e19a in QV4::QObjectMethod::callInternal(QV4::Value 
const*, QV4::Value const*, int) const () at /usr/lib/libQt5Qml.so.5
#15 0x76f7953a in  () at /usr/lib/libQt5Qml.so.5
#16 0x76f7c60f in  () at /usr/lib/libQt5Qml.so.5
#17 0x76f09f74 in QV4::Function::call(QV4::Value const*, QV4::Value 
const*, int, QV4::ExecutionContext const*) () at /usr/lib/libQt5Qml.so.5
#18 0x77082978 in 
QQmlJavaScriptExpression::evaluate(QV4::CallData*, bool*) ()
at /usr/lib/libQt5Qml.so.5
#19 0x770264f8 in QQmlBoundSignalExpression::evaluate(void**) ()
at /usr/lib/libQt5Qml.so.5
#20 0x770276ac in  () at /usr/lib/libQt5Qml.so.5
#21 0x77065173 in QQmlNotifier::emitNotify(QQmlNotifierEndpoint*, 
void**) ()
at /usr/lib/libQt5Qml.so.5
#22 0x770087f4 in 
QQmlData::signalEmitted(QAbstractDeclarativeData*, QObject*, int, void**) () at 
/usr/lib/libQt5Qml.so.5
#23 0x751e7aff in QMetaObject::activate(QObject*, int, int, void**) 
()
at /usr/lib/libQt5Core.so.5
#24 0x770044b6 in QQmlVMEMetaObject::metaCall(QObject*, 
QMetaObject::Call, int, void**) () at /usr/lib/libQt5Qml.so.5
#25 0x7708c9f9 in  () at /usr/lib/libQt5Qml.so.5
#26 0x7708d1b8 in  () at /usr/lib/libQt5Qml.so.5

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 61081.
davidre added a comment.


  - Bool is now other way around

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=61078=61081

BRANCH
  slideshow (branched from master)

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

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 61078.
davidre added a comment.


  - Don't invalidate if we are in the config view and the mode is set to random

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=60826=61078

BRANCH
  slideshow (branched from master)

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

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre added a comment.


  Ah I forgot a word there.
  I wanted to say don't change the current order if the sort order is switched 
to random. And when opened and the current mode is random whatever.
  Displaying the order for the other modes is quite good I think.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[Nathaniel Graham]

ngraham added a comment.


  Yeah maybe we should always show them in alphabetical order, or maybe we 
should show them in alphabetical order just for the alphabetical and random 
modes?

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre added a comment.


  In D22121#488479 , @ngraham wrote:
  
  > That fixed my crash, yay!
  >
  > I see a UX issues with the Random mode though: since the list re-sorts 
itself randomly, it becomes harder to locale any given wallpaper in there (say, 
for the purposes of unchecking it). Also, when you scroll all the way to the 
bottom and then back up to the top, the order has changed to a new random 
order. While you're scrolling, the delegates are visible changing before your 
eyes. It's all a bit odd.
  >
  > I wonder if in the Random mode, we should just keep the view sorted 
alphabetically?
  
  
  Yes I had the same thoughts about random. Another way would be to not resort 
the slides at all in the config view when you change it and alphabetically/how 
we get them when you open the config view. Either way I guess I will have to 
change `filterUncheckedSlides` to a generic `isUsedinConfig` to enable that.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[Nathaniel Graham]

ngraham added a comment.


  That fixed my crash, yay!
  
  I see a UX issues with the Random mode though: since the list re-sorts itself 
randomly, it becomes harder to locale any given wallpaper in there (say, for 
the purposes of unchecking it). Also, when you scroll all the way to the bottom 
and then back up to the top, the order has changed to a new random order. While 
you're scrolling, the delegates are visible changing before your eyes. It's all 
a bit odd.
  
  I wonder if in the Random mode, we should just keep the view sorted 
alphabetically?

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 60826.
davidre added a comment.


  - add my copyright

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=60825=60826

BRANCH
  slideshow (branched from master)

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

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 60825.
davidre marked 4 inline comments as done.
davidre added a comment.


  - String changes and remove debug statement

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=60823=60825

BRANCH
  slideshow (branched from master)

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

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

To: davidre, #plasma
Cc: 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

[David Redondo]

davidre updated this revision to Diff 60823.
davidre added a comment.


  Sort only if a sortmode has been specified
  
  Otherwise it could have been initialized to an invalid value which caused us 
to
  reach Q_UNREACHABLE and crash.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22121?vs=60725=60823

BRANCH
  slideshow (branched from master)

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

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

To: davidre, #plasma
Cc: 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

[Nathaniel Graham]

ngraham added a comment.


  +100
  
  Will do a bigger review and test tomorrow!

INLINE COMMENTS

> config.qml:147
> +{
> +'label': i18nd("plasma_wallpaper_org.kde.image", 
> "Modification time (oldest first"),
> +'slideshowMode':  Wallpaper.Image.Modified

Needs a trailing ")" in the label

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[Filip Fila]

filipf added a comment.


  Awesome 

INLINE COMMENTS

> config.qml:135
> +{
> +'label': i18nd("plasma_wallpaper_org.kde.image", 
> "Alphabetical (A first)"),
> +'slideshowMode':  Wallpaper.Image.Alphabetical

Instead of "A first" I think we can just say "A to Z" or even "A-Z"

> config.qml:139
> +{
> +'label': i18nd("plasma_wallpaper_org.kde.image", 
> "Alphabetical (Z first)"),
> +'slideshowMode':  Wallpaper.Image.AlphabeticalReversed

Same comment as above just reverse A and Z.

> config.qml:143
> +{
> +'label': i18nd("plasma_wallpaper_org.kde.image", 
> "Modification time (newest first)"),
> +'slideshowMode':  Wallpaper.Image.ModifiedReversed

Something like "Date modified" might be closer to what we have in file managers.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #plasma
Cc: 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

[David Redondo]

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

REVISION SUMMARY
  Allows setting of other sorting orders like alphabetical or last modified 
date.
  To enable this a new ProxyModel is introduced which handles the sorting and
  filtering (as indicated by the checkboxes). This is backed by the 
slideshowModel
  whcih previously as only used for the configutation. The lists of slides and  
unseen
  slides are dropped as now the slides that are shown are taken from the model.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  slideshow (branched from master)

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

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

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