Review Request: Plasma D-Bus Interface: Setting wallpaper image

[Ivan Cukic]

> On 2009-10-06 19:05:00, Aaron Seigo wrote:
> > shouldn't the wallpaper export its own dbus interface, and the path to that dbus object be dependent on the containment id? e.g. something like Containments/1/Wallpaper? then instead of a plugin-specific hack, we could have per-plugin controls on the bus?
> 
> Ivan Cukic wrote:
>     The main reason for this is that it provides a quick, minimal change that provides the desired functionality (no changes in the libplasma, wallpaper plugins, ... just PlasmaApp).
>     
>     I agree that the d-bus objects for each containment/wallpaper would be a much cleaner solution*, but it would need a lot of work that nobody (including myself) doesn't seem to have the time to do now (as it seems).
>     
>     So, we have a lot ppl asking for this while we could remove the function once we get the more detailed one (if there is a need for that at all) since we don't need to probide d-bus API compatibility.
>     
>     * Although a bit overblown IMO.
> 
> Aaron Seigo wrote:
>     if nobody is willing to do the work, then the feature doesn't make it in. the people asking for the feature can come up with the resources to make it happen, as far as i'm concerned. if this patch is allowed in then people will start relying on it being there and we will not be able to remove it, even if a proper solution comes along. moreover, by giving people a half-way-done solution that takes away just enough of the annoyance/pain, there is even less motivation to do it properly. so we'll end up with a poor solution to a problem that actually has a clear solution but which people even less likely to implement than they are now.
> 
> Ivan Cukic wrote:
>     Idea: We have the arguments for creating wallpapers (like for all other plugins) - what about making wallpaper plugins to use those arguments (if they exist) and making the dbus function that uses those for setting the wallpaper parameters?
>     
>     benefits:
>      - still a very simple dbus interface (no need for registering multiple objects*)
>      - plugin independent
>     
>     drawbacks:
>      - the caller of the method would need to know what the plugin accepts as parameters
>     
>     * why I'm against the per-containment dbus objects - it is all powerful approach, but since we decided to go for the javascript we don't need the all powerful approach using dbus.
> 
> Aaron Seigo wrote:
>     that would imply deleting and creating a new wallpaper object, and as you note it wouldn't be perfect.
>     
>     it's really not a big deal to simply have the Image wallpaper plugin export a small dbus interface and register it under the "right" path on the bus. nothing in Containment is needed to do that and your patch already adds the "list containments of type Foo by ID" so this information would all be available.
>     
>     iow, you don't need to create a Containment DBUS object for this, just create the Image wallpaper plugin one and put it in the right path in DBUS (/Containments/$ID/Wallpaper)
> 
> Ivan Cukic wrote:
>     OK. But there still are some questions :)
>     
>     - How to handle switching the wallpaper plugins? (If there is no Containment object in D-Bus)
>     - Wallpaper can not access its containment's ID to set the path
>     
>     
>     
>
> 
> Aaron Seigo wrote:
>     "- How to handle switching the wallpaper plugins? (If there is no Containment object in D-Bus)"
>     
>     your current patch doesn't address that either, so not an issue for this feature.
>     
>     "- Wallpaper can not access its containment's ID to set the path"
>     
>     a solution would be to have Containment set the D-Bus path for the Wallpaper. in fact, Containment could export the Wallpaper object using QDBusConnection::registerObject on it; it could be limited to properties or perhaps signals/slots marked as Q_SCRIPTABLE using QDBusConnection::RegisterOptions. that would allow Wallpaper plugins to simply provide a set of properties and/or scriptable methods and voila they get a DBus interface based on that for free.

"your current patch doesn't address that either, so not an issue for this feature."

It does, it switches automatically from any plugin to the image plugin.

For the rest, ok


- Ivan


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


On 2009-10-06 11:28:00, Ivan Cukic wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1798/
> -----------------------------------------------------------
> 
> (Updated 2009-10-06 11:28:00)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> There are many users who want the way to set the wallpaper via d-bus.
> 
> This enables them to do so, but only for the image wallpaper.
> 
> Since there is no mechanism in Plasma::Wallpaper (as far as I know) to set wallpaper options from outside of the wallpaper plugin, this patch relies on the structure of the Image wallpaper plugin and its configuration file format.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/desktop/shell/dbus/org.kde.plasma.App.xml 1031712 
>   /trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.h 1031712 
>   /trunk/KDE/kdebase/workspace/plasma/desktop/shell/plasmaapp.cpp 1031712 
> 
> Diff: http://reviewboard.kde.org/r/1798/diff
> 
> 
> Testing
> -------
> 
> Testing done - changing the wallpaper from one image to another, from another plugin to image plugin.
> 
> 
> Thanks,
> 
> Ivan
> 
>