Make expandedParentsCount() work without accessing the data hash

The idea is that we no longer assume that the "expandedParentsCount"
for each item will be stored in the QHash. It is only accessed for
items which are expanded, and which are not top-level items (i.e.,
which have an expandedParentsCount > 1).

Some unit tests are added to improve the coverage of the affected code.

REVIEW: 112562

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index 37a30519af74277a985d977cd2acbc0eb8bb3618..51359bbce2e4a8a48e200b1e6ac690c84dbdcd48 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -494,10 +494,7 @@ bool KFileItemModel::isExpandable(int index) const
 int KFileItemModel::expandedParentsCount(int index) const
 {
     if (index >= 0 && index < count()) {
-        const int parentsCount = m_itemData.at(index)->values.value("expandedParentsCount").toInt();
-        if (parentsCount > 0) {
-            return parentsCount;
-        }
+        return expandedParentsCount(m_itemData.at(index));
     }
     return 0;
 }
@@ -1234,6 +1231,23 @@ QList<KFileItemModel::ItemData*> KFileItemModel::createItemDataList(const KUrl&
     return itemDataList;
 }
 
+int KFileItemModel::expandedParentsCount(const ItemData* data)
+{
+    // The hash 'values' is only guaranteed to contain the key "expandedParentsCount"
+    // if the corresponding item is expanded, and it is not a top-level item.
+    const ItemData* parent = data->parent;
+    if (parent) {
+        if (parent->parent) {
+            Q_ASSERT(parent->values.contains("expandedParentsCount"));
+            return parent->values.value("expandedParentsCount").toInt() + 1;
+        } else {
+            return 1;
+        }
+    } else {
+        return 0;
+    }
+}
+
 void KFileItemModel::removeExpandedItems()
 {
     KFileItemList expandedItems;
@@ -1241,15 +1255,12 @@ void KFileItemModel::removeExpandedItems()
     const int maxIndex = m_itemData.count() - 1;
     for (int i = 0; i <= maxIndex; ++i) {
         const ItemData* itemData = m_itemData.at(i);
-        if (itemData->values.value("expandedParentsCount").toInt() > 0) {
+        if (itemData->parent) {
             expandedItems.append(itemData->item);
         }
     }
 
-    // The m_expandedParentsCountRoot may not get reset before all items with
-    // a bigger count have been removed.
     removeItems(expandedItems, DeleteItemData);
-
     m_expandedDirs.clear();
 }
 
@@ -1394,7 +1405,7 @@ QHash<QByteArray, QVariant> KFileItemModel::retrieveData(const KFileItem& item,
 
     if (m_requestRole[ExpandedParentsCountRole]) {
         if (parent) {
-            const int level = parent->values["expandedParentsCount"].toInt() + 1;
+            const int level = expandedParentsCount(parent) + 1;
             data.insert(sharedValue("expandedParentsCount"), level);
         }
     }
@@ -1418,8 +1429,8 @@ bool KFileItemModel::lessThan(const ItemData* a, const ItemData* b) const
     int result = 0;
 
     if (a->parent != b->parent) {
-        const int expansionLevelA = a->values.value("expandedParentsCount").toInt();
-        const int expansionLevelB = b->values.value("expandedParentsCount").toInt();
+        const int expansionLevelA = expandedParentsCount(a);
+        const int expansionLevelB = expandedParentsCount(b);
 
         // If b has a higher expansion level than a, check if a is a parent
         // of b, and make sure that both expansion levels are equal otherwise.
@@ -1439,7 +1450,7 @@ bool KFileItemModel::lessThan(const ItemData* a, const ItemData* b) const
             a = a->parent;
         }
 
-        Q_ASSERT(a->values.value("expandedParentsCount").toInt() == b->values.value("expandedParentsCount").toInt());
+        Q_ASSERT(expandedParentsCount(a) == expandedParentsCount(b));
 
         // Compare the last parents of a and b which are different.
         while (a->parent != b->parent) {
@@ -1938,9 +1949,9 @@ KFileItemList KFileItemModel::childItems(const KFileItem& item) const
 
     int index = m_items.value(item.url(), -1);
     if (index >= 0) {
-        const int parentLevel = m_itemData.at(index)->values.value("expandedParentsCount").toInt();
+        const int parentLevel = expandedParentsCount(index);
         ++index;
-        while (index < m_itemData.count() && m_itemData.at(index)->values.value("expandedParentsCount").toInt() > parentLevel) {
+        while (index < m_itemData.count() && expandedParentsCount(index) > parentLevel) {
             items.append(m_itemData.at(index)->item);
             ++index;
         }
@@ -2075,7 +2086,7 @@ bool KFileItemModel::isConsistent() const
         const ItemData* data = m_itemData.at(i);
         const ItemData* parent = data->parent;
         if (parent) {
-            if (data->values.value("expandedParentsCount").toInt() != parent->values.value("expandedParentsCount").toInt() + 1) {
+            if (expandedParentsCount(data) != expandedParentsCount(parent) + 1) {
                 qWarning() << "expandedParentsCount is inconsistent for parent" << parent->item << "and child" << data->item;
                 return false;
             }

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index 5917e681848f2fc2a3d46d397380d6578393b63f..f364e7ce0f406881a9454b54e8947f2a73b32dd4 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -319,6 +319,8 @@ private:
      */
     QList<ItemData*> createItemDataList(const KUrl& parentUrl, const KFileItemList& items) const;
 
+    static int expandedParentsCount(const ItemData* data);
+
     void removeExpandedItems();
 
     /**

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index e55e3eb332db313e57be4e74f9b57c4c4dbb6ed4..1d9646b3efb2a5175b71d41ceba21db3538a2bf8 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -486,7 +486,8 @@ void KFileItemModelTest::testExpandItems()
     // KFileItemModel::expansionLevelsCompare(const KFileItem& a, const KFileItem& b)
     // yields the correct result for "a/a/1" and "a/a-1/", whis is non-trivial because they share the
     // first three characters.
-    QSet<QByteArray> modelRoles = m_model->roles();
+    QSet<QByteArray> originalModelRoles = m_model->roles();
+    QSet<QByteArray> modelRoles = originalModelRoles;
     modelRoles << "isExpanded" << "isExpandable" << "expandedParentsCount";
     m_model->setRoles(modelRoles);
 
@@ -593,6 +594,18 @@ void KFileItemModelTest::testExpandItems()
     QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(directoryLoadingCompleted()), DefaultTimeout));
     QCOMPARE(m_model->count(), 5);  // 5 items: "a/", "a/a/", "a/a/1", "a/a-1/", "a/a-1/1"
     QCOMPARE(m_model->expandedDirectories(), allFolders);
+
+    // Remove all expanded items by changing the roles
+    spyRemoved.clear();
+    m_model->setRoles(originalModelRoles);
+    QVERIFY(!m_model->isExpanded(0));
+    QCOMPARE(m_model->count(), 1);
+    QVERIFY(!m_model->expandedDirectories().contains(KUrl(m_testDir->name() + 'a')));
+
+    QCOMPARE(spyRemoved.count(), 1);
+    itemRangeList = spyRemoved.takeFirst().at(0).value<KItemRangeList>();
+    QCOMPARE(itemRangeList, KItemRangeList() << KItemRange(1, 4)); // 4 items removed
+    QVERIFY(m_model->isConsistent());
 }
 
 void KFileItemModelTest::testExpandParentItems()
@@ -645,6 +658,28 @@ void KFileItemModelTest::testExpandParentItems()
     QVERIFY(m_model->isExpanded(3));
     QVERIFY(!m_model->isExpanded(4));
     QVERIFY(m_model->isConsistent());
+
+    // Expand "a 1/b1/".
+    m_model->setExpanded(1, true);
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(directoryLoadingCompleted()), DefaultTimeout));
+    QCOMPARE(m_model->count(), 6);
+    QVERIFY(m_model->isExpanded(0));
+    QVERIFY(m_model->isExpanded(1));
+    QVERIFY(!m_model->isExpanded(2));
+    QVERIFY(m_model->isExpanded(3));
+    QVERIFY(m_model->isExpanded(4));
+    QVERIFY(!m_model->isExpanded(5));
+    QVERIFY(m_model->isConsistent());
+
+    // Collapse "a 1/b1/" again, and verify that the previous state is restored.
+    m_model->setExpanded(1, false);
+    QCOMPARE(m_model->count(), 5);
+    QVERIFY(m_model->isExpanded(0));
+    QVERIFY(!m_model->isExpanded(1));
+    QVERIFY(m_model->isExpanded(2));
+    QVERIFY(m_model->isExpanded(3));
+    QVERIFY(!m_model->isExpanded(4));
+    QVERIFY(m_model->isConsistent());
 }
 
 /**