Always sort items correctly when the refreshItems() signal is received

When sorting by, e.g., "Size", and the name is used as a fallback
because there are multiple files with the same size, the refreshItems
signal that is received when a file's name is changed either with the
dialog or outside the current view did not cause the view to be resorted
after commit d70a4811807776966c3241a72121242f4d1eaee8. This patch fixes
it.

BUG: 324713
FIXED-IN: 4.11.2
REVIEW: 112561

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index 7bbc1d17f46d1736bc0aa4c48db80e63f8a7880f..b6b6ee0e2f38492bbec055c82c0a4fd6d366eda8 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -190,33 +190,7 @@ bool KFileItemModel::setData(int index, const QHash<QByteArray, QVariant>& value
         m_itemData[index]->item.setUrl(url);
     }
 
-    emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles);
-
-    // Trigger a resorting if the item's correct position has changed. Note
-    // that this can happen even if the sort role has not changed at all
-    // because the file name can be used as a fallback.
-    if (changedRoles.contains(sortRole()) || changedRoles.contains(roleForType(NameRole))) {
-        // Compare the changed item with its neighbors to see
-        // if an expensive resorting is needed at all.
-        const ItemData* changedItem = m_itemData.at(index);
-        const ItemData* previousItem = (index == 0) ? 0 : m_itemData.at(index - 1);
-        const ItemData* nextItem = (index == m_itemData.count() - 1) ? 0 : m_itemData.at(index + 1);
-
-        if ((previousItem && lessThan(changedItem, previousItem))
-            || (nextItem && lessThan(nextItem, changedItem))) {
-            m_resortAllItemsTimer->start();
-        } else if (groupedSorting() && changedRoles.contains(sortRole())) {
-            // The position is still correct, but the groups might have changed
-            // if the changed item is either the first or the last item in a
-            // group.
-            // In principle, we could try to find out if the item really is the
-            // first or last one in its group and then update the groups
-            // (possibly with a delayed timer to make sure that we don't
-            // re-calculate the groups very often if items are updated one by
-            // one), but starting m_resortAllItemsTimer is easier.
-            m_resortAllItemsTimer->start();
-        }
-    }
+    emitItemsChangedAndTriggerResorting(KItemRangeList() << KItemRange(index, 1), changedRoles);
 
     return true;
 }
@@ -945,11 +919,7 @@ void KFileItemModel::slotRefreshItems(const QList<QPair<KFileItem, KFileItem> >&
         itemRangeList.append(KItemRange(rangeIndex, rangeCount));
     }
 
-    emit itemsChanged(itemRangeList, changedRoles);
-
-    if (changedRoles.contains(sortRole())) {
-        m_resortAllItemsTimer->start();
-    }
+    emitItemsChangedAndTriggerResorting(itemRangeList, changedRoles);
 }
 
 void KFileItemModel::slotClear()
@@ -1229,6 +1199,58 @@ void KFileItemModel::removeExpandedItems()
     m_expandedDirs.clear();
 }
 
+void KFileItemModel::emitItemsChangedAndTriggerResorting(const KItemRangeList& itemRanges, const QSet<QByteArray>& changedRoles)
+{
+    emit itemsChanged(itemRanges, changedRoles);
+
+    // Trigger a resorting if necessary. Note that this can happen even if the sort
+    // role has not changed at all because the file name can be used as a fallback.
+    if (changedRoles.contains(sortRole()) || changedRoles.contains(roleForType(NameRole))) {
+        foreach (const KItemRange& range, itemRanges) {
+            bool needsResorting = false;
+
+            const int first = range.index;
+            const int last = range.index + range.count - 1;
+
+            // Resorting the model is necessary if
+            // (a)  The first item in the range is "lessThan" its predecessor,
+            // (b)  the successor of the last item is "lessThan" the last item, or
+            // (c)  the internal order of the items in the range is incorrect.
+            if (first > 0
+                && lessThan(m_itemData.at(first), m_itemData.at(first - 1))) {
+                needsResorting = true;
+            } else if (last < count() - 1
+                && lessThan(m_itemData.at(last + 1), m_itemData.at(last))) {
+                needsResorting = true;
+            } else {
+                for (int index = first; index < last; ++index) {
+                    if (lessThan(m_itemData.at(index + 1), m_itemData.at(index))) {
+                        needsResorting = true;
+                        break;
+                    }
+                }
+            }
+
+            if (needsResorting) {
+                m_resortAllItemsTimer->start();
+                return;
+            }
+        }
+    }
+
+    if (groupedSorting() && changedRoles.contains(sortRole())) {
+        // The position is still correct, but the groups might have changed
+        // if the changed item is either the first or the last item in a
+        // group.
+        // In principle, we could try to find out if the item really is the
+        // first or last one in its group and then update the groups
+        // (possibly with a delayed timer to make sure that we don't
+        // re-calculate the groups very often if items are updated one by
+        // one), but starting m_resortAllItemsTimer is easier.
+        m_resortAllItemsTimer->start();
+    }
+}
+
 void KFileItemModel::resetRoles()
 {
     for (int i = 0; i < RolesCount; ++i) {

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index 5917e681848f2fc2a3d46d397380d6578393b63f..c87ee9736d3812b5a5296b097efeec3da02df106 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -321,6 +321,13 @@ private:
 
     void removeExpandedItems();
 
+    /**
+     * This function is called by setData() and slotRefreshItems(). It emits
+     * the itemsChanged() signal, checks if the sort order is still correct,
+     * and starts m_resortAllItemsTimer if that is not the case.
+     */
+    void emitItemsChangedAndTriggerResorting(const KItemRangeList& itemRanges, const QSet<QByteArray>& changedRoles);
+
     /**
      * Resets all values from m_requestRole to false.
      */

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index b0a27cd01267628e0b7222ed1ffaedc782127df6..391fe5be5878c7f20df684ad0e162216f14f00c2 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -391,6 +391,18 @@ void KFileItemModelTest::testResortAfterChangingName()
 
     QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList<int>)), DefaultTimeout));
     QCOMPARE(itemsInModel(), QStringList() << "b.txt" << "c.txt" << "d.txt");
+
+    // We rename d.txt back to a.txt using the dir lister's refreshItems() signal.
+    const KFileItem fileItemD = m_model->fileItem(2);
+    KFileItem fileItemA = fileItemD;
+    KUrl urlA = fileItemA.url();
+    urlA.setFileName("a.txt");
+    fileItemA.setUrl(urlA);
+
+    m_model->slotRefreshItems(QList<QPair<KFileItem, KFileItem> >() << qMakePair(fileItemD, fileItemA));
+
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList<int>)), DefaultTimeout));
+    QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "b.txt" << "c.txt");
 }
 
 void KFileItemModelTest::testModelConsistencyWhenInsertingItems()