[GUI] Implement stable sort (#7703)

* NaturalCompare now returns compare result instead of "less than" result
* Change to stable sort in GUI components
* Add Utils::String::naturalLessThan() helper function
* Use Qt::CaseSensitivity type

src/base/utils/string.cpp

@@ -33,9 +33,9 @@
 
 #include <QByteArray>
 #include <QCollator>
-#include <QtGlobal>
 #include <QLocale>
 #include <QRegExp>
+#include <QtGlobal>
 #ifdef Q_OS_MAC
 #include <QThreadStorage>
 #endif
@@ -45,110 +45,103 @@ namespace
     class NaturalCompare
     {
     public:
-        explicit NaturalCompare(const bool caseSensitive = true)
-            : m_caseSensitive(caseSensitive)
+        explicit NaturalCompare(const Qt::CaseSensitivity caseSensitivity = Qt::CaseSensitive)
+            : m_caseSensitivity(caseSensitivity)
         {
-#if defined(Q_OS_WIN)
+#ifdef Q_OS_WIN
             // Without ICU library, QCollator uses the native API on Windows 7+. But that API
             // sorts older versions of μTorrent differently than the newer ones because the
             // 'μ' character is encoded differently and the native API can't cope with that.
             // So default to using our custom natural sorting algorithm instead.
             // See #5238 and #5240
-            // Without ICU library, QCollator doesn't support `setNumericMode(true)` on OS older than Win7
-            // if (QSysInfo::windowsVersion() < QSysInfo::WV_WINDOWS7)
-                return;
-#endif
+            // Without ICU library, QCollator doesn't support `setNumericMode(true)` on an OS older than Win7
+#else
             m_collator.setNumericMode(true);
-            m_collator.setCaseSensitivity(caseSensitive ? Qt::CaseSensitive : Qt::CaseInsensitive);
+            m_collator.setCaseSensitivity(caseSensitivity);
+#endif
         }
 
-        bool operator()(const QString &left, const QString &right) const
+        int operator()(const QString &left, const QString &right) const
         {
-#if defined(Q_OS_WIN)
-            // Without ICU library, QCollator uses the native API on Windows 7+. But that API
-            // sorts older versions of μTorrent differently than the newer ones because the
-            // 'μ' character is encoded differently and the native API can't cope with that.
-            // So default to using our custom natural sorting algorithm instead.
-            // See #5238 and #5240
-            // Without ICU library, QCollator doesn't support `setNumericMode(true)` on OS older than Win7
-            // if (QSysInfo::windowsVersion() < QSysInfo::WV_WINDOWS7)
-                return lessThan(left, right);
+#ifdef Q_OS_WIN
+            return compare(left, right);
+#else
+            return m_collator.compare(left, right);
 #endif
-            return (m_collator.compare(left, right) < 0);
         }
 
-        bool lessThan(const QString &left, const QString &right) const
+    private:
+        int compare(const QString &left, const QString &right) const
         {
-            // Return value `false` indicates `right` should go before `left`, otherwise, after
+            // Return value <0: `left` is smaller than `right`
+            // Return value >0: `left` is greater than `right`
+            // Return value =0: both strings are equal
+
             int posL = 0;
             int posR = 0;
             while (true) {
-                while (true) {
-                    if ((posL == left.size()) || (posR == right.size()))
-                        return (left.size() < right.size());  // when a shorter string is another string's prefix, shorter string place before longer string
-
-                    QChar leftChar = m_caseSensitive ? left[posL] : left[posL].toLower();
-                    QChar rightChar = m_caseSensitive ? right[posR] : right[posR].toLower();
-                    if (leftChar == rightChar)
-                        ;  // compare next character
-                    else if (leftChar.isDigit() && rightChar.isDigit())
-                        break; // Both are digits, break this loop and compare numbers
-                    else
-                        return leftChar < rightChar;
+                if ((posL == left.size()) || (posR == right.size()))
+                    return (left.size() - right.size());  // when a shorter string is another string's prefix, shorter string place before longer string
 
+                const QChar leftChar = (m_caseSensitivity == Qt::CaseSensitive) ? left[posL] : left[posL].toLower();
+                const QChar rightChar = (m_caseSensitivity == Qt::CaseSensitive) ? right[posR] : right[posR].toLower();
+                if (leftChar == rightChar) {
+                    // compare next character
                     ++posL;
                     ++posR;
                 }
-
-                int startL = posL;
-                while ((posL < left.size()) && left[posL].isDigit())
-                    ++posL;
-                int numL = left.midRef(startL, posL - startL).toInt();
-
-                int startR = posR;
-                while ((posR < right.size()) && right[posR].isDigit())
-                    ++posR;
-                int numR = right.midRef(startR, posR - startR).toInt();
-
-                if (numL != numR)
-                    return (numL < numR);
-
-                // Strings + digits do match and we haven't hit string end
-                // Do another round
+                else if (leftChar.isDigit() && rightChar.isDigit()) {
+                    // Both are digits, compare the numbers
+                    const auto consumeNumber = [](const QString &str, int &pos) -> int
+                    {
+                        const int start = pos;
+                        while ((pos < str.size()) && str[pos].isDigit())
+                            ++pos;
+                        return str.midRef(start, (pos - start)).toInt();
+                    };
+
+                    const int numL = consumeNumber(left, posL);
+                    const int numR = consumeNumber(right, posR);
+                    if (numL != numR)
+                        return (numL - numR);
+
+                    // String + digits do match and we haven't hit the end of both strings
+                    // then continue to consume the remainings
+                }
+                else {
+                    return (leftChar.unicode() - rightChar.unicode());
+                }
             }
-            return false;
         }
 
-    private:
         QCollator m_collator;
-        const bool m_caseSensitive;
+        const Qt::CaseSensitivity m_caseSensitivity;
     };
 }
 
-bool Utils::String::naturalCompareCaseSensitive(const QString &left, const QString &right)
+int Utils::String::naturalCompare(const QString &left, const QString &right, const Qt::CaseSensitivity caseSensitivity)
 {
     // provide a single `NaturalCompare` instance for easy use
     // https://doc.qt.io/qt-5/threads-reentrancy.html
+    if (caseSensitivity == Qt::CaseSensitive) {
 #ifdef Q_OS_MAC  // workaround for Apple xcode: https://stackoverflow.com/a/29929949
-    static QThreadStorage<NaturalCompare> nCmp;
-    if (!nCmp.hasLocalData()) nCmp.setLocalData(NaturalCompare(true));
-    return (nCmp.localData())(left, right);
+        static QThreadStorage<NaturalCompare> nCmp;
+        if (!nCmp.hasLocalData())
+            nCmp.setLocalData(NaturalCompare(Qt::CaseSensitive));
+        return (nCmp.localData())(left, right);
 #else
-    thread_local NaturalCompare nCmp(true);
-    return nCmp(left, right);
+        thread_local NaturalCompare nCmp(Qt::CaseSensitive);
+        return nCmp(left, right);
 #endif
-}
+    }
 
-bool Utils::String::naturalCompareCaseInsensitive(const QString &left, const QString &right)
-{
-    // provide a single `NaturalCompare` instance for easy use
-    // https://doc.qt.io/qt-5/threads-reentrancy.html
-#ifdef Q_OS_MAC  // workaround for Apple xcode: https://stackoverflow.com/a/29929949
+#ifdef Q_OS_MAC
     static QThreadStorage<NaturalCompare> nCmp;
-    if (!nCmp.hasLocalData()) nCmp.setLocalData(NaturalCompare(false));
+    if (!nCmp.hasLocalData())
+        nCmp.setLocalData(NaturalCompare(Qt::CaseInsensitive));
     return (nCmp.localData())(left, right);
 #else
-    thread_local NaturalCompare nCmp(false);
+    thread_local NaturalCompare nCmp(Qt::CaseInsensitive);
     return nCmp(left, right);
 #endif
 }
@@ -188,4 +181,3 @@ QString Utils::String::wildcardToRegex(const QString &pattern)
 {
     return qt_regexp_toCanonical(pattern, QRegExp::Wildcard);
 }
-

src/base/utils/string.h

@@ -45,8 +45,12 @@ namespace Utils
         // Taken from https://crackstation.net/hashing-security.htm
         bool slowEquals(const QByteArray &a, const QByteArray &b);
 
-        bool naturalCompareCaseSensitive(const QString &left, const QString &right);
-        bool naturalCompareCaseInsensitive(const QString &left, const QString &right);
+        int naturalCompare(const QString &left, const QString &right, const Qt::CaseSensitivity caseSensitivity);
+        template <Qt::CaseSensitivity caseSensitivity>
+        bool naturalLessThan(const QString &left, const QString &right)
+        {
+            return (naturalCompare(left, right, caseSensitivity) < 0);
+        }
 
         QString wildcardToRegex(const QString &pattern);
 

src/gui/addnewtorrentdialog.cpp

@@ -128,7 +128,7 @@ AddNewTorrentDialog::AddNewTorrentDialog(const BitTorrent::AddTorrentParams &inP
 
     // Load categories
     QStringList categories = session->categories().keys();
-    std::sort(categories.begin(), categories.end(), Utils::String::naturalCompareCaseInsensitive);
+    std::sort(categories.begin(), categories.end(), Utils::String::naturalLessThan<Qt::CaseInsensitive>);
     QString defaultCategory = settings()->loadValue(KEY_DEFAULTCATEGORY).toString();
 
     if (!m_torrentParams.category.isEmpty())

src/gui/categoryfilterproxymodel.cpp

@@ -50,8 +50,12 @@ bool CategoryFilterProxyModel::lessThan(const QModelIndex &left, const QModelInd
 {
     // "All" and "Uncategorized" must be left in place
     if (CategoryFilterModel::isSpecialItem(left) || CategoryFilterModel::isSpecialItem(right))
-        return left.row() < right.row();
-    else
-        return Utils::String::naturalCompareCaseInsensitive(
-                left.data().toString(), right.data().toString());
+        return (left < right);
+
+    int result = Utils::String::naturalCompare(left.data().toString(), right.data().toString()
+        , Qt::CaseInsensitive);
+    if (result != 0)
+        return (result < 0);
+
+    return (mapFromSource(left) < mapFromSource(right));
 }

src/gui/properties/peerlistsortmodel.h

@@ -50,13 +50,21 @@ protected:
         switch (sortColumn()) {
         case PeerListDelegate::IP:
         case PeerListDelegate::CLIENT: {
-                QString vL = left.data().toString();
-                QString vR = right.data().toString();
-                return Utils::String::naturalCompareCaseInsensitive(vL, vR);
+                const QString strL = left.data().toString();
+                const QString strR = right.data().toString();
+                const int result = Utils::String::naturalCompare(strL, strR, Qt::CaseInsensitive);
+                if (result != 0)
+                    return (result < 0);
+
+                return (mapFromSource(left) < mapFromSource(right));
             }
-        };
+            break;
+        default:
+            if (left.data() != right.data())
+                return QSortFilterProxyModel::lessThan(left, right);
 
-        return QSortFilterProxyModel::lessThan(left, right);
+            return (mapFromSource(left) < mapFromSource(right));
+        };
     }
 };
 

