[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Speedup KIconEngine::actualSize by not loading the
From:       "Peter Penz" <peter.penz () gmx ! at>
Date:       2010-05-29 10:28:11
Message-ID: 20100529102811.19654.26726 () localhost
[Download RAW message or body]



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


[prev in list] [next in list] [prev in thread] [next in thread]