Consideration requested for merging of kdelibs-action branch

[Hamish Rodda]

On Tuesday 14 March 2006 08:33, David Faure wrote:
> Hi Hamish,
>
> great work on the branch, I'm convinced that it should be merged in.

Thanks, that means a lot :)

> Just one comment on a possible future change:
>
> On Sunday 12 March 2006 16:37, Hamish Rodda wrote:
> > * Fix the issue with icon loading:
> >
> > "The icon name we should be able to solve nicely through QIconEngine.
> > Instead of passing the QString iconName around we can pass around a QIcon
> > that was constructed with an internal KIconEngine. Then we call addFile
> > on it so that the icon engine can remember it. That's it, from then on we
> > can pass the QIcon around and whenever someone needs the actual pixmap he
> > can request it through QIcon or (more likely) call QIcon's paint method
> > directly. That way we can also implement svg icons."
>
> At the KAction API level, I am rather against this change. Being able to
> pass a string with the icon name when constructing the action, means that
> the app code is completely independent from the underlying technologies
> used to load the icon. new KAction( ... "reload" ... ) will always be
> valid, whichever image format is used, whichever icon theme mechanism is in
> place, whichever Qt or KDE class has to be used to load the icon, etc.
>
> On the other hand, having to write
>   QIcon reloadIcon( KIconEngine::self() );   // or KGlobal::iconEngine() or
> something reloadIcon.addFile( "reload" ); // or worse,
> KGlobal::dirs()->...("reload") .... new KAction( ... reloadIcon ... )
> would be a major PITA, obviously. Number of lines gets multiplied by 3,
> with lots of technology-dependent code added.
>
> OK this is obvious enough so that it's likely I just misunderstood your
> paragraph; but let me send this mail to make sure it's the case, and that
> passing anything else than a QString at the API level isn't even considered
> :)

Sure, my frustration with the current API is mostly that the icon size 
shouldn't have to be specified, but currently does (KAction guesses at the 
moment, which is bad when expectedSize() > guessedSize()).  So I like Simon's 
idea of creating a QIcon (inside KAction) from the kde name string, which 
will take care of loading the correct size icon.  I envisage that 
setIconName() will be a convenience method.

The only other messy part of this is that it will be easy to switch the order 
of the action text and the icon name in the KAction constructor.  However 
this is fairly noticeable when it happens, and probably not worth replacing 
all "kde_icon_name" strings with KIcon*::fromName("kde_icon_name" ) or 
similar, so I'm not too concerned about it.

I'll merge the branch either today or tomorrow, depending on final 
preparations + technical reasons.

Cheers,
Hamish.
-------------- 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/20060314/dbb663d7/attachment.sig>