[Models] Use QCollator for natural, cached sorting (#7633)

* [Models] Use QCollator for natural, cached sorting

QString::localeAwareCompare rebuilds its collation data for every
comparison, and it compares numbers as text. Keep one QCollator per model,
enable numeric mode, and use it for the card name, set and user name sorts.

Add a test that sorts "Forest 1", "Forest 2", "Forest 10" through
DeckListSortFilterProxyModel and checks the natural order.

* [Models] Sort naturally under the C locale too

Numeric mode only takes effect through a real locale: under the C locale,
which headless CI processes run in, QCollator silently falls back to string
order and sorts "Forest 10" before "Forest 2". That made collator_sort_test
fail on every Linux leg while passing locally.

Add makeNaturalSortCollator(), which enables numeric mode and pins English
only when the process locale is the degenerate one, and build the models'
collators through it. Users with a real locale keep their own collation.

* [Models] Fix compile of natural sorting (QStringView mismatch)

* nit: initialize collators in member initializer lists

* nit: namespace makeNaturalSortCollator under NaturalSort

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/interface/widgets/server/user/user_list_widget.cpp

@@ -16,6 +16,7 @@
 #include <QAbstractItemModel>
 #include <QApplication>
 #include <QByteArray>
+#include <QCollator>
 #include <QColor>
 #include <QContextMenuEvent>
 #include <QCursor>
@@ -55,6 +56,7 @@
 #include <QVariant>
 #include <QWidget>
 #include <QtGlobal>
+#include <libcockatrice/models/natural_sort_collator.h>
 #include <libcockatrice/settings/appearance_settings.h>
 #include <libcockatrice/utility/qt_utils.h>
 #include <qnamespace.h>
@@ -272,6 +274,11 @@ void UserListTWI::setOnline(bool online)
  */
 bool UserListTWI::operator<(const QTreeWidgetItem &other) const
 {
+    // QCollator caches the locale data, unlike QString::localeAwareCompare,
+    // which rebuilds it for every pair. The tree sorts one list at a time, so a
+    // single shared, immutable collator is enough.
+    static const QCollator collator = NaturalSort::makeNaturalSortCollator();
+
     // Sort by online/offline
     if (data(0, UserListRoles::Online) != other.data(0, UserListRoles::Online)) {
         return data(0, UserListRoles::Online).toBool();
@@ -287,8 +294,7 @@ bool UserListTWI::operator<(const QTreeWidgetItem &other) const
     for (const auto &userLevelEntry : userLevelWithNoOtherPrefOrder) {
         if (lhsUserLevelFlags.testFlag(userLevelEntry) &&
             lhsUserLevelFlags.testFlag(userLevelEntry) == rhsUserLevelFlags.testFlag(userLevelEntry)) {
-            return QString::localeAwareCompare(data(2, Qt::UserRole).toString(),
+            return collator.compare(data(2, Qt::UserRole).toString(), other.data(2, Qt::UserRole).toString()) < 0;
-                                               other.data(2, Qt::UserRole).toString()) < 0;
         } else if (lhsUserLevelFlags.testFlag(userLevelEntry) != rhsUserLevelFlags.testFlag(userLevelEntry)) {
             return lhsUserLevelFlags.testFlag(userLevelEntry) > rhsUserLevelFlags.testFlag(userLevelEntry);
         }
@@ -313,7 +319,7 @@ bool UserListTWI::operator<(const QTreeWidgetItem &other) const
     }
 
     // Sort by name
-    return QString::localeAwareCompare(data(2, Qt::UserRole).toString(), other.data(2, Qt::UserRole).toString()) < 0;
+    return collator.compare(data(2, Qt::UserRole).toString(), other.data(2, Qt::UserRole).toString()) < 0;
 }
 
 UserListWidget::UserListWidget(TabSupervisor *_tabSupervisor,

libcockatrice_models/libcockatrice/models/database/card_database_display_model.cpp

@@ -1,5 +1,6 @@
 #include "card_database_display_model.h"
 
+#include "../natural_sort_collator.h"
 #include "card_database_model.h"
 #include "libcockatrice/card/card_localization.h"
 #include "libcockatrice/filters/filter_string.h"
@@ -20,7 +21,8 @@
 class QObject;
 
 CardDatabaseDisplayModel::CardDatabaseDisplayModel(QObject *parent)
-    : QSortFilterProxyModel(parent), isToken(ShowAll), filterString(nullptr)
+    : QSortFilterProxyModel(parent), isToken(ShowAll), filterString(nullptr),
+      collator(NaturalSort::makeNaturalSortCollator())
 {
     filterTree = nullptr;
     setFilterCaseSensitivity(Qt::CaseInsensitive);
@@ -120,10 +122,10 @@ bool CardDatabaseDisplayModel::lessThan(const QModelIndex &left, const QModelInd
             return lessThanNumerically(leftPT.mid(leftSlash + 1), rightPT.mid(rightSlash + 1)) < 0;
         }
     }
-    return QString::localeAwareCompare(leftString, rightString) < 0;
+    return collator.compare(leftString, rightString) < 0;
 }
 
-int CardDatabaseDisplayModel::lessThanNumerically(QStringView left, QStringView right)
+int CardDatabaseDisplayModel::lessThanNumerically(QStringView left, QStringView right) const
 {
     if (left == right) {
         return 0;
@@ -182,7 +184,7 @@ int CardDatabaseDisplayModel::lessThanNumerically(QStringView left, QStringView
         } else {
             // both parsed, same number, but at least one has something else
             // so compare the part after the number - prefer nothing
-            return QString::localeAwareCompare(leftAfterNum, rightAfterNum);
+            return collator.compare(leftAfterNum, rightAfterNum);
         }
     } else if (okLeft) {
         return -1;
@@ -190,7 +192,7 @@ int CardDatabaseDisplayModel::lessThanNumerically(QStringView left, QStringView
         return 1;
     }
     // couldn't parse it, just return String comparison
-    return QString::localeAwareCompare(left, right);
+    return collator.compare(left, right);
 }
 bool CardDatabaseDisplayModel::filterAcceptsRow(int sourceRow, const QModelIndex & /*sourceParent*/) const
 {

libcockatrice_models/libcockatrice/models/database/card_database_display_model.h

@@ -10,6 +10,7 @@
 
 #include "libcockatrice/card/card_info.h"
 
+#include <QCollator>
 #include <QModelIndex>
 #include <QSet>
 #include <QSortFilterProxyModel>
@@ -49,6 +50,9 @@ private:
     /** The translation table that will be used for sanitizeCardName. */
     static QMap<wchar_t, wchar_t> characterTranslation;
 
+    /** Natural, locale-aware comparison shared by every sort in this model. */
+    QCollator collator;
+
 public:
     explicit CardDatabaseDisplayModel(QObject *parent = nullptr);
     void setSourceModel(QAbstractItemModel *model) override;
@@ -89,7 +93,7 @@ signals:
 
 protected:
     [[nodiscard]] bool lessThan(const QModelIndex &left, const QModelIndex &right) const override;
-    static int lessThanNumerically(QStringView left, QStringView right);
+    int lessThanNumerically(QStringView left, QStringView right) const;
     [[nodiscard]] bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;
     [[nodiscard]] bool rowMatchesCardName(CardInfoPtr info) const;
 

libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp

@@ -1,5 +1,6 @@
 #include "card_sets_model.h"
 
+#include "../../natural_sort_collator.h"
 #include "libcockatrice/card/database/card_database.h"
 #include "libcockatrice/card/set/card_set.h"
 #include "libcockatrice/interfaces/interface_card_set_priority_controller.h"
@@ -296,7 +297,8 @@ QStringList SetsModel::mimeTypes() const
     return QStringList() << "application/x-cockatricecardset";
 }
 
-SetsDisplayModel::SetsDisplayModel(QObject *parent) : QSortFilterProxyModel(parent)
+SetsDisplayModel::SetsDisplayModel(QObject *parent)
+    : QSortFilterProxyModel(parent), collator(NaturalSort::makeNaturalSortCollator())
 {
     setFilterCaseSensitivity(Qt::CaseInsensitive);
     setSortCaseSensitivity(Qt::CaseInsensitive);
@@ -331,5 +333,5 @@ bool SetsDisplayModel::lessThan(const QModelIndex &left, const QModelIndex &righ
     QString leftString = sourceModel()->data(left, SetsModel::SortRole).toString();
     QString rightString = sourceModel()->data(right, SetsModel::SortRole).toString();
 
-    return QString::localeAwareCompare(leftString, rightString) < 0;
+    return collator.compare(leftString, rightString) < 0;
 }

libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.h

@@ -11,6 +11,7 @@
 #include "libcockatrice/card/set/card_set_list.h"
 
 #include <QAbstractTableModel>
+#include <QCollator>
 #include <QList>
 #include <QMimeData>
 #include <QModelIndex>
@@ -112,6 +113,9 @@ protected:
     [[nodiscard]] bool lessThan(const QModelIndex &left, const QModelIndex &right) const override;
     [[nodiscard]] bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;
     void fetchMore(const QModelIndex &index) override;
+
+private:
+    QCollator collator;
 };
 
 #endif

libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.cpp

@@ -23,7 +23,7 @@ bool DeckListSortFilterProxyModel::lessThan(const QModelIndex &left, const QMode
     if (!leftIsCard || !rightIsCard) {
         QString lName = src->data(left.siblingAtColumn(DeckListModelColumns::CARD_NAME), Qt::EditRole).toString();
         QString rName = src->data(right.siblingAtColumn(DeckListModelColumns::CARD_NAME), Qt::EditRole).toString();
-        return lName.localeAwareCompare(rName) < 0;
+        return collator.compare(lName, rName) < 0;
     }
 
     // Both are cards -> apply sort criteria
@@ -38,7 +38,7 @@ bool DeckListSortFilterProxyModel::lessThan(const QModelIndex &left, const QMode
         if (crit == "name") {
             QString ln = lNode->getName();
             QString rn = rNode->getName();
-            int cmp = ln.localeAwareCompare(rn);
+            int cmp = collator.compare(ln, rn);
             if (cmp != 0) {
                 return cmp < 0;
             }
@@ -51,14 +51,14 @@ bool DeckListSortFilterProxyModel::lessThan(const QModelIndex &left, const QMode
         } else if (crit == "colors") {
             QString lr = lInfo ? lInfo->getColors() : QString();
             QString rr = rInfo ? rInfo->getColors() : QString();
-            int cmp = lr.localeAwareCompare(rr);
+            int cmp = collator.compare(lr, rr);
             if (cmp != 0) {
                 return cmp < 0;
             }
         } else if (crit == "maintype") {
             QString lr = lInfo ? lInfo->getMainCardType() : QString();
             QString rr = rInfo ? rInfo->getMainCardType() : QString();
-            int cmp = lr.localeAwareCompare(rr);
+            int cmp = collator.compare(lr, rr);
             if (cmp != 0) {
                 return cmp < 0;
             }

libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.h

@@ -7,6 +7,9 @@
 #ifndef COCKATRICE_DECK_LIST_SORT_FILTER_PROXY_MODEL_H
 #define COCKATRICE_DECK_LIST_SORT_FILTER_PROXY_MODEL_H
 
+#include "../natural_sort_collator.h"
+
+#include <QCollator>
 #include <QList>
 #include <QSortFilterProxyModel>
 #include <QString>
@@ -19,7 +22,8 @@ class DeckListSortFilterProxyModel : public QSortFilterProxyModel
 {
     Q_OBJECT
 public:
-    explicit DeckListSortFilterProxyModel(QObject *parent = nullptr) : QSortFilterProxyModel(parent)
+    explicit DeckListSortFilterProxyModel(QObject *parent = nullptr)
+        : QSortFilterProxyModel(parent), collator(NaturalSort::makeNaturalSortCollator())
     {
     }
 
@@ -34,6 +38,7 @@ protected:
 
 private:
     QStringList sortCriteria;
+    QCollator collator;
 };
 
 #endif // COCKATRICE_DECK_LIST_SORT_FILTER_PROXY_MODEL_H

libcockatrice_models/libcockatrice/models/natural_sort_collator.h

@@ -0,0 +1,30 @@
+#ifndef NATURAL_SORT_COLLATOR_H
+#define NATURAL_SORT_COLLATOR_H
+
+#include <QCollator>
+#include <QLocale>
+
+namespace NaturalSort
+{
+
+/**
+ * @brief A collator that compares runs of digits numerically, whatever the process locale is.
+ *
+ * Numeric mode only takes effect through a real locale. Under the C locale, which
+ * headless CI processes run in, QCollator silently falls back to plain string order
+ * and sorts "Forest 10" before "Forest 2". Pin English for that single degenerate
+ * locale so sorting is the same everywhere, and keep the user's locale otherwise.
+ */
+inline QCollator makeNaturalSortCollator()
+{
+    QCollator collator;
+    collator.setNumericMode(true);
+    if (QLocale().language() == QLocale::C) {
+        collator.setLocale(QLocale(QLocale::English));
+    }
+    return collator;
+}
+
+} // namespace NaturalSort
+
+#endif // NATURAL_SORT_COLLATOR_H

tests/deck_list_model/CMakeLists.txt

@@ -21,3 +21,15 @@ cockatrice_add_test(
   libcockatrice_card
   libcockatrice_deck_list
 )
+
+cockatrice_add_test(
+  NAME
+  collator_sort_test
+  SOURCES
+  ${VERSION_STRING_CPP}
+  collator_sort_test.cpp
+  LIBS
+  libcockatrice_models
+  libcockatrice_card
+  libcockatrice_deck_list
+)

tests/deck_list_model/collator_sort_test.cpp

@@ -0,0 +1,30 @@
+#include <QStandardItemModel>
+#include <QString>
+#include <QStringList>
+#include <gtest/gtest.h>
+#include <libcockatrice/models/deck_list/deck_list_model.h>
+#include <libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.h>
+
+namespace
+{
+TEST(CollatorSort, NumericNamesSortNaturally)
+{
+    QStandardItemModel source(0, 2);
+    const QStringList names = {"Forest 10", "Forest 2", "Forest 1"};
+    for (int row = 0; row < names.size(); ++row) {
+        source.setItem(row, DeckListModelColumns::CARD_NAME, new QStandardItem(names.at(row)));
+    }
+
+    DeckListSortFilterProxyModel proxy;
+    proxy.setSourceModel(&source);
+    proxy.setSortCriteria({"name"});
+    proxy.sort(DeckListModelColumns::CARD_NAME, Qt::AscendingOrder);
+
+    QStringList actual;
+    for (int row = 0; row < proxy.rowCount(); ++row) {
+        actual.append(proxy.index(row, DeckListModelColumns::CARD_NAME).data(Qt::DisplayRole).toString());
+    }
+
+    EXPECT_EQ(actual, (QStringList{"Forest 1", "Forest 2", "Forest 10"}));
+}
+} // namespace