natural sort: exclude extension when comparing filenames

Currently natural sort compares the entire filenames
(basename.extension) when sorting. This causes eg.
"a 2.txt" to appear before "a.txt" when sorted by ascending.
This is unintuitive since people prioritize basenames more
than file extensions.

Instead, change natural sort to compare by basename only and
fallback to comparing extensions if basenames were equal.
This change causes "a.txt" to appear before "a 2.txt" and
matches how other platforms such as GNOME and Windows behave.

BUG: 416025
BUG: 470538
BUG: 421869
BUG: 312027

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index fb5851c36c52ee07ba04cc4eaaa615c16c86d387..f8c30b9e45d42ff04404240b346f0f3dd283534a 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -2249,7 +2249,24 @@ int KFileItemModel::stringCompare(const QString &a, const QString &b, const QCol
     QMutexLocker collatorLock(s_collatorMutex());
 
     if (m_naturalSorting) {
-        return collator.compare(a, b);
+        // Split extension, taking into account it can be empty
+        constexpr QString::SectionFlags flags = QString::SectionSkipEmpty | QString::SectionIncludeLeadingSep;
+
+        // Sort by baseName first
+        const QString aBaseName = a.section('.', 0, 0, flags);
+        const QString bBaseName = b.section('.', 0, 0, flags);
+
+        const int res = collator.compare(aBaseName, bBaseName);
+        if (res != 0 || (aBaseName.length() == a.length() && bBaseName.length() == b.length())) {
+            return res;
+        }
+
+        // sliced() has undefined behavior when pos < 0 or pos > size().
+        Q_ASSERT(aBaseName.length() <= a.length() && aBaseName.length() >= 0);
+        Q_ASSERT(bBaseName.length() <= b.length() && bBaseName.length() >= 0);
+
+        // baseNames were equal, sort by extension
+        return collator.compare(a.sliced(aBaseName.length()), b.sliced(bBaseName.length()));
     }
 
     const int result = QString::compare(a, b, collator.caseSensitivity());

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index 5662d4fa8b00eda322e02caf000288bd42848b74..10be27128832d3bb8e02ff87bdf55a23ec2d0de4 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -365,7 +365,11 @@ private:
         ItemData *parent;
     };
 
-    enum RemoveItemsBehavior { KeepItemData, DeleteItemData, DeleteItemDataIfUnfiltered };
+    enum RemoveItemsBehavior {
+        KeepItemData,
+        DeleteItemData,
+        DeleteItemDataIfUnfiltered
+    };
 
     void insertItems(QList<ItemData *> &items);
     void removeItems(const KItemRangeList &itemRanges, RemoveItemsBehavior behavior);
@@ -588,7 +592,9 @@ inline bool KFileItemModel::isRoleValueNatural(RoleType roleType)
 
 inline bool KFileItemModel::nameLessThan(const ItemData *a, const ItemData *b)
 {
-    return a->item.text() < b->item.text();
+    // Split extension, taking into account it can be empty
+    constexpr QString::SectionFlags flags = QString::SectionSkipEmpty | QString::SectionIncludeLeadingSep;
+    return a->item.text().section('.', 0, 0, flags) < b->item.text().section('.', 0, 0, flags);
 }
 
 inline bool KFileItemModel::isChildItem(int index) const

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 7af5690ff30c5334da8305bde490286478a6b2e0..4b1814391cb5cb9a98bcac7d34253a4c77069bea 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -69,6 +69,7 @@ private Q_SLOTS:
     void testMakeExpandedItemHidden();
     void testRemoveFilteredExpandedItems();
     void testSorting();
+    void testNaturalSorting();
     void testIndexForKeyboardSearch();
     void testNameFilter();
     void testEmptyPath();
@@ -1275,6 +1276,83 @@ void KFileItemModelTest::testSorting()
     QCOMPARE(itemsMovedSpy.takeFirst().at(1).value<QList<int>>(), QList<int>() << 2 << 4 << 5 << 3 << 0 << 1 << 6 << 7 << 9 << 8);
 }
 
+void KFileItemModelTest::testNaturalSorting()
+{
+    QSignalSpy itemsInsertedSpy(m_model, &KFileItemModel::itemsInserted);
+    QSignalSpy itemsMovedSpy(m_model, &KFileItemModel::itemsMoved);
+    QVERIFY(itemsMovedSpy.isValid());
+
+    m_model->setSortRole("text");
+    m_model->setShowHiddenFiles(true);
+
+    m_testDir->createFiles({".a", "a.txt", "b.txt", "a 1.txt", "a 2.txt", "a 10.txt", "a.tar", "b.tar"});
+
+    m_model->loadDirectory(m_testDir->url());
+    QVERIFY(itemsInsertedSpy.wait());
+
+    // Sort by Name, ascending, natural sorting enabled
+    QCOMPARE(m_model->sortRole(), QByteArray("text"));
+    QCOMPARE(m_model->sortOrder(), Qt::AscendingOrder);
+    QCOMPARE(itemsInModel(),
+             QStringList() << ".a"
+                           << "a.tar"
+                           << "a.txt"
+                           << "a 1.txt"
+                           << "a 2.txt"
+                           << "a 10.txt"
+                           << "b.tar"
+                           << "b.txt");
+
+    // Sort by Extension
+    m_model->setSortRole("extension");
+    QCOMPARE(m_model->sortRole(), QByteArray("extension"));
+    QCOMPARE(itemsInModel(),
+             QStringList() << ".a"
+                           << "a.tar"
+                           << "b.tar"
+                           << "a.txt"
+                           << "a 1.txt"
+                           << "a 2.txt"
+                           << "a 10.txt"
+                           << "b.txt");
+    QCOMPARE(itemsMovedSpy.count(), 1);
+    QCOMPARE(itemsMovedSpy.first().at(0).value<KItemRange>(), KItemRange(2, 5));
+    QCOMPARE(itemsMovedSpy.takeFirst().at(1).value<QList<int>>(), QList<int>() << 3 << 4 << 5 << 6 << 2);
+
+    // Disable natural sorting, refresh directory for the change to take effect
+    m_model->m_naturalSorting = false;
+    m_model->refreshDirectory(m_model->directory());
+    QVERIFY(itemsInsertedSpy.wait());
+
+    // Sort by Extension, ascending, natural sorting disabled
+    QCOMPARE(m_model->sortRole(), QByteArray("extension"));
+    QCOMPARE(itemsInModel(),
+             QStringList() << ".a"
+                           << "a.tar"
+                           << "b.tar"
+                           << "a 1.txt"
+                           << "a 10.txt"
+                           << "a 2.txt"
+                           << "a.txt"
+                           << "b.txt");
+
+    // Sort by Name
+    m_model->setSortRole("text");
+    QCOMPARE(m_model->sortRole(), QByteArray("text"));
+    QCOMPARE(itemsInModel(),
+             QStringList() << ".a"
+                           << "a 1.txt"
+                           << "a 10.txt"
+                           << "a 2.txt"
+                           << "a.tar"
+                           << "a.txt"
+                           << "b.tar"
+                           << "b.txt");
+    QCOMPARE(itemsMovedSpy.count(), 1);
+    QCOMPARE(itemsMovedSpy.first().at(0).value<KItemRange>(), KItemRange(1, 6));
+    QCOMPARE(itemsMovedSpy.takeFirst().at(1).value<QList<int>>(), QList<int>() << 4 << 6 << 1 << 2 << 3 << 5);
+}
+
 void KFileItemModelTest::testIndexForKeyboardSearch()
 {
     QSignalSpy itemsInsertedSpy(m_model, &KFileItemModel::itemsInserted);