D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Filip Fila]

filipf added a comment.


  In D18809#436246 , @victorr wrote:
  
  > File /wallpapers/image/imagepackage/contents/ui/config.qml
  >  Word "Folders" without translation
  >
  >   Kirigami.Heading {
  >   text: "Folders"
  >   level: 2
  >   }
  
  
  I'll fix this in D19873 

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #vdg, ngraham, davidedmundson
Cc: victorr, filipf, mart, alexde, davidedmundson, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Victor Ryzhykh]

victorr added a comment.


  File /wallpapers/image/imagepackage/contents/ui/config.qml
  Word "Folders" without translation
  
Kirigami.Heading {
text: "Folders"
level: 2
}

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, #vdg, ngraham, davidedmundson
Cc: victorr, filipf, mart, alexde, davidedmundson, ngraham, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:2003b267d4bc: Image Wallpaper Slideshow - display the 
list of images that will be shown (authored by davidre, committed by ngraham).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18809?vs=53512=53524#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=53512=53524

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  const 

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=52797=53512

BRANCH
  arcpatch-D18809_2

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  In D18809#427602 , @filipf wrote:
  
  > First and foremost I thank you as a user for giving this bit of code much 
needed love.
  >
  > If you'll have more interest in the slideshow in the future, implementing 
this might also be useful: https://bugs.kde.org/show_bug.cgi?id=186181
  
  
  FWIW the Image Frame widget allows toggling between random and alphabetical 
ordering for its slideshow, so that might be useful as a template.
  
  For that matter, it might make sense to use this view in the Image Frame 
widget directly--after porting that feature over of course. Less code 
duplication for two views that are doing the same thing.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D18809_2

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Filip Fila]

filipf added a comment.


  First and foremost I thank you as a user for giving this bit of code much 
needed love.
  
  If you'll have more interest in the slideshow in the future, implementing 
this might also be useful: https://bugs.kde.org/show_bug.cgi?id=186181

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D18809_2

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  Good stuff! I'm hoping we see more of your commits in the future

INLINE COMMENTS

