Review Request 109045: Big rewritte in KFileItemModel

[Frank Reininghaus]

> 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.
> 
> Emmanuel Pescosta wrote:
>     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 Pescosta wrote:
>     Btw.:
>     
>     > + What if we try to move the filtering code out of the KFileItemModel and create a proxy model instead? (KFileItemModel -> FilterProxyModel -> View)
>     This also gives us the ability to create a new awesome filtering mechanism (like http://wstaw.org/m/2013/02/20/plasma-desktopqo5985.png) ;)
>     
>     > + Add a "QList<ItemData*>* childs;" to the ItemData struct
>     This will also fix the mentioned KFileItemModel::childItems() problem

About your new ideas: You say that they make it "easier to improve the performance", but what are the actual performance problems that these ideas are supposed so solve?

The idea to add a list of children to each item is mostly what is done in Qt's itemviews (look at KDirModel, for example). From my point of view, this mostly has the effect that the implementation of the model becomes a bit more straightforward, and that you cannot have the kind trouble that we had with some issues in expandedParentCountsCompare(). On the other hand, these troubles are mostly solved now (or at least, will be as soon as your ideas from this request are in master), and changing the internal structure of the model from a plain list, indexed with ints, to some sort of hierarchical structure, will require lots of changes in other classes.

About the proxy model idea: this is also just what Qt's itemviews do. It would have the advantage that the implementation of the actual model becomes a bit simpler. But other parts of the code, which need access to the model where the file items are stored, would have to be made more complex. We had that in Dolphin 1.x, and it was not always fun. Moreover, I cannot see how such an approach would improve the performance - if you always have to go through a proxy model to access the actual data, this looks like it might actually incur a small performance penalty from my point of view.

Please note that I'm not saying at all that these ideas are bad. Quite the contrary, the use of these ideas in Qt's itemviews shows that they certainly have some advantages and make it easier to implement a model. However, they also have some drawbacks which can make certain things inconvenient, and I guess this is why Peter decided to drop these concepts when he re-wrote the view engine some time ago. Therefore, I don't really see a compelling reason to change it again now.

Moreover, I think any major restructuring (not talking about the things in your patch, the ideas in there look fine), which would certainly require a lot of work, should be done with a possible transition to QML in mind. I guess that there must be some steps in that direction at some point, in particular if we want to see more "file view" code shared between Dolphin, the file dialogs and other places in KDE in the Frameworks era. But I have rather little knowledge of QML, all I know is that it currently does not have tree views and that the existing views are designed to work with subclasses of QAbstractItemModel, so I don't know what a possible transition from our existing code base to QML would look like.


- Frank


-----------------------------------------------------------
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/20130221/3b4807c7/attachment.htm>