Review Request: Speedup KIconEngine::actualSize by not loading the actual icon.

[Christoph Feck]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4052/#review5909
-----------------------------------------------------------


While I am in total favor of this patch, we should think about what to do with icon themes that announce fixed size icons, instead of freely scalable ones.

I am not talking about SVG vs. PNG, but the fact that you can request a 50x50 pixel icon from Oxygen, and the request will be granted by either upscaling or downscaling a near-sized icon. For other icon themes, a different (fixed) size (e.g. 48x48) could indeed be returned. KIconLoader carefully avoids scaling icons when the theme author has specified a fixed size theme.

For details, refer to the "Type" entry in the freedesktop.org icon theme specification.

- Christoph


On 2010-05-19 23:41:07, David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4052/
> -----------------------------------------------------------
> 
> (Updated 2010-05-19 23:41:07)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> KIconEngine::actualSize was loading the icon (from the cache, fortunately, but that still takes time), just to return its size. However the size is mostly mandated by the caller of KIconLoader::loadIcon...
> 
> Looking at the (complex) KIconLoader code I think there are indeed a few cases where the icon wouldn't have the requested size, but the question is whether it's a big deal for the users of actualSize()... In the case of file manager views, the user is KFileItemDelegate::Private::decorationSizeHint which is surely happier to reserve the same space for every icon, even if a few ones are only available as somewhat different sizes. I just wonder 1) how to test the case where we get a different size than the requested one, and 2) if this faster (but only correct 99% of the time, I suppose) actualSize() would break anything.
> 
> 
> This addresses bug 237668.
>     https://bugs.kde.org/show_bug.cgi?id=237668
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kio/kio/kfileitemdelegate.cpp 1128493 
> 
> Diff: http://reviewboard.kde.org/r/4052/diff
> 
> 
> Testing
> -------
> 
> konqueror /usr/bin
> (after the kio fix r1128475)
> This is much faster now than before. And to make sure actualSize() wasn't returning wrong results, I added asserts in pixmap() and paint().
> 
> Plus existing unittests and adding a rudimentary unit-test for actualSize.
> 
> 
> Thanks,
> 
> David
> 
>