From: Frank Reininghaus <frank78ac@googlemail.com>
Date: Tue, 13 May 2014 17:04:09 +0000 (+0200)
Subject: Ensure that all children of a collapsed folder are removed
X-Git-Url: https://cloud.milkyroute.net/gitweb/dolphin.git/commitdiff_plain/66f1759b6f993c237b9652b9aa51ac9df52e5238

Ensure that all children of a collapsed folder are removed

Before this patch, any (direct or indirect) children that might have
been in m_pendingItemsToInsert, i.e., that were not inserted into the
model yet because KDirLister had not finished listing the directory
yet, would be added to the model later without a proper parent. This
could cause a crash later on.

CCBUG: 332102
FIXED-IN: 4.13.2
---

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index a0f9305cb..34a97de4c 100644
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -486,6 +486,18 @@ bool KFileItemModel::setExpanded(int index, bool expanded)
             m_urlsToExpand.insert(url);
         }
     } else {
+        // Note that there might be (indirect) children of the folder which is to be collapsed in
+        // m_pendingItemsToInsert. To prevent that they will be inserted into the model later,
+        // possibly without a parent, which might result in a crash, we insert all pending items
+        // right now. All new items which would be without a parent will then be removed.
+        dispatchPendingItemsToInsert();
+
+        // Check if the index of the collapsed folder has changed. If that is the case, then items
+        // were inserted before the collapsed folder, and its index needs to be updated.
+        if (m_itemData.at(index)->item != item) {
+            index = this->index(item);
+        }
+
         m_expandedDirs.remove(targetUrl);
         m_dirLister->stop(url);
 
diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 99ee3368e..48e72e83f 100644
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -93,6 +93,7 @@ private slots:
     void testChangeRolesForFilteredItems();
     void testChangeSortRoleWhileFiltering();
     void testRefreshFilteredItems();
+    void testCollapseFolderWhileLoading();
     void testCreateMimeData();
 
 private:
@@ -1619,6 +1620,83 @@ void KFileItemModelTest::testCreateMimeData()
     delete mimeData;
 }
 
+void KFileItemModelTest::testCollapseFolderWhileLoading()
+{
+    QSet<QByteArray> modelRoles = m_model->roles();
+    modelRoles << "isExpanded" << "isExpandable" << "expandedParentsCount";
+    m_model->setRoles(modelRoles);
+
+    QStringList files;
+    files << "a2/b/c1.txt";
+    m_testDir->createFiles(files);
+
+    m_model->loadDirectory(m_testDir->url());
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a2");
+
+    // Expand "a2/".
+    m_model->setExpanded(0, true);
+    QVERIFY(m_model->isExpanded(0));
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a2" << "b");
+
+    // Expand "a2/b/".
+    m_model->setExpanded(1, true);
+    QVERIFY(m_model->isExpanded(1));
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a2" << "b" << "c1.txt");
+
+    // Simulate that a new item "c2.txt" appears, but that the dir lister's completed()
+    // signal is not emitted yet.
+    const KFileItem fileItemC1 = m_model->fileItem(2);
+    KFileItem fileItemC2 = fileItemC1;
+    KUrl urlC2 = fileItemC2.url();
+    urlC2.setFileName("c2.txt");
+    fileItemC2.setUrl(urlC2);
+
+    const KUrl urlB = m_model->fileItem(1).url();
+    m_model->slotItemsAdded(urlB, KFileItemList() << fileItemC2);
+    QCOMPARE(itemsInModel(), QStringList() << "a2" << "b" << "c1.txt");
+
+    // Collapse "a2/". This should also remove all its (indirect) children from
+    // the model and from the model's m_pendingItemsToInsert member.
+    m_model->setExpanded(0, false);
+    QCOMPARE(itemsInModel(), QStringList() << "a2");
+
+    // Simulate that the dir lister's completed() signal is emitted. If "c2.txt"
+    // is still in m_pendingItemsToInsert, then we might get a crash, see
+    // https://bugs.kde.org/show_bug.cgi?id=332102. Even if the crash is not
+    // reproducible here, Valgrind will complain, and the item "c2.txt" will appear
+    // without parent in the model.
+    m_model->slotCompleted();
+    QCOMPARE(itemsInModel(), QStringList() << "a2");
+
+    // Expand "a2/" again.
+    m_model->setExpanded(0, true);
+    QVERIFY(m_model->isExpanded(0));
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a2" << "b");
+
+    // Now simulate that a new folder "a1/" is appears, but that the dir lister's
+    // completed() signal is not emitted yet.
+    const KFileItem fileItemA2 = m_model->fileItem(0);
+    KFileItem fileItemA1 = fileItemA2;
+    KUrl urlA1 = fileItemA1.url();
+    urlA1.setFileName("a1");
+    fileItemA1.setUrl(urlA1);
+
+    m_model->slotItemsAdded(m_model->directory(), KFileItemList() << fileItemA1);
+    QCOMPARE(itemsInModel(), QStringList() << "a2" << "b");
+
+    // Collapse "a2/". Note that this will cause "a1/" to be added to the model,
+    // i.e., the index of "a2/" will change from 0 to 1. Check that this does not
+    // confuse the code which collapses the folder.
+    m_model->setExpanded(0, false);
+    QCOMPARE(itemsInModel(), QStringList() << "a1" << "a2");
+    QVERIFY(!m_model->isExpanded(0));
+    QVERIFY(!m_model->isExpanded(1));
+}
+
 QStringList KFileItemModelTest::itemsInModel() const
 {
     QStringList items;