Review Request 113070: Make the code that removes items from KFileItemModel more robust

[Emmanuel Pescosta]

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

Ship it!


This patch brings in some really nice (code) improvements - always great to read 
such code and hopefully learn from it ;)

I haven't found any problems with these changes yet - expanding + filtering works.
All unit tests still pass.

So a big "ship it" from my side!

- Emmanuel Pescosta


On Oct. 3, 2013, 12:17 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113070/
> -----------------------------------------------------------
> 
> (Updated Oct. 3, 2013, 12:17 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Bugs: 324371 and 325359
>     http://bugs.kde.org/show_bug.cgi?id=324371
>     http://bugs.kde.org/show_bug.cgi?id=325359
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> When we remove items from the model, we call the function KFileItemModel::removeItems(const KFileItemList&, RemoveItemsBehavior). This function then looks up the indexes of the items using the hash m_items. This is wasteful in the situations when the indexes of the removed items are known in advance (like when an expanded folder is collapsed in Details View), and it can cause trouble if one item is contained in the model multiple times (can happen when searching, and a file both matches the search and is a child of a folder that matches the search). One could argue that expanding folders in the search results list might not be particularly useful most of the time, but still, I think that we should better make the model more robust to deal with such situations.
> 
> This patch makes the following changes to achieve that goal:
> 
> * Change the argument of removeItems() from KFileItemList to KItemRangeList. To make this work, the "look the indexes up in m_items" code is moved from that function to slotItemsDeleted(). In the other places where removeItems() is called, the indexes are calculated directly (which is not more difficult than determining the removed items as a KFileItemList, if you consider that we needed the function childItems(KFileItem) for that, which is not needed any more with this patch).
> 
> * Also removeFilteredChildren() takes a KItemRangeList now. Rather than putting the parent KFileItems into a QSet for O(1) lookup (which prevents O(N^2) worst case behavior for the entire function), it uses a QSet<ItemData*> now, which should even be more efficient (hashing a pointer is cheaper than hashing a KFileItem/KUrl).
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.h 50e3e32 
>   dolphin/src/kitemviews/kfileitemmodel.cpp d61de00 
>   dolphin/src/tests/kfileitemmodelbenchmark.cpp f72e43e 
>   dolphin/src/tests/kfileitemmodeltest.cpp 299ca6f 
> 
> Diff: http://git.reviewboard.kde.org/r/113070/diff/
> 
> 
> Testing
> -------
> 
> Fixes the two bugs for me. Old and new unit tests pass, and I haven't seen any new problems yet. But still, it's a rather non-trivial change, and the bug is not really happening very frequently AFAICS, so I think that this is more suitable for master than KDE/4.11.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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