Allow single images to be excluded from the slideshow
ClosedPublic

Authored by davidre on Mar 11 2019, 4:05 PM.

Details

Summary

This allows to exlude single images from the slideshow by unchecking the little checkbox in the top right corner.
Feature or todo?: If a folder is removed I don't touch the unchecked Slides. So the list could get very big but if a formerly removed folder is
added again an excluded image will be excluded again.

Test Plan

  • Uncheck some images
  • They don't appear in the slideshow

Diff Detail

Repository
R120 Plasma Workspace
Branch
toggleSlides
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9571
Build 9589: arc lint + arc unit
davidre created this revision.Mar 11 2019, 4:05 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 11 2019, 4:05 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Mar 11 2019, 4:05 PM
davidre planned changes to this revision.Mar 11 2019, 4:08 PM
davidre edited the test plan for this revision. (Show Details)Mar 11 2019, 4:15 PM
davidre updated this revision to Diff 53672.Mar 11 2019, 4:15 PM
  • Remove debug statement
ngraham requested changes to this revision.Mar 11 2019, 8:54 PM
ngraham added a subscriber: ngraham.

This puts permanently disabled checkboxes on the wallpaper delegates that aren't used in the slideshow version, possibly related to a flood of warnings that gets printed to the console:

file:///usr/share/plasma/wallpapers/org.kde.slideshow/contents/ui/WallpaperDelegate.qml:123:13: Unable to assign [undefined] to bool
file:///usr/share/plasma/wallpapers/org.kde.slideshow/contents/ui/WallpaperDelegate.qml:123:13: Unable to assign [undefined] to bool
file:///usr/share/plasma/wallpapers/org.kde.slideshow/contents/ui/WallpaperDelegate.qml:123:13: Unable to assign [undefined] to bool

Let's not show the checkboxes at all unless we're in the slideshow view.


Also, this has nothing to do with your patch specifically, but seeing so many checkesd checkboxes in the slideshow's preview pane makes me realize just how much I want the Breeze style checkbox to actually look like a checkbox, with a checkmark instead of just a filled in roundrect.

This revision now requires changes to proceed.Mar 11 2019, 8:54 PM
davidre updated this revision to Diff 53696.Mar 11 2019, 10:58 PM

Sorry for that. Actually these warnings were the reason why the checkboxes were visible everywhere. Even if the checkboxes are not visible I got these warnings. In an attempt to get rid of them I tried enabled.

davidre updated this revision to Diff 53697.Mar 11 2019, 11:01 PM

Set checked to false if checkboxes are not visible

abetts added a subscriber: abetts.Mar 11 2019, 11:04 PM

How would the folders on the folder list on the left be removed as sources?

How would the folders on the folder list on the left be removed as sources?

In D18809 I changedthe ListItems to Kirigami SwipeListItems. This changed the remove icon from always visible to visible on hover

Also, this has nothing to do with your patch specifically, but seeing so many checkesd checkboxes in the slideshow's preview pane makes me realize just how much I want the Breeze style checkbox to actually look like a checkbox, with a checkmark instead of just a filled in roundrect.

Yeah also not a fan of that. I had the idea to give it maybe a small grey rectangle like the action icons as the empty checkboxes or the blue blob can be hard to spot. See the empty checkbox in the last row or the checkbox on Cascade in the screenshot.

Also, this has nothing to do with your patch specifically, but seeing so many checkesd checkboxes in the slideshow's preview pane makes me realize just how much I want the Breeze style checkbox to actually look like a checkbox, with a checkmark instead of just a filled in roundrect.

Yeah also not a fan of that. I had the idea to give it maybe a small grey rectangle like the action icons as the empty checkboxes or the blue blob can be hard to spot. See the empty checkbox in the last row or the checkbox on Cascade in the screenshot.

If the Breeze checkbox style is presenting problems, we should fix it in Breeze rather than here in downstream users. :)

Also, this has nothing to do with your patch specifically, but seeing so many checkesd checkboxes in the slideshow's preview pane makes me realize just how much I want the Breeze style checkbox to actually look like a checkbox, with a checkmark instead of just a filled in roundrect.

Yeah also not a fan of that. I had the idea to give it maybe a small grey rectangle like the action icons as the empty checkboxes or the blue blob can be hard to spot. See the empty checkbox in the last row or the checkbox on Cascade in the screenshot.

One thing that might work is the behavior that google photos uses. When you select an item, the item shrinks by about 20% and a blue selection box appears around the image. It just makes it more noticeable that the item has been selected.

One thing that might work is the behavior that google photos uses. When you select an item, the item shrinks by about 20% and a blue selection box appears around the image. It just makes it more noticeable that the item has been selected.

Since all items are selected by default here, I'm not sure that behavior would be the most appropriate.

One thing that might work is the behavior that google photos uses. When you select an item, the item shrinks by about 20% and a blue selection box appears around the image. It just makes it more noticeable that the item has been selected.

Since all items are selected by default here, I'm not sure that behavior would be the most appropriate.

Oh I thought it was a selection. Could the checkbox be located at a top corner sticking out a bit?

It is a selection, it's just that everything is selected by default. :) I think we should try to improve the checkbox's appearance rather than working around its deficiencies IMO.

One thing that might work is the behavior that google photos uses. When you select an item, the item shrinks by about 20% and a blue selection box appears around the image. It just makes it more noticeable that the item has been selected.

Since all items are selected by default here, I'm not sure that behavior would be the most appropriate.

Oh I thought it was a selection. Could the checkbox be located at a top corner sticking out a bit?

I tried to do this but the checkbox is cut off. If I move it out if the Rectangle I can't anchor against it because they are not silblings nor is it it's parent. Anchoring to thumbnail also doesn't work.
Anchoring to the whole delegate:

davidre updated this revision to Diff 53870.Mar 14 2019, 10:24 AM

Change email address

Anchoring to the whole delegate:

Yeah that doesn't look great with the Breeze style checkbox. Within the frame seems like the way to go, unless we want to take the time to redo Breeze checkboxes first. :)

filipf added a subscriber: filipf.Mar 15 2019, 12:13 AM

The checkboxes are right where they're supposed to be, as for their visibility: maybe it would be useful if we had a variable that would allow us to add a background behind a checkbox.

ngraham accepted this revision as: VDG.Mar 16 2019, 9:47 AM

UI looks good now. I've done some code review below:

wallpapers/image/image.cpp
623

Don't use Q_FOREACH in new code. See https://www.kdab.com/goodbye-q_foreach/

davidre updated this revision to Diff 54010.Mar 16 2019, 11:41 AM
  • Use range-based for

UI looks good now. I've done some code review below:

Thanks for the heads-up! I was orienting myself on the Code around.

ngraham accepted this revision.Mar 16 2019, 4:06 PM

LGTM now. Let's see what some of the Plasma people think. :)

This revision is now accepted and ready to land.Mar 16 2019, 4:06 PM

almost weekly ping

davidedmundson accepted this revision.Mar 30 2019, 4:50 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
wallpapers/image/imagepackage/contents/ui/config.qml
41

default should be []

wallpapers/image/slidemodel.cpp
57

toggled is a misleading term as toggling in either direction is toggled. You want to represent state.
The typical QAction term is "checked"

davidre updated this revision to Diff 55164.Apr 1 2019, 8:01 AM
  • Change default of cfg_UncheckedSlides to []
  • Change toggled to checked
davidre marked 2 inline comments as done.Apr 1 2019, 8:02 AM
This revision was automatically updated to reflect the committed changes.