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

[Frank Reininghaus]

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

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/20131002/582b17b9/attachment.htm>