[PATCH] Using KIcon for icons with overlay

[Gustavo Pichorim Boiko]

A Wednesday 29 August 2007 14:30:48, Aaron J. Seigo escreveu:
> On Wednesday 29 August 2007, Gustavo Pichorim Boiko wrote:
> > Should I understand this silence as an approval for my API suggestion?
>
> sorry, i was taking a semi-vacation when this was posted to the list.
>
> first, i think there is a technical problem with the patch in that it call
> mIconCache->discard() when setEffect is called. i think this is hardly what
> is desired as it will penalize each and every kde app and really invalidate
> the point and purpose of mIconCache. the effect modifiers need to be added
> to the cache key, which then insures the iconcache doesn't need to get
> discarded.

You are right. I thought that the cache was internal to the KIconLoader 
instance, sorry. Adding the effect in the cache key is the right way to go.

> beyond that bug .... i'm pretty torn with this patch since it really
> doesn't seem to be the "right place" for this code and seems to fix an
> issue for kopete the way kopete needs it rather than in a general way.
> specifically, it duplicates the purpose of KIconEffect and adds even more
> overhead to the icon loader.

Well, I was trying to cleanup all this overlay stuff in kopete, but if we end 
up deciding this is not something other apps might need, I can live with 
that.
While I agree that KIconLoader might not be the right place for this to go 
into, I'm not sure where would be the right place for this. Might be in 
KIconEffect, but I'm not completelly convinced KIconEffect is a better place 
than KIconLoader itself.

>
> looking at the code, it seems this ought to go, at least in part, into
> KIconEffect. in fact, with a quick glance all KIconEffect is missing is a
> setEffect(int group, int state, KIconEffect::Effects effect, float value,
> const QColor &color) method.

I would like it to be like a "preprocessing" effect (or a postprocessing one 
as my implementation would say), that is applied before the effects for the 
states are applied, I don't know if this would be possible by this interface.

> then kopete could do sth like:
>
> iconLoader->iconEffect()->setEffect(..)

If the idea is not to have an extra KIconLoader, I don't like this approach, 
as it is easier for applications to make errors (one can forget to reset the 
effect and get all subsequent icons  to have the effect applied). 
As you say later, the ideal place for this would be KIcon, but then I don't 
know how could we implement that.

> and it should be applied to the icon. the other half of this would be to
> allow one to inhibit the application of this effect to the icons loaded in
> drawOverlay. perhaps a overlaysUseEffects and setOverlaysUserEffects? it
> would default to true, of course, and then the call to loadIcon in
> Private::drawOverlays would need to be modified in some way to obey that
> setting.

I guess now you understand why I did opt for a simpler interface, the other 
ideas I had would imply in more changes, and at this point I'm not sure it is 
a good idea.

> i really wish these things could be done in KIcon itself but the
> QIconEngine related API is far too limiting =/

Indeed.

> looking at it a bit closer, does your patch even work? i ask because
> drawOverlays calls loadIcon, which seems to apply the effect no matter what
> so it would also get applied to the loaded overlay emblems, no?

Oh, ops, you are right (I didn't notice that when I tested the patch because 
the overlays I checked were black and white, so hard to see when they get 
effects applied or not), but ok, the patch does not work as expected :(

So I'm open for suggestions on how this could be handle. 

Cheers
-- 
Gustavo Pichorim Boiko
-----------------------------------
KDE Developer      www.kde.org
Mandriva Labs      www.mandriva.com