Review Request 111195: Make use of the "resort all items timer" in KFileItemModel::slotRefreshItems

[Frank Reininghaus]

> On June 23, 2013, 6:07 p.m., Frank Reininghaus wrote:
> > Thanks Emmanuel for the patch!
> > 
> > In principle you're right that many updates could cause (too) many resortings, and your patch would fix this problem.
> > 
> > However, it has the side effect that there will always be a 0.5 second delay between any change of a file and the resorting. This can be a bit irritating, like when you rename a file, and it takes some time until it is moved to the new position.
> > 
> > So to fix the problem properly, a solution like the "changes items timer" in KFileItemModelRolesUpdater would be better (i.e., do the first resorting immediately, but if there are many changes in a row, wait until all updates are done). But then I'm not sure how often such a situation will really happen and how urgent is is that we fix that. The "too many resortings" issue will only appear if the sort role is changed for many files at different times. I think that this is rather unrealistic for the "Name" and the "Size", maybe it could happen for the "Date"?
> > 
> > Note that the reason why "m_resortAllItemsTimer->start();" is used in KFileItemModel::setData() is that this function can indeed be called many times in a row by KFileItemModelRolesUpdater if it resolves the sort role (like the number of items in a folder or a Nepomuk role) asynchronously. In that case, we really have to avoid that every call of setData triggers a new resorting.
> 
> Emmanuel Pescosta wrote:
>     > However, it has the side effect that there will always be a 0.5 second delay between any change of a file and the resorting.
>     This delay also happens when you change tags/description/... (if sort by tags/description/... is enabled). 
>     
>     > I think that this is rather unrealistic for the "Name" and the "Size", maybe it could happen for the "Date"?
>     Date and size can change very often in download folders, build directories, ... (Esp. when you have expanded folders, because of multiple dir listers)
>     
>     But if you don't like it, I'll discard this patch (was just an idea, because the code for delayed resorting is already there) ;)
> 
> Frank Reininghaus wrote:
>     Thanks for your comments. When many files in a folder are changed, we will receive the refreshItems signal only once, I think. However, you are right, this might be different if many folders are expanded.
>     
>     But still, is this problem really big enough that you can justify adding the 0.5 second delay after renaming? To be honest, I find it irritating.
>     
>     If the multiple resortings cause real problems for users (do you have some evidence for that?), then I think that a more sophisticated solution is required.

It seems that I missed an important use case when the refreshItems signal is emitted many times in a row: renaming many files at once. This makes Dolphin extremely slow (see https://bugs.kde.org/show_bug.cgi?id=323789), and I found out by adding some debug output that the refreshItems signal is indeed emitted for every single file separately, and then the entire model is resorted many times.

So I'd say that you should push it to KDE/4.11 and close that bug report. Sorry for not realizing earlier how much sense this makes!


- Frank


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


On June 24, 2013, 7:12 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111195/
> -----------------------------------------------------------
> 
> (Updated June 24, 2013, 7:12 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Followup to patch 111146
> 
> Make use of the "resort all items timer" in KFileItemModel::slotRefreshItems 
> to avoid too much expensive resorting calls, in case of many refresh items signals.
> 
> CCBUG: 303873
> CCBUG: 299565
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 7ea5e80 
> 
> Diff: http://git.reviewboard.kde.org/r/111195/diff/
> 
> 
> Testing
> -------
> 
> Works!
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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