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