From b6e03e05f4b63dc86c763f73ec863137ea59d2f0 Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Fri, 7 Jan 2022 17:05:30 +0000 Subject: [PATCH] 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. --- src/kitemviews/kfileitemmodel.cpp | 7 +++ src/tests/kfileitemmodeltest.cpp | 93 +++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 23afb087a..6391d7d3f 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -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 679b8ab3a..e9bb7efb7 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -14,6 +14,8 @@ #include #include +#include + #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 originalModelRoles = m_model->roles(); + QSet 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(); + 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(); + 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(); + 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; -- 2.47.3