src/gui/rss/automatedrssdownloader.cpp

@@ -311,7 +311,7 @@ void AutomatedRssDownloader::initCategoryCombobox()
 {
     // Load torrent categories
     QStringList categories = BitTorrent::Session::instance()->categories().keys();
-    std::sort(categories.begin(), categories.end(), Utils::String::naturalCompareCaseInsensitive);
+    std::sort(categories.begin(), categories.end(), Utils::String::naturalLessThan<Qt::CaseInsensitive>);
     m_ui->comboCategory->addItem("");
     m_ui->comboCategory->addItems(categories);
 }

src/gui/search/searchsortmodel.cpp

@@ -110,13 +110,20 @@ bool SearchSortModel::lessThan(const QModelIndex &left, const QModelIndex &right
     switch (sortColumn()) {
     case NAME:
     case ENGINE_URL: {
-        QString vL = left.data().toString();
-        QString vR = right.data().toString();
-        return Utils::String::naturalCompareCaseInsensitive(vL, vR);
-    }
+            const QString strL = left.data().toString();
+            const QString strR = right.data().toString();
+            const int result = Utils::String::naturalCompare(strL, strR, Qt::CaseInsensitive);
+            if (result != 0)
+                return (result < 0);
 
+            return (mapFromSource(left) < mapFromSource(right));
+        }
+        break;
     default:
-        return base::lessThan(left, right);
+        if (left.data() != right.data())
+            return base::lessThan(left, right);
+
+        return (mapFromSource(left) < mapFromSource(right));
     };
 }
 

