Review Request 109045: Big rewritte in KFileItemModel

[Frank Reininghaus]

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


Thanks Emmanuel for the new patch!

I just tried to test your patch, but it seems that your patch is not based on latest master - I just applied it, and patching master with the 'patch' command actually works, but it says that "Hunk #8 succeeded at 783 with fuzz 2", and the result of the fuzziness is that the code is in a state that does not compile.

The problem is in KFileItemModel::slotItemsDeleted(). Your patch contains a new, different solution to the "make sure that filtered children of removed parents are also removed" issue. There is a fix for that one in KDE/4.10 and master already - see https://git.reviewboard.kde.org/r/108976/.

The advantages of the existing approach are that the code to locate the filtered children is in one function only, preventing that the filtering-related complexity spreads out to too many different places, and also that its run-time complexity is at most O(N), where N is the maximum of the number of deleted items and filtered items. On the other hand, your solution to the problem has two nested foreach loops - one for the deleted items and one for the filtered items, and then another foreach loop over all (filtered) items in the function childItems(), such that we can get O(N^3) worst-case complexity. To be honest, I already find the places in the existing code where we have O(N^2) worst-case behaviour a bit worrying, and I hope it's understandable that I'm not in favour of replacing an ex
 isting O(N) solution with a new O(N^3) one. If we do that, one could easily come up with a folder structure with a moderate number of files, in which filtering one part and then deleting ano
 ther part would freeze Dolphin basically forever.

I can try to help with splitting this into smaller, self-contained commits, but having something that compiles as a starting point would be helpful. About "Fixed a bug, which was introduced during the rewrite of some code parts.": this is exactly why I think that making changes in smaller steps is better. At some point, the changes caused by a single patch that mixes different things get so complex that it's impossible to understand everything that's going on. If you do it in smaller steps, and you notice a regression at some point, you can at least easily find the problematic change with git bisect or something like that.

- Frank Reininghaus


On Feb. 21, 2013, 10:15 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109045/
> -----------------------------------------------------------
> 
> (Updated Feb. 21, 2013, 10:15 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Big rewritte in KFileItemModel: 
> + Get rid of the Url based item management 
> + Some performance improvements
> + Better handling of filtered items
> + Fix the treeview for timeline, trash, network-browser, ... (Different Url scheme)
> 
> Should fix bugs:
> + Bug 304565 - Network browser: details view breaks when expanding
> + Bug 312890 - Dolphin tries to render timeline:/ results as tree
> + Bug 311912 - After erasing a "filter", some thumbnails randomly disappear 
> 
> 
> This addresses bugs 304565, 311912 and 312890.
>     http://bugs.kde.org/show_bug.cgi?id=304565
>     http://bugs.kde.org/show_bug.cgi?id=311912
>     http://bugs.kde.org/show_bug.cgi?id=312890
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.h a05d1f9 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 60fc275 
>   dolphin/src/tests/kfileitemmodelbenchmark.cpp b1e777c 
>   dolphin/src/tests/kfileitemmodeltest.cpp c9f8a34 
> 
> Diff: http://git.reviewboard.kde.org/r/109045/diff/
> 
> 
> Testing
> -------
> 
> + Passes all tests
> + Renders timeline as a treeview without problems for files, timeline, trash (yes we can re-enable this), ...
> + Doesn't lose thumbnails anymore, when using the filterbar - Tested with 500 pictures (Also much faster response, esp. when you delete the filter string)
> 
> The bug 304565 needs some testing
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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