D19687: Allow single images to be excluded from the slideshow

This revision was automatically updated to reflect the committed changes. Closed by commit R120:b1ae890a6ee6: Allow single images to be excluded from the slideshow (authored by davidre, committed by ngraham). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 55164. davidre added a comment. - Change default of cfg_UncheckedSlides to [] - Change toggled to checked REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19687?vs=54010=55164 BRANCH toggleSlides REVISION

D19687: Allow single images to be excluded from the slideshow

davidedmundson accepted this revision. davidedmundson added inline comments. INLINE COMMENTS > config.qml:41 > property int cfg_SlideInterval: 0 > +property var cfg_UncheckedSlides: "" > default should be [] > slidemodel.cpp:57 > + QHash roleNames =

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. almost weekly ping REPOSITORY R120 Plasma Workspace BRANCH toggleSlides REVISION DETAIL https://phabricator.kde.org/D19687 To: davidre, #plasma, #vdg, ngraham Cc: filipf, abetts, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai,

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. Ping! REPOSITORY R120 Plasma Workspace BRANCH toggleSlides REVISION DETAIL https://phabricator.kde.org/D19687 To: davidre, #plasma, #vdg, ngraham Cc: filipf, abetts, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,

D19687: Allow single images to be excluded from the slideshow

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. LGTM now. Let's see what some of the #plasma people think. :) REPOSITORY R120 Plasma Workspace BRANCH toggleSlides REVISION DETAIL

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. In D19687#431950 , @ngraham wrote: > UI looks good now. I've done some code review below: Thanks for the heads-up! I was orienting myself on the Code around. REPOSITORY R120 Plasma Workspace REVISION DETAIL

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 54010. davidre added a comment. - Use range-based for REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19687?vs=53870=54010 BRANCH toggleSlides REVISION DETAIL https://phabricator.kde.org/D19687 AFFECTED

D19687: Allow single images to be excluded from the slideshow

ngraham accepted this revision as: VDG. ngraham added a comment. UI looks good now. I've done some code review below: INLINE COMMENTS > image.cpp:623 > m_slideshowBackgrounds = paths; > +Q_FOREACH(const QString , m_uncheckedSlides) { > +

D19687: Allow single images to be excluded from the slideshow

filipf added a comment. 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. F6692107: image.png REPOSITORY R120

D19687: Allow single images to be excluded from the slideshow

ngraham added a comment. In D19687#429746 , @davidre wrote: > Anchoring to the whole delegate: F6686885: grafik.png Yeah that doesn't look great with the Breeze style checkbox. Within the frame

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 53870. davidre added a comment. Change email address REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19687?vs=53697=53870 BRANCH toggleSlides REVISION DETAIL https://phabricator.kde.org/D19687 AFFECTED

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. In D19687#429710 , @abetts wrote: > In D19687#429709 , @ngraham wrote: > > > In D19687#429700 , @abetts wrote: > > > > > One

D19687: Allow single images to be excluded from the slideshow

ngraham added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL

D19687: Allow single images to be excluded from the slideshow

abetts added a comment. In D19687#429709 , @ngraham wrote: > In D19687#429700 , @abetts wrote: > > > One thing that might work is the behavior that google photos uses. When you select an item, the

D19687: Allow single images to be excluded from the slideshow

ngraham added a comment. In D19687#429700 , @abetts wrote: > 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

D19687: Allow single images to be excluded from the slideshow

abetts added a comment. In D19687#429373 , @davidre wrote: > In D19687#429332 , @ngraham wrote: > > > Also, this has nothing to do with your patch specifically, but seeing so many checkesd

D19687: Allow single images to be excluded from the slideshow

ngraham added a comment. In D19687#429373 , @davidre wrote: > In D19687#429332 , @ngraham wrote: > > > Also, this has nothing to do with your patch specifically, but seeing so many checkesd

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. In D19687#429332 , @ngraham wrote: > 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

D19687: Allow single images to be excluded from the slideshow

davidre added a comment. In D19687#429367 , @abetts wrote: > How would the folders on the folder list on the left be removed as sources? In D18809 I changedthe ListItems to Kirigami SwipeListItems.

D19687: Allow single images to be excluded from the slideshow

abetts added a comment. How would the folders on the folder list on the left be removed as sources? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D19687 To: davidre, #plasma, #vdg, ngraham Cc: abetts, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel,

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 53697. davidre added a comment. Set checked to false if checkboxes are not visible REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19687?vs=53696=53697 BRANCH toggleSlides REVISION DETAIL

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 53696. davidre added a comment. 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.

D19687: Allow single images to be excluded from the slideshow

ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. 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

D19687: Allow single images to be excluded from the slideshow

davidre updated this revision to Diff 53672. davidre added a comment. - Remove debug statement REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19687?vs=53671=53672 BRANCH toggleSlides REVISION DETAIL https://phabricator.kde.org/D19687

D19687: Allow single images to be excluded from the slideshow

davidre created this revision. davidre added reviewers: Plasma, VDG. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidre requested review of this revision. REVISION SUMMARY This allows to exlude single images from the slideshow by unchecking the little checkbox in