src/gui/tagfilterproxymodel.cpp

@@ -50,7 +50,12 @@ bool TagFilterProxyModel::lessThan(const QModelIndex &left, const QModelIndex &r
 {
     // "All" and "Untagged" must be left in place
     if (TagFilterModel::isSpecialItem(left) || TagFilterModel::isSpecialItem(right))
-        return left.row() < right.row();
-    return Utils::String::naturalCompareCaseInsensitive(
-                left.data().toString(), right.data().toString());
+        return (left < right);
+
+    int result = Utils::String::naturalCompare(left.data().toString(), right.data().toString()
+        , Qt::CaseInsensitive);
+    if (result != 0)
+        return (result < 0);
+
+    return (mapFromSource(left) < mapFromSource(right));
 }

src/gui/torrentcontentfiltermodel.cpp

@@ -88,21 +88,24 @@ bool TorrentContentFilterModel::lessThan(const QModelIndex &left, const QModelIn
 {
     switch (sortColumn()) {
     case TorrentContentModelItem::COL_NAME: {
-            QString vL = left.data().toString();
-            QString vR = right.data().toString();
-            TorrentContentModelItem::ItemType leftType = m_model->itemType(m_model->index(left.row(), 0, left.parent()));
-            TorrentContentModelItem::ItemType rightType = m_model->itemType(m_model->index(right.row(), 0, right.parent()));
-
-            if (leftType == rightType)
-                return Utils::String::naturalCompareCaseInsensitive(vL, vR);
-            else if ((leftType == TorrentContentModelItem::FolderType) && (sortOrder() == Qt::AscendingOrder))
+            const TorrentContentModelItem::ItemType leftType = m_model->itemType(m_model->index(left.row(), 0, left.parent()));
+            const TorrentContentModelItem::ItemType rightType = m_model->itemType(m_model->index(right.row(), 0, right.parent()));
+
+            if (leftType == rightType) {
+                const QString strL = left.data().toString();
+                const QString strR = right.data().toString();
+                return Utils::String::naturalLessThan<Qt::CaseInsensitive>(strL, strR);
+            }
+            else if ((leftType == TorrentContentModelItem::FolderType) && (sortOrder() == Qt::AscendingOrder)) {
                 return true;
-            else
+            }
+            else {
                 return false;
+            }
         }
+    default:
+        return QSortFilterProxyModel::lessThan(left, right);
     };
-
-    return QSortFilterProxyModel::lessThan(left, right);
 }
 
 void TorrentContentFilterModel::selectAll()

src/gui/transferlistfilterswidget.cpp

@@ -259,7 +259,7 @@ void TrackerFiltersList::addItem(const QString &tracker, const QString &hash)
     Q_ASSERT(count() >= 4);
     int insPos = count();
     for (int i = 4; i < count(); ++i) {
-        if (Utils::String::naturalCompareCaseSensitive(host, item(i)->text())) {
+        if (Utils::String::naturalLessThan<Qt::CaseSensitive>(host, item(i)->text())) {
             insPos = i;
             break;
         }

src/gui/transferlistsortmodel.cpp

@@ -89,12 +89,16 @@ bool TransferListSortModel::lessThan(const QModelIndex &left, const QModelIndex
     case TorrentModel::TR_CATEGORY:
     case TorrentModel::TR_TAGS:
     case TorrentModel::TR_NAME: {
-        QVariant vL = left.data();
-        QVariant vR = right.data();
+        const QVariant vL = left.data();
+        const QVariant vR = right.data();
         if (!vL.isValid() || !vR.isValid() || (vL == vR))
             return lowerPositionThan(left, right);
 
-        return Utils::String::naturalCompareCaseInsensitive(vL.toString(), vR.toString());
+        const int result = Utils::String::naturalCompare(vL.toString(), vR.toString(), Qt::CaseInsensitive);
+        if (result != 0)
+            return (result < 0);
+
+        return (mapFromSource(left) < mapFromSource(right));
     }
 
     case TorrentModel::TR_ADD_DATE:
@@ -109,59 +113,61 @@ bool TransferListSortModel::lessThan(const QModelIndex &left, const QModelIndex
 
     case TorrentModel::TR_SEEDS:
     case TorrentModel::TR_PEERS: {
-        int left_active = left.data().toInt();
-        int left_total = left.data(Qt::UserRole).toInt();
-        int right_active = right.data().toInt();
-        int right_total = right.data(Qt::UserRole).toInt();
+        const int leftActive = left.data().toInt();
+        const int leftTotal = left.data(Qt::UserRole).toInt();
+        const int rightActive = right.data().toInt();
+        const int rightTotal = right.data(Qt::UserRole).toInt();
 
         // Active peers/seeds take precedence over total peers/seeds.
-        if (left_active == right_active) {
-            if (left_total == right_total)
-                return lowerPositionThan(left, right);
-            return (left_total < right_total);
-        }
-        else {
-            return (left_active < right_active);
-        }
+        if (leftActive != rightActive)
+            return (leftActive < rightActive);
+
+        if (leftTotal != rightTotal)
+            return (leftTotal < rightTotal);
+
+        return lowerPositionThan(left, right);
     }
 
     case TorrentModel::TR_ETA: {
-        TorrentModel *model = qobject_cast<TorrentModel *>(sourceModel());
-        const int prioL = model->data(model->index(left.row(), TorrentModel::TR_PRIORITY)).toInt();
-        const int prioR = model->data(model->index(right.row(), TorrentModel::TR_PRIORITY)).toInt();
-        const qlonglong etaL = left.data().toLongLong();
-        const qlonglong etaR = right.data().toLongLong();
-        const bool ascend = (sortOrder() == Qt::AscendingOrder);
-        const bool invalidL = (etaL < 0 || etaL >= MAX_ETA);
-        const bool invalidR = (etaR < 0 || etaR >= MAX_ETA);
-        const bool seedingL = (prioL < 0);
-        const bool seedingR = (prioR < 0);
-
-        bool activeR = TorrentFilter::ActiveTorrent.match(model->torrentHandle(model->index(right.row())));
-        bool activeL = TorrentFilter::ActiveTorrent.match(model->torrentHandle(model->index(left.row())));
+        const TorrentModel *model = qobject_cast<TorrentModel *>(sourceModel());
 
         // Sorting rules prioritized.
         // 1. Active torrents at the top
         // 2. Seeding torrents at the bottom
         // 3. Torrents with invalid ETAs at the bottom
 
-        if (activeL != activeR) return activeL;
-        if (seedingL != seedingR) {
-            if (seedingL) return !ascend;
-            else return ascend;
+        const bool isActiveL = TorrentFilter::ActiveTorrent.match(model->torrentHandle(model->index(left.row())));
+        const bool isActiveR = TorrentFilter::ActiveTorrent.match(model->torrentHandle(model->index(right.row())));
+        if (isActiveL != isActiveR)
+            return isActiveL;
+
+        const int prioL = model->data(model->index(left.row(), TorrentModel::TR_PRIORITY)).toInt();
+        const int prioR = model->data(model->index(right.row(), TorrentModel::TR_PRIORITY)).toInt();
+        const bool isSeedingL = (prioL < 0);
+        const bool isSeedingR = (prioR < 0);
+        if (isSeedingL != isSeedingR) {
+            const bool isAscendingOrder = (sortOrder() == Qt::AscendingOrder);
+            if (isSeedingL)
+                return !isAscendingOrder;
+            else
+                return isAscendingOrder;
         }
 
-        if (invalidL && invalidR) {
-            if (seedingL) // Both seeding
+        const qlonglong etaL = left.data().toLongLong();
+        const qlonglong etaR = right.data().toLongLong();
+        const bool isInvalidL = ((etaL < 0) || (etaL >= MAX_ETA));
+        const bool isInvalidR = ((etaR < 0) || (etaR >= MAX_ETA));
+        if (isInvalidL && isInvalidR) {
+            if (isSeedingL)  // Both seeding
                 return dateLessThan(TorrentModel::TR_SEED_DATE, left, right, true);
             else
-                return prioL < prioR;
+                return (prioL < prioR);
         }
-        else if (!invalidL && !invalidR) {
-            return etaL < etaR;
+        else if (!isInvalidL && !isInvalidR) {
+            return (etaL < etaR);
         }
         else {
-            return !invalidL;
+            return !isInvalidL;
         }
     }
 
@@ -186,9 +192,10 @@ bool TransferListSortModel::lessThan(const QModelIndex &left, const QModelIndex
     }
 
     default: {
-        if (left.data() == right.data())
-            return lowerPositionThan(left, right);
-        return QSortFilterProxyModel::lessThan(left, right);
+        if (left.data() != right.data())
+            return QSortFilterProxyModel::lessThan(left, right);
+
+        return lowerPositionThan(left, right);
     }
     }
 }

src/gui/transferlistwidget.cpp

@@ -988,7 +988,7 @@ void TransferListWidget::displayListMenu(const QPoint&)
         listMenu.addAction(&actionRename);
     // Category Menu
     QStringList categories = BitTorrent::Session::instance()->categories().keys();
-    std::sort(categories.begin(), categories.end(), Utils::String::naturalCompareCaseInsensitive);
+    std::sort(categories.begin(), categories.end(), Utils::String::naturalLessThan<Qt::CaseInsensitive>);
     QList<QAction*> categoryActions;
     QMenu *categoryMenu = listMenu.addMenu(GuiIconProvider::instance()->getIcon("view-categories"), tr("Category"));
     categoryActions << categoryMenu->addAction(GuiIconProvider::instance()->getIcon("list-add"), tr("New...", "New category..."));
@@ -1007,7 +1007,7 @@ void TransferListWidget::displayListMenu(const QPoint&)
 
     // Tag Menu
     QStringList tags(BitTorrent::Session::instance()->tags().toList());
-    std::sort(tags.begin(), tags.end(), Utils::String::naturalCompareCaseInsensitive);
+    std::sort(tags.begin(), tags.end(), Utils::String::naturalLessThan<Qt::CaseInsensitive>);
     QList<QAction *> tagsActions;
     QMenu *tagsMenu = listMenu.addMenu(GuiIconProvider::instance()->getIcon("view-categories"), tr("Tags"));
     tagsActions << tagsMenu->addAction(GuiIconProvider::instance()->getIcon("list-add"), tr("Add...", "Add / assign multiple tags..."));