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

List:       kde-commits
Subject:    Re: branches/KDE/4.2/kdelibs/kio/kio
From:       "Peter Penz" <peter.penz () gmx ! at>
Date:       2009-02-16 13:29:35
Message-ID: 20090216132935.38370 () gmx ! net
[Download RAW message or body]

Hi David,

> SVN commit 926922 by dfaure:
> 
> Backport r907918 (makes the code more logical)

I've just read your commit comment for r907918:

"Calling itemChanged only if isVisible=true doesn't make sense, we need to tell the \
model about the change, it won't be told when scrolling around later on. Must be a \
kde3 relic. I wonder if findVisibleIcon really works, too, because isVisible was true \
even when the icon wasn't visible..."

I've committed this 'isVisible' code in r729435 on 25.10.2007:

"Fixed performance bottleneck when resolving the mimetypes for several thousand \
items: Only invoke KDirModel::itemChanged() for items that are visible within the \
view. This decreases the CPU-load from ~100 % down to ~7 % during MIME type resolving \
(reviewed by David Faure).

CCMAIL: faure@kde.org"

The root cause of the bottleneck was: When having a directory of e. g. 10000 items in \
an icon view, then a single call of QAbstractItemModel::itemChanged() resulted in a \
complete internal relayout of QListView -> on each (!) call of itemChanged() the size \
hint of the file item delegate was called 10000 times. The file item delegate by \
Dolphin/Konqueror does a text wrapping to calculate the optimum size :-( If I \
remember correctly, even for the details view a high CPU load was given.

From my point of view this is an issue in QListView: If a grid layout is specified \
(like currently in Dolpin/Konqueror), no relayout needs to get triggered at all in \
this case. (I did not check yet whether this performance issue has been fixed in Qt \
4.5 already.)

I fully agree that emitting the itemChanged() signal should not depend on whether the \
item is visible, as in theory a change of an item can lead to a complete relayout...

Still the performance problem should be checked. In KFilePreviewGenerator I used the \
following trick:

- Instead of directly emitting the itemChanged() signal on each timeout, I collected \
the changes in a list.

- After an internal timeout (or a number of maximum elements) for all items of the \
list the emitChanged() signal is emitted synchronously within a loop. Assuming that \
100 items got collected, this means having only one relayout for 100 items instead of \
100 relayouts.

Maybe this would be a sufficient solution here too?

> 
> 
> M  +2 -6      kmimetyperesolver.cpp  
> 
> 
> --- branches/KDE/4.2/kdelibs/kio/kio/kmimetyperesolver.cpp #926921:926922
> @@ -19,6 +19,7 @@
> */
> 
> #include "kmimetyperesolver.h"
> +#include <kdebug.h>
> #include "defaultviewadapter_p.h"
> #include <kdirmodel.h>
> #include <kfileitem.h>
> @@ -148,13 +149,11 @@
> }
> 
> int nextDelay = 0;
> -    bool isVisible = false;
> QModelIndex index = findVisibleIcon();
> if (index.isValid()) {
> // Found a visible item.
> const int numFound = m_pendingIndexes.removeAll(index);
> Q_ASSERT(numFound == 1);
> -        isVisible = true;
> } else {
> // No more visible items.
> // Do the unvisible ones, then, but with a bigger delay, if so
> configured
> @@ -164,11 +163,8 @@
> KFileItem item = m_dirModel->itemForIndex(index);
> if (!item.isNull()) { // check that item still exists
> if (!item.isMimeTypeKnown()) { // check if someone did it
> meanwhile
> -            //kDebug() << "Determining mimetype for " << item.url();
> item.determineMimeType();
> -            if (isVisible) {
> -                m_dirModel->itemChanged(index);
> -            }
> +            m_dirModel->itemChanged(index);
> }
> }
> m_timer.start(nextDelay); // singleshot


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