> slidemodel.cpp:41
> +{
> +Q_FOREACH (QString file, paths) {
> +removeBackground(file);

const 

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D18809_2

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Finally, this latest version works for me! #plasma 
 folks?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  Add addDirs method - reload really reloads now
  To correctly handle cases when there are parent and child directories added 
to the slide paths the reload methods really reloads now before it 
  added only images. As a replacement I added the addDir method.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=52586=52797

BRANCH
  arcpatch-D18809_2

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#419441 , @davidedmundson 
wrote:
  
  > I have a sequence that breaks.
  >
  > Add /opt/kde5/share/wallpapers
  >  Add /opt/kde5/share/wallpapers/Cascade
  >
  > (weirdly this last one now lists every size separately!)
  >
  > Remove the latter, nothing updates
  
  
  Hi I see a different behavior. If I add a subfolder of a added folder nothing 
changes. If I remove it the images in it are removed, however they should be 
retained as they're included by the parent folder. Here The other way around 
all pictures are removed even though the ones on the subfolder should be 
retained.
  F6642848: test.mp4 
  
  F6642849: test2.mp4 
  
  I can see it listing all sizes, if I just add the package. I think this is a 
bug in BackgroundFinder::run. It doesn't find ./metadata.desktop. If you add 
just the package folder and enable the commented out qCDebug statements or 
inspect m_SlideshowBackgrounds after starting a slideshow you can see that all 
sizes are added to the slideshow.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham resigned from this revision.
ngraham added a comment.


  Cleaned the build dir and rebuilt everything from scratch, and now it works 
again, at least as good as it did before. I still don't see images removed from 
the right pane when their folder is removed, but seeing as how it works for 
everyone else, I won't block this anymore. The UI looks good anyway!

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I copied the newer one (the one with the patch applied) over the older one 
and it works again. I guess plasmashell loaded the old library?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  I only see one on my home machine, too:
  
nate@Spectre:/usr$  find -name libplasma_wallpaper_imageplugin.so

./lib/qt/qml/org/kde/plasma/wallpapers/image/libplasma_wallpaper_imageplugin.so

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I find two:
  
268756140 -rw-r--r--   1 root root   139576 Feb 12 13:18 
./lib/x86_64-linux-gnu/qt5/qml/org/kde/plasma/wallpapers/image/libplasma_wallpaper_imageplugin.so
536558340 -rw-r--r--   1 root root   347704 Feb 26 20:27 
./lib/x86_64-linux-gnu/qml/org/kde/plasma/wallpapers/image/libplasma_wallpaper_imageplugin.so

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Nope, just one:
  
dev@dev-pc:/usr$  find -name libplasma_wallpaper_imageplugin.so

./lib/x86_64-linux-gnu/qt5/qml/org/kde/plasma/wallpapers/image/libplasma_wallpaper_imageplugin.so

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson added a comment.


  @ngraham Run find -name libplasma_wallpaper_imageplugin.so  in /usr
  and see if you get two results.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#420509 , @davidedmundson 
wrote:
  
  > > I've found out the difference in why it works for me and not for 
@ngraham. He installs to /usr and I tested from a folder in my home directory. 
Has anyone any hints how to fix this?
  >
  > Why would that make a difference?
  
  
  It shouldn't.
  If I install to /usr I get `config.qml:317: TypeError: Cannot call method 
'indexOf' of undefined`.
  I checked again with Console.log inside Gridview and 
`imageWallpaper.slideshowModel` is undefined and currentWallpaper is 
org.kde.slideshow. However if I install it to somewhere in my home folder 
`imageModel.slideshowModel` is not undefined (`qml: 
SlideModel(0x558e383d0e00)`).

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson added a comment.


  > I've found out the difference in why it works for me and not for @ngraham. 
He installs to /usr and I tested from a folder in my home directory. Has anyone 
any hints how to fix this?
  
  Why would that make a difference?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I've found out the difference in why it works for me and not for @ngraham. He 
installs to /usr and I tested from a folder in my home directory. Has anyone 
any hints how to fix this?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Hmm, with this version, the right panel never gets populated at all, and the 
Open Folder button no longer works.
  
  Also the buttons are too far apart and too far from the right edge of the 
SwipeListItem, but that may be a Kirigami issue for @mart.
  
  Here's a screen recording:
  
  F6639869: weird-2019-02-26_09.32.43.webm 


REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  explicitly test for slideshow inside loader

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=52585=52586

BRANCH
  arcpatch-D18809_2

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  explicitly test for slideshow inside loader

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51402=52585

BRANCH
  arcpatch-D18809_2

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#419441 , @davidedmundson 
wrote:
  
  > I have a sequence that breaks.
  >
  > Add /opt/kde5/share/wallpapers
  >  Add /opt/kde5/share/wallpapers/Cascade
  >
  > (weirdly this last one now lists every size separately!)
  >
  > Remove the latter, nothing updates
  
  
  What is the expected behavior here (apart from the sizes) ? Because currently 
the images are added recursively

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson added a comment.


  I have a sequence that breaks.
  
  Add /opt/kde5/share/wallpapers
  Add /opt/kde5/share/wallpapers/Cascade
  
  (weirdly this last one now lists every size separately!)
  
  Remove the latter, nothing updates

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Marco Martin]

mart added a comment.


  applied the patch locally, and to me removing a folder immediately removes 
its images from the right thumbnail view

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

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


  It's a bit wasteful as we ultimately end up scanning all the wallpaper 
directories once in the main view then again when we have the config view open.
  But we should follow that up another day. It's non-trivial.
  
  
  
  One minor change, which isn't really your fault, it just shows up now.

INLINE COMMENTS

> config.qml:350
>  anchors.fill: parent
>  sourceComponent: (configDialog.currentWallpaper == 
> "org.kde.image") ? thumbnailsComponent : foldersComponent
>  }

There's a funny bug here:

When you're on the Image (not slideshow!) tab, and then close the window or 
select "Plain color" it will instantiate an instance of SlideModel during 
closure.
It's not visible, I only noticed it because I had gdb connected to check 
something, but given SlideModel spawns heavy background threads, that's worth 
fixing

It seems to be because configDialog.currentWallpaper changing gets evaluated 
here before the main view reloads.

As an easy test put

  Component.onCompleted: {
console.log("Dave")
}

Inside the ColumnView of foldersComponent

it shouldn't  be emitted when changing between image and colour.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I can't think of a reason why it doesn't work for you :/.
  Maybe we can debug this on your side. Or the problem is on my side and it 
