From kde-commits  Mon Feb 16 13:29:35 2009
From: "Peter Penz" <peter.penz () gmx ! at>
Date: Mon, 16 Feb 2009 13:29:35 +0000
To: kde-commits
Subject: Re: branches/KDE/4.2/kdelibs/kio/kio
Message-Id: <20090216132935.38370 () gmx ! net>
X-MARC-Message: https://marc.info/?l=kde-commits&m=123479102522057

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