Fix issue where newly inserted items end up in the wrong directory

If we have directory "a" and "a/b" and expand both, then collapse "a" we
tell KDirWatcher to stop watching both these directories.

However, KDirWatcher keeps them in the listersCurrentlyHolding list as
well as the listersCurrentlyListing list and will still notify of
changes. If a new file appears in "a/b/" we will still get change
notifications.

When dolphin processes these changes we cannot find the relevant parent
node. It then gets confused and inserts the item into the root directory
from the POV of the model notifications. When we then open the relevant
folder the model knows a node with that URL exists and fails to add it
correctly.

This can also be reproduced by continually downloading files into a
subdirectory tree and rapidly expanding and collapsing folders a few
levels deep.

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index 23afb087a31a29d09730529f8f7cbd50db37b694..6391d7d3facd1f2154e303e5867ac2235f15032d 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -15,6 +15,7 @@
 
 #include <KDirLister>
 #include <KIO/Job>
+#include <KIO/kio_version.h>
 #include <KLocalizedString>
 #include <KLazyLocalizedString>
 #include <KUrlMimeData>
@@ -575,6 +576,9 @@ bool KFileItemModel::setExpanded(int index, bool expanded)
 
         m_expandedDirs.remove(targetUrl);
         m_dirLister->stop(url);
+#if KIO_VERSION >= QT_VERSION_CHECK(5, 92, 0)
+        m_dirLister->forgetDirs(url);
+#endif
 
         const int parentLevel = expandedParentsCount(index);
         const int itemCount = m_itemData.count();
@@ -590,6 +594,9 @@ bool KFileItemModel::setExpanded(int index, bool expanded)
                 const QUrl url = itemData->item.url();
                 m_expandedDirs.remove(targetUrl);
                 m_dirLister->stop(url);     // TODO: try to unit-test this, see https://bugs.kde.org/show_bug.cgi?id=332102#c11
+#if KIO_VERSION >= QT_VERSION_CHECK(5, 92, 0)
+                m_dirLister->forgetDirs(url);
+#endif
                 expandedChildren.append(targetUrl);
             }
             ++childIndex;

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 679b8ab3a3b5739b697c01856d40374889d7e113..e9bb7efb78ca8a5760ea1d1f84cbf2b1ebb2d15e 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -14,6 +14,8 @@
 
 #include <KDirLister>
 #include <kio/job.h>
+#include <KIO/kio_version.h>
+
 
 #include "kitemviews/kfileitemmodel.h"
 #include "testdir.h"
@@ -90,6 +92,7 @@ private Q_SLOTS:
     void testCollapseFolderWhileLoading();
     void testCreateMimeData();
     void testDeleteFileMoreThanOnce();
+    void testInsertAfterExpand();
 
 private:
     QStringList itemsInModel() const;
@@ -2031,6 +2034,96 @@ void KFileItemModelTest::testDeleteFileMoreThanOnce()
     QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "c.txt" << "d.txt");
 }
 
