[wallpapers/image] Add default XDG wallpaper locations for slideshow
ClosedPublic

Authored by ngraham on Tue, Jan 7, 12:28 AM.

Details

Summary

BUG: 415461
FIXED-IN: 5.18.0
Depends on D26510

Right now when you first change the wallpaper plugin to slideshow, no locations are
pre-configured, so you have to add one yourself. This patch adds a
preferred:wallpaperlocations token as the default that gets replaced in the user's
config file at runtime with the XDG wallpaper locations.

Test Plan

Log in as new user > right-click desktop > configure > change to slideshow wallpaper
See that`/usr/share/wallpapers` is already in the list of wallpaper locations; how nice

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.
ngraham created this revision.Tue, Jan 7, 12:28 AM
Restricted Application added a project: Plasma. · View Herald TranscriptTue, Jan 7, 12:28 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Tue, Jan 7, 12:28 AM
ndavis accepted this revision.Tue, Jan 7, 12:49 AM
ndavis added a subscriber: ndavis.

+1

This revision is now accepted and ready to land.Tue, Jan 7, 12:49 AM

Wondering if this is something that should be set in CMake or if it's so universal that it's okay to have it hardcoded in the file like this?

davidedmundson requested changes to this revision.Tue, Jan 7, 9:10 AM
davidedmundson added a subscriber: davidedmundson.

It's not ok for the reasons you identified.

This revision now requires changes to proceed.Tue, Jan 7, 9:10 AM
broulik added a subscriber: broulik.Tue, Jan 7, 9:29 AM

Perhaps we could leverage some code from Task Manager to have a fake preferred://wallpaperlocation URL or something like that?

davidre added a subscriber: davidre.Tue, Jan 7, 1:54 PM

Couldn't we just use wrap the folders from the single image case in a code tag?
<code>QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, QStringLiteral("wallpapers/"), QStandardPaths::LocateDirectory)</code>

I don't think so, as we use a runtime evaluation of kcfg files within plasma. Code tags won't work there.

Perhaps we could leverage some code from Task Manager to have a fake preferred://wallpaperlocation URL or something like that?

That might be able to work. @davidedmundson, could this be a path forward, or did you have something else in mind?

ngraham planned changes to this revision.Tue, Jan 7, 10:48 PM
ngraham updated this revision to Diff 73023.Tue, Jan 7, 11:55 PM

Add a preferred://wallpaperlocations token and replace it with the real values

ngraham edited the summary of this revision. (Show Details)Tue, Jan 7, 11:59 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)
davidre added inline comments.Wed, Jan 8, 9:41 AM
wallpapers/image/image.cpp
359–367

I know this was there before but didn't work before as intended because of the wrong path but imo now that there's a default in kcfg if the path is empty it should stay empty and not magically go back to the default if the user removes every path.

ngraham updated this revision to Diff 73067.Wed, Jan 8, 2:53 PM
ngraham marked an inline comment as done.

Don't auto-append paths when the paths list is empty

ngraham retitled this revision from Add /usr/share/wallpapers as a default wallpaper slideshow location to [wallpapers/image] Add default XDG wallpaper locations for slideshow.Wed, Jan 8, 7:04 PM

Dependent patches have landed, is this okay now?

davidedmundson requested changes to this revision.Tue, Jan 14, 9:03 PM
davidedmundson added inline comments.
wallpapers/image/image.cpp
361

there's two mistakes that cancel kinda each other out here.

typically, this should be qAsConst(m_slidePaths)
m_slidePaths here detatches and does a full deep-copy

https://www.dvratil.cz/2015/06/qt-containers-and-c11-range-based-loops/

BUT:

you modify m_slidePaths whilst you are iterating through m_slidePaths. If we hadn't accidentally detached this would be very crashy dangerous code.

Feel free to claim it was deliberate because you are a genius.


However, rather than a deep copy ideally we would just force a shallow copy i.e

const QStringList preprocssedPaths = m_slidePaths;
for (const QString &path: preprocssedPaths) {
...
}

best of all worlds

This revision now requires changes to proceed.Tue, Jan 14, 9:03 PM
ngraham updated this revision to Diff 73571.Tue, Jan 14, 9:49 PM
ngraham marked an inline comment as done.

Shallow copy instead of deep copy

ngraham added inline comments.Tue, Jan 14, 9:50 PM
wallpapers/image/image.cpp
361

haha, in fact it was deliberate, but I'm hardly a genius, and your suggestion makes more sense. I'll do that.

ngraham marked an inline comment as done.Tue, Jan 14, 9:50 PM
davidedmundson accepted this revision.Tue, Jan 14, 9:50 PM
This revision is now accepted and ready to land.Tue, Jan 14, 9:50 PM

Thanks David!

This revision was automatically updated to reflect the committed changes.