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

[David Faure]

On Tuesday 02 January 2007 20:21, Aaron J. Seigo wrote:
> SVN commit 619104 by aseigo:
> 
> port to api changes
>
> -        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? 

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.

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.

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.

The first solution that comes to mind, then, is to pass the iconloader as parameter to KMMainView, then your 
kdebase change will indeed have an effect on the icon loading.
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...

The instance/iconloader split makes little sense because the settings inside an instance define a 
unique iconloader, so I don't like having to deal with both everywhere in code.
The above thought is what led to my earlier k-c-d post about how to keep the kinstance<->kiconloader
association, but this kdeprint example shows that it's not exactly true. With e.g. addAppDirs(), one can
define two iconloaders with different settings, even though they are both based on the same 
kinstance-provided settings (appName + KStandardDirs).
However that's probably rarely wanted; the point of this kde3 code was exactly the opposite:
to make sure that any use of this instance's iconloader would use the additional app directory.

Anyway, I'm rambling but I'm not coming to a suggested solution. I like KIcon("foo"), but
I see now how its complete lack of context makes it hard to automatically use the right
iconloader. So I guess it will have to be an explicit icon loader everywhere... Less magic, 
more things to remember typing to avoid bugs - not sure this is going into the right direction...

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

David.

>  M  +3 -1      kdeprint_part/printpart.cpp  
>  M  +2 -0      kdeprint_part/printpart.h  
> 
> 
> --- trunk/KDE/kdebase/kdeprint/kdeprint_part/printpart.cpp #619103:619104
> @@ -39,7 +39,8 @@
>  : KParts::ReadOnlyPart(parent)
>  {
>  	setInstance(PrintPartFactory::instance());
> -        instance()->iconLoader()->addAppDir("kdeprint");
> +        m_iconLoader = new KIconLoader(instance()->instanceName(), instance()->dirs());
> +        m_iconLoader->addAppDir("kdeprint");
>  	m_extension = new PrintPartExtension(this);
>  
>  	m_view = new KMMainView(parentWidget, actionCollection());
> @@ -53,6 +54,7 @@
>  
>  PrintPart::~PrintPart()
>  {
> +	delete m_iconLoader;
>  }
>  
>  KAboutData *PrintPart::createAboutData()
> --- trunk/KDE/kdebase/kdeprint/kdeprint_part/printpart.h #619103:619104
> @@ -26,6 +26,7 @@
>  class PrintPartExtension;
>  class KMMainView;
>  class KAboutData;
> +class KIconLoader;
>  
>  class PrintPart : public KParts::ReadOnlyPart
>  {
> @@ -45,6 +46,7 @@
>  private:
>  	PrintPartExtension	*m_extension;
>  	KMMainView		*m_view;
> +	KIconLoader		*m_iconLoader;
>  };
>  
>  class PrintPartExtension : public KParts::BrowserExtension

-- 
David Faure, faure at kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).