Update the roles for filtered items if necessary

Since Dolphin 4.11, we store not only KFileItems, but also the
corresponding ItemData struct for filtered items. This is required for
keeping track of the parent-child relationships, and has the nice side
effect that the ItemData need not be re-determined when the items are
shown again.

However, this can become a problem if the visible roles or the sort role
change while some items are filtered.

This is fixed by is fixed by clearing the QHash "values" for the
filtered items if the visible roles change. The hash will be
re-populated with all requested data as soon as the items are shown
again and the data(int) method of the model is called.

Moreover, before the items are inserted into the model after filtering,
we have to make sure that the sort role "Permissions"/"User"/etc. is
present in the hash "values". This is achieved by factoring out the code
that currently does this job for new items in createItemDataList() into
a new function, and calling this in insertItems(), because the same
treatment is required for the previously filtered files.

BUG: 328791
FIXED-IN: 4.12.1
REVIEW: 114266

3 files changed, 130 insertions, 7 deletions

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index 4c8577543..739384bf9 100644
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -413,6 +413,15 @@ void KFileItemModel::setRoles(const QSet<QByteArray>& roles)
         kWarning() << "TODO: Emitting itemsChanged() with no information what has changed!";
         emit itemsChanged(KItemRangeList() << KItemRange(0, count()), QSet<QByteArray>());
     }
+
+    // Clear the 'values' of all filtered items. They will be re-populated with the
+    // correct roles the next time 'values' will be accessed via data(int).
+    QHash<KFileItem, ItemData*>::iterator filteredIt = m_filteredItems.begin();
+    const QHash<KFileItem, ItemData*>::iterator filteredEnd = m_filteredItems.end();
+    while (filteredIt != filteredEnd) {
+        (*filteredIt)->values.clear();
+        ++filteredIt;
+    }
 }
 
 QSet<QByteArray> KFileItemModel::roles() const
@@ -1033,6 +1042,7 @@ void KFileItemModel::insertItems(QList<ItemData*>& newItems)
 #endif
 
     m_groups.clear();
