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

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 > }

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

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:

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

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

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

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

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

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

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

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

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

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) { > +

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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,

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

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,

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

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

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

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?

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

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 !=

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

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

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

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,

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

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

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

ngraham added inline comments. INLINE COMMENTS > config.qml:269 > +icon.name: "document-open-folder" > +tooltip: > i18nd("plasma_wallpaper_org.kde.image", "Open Folder") > +onTriggered:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:

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

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:

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

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

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

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

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

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: >

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

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

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

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:

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

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

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

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:

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

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

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

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

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

davidre added a comment. In D18809#408086 , @ngraham wrote: > In D18809#408015 , @davidre wrote: > > > In D18809#407355 , @ngraham wrote: > > > > > -

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

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

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

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 >

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

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:

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

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

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

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