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

[Peter Penz]

> On 2010-05-19 23:45:16, Fredrik Höglund wrote:
> > KFileItemDelegate used to use the nominal size (which is available in the style options passed to sizeHint()), but unfortunately it had to be changed to use the real size.
> > 
> > I don't remember the details, but the actual size can deviate quite a bit from the nominal size when previews are enabled.
> > 
> > sizeHint() is also not the only place where the actual size is used, it's also used during painting to center the icon in the item rect.
> 
> Peter Penz wrote:
>     @David: I did some tests and Fredrik is right, it has some sideeffects when turning on image previews. However I think the actual size is not required for the time consuming path (only for the painting) and think this can be optimized in KFileItemDelegate. If it is OK for you, I'll have a look on this during the next week.

After some further investigations, debugging and e-mail conversations, we came to the conclusion that diff r1 should be applied. 

Bypassing the performance bottleneck in KFileItemDelegate like done in diff r2 has side effects, as it does not work with icons that have a custom size (e. g. like when previews are turned on in Dolphin/Konqueror). This is because QIcon completely hides whether the icon engine is used or the size of a custom icon is returned when calling QIcon::actualSize(). But in the case if the icon engine is used, it is assured that QIconEngine::actualSize() is not called for custom icons, hence the patch diff r1 works well.

I've added some Q_ASSERTs and verified diff r1 with several applications during the last days and it works well.


- Peter


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


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