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/#review27770
-----------------------------------------------------------


Many thanks for the patch, Emmanuel! I greatly appreciate all the work you put into this, and I'm looking forward to seeing these improvements in the next release.

However, properly discussing and reviewing a patch of this size, which mixes quite a few different things, is basically impossible. Moreover, also the git history benefits greatly from a series of smaller, self-contained commits IMHO - I often find myself using 'git log' and 'git blame' to find out why some code is as it is and what reason there was for a particular change. This does not work if there is a big "rewrite everything" commit in the history.

Just a few examples of things in your patch that are mostly unrelated to the core idea, and make it harder to understand:
- the changes in KFileItemModel::retrieveData(),
- the changes in KFileItemModel::childItems() (which look at first sight like they might actually harm performance in some situations),
- the changes in KFileItemModel::slotRefreshItems() (do I assume correctly that this is what fixes Bug 311912?)

Therefore, I'm wondering if we should better do this in a couple of smaller steps. I think that this will lead to a better result for everyone in the end.

How about this:

a) First do the changes in KFileItemModel::slotNewItems(), KFileItemModel::insertItems() (and the minimum of changes in other functions that are related to these) that are required to make the "tree view stuff in network:/" work?
b) If the change in KFileItemModel::slotRefreshItems() really is what fixes Bug 311912, can't we use that in the KDE/4.10 branch as well?

I have to leave now, therefore, I cannot write more at the moment.

- Frank Reininghaus


On Feb. 19, 2013, 6:31 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109045/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2013, 6:31 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/20130220/cccbae97/attachment.htm>