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

[David Faure]

> On 2010-05-29 11:18:29, Christoph Feck wrote:
> > 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.

Yes, you could get a 48x48 pixmap when asking for a 50x50 one. But if you then reserve space for 50x50 (like the patch does), then things should still be fine, the pixmap will fit.

In the reverse case, you risk truncation, but this risk was there before I guess? I mean for any code not calling actualSize() first, and just asking for a pixmap of a given size (and getting a bigger one, with a Fixed icon theme).

OK, to be safe, maybe we can enable the fast-path only with non-fixed icon-themes; but I suspect these might already create quite some trouble, since they are rarely tested...


- David


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


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
> 
>