Review Request: DBus-interface for changing wallpapers

[David Edmundson]

> On June 24, 2012, 1:41 a.m., David Edmundson wrote:
> > I think you've overcomplicated this:
> > 
> > Everything after currentContainment could be replaced with simply:
> > 
> > ---
> >     currentContainment->setWallpaper(name, mode);
> >     if (!path.isEmpty()) {
> >         currentContainment->wallpaper()->setUrls(KUrl(path));
> >     }
> > ---
> > 
> > if the name and mode haven't changed setWallpaper will do nothing.
> > if the plugin doesn't support setUrls it will do nothing, if it does - your API will work with it \o/.
> > Containment::setWallpaper appears to save when changing, same for setUrls on the plugin.
> > 
> > I tested the above and it saves and restores fine :)
> 
> Varun Herale wrote:
>     When the name is "image" and mode is "Slideshow", image with path doesn't seem to be added to the slideshow. So I wasn't sure if using setUrls for all cases was a good idea. Is it okay to do that ?
>     
>     As for the rest - It just didn't strike me to use setUrls after setWallpaper and that lead to over complication :P Learnt something. And will update the diff soon.

No problem, it was a good first draft. The point of reviews is to iteratively make things better.

I only found out from reading through the source of Containment, Wallpaper and Image. I think calling setUrls for all cases will be fine. There's a base implementation in the wallpaper that does nothing, and then the different wallpaper plugins override it.

If the wallpaper is "image" "Slideshow" and you call setUrls() it _should_ (from the code) add it. If not, it's a bug there, and should be fixed there.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105319/#review15052
-----------------------------------------------------------


On June 22, 2012, 2:36 p.m., Varun Herale wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105319/
> -----------------------------------------------------------
> 
> (Updated June 22, 2012, 2:36 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> This patch is for hosting a dbus-interface that can be used to load any installed wallpaper plugin onto current desktop containment. In case of default "image" plugin, the path to the image can also be sent which will change the wallpaper.  
> 
> 
> Diffs
> -----
> 
>   plasma/desktop/shell/dbus/org.kde.plasma.App.xml eefce32 
>   plasma/desktop/shell/plasmaapp.h 6ae0c89 
>   plasma/desktop/shell/plasmaapp.cpp 7abd8fc 
> 
> Diff: http://git.reviewboard.kde.org/r/105319/diff/
> 
> 
> Testing
> -------
> 
> Tested on different activities and made sure it works for per-virtual desktop containment.
> 
> Haven't tested on a system with multiple screens though, as I don't have access to one. Could someone please test for that ?
> 
> 
> Thanks,
> 
> Varun Herale
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120624/3e05acdf/attachment.html>