Fix crash #1 when filtering items

When filtering items it was possible that the current index got
an invalid value which resulted in accessing the URL of a
null-KFileItem.

There is still one (general) open issue in KFileItemModelRolesUpdater
(crash #2) where a KFileItem that is already null gets read.
It is not really related to filtering but can be triggered
quite easy when filtering huge directories with enabled previews.

CCBUG: 287642

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index bd83bf62137c0988d256ea735e8ade0f020f5e5e..6ed10a6021ab1e602ec2581a3ca93f449aefd64a 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -480,46 +480,37 @@ void KFileItemModel::setExpanded(const QSet<KUrl>& urls)
 void KFileItemModel::setNameFilter(const QString& nameFilter)
 {
     if (m_nameFilter != nameFilter) {
-        // TODO #1: Assure that expanded items only can get hidden
-        // if no child item is visible
-
-        // TODO #2: If the user entered a '*' use a regular expression
+        dispatchPendingItemsToInsert();
 
         m_nameFilter = nameFilter;
 
-        const QString filter = nameFilter.toLower();
-
         // Check which shown items from m_itemData must get
         // hidden and hence moved to m_filteredItems.
         KFileItemList newFilteredItems;
 
         foreach (ItemData* itemData, m_itemData) {
-            if (!matchesNameFilter(itemData->item, filter)) {
-                m_filteredItems.append(itemData->item);
+            if (!matchesNameFilter(itemData->item)) {
                 newFilteredItems.append(itemData->item);
+                m_filteredItems.insert(itemData->item);
             }
         }
 
-        if (!newFilteredItems.isEmpty()) {
-            slotItemsDeleted(newFilteredItems);
-        }
+        removeItems(newFilteredItems);
 
         // Check which hidden items from m_filteredItems should
         // get visible again and hence removed from m_filteredItems.
         KFileItemList newVisibleItems;
 
-        for (int i = m_filteredItems.count() - 1; i >= 0; --i) {
-            const KFileItem item = m_filteredItems.at(i);
-            if (matchesNameFilter(item, filter)) {
+        QMutableSetIterator<KFileItem> it(m_filteredItems);
+        while (it.hasNext()) {
+            const KFileItem item = it.next();
+            if (matchesNameFilter(item)) {
                 newVisibleItems.append(item);
-                m_filteredItems.removeAt(i);
+                m_filteredItems.remove(item);
             }
         }
 
-        if (!newVisibleItems.isEmpty()) {
-            slotNewItems(newVisibleItems);
-            dispatchPendingItemsToInsert();
-        }
+        insertItems(newVisibleItems);
     }
 }
 
@@ -657,7 +648,23 @@ void KFileItemModel::slotCanceled()
 
 void KFileItemModel::slotNewItems(const KFileItemList& items)
 {
-    m_pendingItemsToInsert.append(items);
+    if (m_nameFilter.isEmpty()) {
+        m_pendingItemsToInsert.append(items);
+    } else {
+        // The name-filter is active. Hide filtered items
+        // before inserting them into the model and remember
+        // the filtered items in m_filteredItems.
+        KFileItemList filteredItems;
+        foreach (const KFileItem& item, items) {
+            if (matchesNameFilter(item)) {
+                filteredItems.append(item);
+            } else {
+                m_filteredItems.insert(item);
+            }
+        }
+
+        m_pendingItemsToInsert.append(filteredItems);
+    }
 
     if (useMaximumUpdateInterval() && !m_maximumUpdateIntervalTimer->isActive()) {
         // Assure that items get dispatched if no completed() or canceled() signal is
@@ -668,10 +675,14 @@ void KFileItemModel::slotNewItems(const KFileItemList& items)
 
 void KFileItemModel::slotItemsDeleted(const KFileItemList& items)
 {
-    if (!m_pendingItemsToInsert.isEmpty()) {
-        insertItems(m_pendingItemsToInsert);
-        m_pendingItemsToInsert.clear();
+    dispatchPendingItemsToInsert();
+
+    if (!m_filteredItems.isEmpty()) {
+        foreach (const KFileItem& item, items) {
+            m_filteredItems.remove(item);
+        }
     }
+
     removeItems(items);
 }
 
@@ -738,6 +749,7 @@ void KFileItemModel::slotClear()
     kDebug() << "Clearing all items";
 #endif
 
+    m_filteredItems.clear();
     m_groups.clear();
 
     m_minimumUpdateIntervalTimer->stop();
@@ -907,7 +919,11 @@ void KFileItemModel::removeItems(const KFileItemList& items)
     // Delete the items
     for (int i = indexesToRemove.count() - 1; i >= 0; --i) {
         const int indexToRemove = indexesToRemove.at(i);
-        delete m_itemData.at(indexToRemove);
+        ItemData* data = m_itemData.at(indexToRemove);
+
+        m_items.remove(data->item.url());
+
+        delete data;
         m_itemData.removeAt(indexToRemove);
     }
 
@@ -1707,10 +1723,15 @@ QList<QPair<int, QVariant> > KFileItemModel::genericStringRoleGroups(const QByte
     return groups;
 }
 
-bool KFileItemModel::matchesNameFilter(const KFileItem& item, const QString& nameFilter)
+bool KFileItemModel::matchesNameFilter(const KFileItem& item) const
 {
+    // TODO #1: A performance improvement would be possible by caching m_nameFilter.toLower().
+    // Before adding yet-another-member it should be checked whether it brings a noticable
+    // improvement at all.
+
+    // TODO #2: If the user entered a '*' use a regular expression
     const QString itemText = item.text().toLower();
-    return itemText.contains(nameFilter);
+    return itemText.contains(m_nameFilter.toLower());
 }
 
 #include "kfileitemmodel.moc"

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index a83d57ba8bf15fa6f9129c8b34891d4822b58a8a..5d0aa4203f4914242fac0620c9e73a67f82abea5 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -275,9 +275,9 @@ private:
     bool isChildItem(int index) const;
 
     /**
-     * @return True if the given item matches with the name filter.
+     * @return True if the given item matches with the current set name filter.
      */
-    static bool matchesNameFilter(const KFileItem& item, const QString& nameFilter);
+    bool matchesNameFilter(const KFileItem& item) const;
 
 private:
     QWeakPointer<KDirLister> m_dirLister;
@@ -293,7 +293,7 @@ private:
     QHash<KUrl, int> m_items; // Allows O(1) access for KFileItemModel::index(const KFileItem& item)
 
     QString m_nameFilter;
-    KFileItemList m_filteredItems; // Items that got hidden by KFileItemModel::setNameFilter()
+    QSet<KFileItem> m_filteredItems; // Items that got hidden by KFileItemModel::setNameFilter()
 
     bool m_requestRole[RolesCount];
 

diff --git a/src/kitemviews/kitemlistselectionmanager.cpp b/src/kitemviews/kitemlistselectionmanager.cpp
index fb279796f66b8e87e8f297b4a3a31c30756c00fa..448e792930699f8d0258317073077856c5a0208b 100644 (file)
--- a/src/kitemviews/kitemlistselectionmanager.cpp
+++ b/src/kitemviews/kitemlistselectionmanager.cpp
@@ -293,15 +293,19 @@ void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges)
             if (currentItem < itemRange.index) {
                 break;
             }
+
             if (currentItem >= itemRange.index + itemRange.count) {
                 currentItem -= itemRange.count;
-            } else if (currentItem >= m_model->count()) {
+            }
+
+            if (currentItem >= m_model->count()) {
                 currentItem = m_model->count() - 1;
             }
         }
         // Calling setCurrentItem would trigger the selectionChanged signal, but we want to
         // emit it only once in this function -> change the current item manually and emit currentChanged
         m_currentItem = currentItem;
+        Q_ASSERT(m_currentItem < m_model->count());
         emit currentChanged(m_currentItem, previousCurrent);
     }
 

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 23b899136860c50ead521402cef15555ef6cf369..18580c24becef1153498c7d67813fc84a3024f7e 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -162,14 +162,17 @@ void KFileItemModelTest::testNewItems()
 void KFileItemModelTest::testRemoveItems()
 {
     m_testDir->createFile("a.txt");
+    m_testDir->createFile("b.txt");
     m_dirLister->openUrl(m_testDir->url());
     QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
-    QCOMPARE(m_model->count(), 1);
+    QCOMPARE(m_model->count(), 2);
+    QVERIFY(isModelConsistent());
 
     m_testDir->removeFile("a.txt");
     m_dirLister->updateDirectory(m_testDir->url());
     QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsRemoved(KItemRangeList)), DefaultTimeout));
-    QCOMPARE(m_model->count(), 0);
+    QCOMPARE(m_model->count(), 1);
+    QVERIFY(isModelConsistent());
 }
 
 void KFileItemModelTest::testSetData()
@@ -675,6 +678,10 @@ void KFileItemModelTest::testNameFilter()
 
 bool KFileItemModelTest::isModelConsistent() const
 {
+    if (m_model->m_items.count() != m_model->m_itemData.count()) {
+        return false;
+    }
+
     for (int i = 0; i < m_model->count(); ++i) {
         const KFileItem item = m_model->fileItem(i);
         if (item.isNull()) {

diff --git a/src/tests/kitemlistselectionmanagertest.cpp b/src/tests/kitemlistselectionmanagertest.cpp
index 2fa6f677b4de579a33ce5e286fd460b893194102..7f79197a90229d71fe5f59eb361899f95b198628 100644 (file)
--- a/src/tests/kitemlistselectionmanagertest.cpp
+++ b/src/tests/kitemlistselectionmanagertest.cpp
@@ -27,18 +27,28 @@ class DummyModel : public KItemModelBase
 {
 public:
     DummyModel();
+    void setCount(int count);
     virtual int count() const;
     virtual QHash<QByteArray, QVariant> data(int index) const;
+
+private:
+    int m_count;
 };
 
 DummyModel::DummyModel() :
-    KItemModelBase()
+    KItemModelBase(),
+    m_count(100)
+{
+}
+
+void DummyModel::setCount(int count)
 {
+    m_count = count;
 }
 
 int DummyModel::count() const
 {
-    return 100;
+    return m_count;
 }
 
 QHash<QByteArray, QVariant> DummyModel::data(int index) const
@@ -48,7 +58,6 @@ QHash<QByteArray, QVariant> DummyModel::data(int index) const
 }
 
 
-
 class KItemListSelectionManagerTest : public QObject
 {
     Q_OBJECT
@@ -67,24 +76,30 @@ private slots:
     void testAnchoredSelection();
     void testChangeSelection_data();
     void testChangeSelection();
+    void testDeleteCurrentItem_data();
+    void testDeleteCurrentItem();
 
 private:
     void verifySelectionChange(QSignalSpy& spy, const QSet<int>& currentSelection, const QSet<int>& previousSelection) const;
 
     KItemListSelectionManager* m_selectionManager;
+    DummyModel* m_model;
 };
 
 void KItemListSelectionManagerTest::init()
 {
+    m_model = new DummyModel();
     m_selectionManager = new KItemListSelectionManager();
-    m_selectionManager->setModel(new DummyModel());
+    m_selectionManager->setModel(m_model);
 }
 
 void KItemListSelectionManagerTest::cleanup()
 {
-    delete m_selectionManager->model();
     delete m_selectionManager;
     m_selectionManager = 0;
+
+    delete m_model;
+    m_model = 0;
 }
 
 void KItemListSelectionManagerTest::testConstructor()
@@ -474,6 +489,36 @@ void KItemListSelectionManagerTest::testChangeSelection()
     verifySelectionChange(spySelectionChanged, QSet<int>(), finalSelection);
 }
 
+void KItemListSelectionManagerTest::testDeleteCurrentItem_data()
+{
+    QTest::addColumn<int>("oldCurrentItemIndex");
+    QTest::addColumn<int>("removeIndex");
+    QTest::addColumn<int>("removeCount");
+    QTest::addColumn<int>("newCurrentItemIndex");
+
+    QTest::newRow("Remove before")               << 50 <<  0 << 10 << 40;
+    QTest::newRow("Remove after")                << 50 << 51 << 10 << 50;
+    QTest::newRow("Remove exactly current item") << 50 << 50 <<  1 << 50;
+    QTest::newRow("Remove around current item")  << 50 << 45 << 10 << 50;
+    QTest::newRow("Remove all except one item")  << 50 <<  1 << 99 <<  0;
+}
+
+void KItemListSelectionManagerTest::testDeleteCurrentItem()
+{
+    QFETCH(int, oldCurrentItemIndex);
+    QFETCH(int, removeIndex);
+    QFETCH(int, removeCount);
+    QFETCH(int, newCurrentItemIndex);
+
+    m_selectionManager->setCurrentItem(oldCurrentItemIndex);
+
+    const int newCount = m_model->count() - removeCount;
+    m_model->setCount(newCount);
+    m_selectionManager->itemsRemoved(KItemRangeList() << KItemRange(removeIndex, removeCount));
+
+    QCOMPARE(m_selectionManager->currentItem(), newCurrentItemIndex);
+}
+
 void KItemListSelectionManagerTest::verifySelectionChange(QSignalSpy& spy,
                                                           const QSet<int>& currentSelection,
                                                           const QSet<int>& previousSelection) const

diff --git a/src/views/dolphinview.cpp b/src/views/dolphinview.cpp
index 857aae395a15494cc1c31bdc65ac1bd68686d77c..455905f6a6a07cb0c4cac132727b4f6d0688d6e1 100644 (file)
--- a/src/views/dolphinview.cpp
+++ b/src/views/dolphinview.cpp
@@ -950,6 +950,7 @@ void DolphinView::saveState(QDataStream& stream)
     const int currentIndex = m_container->controller()->selectionManager()->currentItem();
     if (currentIndex != -1) {
         KFileItem item = fileItemModel()->fileItem(currentIndex);
+        Q_ASSERT(!item.isNull()); // If the current index is valid a item must exist
         KUrl currentItemUrl = item.url();
         stream << currentItemUrl;
     } else {