+    prepareItemsForSorting(newItems);
 
     if (m_sortRole == NameRole && m_naturalSorting) {
         // Natural sorting of items can be very slow. However, it becomes much
@@ -1196,6 +1206,11 @@ QList<KFileItemModel::ItemData*> KFileItemModel::createItemDataList(const KUrl&
         itemDataList.append(itemData);
     }
 
+    return itemDataList;
+}
+
+void KFileItemModel::prepareItemsForSorting(QList<ItemData*>& itemDataList)
+{
     switch (m_sortRole) {
     case PermissionsRole:
     case OwnerRole:
@@ -1205,16 +1220,20 @@ QList<KFileItemModel::ItemData*> KFileItemModel::createItemDataList(const KUrl&
         // These roles can be determined with retrieveData, and they have to be stored
         // in the QHash "values" for the sorting.
         foreach (ItemData* itemData, itemDataList) {
-            itemData->values = retrieveData(itemData->item, parentItem);
+            if (itemData->values.isEmpty()) {
+                itemData->values = retrieveData(itemData->item, itemData->parent);
+            }
         }
         break;
 
     case TypeRole:
         // At least store the data including the file type for items with known MIME type.
         foreach (ItemData* itemData, itemDataList) {
-            const KFileItem item = itemData->item;
-            if (item.isDir() || item.isMimeTypeKnown()) {
-                itemData->values = retrieveData(itemData->item, parentItem);
+            if (itemData->values.isEmpty()) {
+                const KFileItem item = itemData->item;
+                if (item.isDir() || item.isMimeTypeKnown()) {
+                    itemData->values = retrieveData(itemData->item, itemData->parent);
+                }
             }
         }
         break;
@@ -1223,12 +1242,10 @@ QList<KFileItemModel::ItemData*> KFileItemModel::createItemDataList(const KUrl&
         // The other roles are either resolved by KFileItemModelRolesUpdater
         // (this includes the SizeRole for directories), or they do not need
         // to be stored in the QHash "values" for sorting because the data can
-        // be retrieved directly from the KFileItem (NameRole, SiezRole for files,
+        // be retrieved directly from the KFileItem (NameRole, SizeRole for files,
         // DateRole).
         break;
     }
-
-    return itemDataList;
 }
 
 int KFileItemModel::expandedParentsCount(const ItemData* data)
diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index c57329fc8..022429023 100644
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -320,6 +320,13 @@ private:
      */
     QList<ItemData*> createItemDataList(const KUrl& parentUrl, const KFileItemList& items) const;
 
+    /**
+     * Prepares the items for sorting. Normally, the hash 'values' in ItemData is filled
+     * lazily to save time and memory, but for some sort roles, it is expected that the
+     * sort role data is stored in 'values'.
+     */
+    void prepareItemsForSorting(QList<ItemData*>& itemDataList);
+
     static int expandedParentsCount(const ItemData* data);
 
     void removeExpandedItems();
diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 62ff4fae2..ede9a3d6e 100644
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -90,6 +90,8 @@ private slots:
     void testNameRoleGroups();
     void testNameRoleGroupsWithExpandedItems();
     void testInconsistentModel();
+    void testChangeRolesForFilteredItems();
+    void testChangeSortRoleWhileFiltering();
 
 private:
     QStringList itemsInModel() const;
@@ -1462,6 +1464,103 @@ void KFileItemModelTest::testInconsistentModel()
 
 }
 
+void KFileItemModelTest::testChangeRolesForFilteredItems()
+{
+    QSet<QByteArray> modelRoles = m_model->roles();
+    modelRoles << "owner";
+    m_model->setRoles(modelRoles);
+
+    QStringList files;
+    files << "a.txt" << "aa.txt" << "aaa.txt";
+    m_testDir->createFiles(files);
+
+    m_model->loadDirectory(m_testDir->url());
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "aa.txt" << "aaa.txt");
+
+    for (int index = 0; index < m_model->count(); ++index) {
+        // All items should have the "text" and "owner" roles, but not "group".
+        QVERIFY(m_model->data(index).contains("text"));
+        QVERIFY(m_model->data(index).contains("owner"));
+        QVERIFY(!m_model->data(index).contains("group"));
+    }
+
+    // Add a filter, such that only "aaa.txt" remains in the model.
+    m_model->setNameFilter("aaa");
+    QCOMPARE(itemsInModel(), QStringList() << "aaa.txt");
+
+    // Add the "group" role.
+    modelRoles << "group";
+    m_model->setRoles(modelRoles);
+
+    // Modify the filter, such that "aa.txt" reappears, and verify that all items have the expected roles.
+    m_model->setNameFilter("aa");
+    QCOMPARE(itemsInModel(), QStringList() << "aa.txt" << "aaa.txt");
+
+    for (int index = 0; index < m_model->count(); ++index) {
+        // All items should have the "text", "owner", and "group" roles.
+        QVERIFY(m_model->data(index).contains("text"));
+        QVERIFY(m_model->data(index).contains("owner"));
+        QVERIFY(m_model->data(index).contains("group"));
+    }
+
+    // Remove the "owner" role.
+    modelRoles.remove("owner");
+    m_model->setRoles(modelRoles);
+
+    // Clear the filter, and verify that all items have the expected roles
+    m_model->setNameFilter(QString());
+    QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "aa.txt" << "aaa.txt");
+
+    for (int index = 0; index < m_model->count(); ++index) {
+        // All items should have the "text" and "group" roles, but now "owner".
+        QVERIFY(m_model->data(index).contains("text"));
+        QVERIFY(!m_model->data(index).contains("owner"));
+        QVERIFY(m_model->data(index).contains("group"));
+    }
+}
+
+void KFileItemModelTest::testChangeSortRoleWhileFiltering()
+{
+    KFileItemList items;
+
+    KIO::UDSEntry entry;
+    entry.insert(KIO::UDSEntry::UDS_FILE_TYPE, 0100000);    // S_IFREG might not be defined on non-Unix platforms.
+    entry.insert(KIO::UDSEntry::UDS_ACCESS, 07777);
+    entry.insert(KIO::UDSEntry::UDS_SIZE, 0);
+    entry.insert(KIO::UDSEntry::UDS_MODIFICATION_TIME, 0);
+    entry.insert(KIO::UDSEntry::UDS_GROUP, "group");
+    entry.insert(KIO::UDSEntry::UDS_ACCESS_TIME, 0);
+
+    entry.insert(KIO::UDSEntry::UDS_NAME, "a.txt");
+    entry.insert(KIO::UDSEntry::UDS_USER, "user-b");
+    items.append(KFileItem(entry, m_testDir->url(), false, true));
+
+    entry.insert(KIO::UDSEntry::UDS_NAME, "b.txt");
+    entry.insert(KIO::UDSEntry::UDS_USER, "user-c");
+    items.append(KFileItem(entry, m_testDir->url(), false, true));
+
+    entry.insert(KIO::UDSEntry::UDS_NAME, "c.txt");
+    entry.insert(KIO::UDSEntry::UDS_USER, "user-a");
+    items.append(KFileItem(entry, m_testDir->url(), false, true));
+
+    m_model->slotItemsAdded(m_testDir->url(), items);
+    m_model->slotCompleted();
+
+    QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "b.txt" << "c.txt");
+
+    // Add a filter.
+    m_model->setNameFilter("a");
+    QCOMPARE(itemsInModel(), QStringList() << "a.txt");
+
+    // Sort by "owner".
+    m_model->setSortRole("owner");
+
+    // Clear the filter, and verify that the items are sorted correctly.
+    m_model->setNameFilter(QString());
+    QCOMPARE(itemsInModel(), QStringList() << "c.txt" << "a.txt" << "b.txt");
+}
+
 QStringList KFileItemModelTest::itemsInModel() const
 {
     QStringList items;