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

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

Details

Reviewers
ngraham
Group Reviewers
VDG
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.

BUG: 403703

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D18809_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8151
Build 8169: arc lint + arc unit
davidre created this revision.Thu, Feb 7, 12:28 PM
Restricted Application added a project: Plasma. · View Herald TranscriptThu, Feb 7, 12:28 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Thu, Feb 7, 12:28 PM
davidre updated this revision to Diff 51088.Thu, Feb 7, 12:29 PM
davidre edited the summary of this revision. (Show Details)Thu, Feb 7, 12:32 PM
davidre edited the summary of this revision. (Show Details)Thu, Feb 7, 12:35 PM
davidre added a reviewer: VDG.
ngraham added a subscriber: ngraham.Thu, Feb 7, 3:15 PM

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?

davidedmundson added inline comments.
wallpapers/image/image.cpp
909

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

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!

wallpapers/image/image.cpp
909

Thanks for spotting this, will try testing this.

davidre updated this revision to Diff 51120.EditedThu, Feb 7, 6:15 PM
  • remove checkbox stuff

Hopefully that should be it if I used arc correctly. I deleted the stuff relating to the checkboxes and ran arc diff origin/master because I messed my commits up

Also It looks like this now because of D18737

ngraham requested changes to this revision.Thu, Feb 7, 7:24 PM

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:

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
This revision now requires changes to proceed.Thu, Feb 7, 7:24 PM
davidre updated this revision to Diff 51193.Fri, Feb 8, 4:22 PM

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

davidre added a comment.EditedFri, Feb 8, 4:22 PM

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:

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)

Done.

  • It might be nice to add an "Open Containing Folder" action to the folder list items, too

Done.

This is how it looks now:

The video showing that the Gridview updates when I remove a Folder:

  • 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.

davidre updated this revision to Diff 51202.Fri, Feb 8, 5:36 PM
  • Also handle paths not ending with /
  • 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.

Are the images added immediately to the Gridview if you add a folder?

Yep.

davidre updated this revision to Diff 51206.Fri, Feb 8, 6:31 PM
  • Show Tooltip with the full path

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?

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.Fri, Feb 8, 7:36 PM
wallpapers/image/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

davidre updated this revision to Diff 51218.Fri, Feb 8, 8:34 PM
  • Use highlightInFileManager instead of KRun
davidre marked an inline comment as done.Fri, Feb 8, 8:36 PM
davidre added inline comments.
wallpapers/image/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.

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

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.Fri, Feb 8, 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.Sat, Feb 9, 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.Sat, Feb 9, 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.EditedSat, Feb 9, 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.Sun, Feb 10, 8:10 PM
  • Use attached tooltip
davidre added a comment.EditedSun, Feb 10, 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.Sun, Feb 10, 8:20 PM
davidre updated this revision to Diff 51348.Sun, Feb 10, 8:21 PM
  • Change copypasted "Open Containing Folder" to "Open Folder"
ngraham added inline comments.Sun, Feb 10, 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.EditedSun, Feb 10, 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.Mon, Feb 11, 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.Mon, Feb 11, 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.Mon, Feb 11, 11:04 AM
  • Don't use token for removal and fix include guard
davidre added a comment.EditedTue, Feb 12, 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?