Review Request 111920: Count the items inside directories in another thread

[Mark Gaiser]

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


Hi Frank,

I'm going to comment on this "in general" and not on specific code parts. Simply because you're now entering an area where i've already done quite a bit of research :).

What you are doing is (or seems to be) the fastest approach to get the number of files in a directory. That is very good! However, i don't think that will work for non mounted devices. So it probably doesn't work for smb://... ftp://... and the range of other kio slaves out there. If you don't want to support that then your current approach seems to be the best way.

However, if you do want to support kio slaves then the code can be made a lot simpler. You could use KIO::listDir with details set to 0 for the fastest kio way (that i know) to get a list of files in any place which you then simply count. While kio is quite fast with details set to 0, it can still do batching! It will if a listdir takes 300ms+ so you'd have to take that into account. 

In your code you have the comment:
// If INotify is used, KDirWatch issues the dirty() signal
// also for changed files inside the directory, even if we
// don't enable this behavior explicitly (see bug 309740).

I know exactly why that happens and am trying to fix that. It's the same issue as i thoroughly explained in "KDirWatch bug and the analysis. Help is welcome!". However, fixing it is likely going to require changing KDirLister as well so i doubt it will find it's way into KDE/4.11 or master. It might end up being a frameworks change only and it also might end up not worth it if the changes are to invasive. It could go anywhere, i just don't know yet since i'm still trying to figure out how to best fix the issue.

Just my 2 cents, hope it helps :)

- Mark Gaiser


On Aug. 6, 2013, 10:13 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111920/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2013, 10:13 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> I think that counting the items inside directories (this number is shown, e.g., in the "Size" column in Details View) could be done in another thread. This not only prevents that the GUI freezes while counting many items in a folder on a slow device [1]. Moving the code that is needed for the counting and directory watching to another class also makes KFileItemModelRolesUpdater, which is complex enough already, a bit simpler.
> 
> Some remarks:
> 
> (a) When sorting by "Size", we still count synchronously in the main thread for 200 ms when new items are inserted into the model. If that succeeds, we can show the items in the correct order immediately.
> 
> (b) KDirectoryContentsCounter is a helper class that lives in the GUI thread. It organizes the dir watching and tells KFileItemModelRolesUpdater via a signal about the results of the counting.
> 
> (c) The actual counting is done in a secondary thread by KDirectoryContentsCounterWorker (Hm, the name might be a bit too long). It communicates with KDirectoryContentsCounter via signals and slots. One problem is that the "Worker" needs to know if it should count hidden files/files and folders/etc. First I used 2 bool arguments for that, but then bool parameters don't really make the code easy to read, so I decided to do it with a flags type KDirectoryContentsCounterWorker::Options. Getting that to work correctly with cross-thread signals was surprisingly complicated, but it works now. I'm not really sure any more if it was worth the effort, but I still think that this is slightly better than the two bools.
> 
> (d) There is one change in
> 
> KFileItemModelRolesUpdater::slotDirectoryContentsCountReceived(const QString& path, int count)
> 
> that is similar to the old function
> 
> KFileItemModelRolesUpdater::slotDirWatchDirty(const QString& path)
> 
> which may not be obvious: when telling the model about the "size" of the directory, it disconnects from the model's "itemsChanged" signal. The effect is that no new preview will be generated if an image is added to/removed from a subfolder shown in the view. This "feature" was only available when the "Size" was shown anyway, so it added some inconsistency. Moreover, one consequence was that a "dirty" signal from KDirWatch resulted in counting the folder contents *twice*: once after the signal was received, and another time after the model's itemsChanged signal was received. Therefore, I think that it makes sense to drop this "feature".
> 
> Any comments are welcome.
> 
> 
> [1] This can be simulated by adding something like
> 
>     QElapsedTimer timer;
>     timer.start();
>     while (timer.elapsed() < 5000);
> 
> to 
> 
> int KFileItemModelRolesUpdater::subItemsCount(const QString& path) const
> 
> 
> This addresses bug 318518.
>     http://bugs.kde.org/show_bug.cgi?id=318518
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 6856991 
>   dolphin/src/kitemviews/kfileitemmodelrolesupdater.h 409f098 
>   dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 698a6c5 
>   dolphin/src/kitemviews/private/kdirectorycontentscounter.h PRE-CREATION 
>   dolphin/src/kitemviews/private/kdirectorycontentscounter.cpp PRE-CREATION 
>   dolphin/src/kitemviews/private/kdirectorycontentscounterworker.h PRE-CREATION 
>   dolphin/src/kitemviews/private/kdirectorycontentscounterworker.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/111920/diff/
> 
> 
> Testing
> -------
> 
> Scrolling the view always feels smooth, even when the items inside a large directory are being counted.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130807/d743c924/attachment.htm>