KDE/kdebase/workspace/plasma/containments/desktop

[Jason Stubbs]

On Sunday 06 January 2008 04:39:28 JST, Aaron J. Seigo wrote:
> On Saturday 05 January 2008, Jason Stubbs wrote:
> > SVN commit 757546 by jstubbs:
> >
> > Set a default wallpaper after constraints have been set so that geometry
> > can be used to find the correct sized wallpaper. This also prevents
> > updateBackground() from being called twice during startup.
> >
> > +    m_wallpaperPath = cg.readEntry("wallpaper", "");
>
> minor nit: QString() should be favoured over "" in code like this.

To be explicit about the type wanted? Right, noted.

> > +        // Only set the wallpaper if constraints have been loaded
> > +        if (screen() != -1) {
> > +            updateBackground();
> > +        }
>
> this will break for desktop containments that are not assigned to a screen
> (e.g. alternate containments). it's an incorrect assumption that all
> desktop containments will have a screen associated with them and will
> result in such containments not repainting their background when they
> should.

This is an oversight on my part. Is there any such uses now that I can check 
behaviour with in the future?

> i also wonder why you did this for just static and not slideshow
> backgrounds?

The only time it's really important is if no wallpaper has been set yet. The 
geometry has not been set yet when screen() == -1 so the setting of a default 
background would be based on a wrong size without the check.

> in any case, perhaps a better fix might be add a parameter to reloadConfig
> that tells it whether or not to also call updateBackground. this would be
> set to false or whatever when called from init() but true when called
> elsewhere.

This works better and is clearer too. Patch attached which handles the 
slideshow case too.

> p.s. this is really on the cusp or even beyond the cusp of the size of
> patches that should be peer reviewed; i assume you had this peer reviewed
> by someone?

Not peer-reviewed, no... The metric I've been using is something to the effect 
of "does it touch more than one class? does it change any interfaces?" Will 
adding "does it add/remove code paths?" to that for 4.0.x changes be enough?

-- 
Jason Stubbs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: skip-updates-flag.patch
Type: text/x-diff
Size: 2195 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080106/03c99515/attachment-0001.bin