D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Nathaniel Graham]

ngraham updated this revision to Diff 73067.
ngraham marked an inline comment as done.
ngraham added a comment.


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

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26483?vs=73023=73067

BRANCH
  add-a-default-wallpaper-slideshow-location (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26483

AFFECTED FILES
  wallpapers/image/image.cpp
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> image.cpp:366
>  if (m_slidePaths.isEmpty()) {
> -m_slidePaths << 
> QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, 
> QStringLiteral("share/wallpapers"), QStandardPaths::LocateDirectory);
> +m_slidePaths << defaultWallpaperPaths;
> +} else {

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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Nathaniel Graham]

ngraham updated this revision to Diff 73023.
ngraham added a comment.


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

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26483?vs=72941=73023

BRANCH
  add-a-default-wallpaper-slideshow-location (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26483

AFFECTED FILES
  wallpapers/image/image.cpp
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[David Edmundson]

davidedmundson added a comment.


  Sure

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Nathaniel Graham]

ngraham added a comment.


  In D26483#589401 , @broulik wrote:
  
  > 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?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[David Redondo]

davidre added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidre, broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: broulik, davidedmundson, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  It's not ok for the reasons you identified.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  add-a-default-wallpaper-slideshow-location (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis
Cc: ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.
This revision is now accepted and ready to land.


  +1

REPOSITORY
  R120 Plasma Workspace

BRANCH
  add-a-default-wallpaper-slideshow-location (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26483

To: ngraham, #plasma, #vdg, ndavis
Cc: ndavis, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26483: Add /usr/share/wallpapers as a default wallpaper slideshow location

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Plasma, VDG.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  BUG: 415461
  FIXED-IN: 5.18.0
  
  Right now when you first change the wallpaper plugin to slideshow, no 
locations are
  pre-configured, so you have to add one yourself. Considering that 
`/usr/share/wallpapers`
  is the default location for image wallpapers, this patch adds it as a default 
location
  for slideshow wallpapers too.

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  add-a-default-wallpaper-slideshow-location (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26483

AFFECTED FILES
  wallpapers/image/slideshowpackage/contents/config/main.xml

To: ngraham, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart