kiconloader api change (Re: KDE/kdebase/kdeprint)
Aaron J. Seigo
aseigo at kde.org
Fri Jan 5 18:32:58 GMT 2007
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,
> 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:
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
Size: 189 bytes
Desc: not available
More information about the kde-core-devel