From 5fbc636f9361d80f94d3f7391745fe5173a2c906 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:01:04 +0200 Subject: [PATCH] [Models] Use QCollator for natural, cached sorting (#7633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 --- .../widgets/server/user/user_list_widget.cpp | 12 ++++++-- .../database/card_database_display_model.cpp | 12 ++++---- .../database/card_database_display_model.h | 6 +++- .../database/card_set/card_sets_model.cpp | 6 ++-- .../database/card_set/card_sets_model.h | 4 +++ .../deck_list_sort_filter_proxy_model.cpp | 8 ++--- .../deck_list_sort_filter_proxy_model.h | 7 ++++- .../models/natural_sort_collator.h | 30 +++++++++++++++++++ tests/deck_list_model/CMakeLists.txt | 12 ++++++++ tests/deck_list_model/collator_sort_test.cpp | 30 +++++++++++++++++++ 10 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 libcockatrice_models/libcockatrice/models/natural_sort_collator.h create mode 100644 tests/deck_list_model/collator_sort_test.cpp diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp index efbdcfb23..a20b39b82 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -55,6 +56,7 @@ #include #include #include +#include #include #include #include @@ -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(), - other.data(2, Qt::UserRole).toString()) < 0; + return collator.compare(data(2, Qt::UserRole).toString(), 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, diff --git a/libcockatrice_models/libcockatrice/models/database/card_database_display_model.cpp b/libcockatrice_models/libcockatrice/models/database/card_database_display_model.cpp index c333f5506..5573deb18 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_database_display_model.cpp +++ b/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 { diff --git a/libcockatrice_models/libcockatrice/models/database/card_database_display_model.h b/libcockatrice_models/libcockatrice/models/database/card_database_display_model.h index 77152a390..343a7482e 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_database_display_model.h +++ b/libcockatrice_models/libcockatrice/models/database/card_database_display_model.h @@ -10,6 +10,7 @@ #include "libcockatrice/card/card_info.h" +#include #include #include #include @@ -49,6 +50,9 @@ private: /** The translation table that will be used for sanitizeCardName. */ static QMap 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; diff --git a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp index a0b9c1061..b8bdc3466 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.cpp +++ b/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; } diff --git a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.h b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.h index 354414dc8..17eab6c53 100644 --- a/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.h +++ b/libcockatrice_models/libcockatrice/models/database/card_set/card_sets_model.h @@ -11,6 +11,7 @@ #include "libcockatrice/card/set/card_set_list.h" #include +#include #include #include #include @@ -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 diff --git a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.cpp b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.cpp index dcb294ec8..cf3410560 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.cpp +++ b/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; } diff --git a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.h b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.h index 522ed1e23..b911b4740 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_sort_filter_proxy_model.h +++ b/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 #include #include #include @@ -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 diff --git a/libcockatrice_models/libcockatrice/models/natural_sort_collator.h b/libcockatrice_models/libcockatrice/models/natural_sort_collator.h new file mode 100644 index 000000000..8c026ca06 --- /dev/null +++ b/libcockatrice_models/libcockatrice/models/natural_sort_collator.h @@ -0,0 +1,30 @@ +#ifndef NATURAL_SORT_COLLATOR_H +#define NATURAL_SORT_COLLATOR_H + +#include +#include + +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 \ No newline at end of file diff --git a/tests/deck_list_model/CMakeLists.txt b/tests/deck_list_model/CMakeLists.txt index cd7d693f3..82a1965d2 100644 --- a/tests/deck_list_model/CMakeLists.txt +++ b/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 +) diff --git a/tests/deck_list_model/collator_sort_test.cpp b/tests/deck_list_model/collator_sort_test.cpp new file mode 100644 index 000000000..b4ed85525 --- /dev/null +++ b/tests/deck_list_model/collator_sort_test.cpp @@ -0,0 +1,30 @@ +#include +#include +#include +#include +#include +#include + +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