Review Request: Color scheme aware plasma theming

[Andrew Lake]

> On 2008-02-18 11:45:37, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/svg.cpp, line 150
> > <http://mattr.info/r/154/diff/1/#file352line150>
> >
> >     i wonder if this shouldn't be cached into a bool read once on setting the svg.

Added bool applyColors, set it and connect to the KGlobalsettings signal in setImagePath.


> On 2008-02-18 11:45:37, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/theme.cpp, line 135
> > <http://mattr.info/r/154/diff/1/#file353line135>
> >
> >     this signal is already emitted when the svg path changes, so all it really does here is create a second repaint.
> >     
> >     i'd suggest instead to not change anything at all in Plasma::Theme and instead put this in Plasma::Svg:
> >     
> >     if the colourization hint is found in the Svg, then connect to this KGlobalSettings signal. in the method it is connected to (colorsChanged?) and in that method do something like:
> >     
> >     d->removeFromCache();
> >     d->setImagePath(d->themed ? d->themePath : d->path);
> >     d->eraseRenderer();
> >     emit repaintNeeded();
> >     
> >     so much like what is in themeChanged(), but specific to the colour change needs.
> >

Agreed. I added a new slot called colorsChanged() as follows:

void Svg::colorsChanged() 
{
    if (!d->applyColors) {
        return;
    }

    d->removeFromCache();
    d->setImagePath(d->themed ? d->themePath : d->path, this);
    d->eraseRenderer();
    emit repaintNeeded();
}

What ends up happening on a color change is that the Svg disappears.  (The normal, non-color-aware svgs are unaffected.)  This happens no matter if I hardcode the 1st argument for d->setImagePath to d->themed or d->themePath.

I tried removing the d-> setImagePath line since the Svg file path hasn't really changed. This results in only an element or two of the Svg getting repainted (which is what originally lead me to use the crude QPixmapCache::clear() hack).  I'll keep fiddling but if anyone has any ideas, I'm all ears.

Thanks much for taking the time to review this.


> On 2008-02-18 11:45:37, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/svg.cpp, lines 360-362
> > <http://mattr.info/r/154/diff/1/#file352line360>
> >
> >     this is going to recreate every svg every time an event happens regardless of whether it needs to or not. obviously not great for performance. there are other ways to accomplish what you are trying to do (see below)

Agreed.


> On 2008-02-18 11:45:37, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/svg.cpp, line 368
> > <http://mattr.info/r/154/diff/1/#file352line368>
> >
> >     this clears the *entire* pixmap cache for the whole application. four lines above we remove this svg from the cache, so this line should be unnecessary in the first place.

Yeah, I was hoping someone would have a better idea during this review process.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://mattr.info/r/154/#review146
-----------------------------------------------------------


On 2008-02-17 18:13:18, Andrew Lake wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://mattr.info/r/154/
> -----------------------------------------------------------
> 
> (Updated 2008-02-17 18:13:18)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch allows plasma themes to react to changes in the current color scheme. Note that theme authors must specifically add the "hint-apply-color-scheme" element to the svgs for the color to be applied.  This patch does not affect svgs that do not have this hint.  (Notice in the screenshots that the clock and the default theme are unaffected.) Theme authors may provide a "colors" file with their theme or omit it to use the current system colors (plasma already provided for this).
> 
> The screenshots show a proof of concept theme.  Couldn't find a way to attach it here.  Let me know what's the best way to make it available in case anyone wants to test it.
> 
> A few notes:
> - I picked the colorize effect from KIconEffect since it allows the theme author to define where, and how much, colors are applied (preserves shadows, etc.).  If this effect is available somewhere else I couldn't find it.
> - Some things I added to just get this to work (the QPixmapCache::clear() and the extra "emit changed()").
> 
> Hope this helps and once again that's so much for this cool stuff.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/libs/plasma/svg.cpp
>   /trunk/KDE/kdebase/workspace/libs/plasma/theme.cpp
> 
> Diff: http://mattr.info/r/154/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> plasmawonton
>   http://mattr.info/r/154/s/10/
> plasmaobsidian
>   http://mattr.info/r/154/s/11/
> plasmanorway
>   http://mattr.info/r/154/s/12/
> plasmadefault
>   http://mattr.info/r/154/s/13/
> 
> 
> Thanks,
> 
> Andrew
> 
>