only works for me. Maybe someone other can comment if it works for them or not?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  No. :(

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  @ngraham, are the icorresponding images now removed after removing a folder 
with the removal of the token check?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Don't use token for removal and fix include guard

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51396=51402

BRANCH
  arcpatch-D18809_1

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre marked 2 inline comments as done.
davidre added inline comments.

INLINE COMMENTS

> davidedmundson wrote in image.h:184-185
> Why do we need two instances?

Can you elaborate on that  please? Are you saying I should assign my Model to  
m_model and then cast it to call the added method? Or something different like 
overriding an existing method or adding the functions BackgroundListModel?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> image.h:184-185
>  int m_currentSlide;
>  BackgroundListModel *m_model;
> +SlideModel* m_slideshowModel;
>  QFileDialog *m_dialog;

Why do we need two instances?

> slidemodel.cpp:30
> +{
> +if (token != m_findToken) {
> +return;

If you hit removeDir twice in quick succession we want it to remove both dirs

I dont' think we want the token for removal

> slidemodel.h:4
> +
> +#endif
> +

this needs to be at the end of the file

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Use KRun again

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51348=51396

BRANCH
  arcpatch-D18809_1

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Sounds good to me.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I just tested it and it seems we're both right . It seems if a path ends with 
a slash (i.e. if you typed the path in in the selection dialog) it opens the 
folder itself while opening the containing folder otherwise. To make this 
consistent i would like to go back to using KRun to get the same behaviour in 
both cases.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> config.qml:269
> +icon.name: "document-open-folder"
> +tooltip: 
> i18nd("plasma_wallpaper_org.kde.image", "Open Folder")
> +onTriggered: 
> imageWallpaper.openFolder(modelData)

`KIO::highlightInFileManager()` does in fact open the containing folder, so the 
old tooltip was correct! :) If you want to use "Open Folder" as the text, you 
should go back to using KRun to open the folder itself.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Change copypasted "Open Containing Folder" to "Open Folder"

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51347=51348

BRANCH
  arcpatch-D18809_1

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#409429 , @davidre wrote:
  
  > I have a hard time figuring out how to hide the tooltip if a tooltip of an 
action is shown. Is there any way to detect this or get to the actions' 
tooltips to check if they are visible?
  
  
  Nevermind. Reading the Documentation again sometimes help!

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Use attached tooltip

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51218=51347

BRANCH
  arcpatch-D18809_1

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I have a hard time figuring out how to hide the tooltip if a tooltip of an 
action is shown. Is there any way to detect this or get to the actions' 
tooltips to check if they are visible?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Actually we can't really put the folder list on the right of the controls 
because most of that area is not drawn by the wallpaper plugin UI itself, but 
by the parent UI that allows selecting plugins. I agree that it's a bit 
wasteful to have a huge tall list for only a few folders (most of the time), 
but on the other hand the list-on-the-left paradigm results in a very common 
and conventional UI that will be instantly understandable by all users. So I 
think it's fine the way it is. :)

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Alex Debus]

alexde added a comment.


  In D18809#408482 , @davidre wrote:
  
  > Do you mean the Controls on the top? (...)  Please correct me if I'm wrong.
  
  
  Exactly, but let's wait for the expertise of Nate and the rest of the VDG 
team as I am not sure myself.
  
  > I guess this could be done like when you remove single images but we would 
need to think about a way to mark the removed folder in the list. Maybe in 
another patch?
  
  Same answer as above. :)

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#408461 , @alexde wrote:
  
  > Great work!
  >
  > Currently you have a very long folder list object. However, in my 
experience most users only have a few folders, where they store their 
wallpapers.
  >  So, what about saving space by positioning the folder list to the right of 
the controls? As we now display only folder names and not whole paths, the 
width should be fine and if you have more than 5 different folders, scrolling 
should be a good option.
  
  
  Do you mean the Controls on the top? I don't know if it's possible because as 
far as I have determined from reading the Code the topmost element that is 
painted by the imagewallpaper-plugin is the "Positioning"-row meaning less 
vertical space. Also if you look at the screenshot ngraham posted you can see 
that it is used in other where the elements are aligned differently and I don't 
know the behaviour then. I guess if the list is right to Positioning and 
"Change every..." this would cause the Positioning row to jump around when the 
mode is switched from single image to slideshow
  
  > Further, if you are about to delete a folder, the images belonging to the 
