Review Request 109045: Big rewritte in KFileItemModel

[Emmanuel Pescosta]

> On Feb. 20, 2013, 7:58 a.m., Frank Reininghaus wrote:
> > 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.

Ok. I will try to split this patch into multiple patches. I hope this is possible ;)

> the changes in KFileItemModel::retrieveData()
These changes are needed, because of changes in other functions (mainly KFileItem -> ItemData*) and they are also part of the bug 312890 fix.

> 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?
> the changes in KFileItemModel::slotRefreshItems() (do I assume correctly that this is what fixes Bug 311912?)
Nope, these changes don't fix the Bug 311912, they are needed because of changes in other functions (mainly KFileItem -> ItemData*)
The Bug 311912 is fixed by changing the logic from KFileItem handling to ItemData handling + multiple changes in insertItems, removeItems and applyFilters 
-> Really hard to split it up, because it affects a lot of places.

> the changes in KFileItemModel::childItems() (which look at first sight like they might actually harm performance in some situations)
Yes you are right, needs a better approach.

Ideas:
+ What if we try to move the filtering code out of the KFileItemModel and create a proxy model instead? (KFileItemModel -> FilterProxyModel -> View)
  -> Result: Much easier KFileItemModel + Easier to improve the performance

+ Add a "QList<ItemData*>* childs;" to the ItemData struct
  -> Result: Easier to improve the performance (e.g. sorting in tree structure only in affected leafs, ...)


- Emmanuel


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


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/7463e0d3/attachment.htm>