Make the code that removes items from KFileItemModel more robust

Review Request #113070 - Created Oct. 2, 2013 and submitted

Information
Frank Reininghaus
kde-baseapps
master
324371, 325359
Reviewers
dolphin
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).
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.
Emmanuel Pescosta
Commit Hook
Frank Reininghaus
Review request changed

Status: Closed (submitted)

Loading...