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

Authored by davidre on Feb 7 2019, 12:28 PM.

Details

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.

FEATURE: 403703
FIXED-IN: 5.16.0

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

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

ngraham added inline comments.Feb 8 2019, 7:36 PM
wallpapers/image/image.cpp
897

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

davidre updated this revision to Diff 51218.Feb 8 2019, 8:34 PM
  • Use highlightInFileManager instead of KRun
davidre marked an inline comment as done.Feb 8 2019, 8:36 PM
davidre added inline comments.
wallpapers/image/image.cpp
897

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.

ngraham added inline comments.Feb 8 2019, 9:04 PM
wallpapers/image/image.cpp
897

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.

wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
40

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

wallpapers/image/imagepackage/contents/ui/config.qml
278

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

davidre marked an inline comment as done.Feb 8 2019, 10:13 PM

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

Just tried again with a clean build directory, and there's no change. :(

Maybe @davidedmundson can shed some light on this puzzle?

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.

davidre added inline comments.Feb 9 2019, 10:31 AM
wallpapers/image/imagepackage/contents/ui/WallpaperDelegate.qml
40

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

alexde added a subscriber: alexde.Feb 9 2019, 11:28 AM

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?

davidre added a comment.EditedFeb 9 2019, 12:17 PM

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. Please correct me if I'm wrong.

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?

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

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

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?

davidre updated this revision to Diff 51347.Feb 10 2019, 8:10 PM
  • Use attached tooltip
davidre added a comment.EditedFeb 10 2019, 8:12 PM

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

davidre marked an inline comment as done.Feb 10 2019, 8:20 PM
davidre updated this revision to Diff 51348.Feb 10 2019, 8:21 PM
  • Change copypasted "Open Containing Folder" to "Open Folder"
ngraham added inline comments.Feb 10 2019, 8:34 PM
wallpapers/image/imagepackage/contents/ui/config.qml
269

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.

davidre added a comment.EditedFeb 10 2019, 8:57 PM

I just tested it and it seems we're both right . It seems if a path nds 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.

Sounds good to me.

davidre updated this revision to Diff 51396.Feb 11 2019, 8:22 AM
  • Use KRun again
wallpapers/image/image.h
184–186

Why do we need two instances?

wallpapers/image/slidemodel.cpp
31

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

I dont' think we want the token for removal

wallpapers/image/slidemodel.h
5

this needs to be at the end of the file

davidre marked 2 inline comments as done.Feb 11 2019, 11:04 AM
davidre added inline comments.
wallpapers/image/image.h
184–186

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?

davidre updated this revision to Diff 51402.Feb 11 2019, 11:04 AM
  • Don't use token for removal and fix include guard
davidre added a comment.EditedFeb 12 2019, 2:16 PM

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

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?

davidedmundson requested changes to this revision.Feb 25 2019, 4:31 PM

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.

wallpapers/image/imagepackage/contents/ui/config.qml
350–351

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.

This revision now requires changes to proceed.Feb 25 2019, 4:31 PM
mart added a subscriber: mart.Feb 25 2019, 5:12 PM

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

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

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

davidre updated this revision to Diff 52585.Feb 26 2019, 8:46 AM

explicitly test for slideshow inside loader

davidre updated this revision to Diff 52586.Feb 26 2019, 8:47 AM

explicitly test for slideshow inside loader

davidre marked an inline comment as done.Feb 26 2019, 8:48 AM

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:

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?

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?

davidre added a comment.EditedFeb 26 2019, 7:29 PM

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)).
I have to investigate more as I don't understand it.

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

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

I find two:

268756    140 -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
536558    340 -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

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

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?

ngraham resigned from this revision.Feb 26 2019, 10:39 PM

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!

ngraham accepted this revision as: VDG.Feb 26 2019, 10:39 PM
davidre added a comment.EditedFeb 27 2019, 12:55 PM

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.

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, too.

davidre updated this revision to Diff 52797.Feb 28 2019, 9:38 AM

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.

ngraham accepted this revision.Mar 8 2019, 8:51 PM

Finally, this latest version works for me! Plasma folks?

davidedmundson accepted this revision.Mar 8 2019, 10:47 PM

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

wallpapers/image/slidemodel.cpp
42

const &file

This revision is now accepted and ready to land.Mar 8 2019, 10:47 PM
ngraham edited the summary of this revision. (Show Details)Mar 8 2019, 10:52 PM
filipf added a subscriber: filipf.Mar 8 2019, 11:26 PM

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

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.

davidre updated this revision to Diff 53512.Mar 9 2019, 2:25 PM

const &file

davidre marked an inline comment as done.Mar 9 2019, 2:25 PM
This revision was automatically updated to reflect the committed changes.
victorr added a subscriber: victorr.EditedMar 22 2019, 12:53 PM

File /wallpapers/image/imagepackage/contents/ui/config.qml
Word "Folders" without translation

Kirigami.Heading {
    text: "Folders"
    level: 2
}

need to
text: i18nd("plasma_wallpaper_org.kde.image", "Folders")

File /wallpapers/image/imagepackage/contents/ui/config.qml
Word "Folders" without translation

Kirigami.Heading {
    text: "Folders"
    level: 2
}

I'll fix this in D19873