kiconloader api change (Re: KDE/kdebase/kdeprint)

[Aaron J. Seigo]

On Friday 05 January 2007 8:04, you wrote:
> > -        instance()->iconLoader()->addAppDir("kdeprint");
> > +        m_iconLoader = new KIconLoader(instance()->instanceName(),
> > instance()->dirs()); +        m_iconLoader->addAppDir("kdeprint");
>
> Yes, it compiles. But does it work?
> Where is m_iconLoader actually used?

wherever loadIcon, etc is used. so yes, this breaks with KIcon.

> In kde3, here's how it worked:
>  the instance's actioncollection was modified with addAppDir,
>  the actioncollection (passed to KMMainView) knew the associated instance,
>  and KAction( ... "iconname" ) called iconSet( KIcon::Small, 0, instance )
> which used that instance's iconloader to load the icon. This made the
> instance()->iconLoader()->addAppDir actually have an effect on the icon
> loading within kaction.

are there cases outside of kparts and plugins where this also impacts?

> Now in kde4 and with your change, KIcon("iconname") is used, which uses the
> default iconloader unless set, so the kdeprint icons won't be found.

well, this was true before my change as well, wasn't it? e.g. it's a 
limitation of KIcon more than anything else.

> This is buggy at two levels then: in the kdelibs/kdeprint port to KIcon,

right...

> and in this kdebase iconloader-related change.
>
> kdelibs/kdeprint/kmmainview.cpp:226:  action->setIcon( KIcon(
> "kdeprint_addprinter" ) ); should have been action->setIcon( KIcon(
> "kdeprint_addprinter", iconLoader ) ); and that iconLoader should be passed
> as parameter, or somehow deduced from the instance as it used to be.

yes; it seems to be a common idiom to include a private iconloader with parts, 
plugins, etc... and the same general 6-8 lines of code tend to be written to 
accomplish this. i started toying with the idea of a KParts::Instance which 
agregates an iconLoader into a KInstance to eliminate this duplication, 
standardize how the icon loader is created and delivered ... perhaps that 
could used as a solution here as well.

> But how much similar code got broken similarly? ...  Any plugin or part has
> an instance with a different name than the app name, and its actions' icons
> were automatically loaded using those settings... I think that somehow we
> should keep this or something similar, or at least make the transition
> easier than KIcon("foo", myIconLoader) everywhere. I can understanding the
> fight against too much magic, but when it means that everything breaks,
> then the fight isn't conducted properly...

agreed. i kept a small catalog of concrete places this affected things 
adversely while porting:

	khtml
	khtml's java
	kdevelop plugins
	kopete plugins?
	koffice parts

there are probably others but these are the ones that i noticed due to extra 
work as i went along.

> PS: let's at least add a KIconLoader(KInstance *) constructor

=)

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

Full time KDE developer sponsored by Trolltech (http://www.trolltech.com)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070105/5bda1a72/attachment.sig>