+void KFileItemModelTest::testInsertAfterExpand()
+{
+    m_model->m_dirLister->setAutoUpdate(true);
+
+    QSignalSpy itemsInsertedSpy(m_model, &KFileItemModel::itemsInserted);
+    QVERIFY(itemsInsertedSpy.isValid());
+    QSignalSpy itemsRemovedSpy(m_model, &KFileItemModel::itemsRemoved);
+    QVERIFY(itemsRemovedSpy.isValid());
+    QSignalSpy loadingCompletedSpy(m_model, &KFileItemModel::directoryLoadingCompleted);
+    QVERIFY(loadingCompletedSpy.isValid());
+
+    // Test expanding subfolders in a folder with the items "a/", "a/a/"
+    QSet<QByteArray> originalModelRoles = m_model->roles();
+    QSet<QByteArray> modelRoles = originalModelRoles;
+    modelRoles << "isExpanded" << "isExpandable" << "expandedParentsCount";
+    m_model->setRoles(modelRoles);
+
+    m_testDir->createFile("a/b/1");
+
+    m_model->loadDirectory(m_testDir->url());
+    QVERIFY(itemsInsertedSpy.wait());
+
+    // So far, the model contains only "a/"
+    QCOMPARE(m_model->count(), 1);
+    QVERIFY(m_model->isExpandable(0));
+    QVERIFY(!m_model->isExpanded(0));
+    QVERIFY(m_model->expandedDirectories().empty());
+
+    QCOMPARE(itemsInsertedSpy.count(), 1);
+    {
+        KItemRangeList itemRangeList = itemsInsertedSpy.takeFirst().at(0).value<KItemRangeList>();
+        QCOMPARE(itemRangeList, KItemRangeList() << KItemRange(0, 1)); // 1 new item "a/" with index 0
+        QCOMPARE(m_model->expandedParentsCount(0), 0);
+
+    }
+    itemsInsertedSpy.clear();
+
+    // Expand the folder "a/" -> "a/b" become visible
+    m_model->setExpanded(0, true);
+    QVERIFY(m_model->isExpanded(0));
+    QVERIFY(itemsInsertedSpy.wait());
+    QCOMPARE(m_model->count(), 2); // 3 items: "a/", "a/a/"
+    QCOMPARE(m_model->expandedDirectories(), {QUrl::fromLocalFile(m_testDir->path() + "/a")});
+
+    QCOMPARE(itemsInsertedSpy.count(), 1);
+    {
+        KItemRangeList itemRangeList = itemsInsertedSpy.takeFirst().at(0).value<KItemRangeList>();
+        QCOMPARE(itemRangeList, KItemRangeList() << KItemRange(1, 1)); // 1 new item "a/b" with index 1
+        QCOMPARE(m_model->expandedParentsCount(1), 1);
+
+    }
+    itemsInsertedSpy.clear();
+
+    // Expand "a/b" -> "a/b/1" becomes visible
+    m_model->setExpanded(1, true);
+    QVERIFY(itemsInsertedSpy.wait());
+    QCOMPARE(m_model->expandedDirectories(), QSet({QUrl::fromLocalFile(m_testDir->path() + "/a"),
+        QUrl::fromLocalFile(m_testDir->path() + "/a/b")}));
+
+    QCOMPARE(itemsInsertedSpy.count(), 1);
+    {
+        KItemRangeList itemRangeList = itemsInsertedSpy.takeFirst().at(0).value<KItemRangeList>();
+        QCOMPARE(itemRangeList, KItemRangeList() << KItemRange(2, 1)); // 1 new item "a/b/1" with index 2
+        QCOMPARE(m_model->expandedParentsCount(2), 2);
+    }
+    itemsInsertedSpy.clear();
+
+    // Collapse "a" whilst leaving "b" expanded
+    m_model->setExpanded(0, false);
+
+    // Insert additional files into "a/b/"
+    m_testDir->createFile("a/b/2");
+    #if KIO_VERSION < QT_VERSION_CHECK(5, 92, 0)
+        QEXPECT_FAIL("", "Requires new API from frameworks", Abort);
+    #endif
+
+    QVERIFY(!itemsInsertedSpy.wait(5000));
+
+    QCOMPARE(itemsInModel(), {"a"});
+
+    m_model->setExpanded(0, true);;
+    QTRY_COMPARE(itemsInModel(), QStringList({"a", "b", "1", "2"}));
+
+    QCOMPARE(m_model->expandedParentsCount(0), 0); // a
+    QCOMPARE(m_model->expandedParentsCount(1), 1); // a/b
+    QCOMPARE(m_model->expandedParentsCount(2), 2); // a/b/1
+    QCOMPARE(m_model->expandedParentsCount(3), 2) ;// a/b/2
+
+}
+
 QStringList KFileItemModelTest::itemsInModel() const
 {
     QStringList items;