selected folder, could be highlighted somehow. That way you know what you are 
about to remove without explicitely doing arduous investigation.
  > 
  > What do you think?
  
  I guess this could be done like when you remove single images but we would 
need to think about a way to mark the removed folder in the list. Maybe in 
another patch?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Alex Debus]

alexde added a comment.


  Great work!
  
  Currently you have a very long folder list object. However, in my experience 
most users only have a few folders, where they store their wallpapers.
  So, what about saving space by positioning the folder list to the right of 
the controls? As we now display only folder names and not whole paths, the 
width should be fine and if you have more than 5 different folders, scrolling 
should be a good option.
  
  Further, if you are about to delete a folder, the images belonging to the 
selected folder, could be highlighted somehow. That way you know what you are 
about to remove without explicitely doing arduous investigation.
  
  What do you think?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> ngraham wrote in WallpaperDelegate.qml:40
> I just realized: setting this to false when in slideshow mode is a quick way 
> to make them non-selectable.

That doesn't work - they're still selectable but now we don't get the "open 
containing folder" action on hover

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I just build the patch in a VM and it works as intended. Sorry I would have 
hoped that it didn't work so that I could investigate your issue.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Just tried again with a clean build directory, and there's no change. :(
  
  Maybe @davidedmundson can shed some light on this puzzle?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre marked an inline comment as done.
davidre added a comment.


  In D18809#408175 , @ngraham wrote:
  
  > Could be. Since this is going to land on the master branch (because it's a 
new feature, not a bugfix), ideally should be developing on the master branch 
too.
  >
  > If your distro packages are too old to accommodate this, you can use 
`kdesrc-build` to build them yourself. See 
https://community.kde.org/Get_Involved/development
  
  
  I have now build it with kdesrc-build and applied the patch and build it 
again as described here: 
https://community.kde.org/Infrastructure/Phabricator#Apply_the_patch_and_compile_the_software
 and it still works for me

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> davidre wrote in image.cpp:877
> Yes I also saw this method since the backgroundlist uses it but the Header 
> said to use KRun if you want to open a folder instead of it.

Well it just depends on what you want to do. If you want to open the folder, 
then yeah KRun is better. But if you want to highlight it in a file manager 
window, then `highlightInFileManager()` is the way to go.

> WallpaperDelegate.qml:40
>  
>  hoverEnabled: true
>  

I just realized: setting this to false when in slideshow mode is a quick way to 
make them non-selectable.

> config.qml:278
> +text: modelData
> +visible: folderDelegate.hovered
> +delay: 1000

This should also hide itself when either of the actions' tooltips is visible 
(otherwise they overlap each other)

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre marked an inline comment as done.
davidre added inline comments.

INLINE COMMENTS

> ngraham wrote in image.cpp:877
> This generates a warning (variable `krun` is unused). I would suggest using 
> `KIO::highlightInFileManager()` instead. For example: 
> https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.cpp#n327

Yes I also saw this method since the backgroundlist uses it but the Header said 
to use KRun if you want to open a folder instead of it.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Use highlightInFileManager instead of KRun

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51206=51218

BRANCH
  arcpatch-D18809

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> image.cpp:877
> +{
> +   KRun* krun = new KRun(QUrl::fromLocalFile(path), nullptr);
> +}

This generates a warning (variable `krun` is unused). I would suggest using 
`KIO::highlightInFileManager()` instead. For example: 
https://cgit.kde.org/spectacle.git/tree/src/Gui/KSMainWindow.cpp#n327

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  Could be. Since this is going to land on the master branch (because it's a 
new feature, not a bugfix), ideally should be developing on the master branch 
too.
  
  If your distro packages are too old to accommodate this, you can use 
`kdesrc-build` to build them yourself. See 
https://community.kde.org/Get_Involved/development

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  In D18809#408101 , @davidre wrote:
  
  > Are the images added immediately to the Gridview if you add a folder?
  
  
  Yep.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  I don't know why the images aren't removed immediately on your side. I just 
did a clean build and removed everything from my installation prefix and it 
still works for me. Are you testing directly on the master branch? Maybe the 
difference is that I'm building on the 5.14 branch because of dependencies?

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Show Tooltip with the full path

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51202=51206

BRANCH
  arcpatch-D18809

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#408086 , @ngraham wrote:
  
  > In D18809#408015 , @davidre 
wrote:
  >
  > > In D18809#407355 , @ngraham 
wrote:
  > >
  > > > - After removing a folder, the wallpaper grid should update immediately 
to reflect that
  > >
  > >
  > > It doesn't work for you? Maybe I missed something in the diff, I will try 
to upload a video.
  >
  >
  > Hmm, it still doesn't work for me, even with the latest version.
  
  
  Are the images added immidiately to the Gridview if you add a folder?
  
  > Also now there's a regression: when I add a folder using the button and 
select a folder in the folder chooser, the name displayed is the name of its 
parent folder, not the actual folder.
  
  Yes that was me being lazy and just assumed that the paths end with a slash 
but that was only the case because I was typing them in rather then clicking.
  
  >>> - The individual elements in the wallpaper grid now can't be individually 
chosen as wallpapers, so there's no need for them to be selectable anymore
  >> 
  >> I think I need help with that.
  > 
  > You would probably need to add a new writable property 
(`delegatesSelectable` maybe?) in 
https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridView.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334
 and 
https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334
  > 
  > Could be material for another patch if you'd like.
  > 
  >> Done. Now the problem is, that if you add two folders with the same name 
you cannot know which is which, Maybe it would be possible to add a tooltip?
  > 
  > Yeah, putting the full path in a tooltip makes sense to me.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - Also handle paths not ending with /

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51193=51202

BRANCH
  slideshow-grid (branched from master)

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  In D18809#408015 , @davidre wrote:
  
  > In D18809#407355 , @ngraham 
wrote:
  >
  > > - After removing a folder, the wallpaper grid should update immediately 
to reflect that
  >
  >
  > It doesn't work for you? Maybe I missed something in the diff, I will try 
to upload a video.
  
  
  Hmm, it still doesn't work for me, even with the latest version.
  
  Also now there's a regression: when I add a folder using the button and 
select a folder in the folder chooser, the name displayed is the name of its 
parent folder, not the actual folder.
  
  >> - The individual elements in the wallpaper grid now can't be individually 
chosen as wallpapers, so there's no need for them to be selectable anymore
  > 
  > I think I need help with that.
  
  You would probably need to add a new writable property (`delegatesSelectable` 
maybe?) in 
https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridView.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334
 and 
https://cgit.kde.org/kdeclarative.git/tree/src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml?id=c49f51c1985499b9210b29a3e31a1220f1063334
  
  Could be material for another patch if you'd like.
  
  > Done. Now the problem is, that if you add two folders with the same name 
you cannot know which is which, Maybe it would be possible to add a tooltip?
  
  Yeah, putting the full path in a tooltip makes sense to me.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  The video showing that the Gridview updates when I remove a Folder:
  F6601697: 2019-02-08 15-30-38.mp4 

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  Show only folder name and show remove and  open actions on hover

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51120=51193

BRANCH
  slideshow-grid (branched from master)

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#407355 , @ngraham wrote:
  
  > This is a huge improvement over what we have right now. Very nice job!
  
  
  Thanks!
  
  > A few behavioral issues:
  > 
  > - After removing a folder, the wallpaper grid should update immediately to 
reflect that
  
  It doesn't work for you? Maybe I missed something in the diff, I will try to 
upload a video.
  
  > - The individual elements in the wallpaper grid now can't be individually 
chosen as wallpapers, so there's no need for them to be selectable anymore
  
  I think I need help with that.
  
  > And a design issue: now that it's not taking up the full width, the folder 
list manages to feel space-inefficient while also eliding the text most of the 
time. And there's not so much room left for the wallpaper grid so it feels a 
bit scrunched, especially in System Settings at the default window size:
  >  F6598236: A bit scrunched.png 
  > 
  > I would recommend showing just the folder name, not the full path. Then you 
can reduce its width a bit, so that it's say, a maximum of 25% of the total 
layout width. Right now it's more like 33-40% of the total width most of the 
time which feels too wide.
  
  Done. Now the problem is, that if you add two folders with the same name you 
cannot know which is which, Maybe it would be possible to add a tooltip?
  
  > Ideas for further improvement (not necessary in this patch, but nice to 
have)
  > 
  > - The Remove button should show up on hover rathet than being always 
visible. If you use a Kirigami `SwipeListItem` instead of a `BasicListItem`, 
this will happen automatically if you implement the remove button as an action. 
Here is an example: 
https://cgit.kde.org/discover.git/tree/discover/qml/SourcesPage.qml#n160
  
  Done.
  
  > - It might be nice to add an "Open Containing Folder" action to the folder 
list items, too
  
  Done.

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

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


  This is a huge improvement over what we have right now. Very nice job!
  
  A few behavioral issues:
  
  - After removing a folder, the wallpaper grid should update immediately to 
reflect that
  - The individual elements in the wallpaper grid now can't be individually 
chosen as wallpapers, so there's no need for them to be selectable anymore
  
  And a design issue: now that it's not taking up the full width, the folder 
list manages to feel space-inefficient while also eliding the text most of the 
time. And there's not so much room left for the wallpaper grid so it feels a 
bit scrunched, especially in System Settings at the default window size:
  
  F6598236: A bit scrunched.png 
  
  I would recommend showing just the folder name, not the full path. Then you 
can reduce its width a bit, so that it's say, a maximum of 25% of the total 
layout width. Right now it's more like 33-40% of the total width most of the 
time which feels too wide.
  
  Ideas for further improvement (not necessary in this patch, but nice to have)
  
  - The Remove button should show up on hover rathet than being always visible. 
If you use a Kirigami `SwipeListItem` instead of a `BasicListItem`, this will 
happen automatically if you implement the remove button as an action. Here is 
an example: 
https://cgit.kde.org/discover.git/tree/discover/qml/SourcesPage.qml#n160
  - It might be nice to add an "Open Containing Folder" action to the folder 
list items, too

REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  Also It looks like this now because of D18737 

  F6597907: Screenshot_20190207_192144.png 


REPOSITORY
  R120 Plasma Workspace

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

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


  - remove checkbox stuff

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51088=51120

BRANCH
  slideshow-grid (branched from master)

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre added a comment.


  In D18809#407144 , @ngraham wrote:
  
  > This looks great!
  >
  > I would recommend removing the checkbox feature for now, since that's 
really a separate thing from the UI overhaul. Then we can get the UI overhaul 
in first and work on the checkbox part after that. Does that sound like a plan?
  
  
  Sounds like a good plan!

INLINE COMMENTS

> davidedmundson wrote in image.cpp:909
> You update m_uncheckedSlides  but you're not updating m_slideshowBackgrounds 
> so it does nothing till you restart

Thanks for spotting this, will try testing this.

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> image.cpp:909
> +m_uncheckedSlides = uncheckedSlides;
> +emit uncheckedSlidesChanged();
> +}

You update m_uncheckedSlides  but you're not updating m_slideshowBackgrounds so 
it does nothing till you restart

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

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[Nathaniel Graham]

ngraham added a comment.


  This looks great!
  
  I would recommend removing the checkbox feature for now, since that's really 
a separate thing from the UI overhaul. Then we can get the UI overhaul in first 
and work on the checkbox part after that. Does that sound like a plan?

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

To: davidre, #vdg
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[David Redondo]

davidre updated this revision to Diff 51088.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18809?vs=51087=51088

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

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/slidemodel.cpp
  wallpapers/image/slidemodel.h
  wallpapers/image/slideshowpackage/contents/config/main.xml

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

D18809: Image Wallpaper Slideshow - display the list of images that will be shown

[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
  This shows all the pictures inside the folders added to the Folders list. I 
also tried to make single pictures excludable via a checkbox on the thumbnail. 
This is the first time for me programming with QT/QML/Singals-Slots and I tried 
to use as much existing code as possible. The thumbnail view is the same as for 
single images and I simply subclassed the listmodel. However even if I tried to 
do everything like the code for slidePaths it doesn't work correctly. The 
checking/unchecking of images only applies on restart of plasmashell. Maybe 
it's a single mistake that is easily spotted by a more experienced programmer 
otherwise if the thumbnail view is accepted I can also revert all the checkbox 
stuff.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  slideshow-grid (branched from master)

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

AFFECTED FILES
  wallpapers/image/backgroundlistmodel.cpp
  wallpapers/image/image.cpp
  wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml

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