add sort options to card view window (#5206)

* refactor to allow for sorting by property of choice

* implement thing

* prevent overlapping sort properties

* enable/disable pile view checkbox if groupBy is off

* fix compiler warnings

* check to disable pile view checkbox on init

* Fix builds on Qt5

* Fix builds on Qt5

---------

Co-authored-by: ZeldaZach <zahalpern+github@gmail.com>

cockatrice/src/game/cards/card_list.cpp

@@ -3,6 +3,7 @@
 #include "card_database.h"
 #include "card_item.h"
 
+#include <QDebug>
 #include <algorithm>
 
 CardList::CardList(bool _contentsKnown) : QList<CardItem *>(), contentsKnown(_contentsKnown)
@@ -31,18 +32,53 @@ CardItem *CardList::findCard(const int cardId) const
     return nullptr;
 }
 
-void CardList::sort(int flags)
+/**
+ * @brief sorts the list by using string comparison on properties extracted from the CardItem
+ * The cards are compared using each property in order.
+ * If two cards have the same value for a property, then the next property in the list is used.
+ *
+ * @param option the option to compare the cards by, in order of usage.
+ */
+void CardList::sortBy(const QList<SortOption> &option)
 {
-    auto comparator = [flags](CardItem *a, CardItem *b) {
+    // early return if we know we won't be sorting
-        if (flags & SortByType) {
+    if (option.isEmpty()) {
-            QString t1 = a->getInfo() ? a->getInfo()->getMainCardType() : QString();
+        return;
-            QString t2 = b->getInfo() ? b->getInfo()->getMainCardType() : QString();
+    }
-            if ((t1 == t2) && (flags & SortByName))
+
-                return a->getName() < b->getName();
+    auto comparator = [&option](CardItem *a, CardItem *b) {
-            return t1 < t2;
+        for (auto prop : option) {
-        } else
+            auto extractor = getExtractorFor(prop);
-            return a->getName() < b->getName();
+            QString t1 = extractor(a);
+            QString t2 = extractor(b);
+            if (t1 != t2) {
+                return t1 < t2;
+            }
+        }
+        return false;
     };
 
     std::sort(begin(), end(), comparator);
 }
+
+/**
+ * @brief returns the function that extracts the given property from the CardItem.
+ */
+std::function<QString(CardItem *)> CardList::getExtractorFor(SortOption option)
+{
+    switch (option) {
+        case NoSort:
+            return [](CardItem *) { return ""; };
+        case SortByName:
+            return [](CardItem *c) { return c->getName(); };
+        case SortByType:
+            return [](CardItem *c) { return c->getInfo() ? c->getInfo()->getMainCardType() : ""; };
+        case SortByManaValue:
+            // getCmc returns the int as a string. We pad with 0's so that string comp also works on it
+            return [](CardItem *c) { return c->getInfo() ? c->getInfo()->getCmc().rightJustified(4, '0') : ""; };
+    }
+
+    // this line should never be reached
+    qDebug() << "cardlist.cpp: Could not find extractor for SortOption" << option;
+    return [](CardItem *) { return ""; };
+}

cockatrice/src/game/cards/card_list.h

@@ -11,10 +11,12 @@ protected:
     bool contentsKnown;
 
 public:
-    enum SortFlags
+    enum SortOption
     {
-        SortByName = 1,
+        NoSort,
-        SortByType = 2
+        SortByName,
+        SortByType,
+        SortByManaValue
     };
     CardList(bool _contentsKnown);
     CardItem *findCard(const int cardId) const;
@@ -22,7 +24,10 @@ public:
     {
         return contentsKnown;
     }
-    void sort(int flags = SortByName);
+
+    void sortBy(const QList<SortOption> &options);
+
+    static std::function<QString(CardItem *)> getExtractorFor(SortOption option);
 };
 
 #endif

cockatrice/src/game/zones/view_zone.cpp

@@ -31,7 +31,7 @@ ZoneViewZone::ZoneViewZone(Player *_p,
                            QGraphicsItem *parent)
     : SelectZone(_p, _origZone->getName(), false, false, true, parent, true), bRect(QRectF()), minRows(0),
       numberCards(_numberCards), origZone(_origZone), revealZone(_revealZone),
-      writeableRevealZone(_writeableRevealZone), sortByName(false), sortByType(false)
+      writeableRevealZone(_writeableRevealZone), groupBy(CardList::NoSort), sortBy(CardList::NoSort)
 {
     if (!(revealZone && !writeableRevealZone)) {
         origZone->getViews().append(this);
@@ -113,11 +113,33 @@ void ZoneViewZone::reorganizeCards()
             cards[i]->setId(i);
 
     CardList cardsToDisplay(cards);
-    if (sortByName || sortByType)
-        cardsToDisplay.sort((sortByName ? CardList::SortByName : 0) | (sortByType ? CardList::SortByType : 0));
 
-    auto gridSize = positionCardsForDisplay(cardsToDisplay, pileView && sortByType);
+    // sort cards
+    QList<CardList::SortOption> sortOptions;
+    if (groupBy != CardList::NoSort) {
+        sortOptions << groupBy;
+    }
 
+    if (sortBy != CardList::NoSort) {
+        sortOptions << sortBy;
+
+        // implicitly sort by name at the end so that cards with the same name appear together
+        if (sortBy != CardList::SortByName) {
+            sortOptions << CardList::SortByName;
+        }
+    }
+
+    cardsToDisplay.sortBy(sortOptions);
+
+    // position cards
+    GridSize gridSize;
+    if (pileView) {
+        gridSize = positionCardsForDisplay(cardsToDisplay, groupBy);
+    } else {
+        gridSize = positionCardsForDisplay(cardsToDisplay);
+    }
+
+    // determine bounding rect
     qreal aleft = 0;
     qreal atop = 0;
     qreal awidth = gridSize.cols * CARD_WIDTH + (CARD_WIDTH / 2) + HORIZONTAL_PADDING;
@@ -132,24 +154,30 @@ void ZoneViewZone::reorganizeCards()
  * @brief Sets the position of each card to the proper position for the view
  *
  * @param cards The cards to reposition. Will modify the cards in the list.
- * @param groupByType Whether to make piles by card type. If true, then expects `cards` to be sorted by type.
+ * @param pileOption Property used to group cards for the piles. Expects `cards` to be sorted by that property. Pass in
+ * NoSort to not make piles.
  *
  * @returns The number of rows and columns to display
  */
-ZoneViewZone::GridSize ZoneViewZone::positionCardsForDisplay(CardList &cards, bool groupByType)
+ZoneViewZone::GridSize ZoneViewZone::positionCardsForDisplay(CardList &cards, CardList::SortOption pileOption)
 {
     int cardCount = cards.size();
-    if (groupByType) {
+
+    if (pileOption != CardList::NoSort) {
         int row = 0;
         int col = 0;
         int longestRow = 0;
-        QString lastCardType;
+
+        QString lastColumnProp;
+
+        const auto extractor = CardList::getExtractorFor(pileOption);
+
         for (int i = 0; i < cardCount; i++) {
             CardItem *c = cards.at(i);
-            QString cardType = c->getInfo() ? c->getInfo()->getMainCardType() : "";
+            QString columnProp = extractor(c);
 
             if (i) { // if not the first card
-                if (cardType == lastCardType)
+                if (columnProp == lastColumnProp)
                     row++; // add below current card
                 else {     // if no match then move card to next column
                     col++;
@@ -157,7 +185,7 @@ ZoneViewZone::GridSize ZoneViewZone::positionCardsForDisplay(CardList &cards, bo
                 }
             }
 
-            lastCardType = cardType;
+            lastColumnProp = columnProp;
             qreal x = col * CARD_WIDTH;
             qreal y = row * CARD_HEIGHT / 3;
             c->setPos(HORIZONTAL_PADDING + x, VERTICAL_PADDING + y);
@@ -194,17 +222,15 @@ ZoneViewZone::GridSize ZoneViewZone::positionCardsForDisplay(CardList &cards, bo
     }
 }
 
-void ZoneViewZone::setSortByName(int _sortByName)
+void ZoneViewZone::setGroupBy(CardList::SortOption _groupBy)
 {
-    sortByName = _sortByName;
+    groupBy = _groupBy;
     reorganizeCards();
 }
 
-void ZoneViewZone::setSortByType(int _sortByType)
+void ZoneViewZone::setSortBy(CardList::SortOption _sortBy)
 {
-    sortByType = _sortByType;
+    sortBy = _sortBy;
-    if (!sortByType)
-        pileView = false;
     reorganizeCards();
 }
 

cockatrice/src/game/zones/view_zone.h

@@ -32,7 +32,7 @@ private:
     void handleDropEvent(const QList<CardDragItem *> &dragItems, CardZone *startZone, const QPoint &dropPoint);
     CardZone *origZone;
     bool revealZone, writeableRevealZone;
-    bool sortByName, sortByType;
+    CardList::SortOption groupBy, sortBy;
     bool pileView;
 
     struct GridSize
@@ -41,7 +41,7 @@ private:
         int cols;
     };
 
-    GridSize positionCardsForDisplay(CardList &cards, bool groupByType = false);
+    GridSize positionCardsForDisplay(CardList &cards, CardList::SortOption pileOption = CardList::NoSort);
 
 public:
     ZoneViewZone(Player *_p,
@@ -75,8 +75,8 @@ public:
     }
     void setWriteableRevealZone(bool _writeableRevealZone);
 public slots:
-    void setSortByName(int _sortByName);
+    void setGroupBy(CardList::SortOption _groupBy);
-    void setSortByType(int _sortByType);
+    void setSortBy(CardList::SortOption _sortBy);
     void setPileView(int _pileView);
 private slots:
     void zoneDumpReceived(const Response &r);

cockatrice/src/game/zones/view_zone_widget.cpp

@@ -46,16 +46,30 @@ ZoneViewWidget::ZoneViewWidget(Player *_player,
         QGraphicsLinearLayout *hPilebox = new QGraphicsLinearLayout(Qt::Horizontal);
         QGraphicsLinearLayout *hFilterbox = new QGraphicsLinearLayout(Qt::Horizontal);
 
-        QGraphicsProxyWidget *sortByNameProxy = new QGraphicsProxyWidget;
+        // groupBy options
-        sortByNameProxy->setWidget(&sortByNameCheckBox);
+        groupBySelector.addItem(tr("Group by ---"), CardList::NoSort);
-        hFilterbox->addItem(sortByNameProxy);
+        groupBySelector.addItem(tr("Group by Type"), CardList::SortByType);
+        groupBySelector.addItem(tr("Group by Mana Value"), CardList::SortByManaValue);
 
-        QGraphicsProxyWidget *sortByTypeProxy = new QGraphicsProxyWidget;
+        QGraphicsProxyWidget *groupBySelectorProxy = new QGraphicsProxyWidget;
-        sortByTypeProxy->setWidget(&sortByTypeCheckBox);
+        groupBySelectorProxy->setWidget(&groupBySelector);
-        hFilterbox->addItem(sortByTypeProxy);
+        groupBySelectorProxy->setZValue(2000000008);
+        hFilterbox->addItem(groupBySelectorProxy);
+
+        // sortBy options
+        sortBySelector.addItem(tr("Sort by ---"), CardList::NoSort);
+        sortBySelector.addItem(tr("Sort by Name"), CardList::SortByName);
+        sortBySelector.addItem(tr("Sort by Type"), CardList::SortByType);
+        sortBySelector.addItem(tr("Sort by Mana Value"), CardList::SortByManaValue);
+
+        QGraphicsProxyWidget *sortBySelectorProxy = new QGraphicsProxyWidget;
+        sortBySelectorProxy->setWidget(&sortBySelector);
+        sortBySelectorProxy->setZValue(2000000007);
+        hFilterbox->addItem(sortBySelectorProxy);
 
         vbox->addItem(hFilterbox);
 
+        // line
         QGraphicsProxyWidget *lineProxy = new QGraphicsProxyWidget;
         QFrame *line = new QFrame;
         line->setFrameShape(QFrame::HLine);
@@ -63,10 +77,12 @@ ZoneViewWidget::ZoneViewWidget(Player *_player,
         lineProxy->setWidget(line);
         vbox->addItem(lineProxy);
 
+        // pile view options
         QGraphicsProxyWidget *pileViewProxy = new QGraphicsProxyWidget;
         pileViewProxy->setWidget(&pileViewCheckBox);
         hPilebox->addItem(pileViewProxy);
 
+        // shuffle options
         if (_origZone->getIsShufflable() && numberCards == -1) {
             shuffleCheckBox.setChecked(true);
             QGraphicsProxyWidget *shuffleProxy = new QGraphicsProxyWidget;
@@ -104,14 +120,18 @@ ZoneViewWidget::ZoneViewWidget(Player *_player,
 
     // only wire up sort options after creating ZoneViewZone, since it segfaults otherwise.
     if (numberCards < 0) {
-        connect(&sortByNameCheckBox, &QCheckBox::QT_STATE_CHANGED, this, &ZoneViewWidget::processSortByName);
+        connect(&groupBySelector, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), this,
-        connect(&sortByTypeCheckBox, &QCheckBox::QT_STATE_CHANGED, this, &ZoneViewWidget::processSortByType);
+                &ZoneViewWidget::processGroupBy);
+        connect(&sortBySelector, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), this,
+                &ZoneViewWidget::processSortBy);
         connect(&pileViewCheckBox, &QCheckBox::QT_STATE_CHANGED, this, &ZoneViewWidget::processSetPileView);
-        sortByNameCheckBox.setChecked(SettingsCache::instance().getZoneViewSortByName());
+        groupBySelector.setCurrentIndex(groupBySelector.findData(SettingsCache::instance().getZoneViewGroupBy()));
-        sortByTypeCheckBox.setChecked(SettingsCache::instance().getZoneViewSortByType());
+        sortBySelector.setCurrentIndex(sortBySelector.findData(SettingsCache::instance().getZoneViewSortBy()));
         pileViewCheckBox.setChecked(SettingsCache::instance().getZoneViewPileView());
-        if (!SettingsCache::instance().getZoneViewSortByType())
+
+        if (CardList::NoSort == static_cast<CardList::SortOption>(groupBySelector.currentData().toInt())) {
             pileViewCheckBox.setEnabled(false);
+        }
     }
 
     retranslateUi();
@@ -122,18 +142,35 @@ ZoneViewWidget::ZoneViewWidget(Player *_player,
     zone->initializeCards(cardList);
 }
 
-void ZoneViewWidget::processSortByType(QT_STATE_CHANGED_T value)
+void ZoneViewWidget::processGroupBy(int index)
 {
-    pileViewCheckBox.setEnabled(value);
+    auto option = static_cast<CardList::SortOption>(groupBySelector.itemData(index).toInt());
-    SettingsCache::instance().setZoneViewSortByType(value);
+    SettingsCache::instance().setZoneViewGroupBy(option);
-    zone->setPileView(pileViewCheckBox.isChecked());
+    zone->setGroupBy(option);
-    zone->setSortByType(value);
+
+    // disable pile view checkbox if we're not grouping by anything
+    pileViewCheckBox.setEnabled(option != CardList::NoSort);
+
+    // reset sortBy if it has the same value as groupBy
+    if (option != CardList::NoSort &&
+        option == static_cast<CardList::SortOption>(sortBySelector.currentData().toInt())) {
+        sortBySelector.setCurrentIndex(1); // set to SortByName
+    }
 }
 
-void ZoneViewWidget::processSortByName(QT_STATE_CHANGED_T value)
+void ZoneViewWidget::processSortBy(int index)
 {
-    SettingsCache::instance().setZoneViewSortByName(value);
+    auto option = static_cast<CardList::SortOption>(sortBySelector.itemData(index).toInt());
-    zone->setSortByName(value);
+
+    // set to SortByName instead if it has the same value as groupBy
+    if (option != CardList::NoSort &&
+        option == static_cast<CardList::SortOption>(groupBySelector.currentData().toInt())) {
+        sortBySelector.setCurrentIndex(1); // set to SortByName
+        return;
+    }
+
+    SettingsCache::instance().setZoneViewSortBy(option);
+    zone->setSortBy(option);
 }
 
 void ZoneViewWidget::processSetPileView(QT_STATE_CHANGED_T value)
@@ -145,8 +182,6 @@ void ZoneViewWidget::processSetPileView(QT_STATE_CHANGED_T value)
 void ZoneViewWidget::retranslateUi()
 {
     setWindowTitle(zone->getTranslatedName(false, CaseNominative));
-    sortByNameCheckBox.setText(tr("sort by name"));
-    sortByTypeCheckBox.setText(tr("sort by type"));
     shuffleCheckBox.setText(tr("shuffle when closing"));
     pileViewCheckBox.setText(tr("pile view"));
 }

cockatrice/src/game/zones/view_zone_widget.h

@@ -4,6 +4,7 @@
 #include "../../utility/macros.h"
 
 #include <QCheckBox>
+#include <QComboBox>
 #include <QGraphicsProxyWidget>
 #include <QGraphicsWidget>
 
@@ -14,7 +15,6 @@ class ZoneViewZone;
 class Player;
 class CardDatabase;
 class QScrollBar;
-class QCheckBox;
 class GameScene;
 class ServerInfo_Card;
 class QGraphicsSceneMouseEvent;
@@ -47,8 +47,8 @@ private:
     QPushButton *closeButton;
     QScrollBar *scrollBar;
     ScrollableGraphicsProxyWidget *scrollBarProxy;
-    QCheckBox sortByNameCheckBox;
+    QComboBox groupBySelector;
-    QCheckBox sortByTypeCheckBox;
+    QComboBox sortBySelector;
     QCheckBox shuffleCheckBox;
     QCheckBox pileViewCheckBox;
 
@@ -58,8 +58,8 @@ private:
 signals:
     void closePressed(ZoneViewWidget *zv);
 private slots:
-    void processSortByType(QT_STATE_CHANGED_T value);
+    void processGroupBy(int value);
-    void processSortByName(QT_STATE_CHANGED_T value);
+    void processSortBy(int value);
     void processSetPileView(QT_STATE_CHANGED_T value);
     void resizeToZoneContents();
     void handleScrollBarChange(int value);

cockatrice/src/settings/cache_settings.cpp

@@ -252,8 +252,8 @@ SettingsCache::SettingsCache()
     chatMentionColor = settings->value("chat/mentioncolor", "A6120D").toString();
     chatHighlightColor = settings->value("chat/highlightcolor", "A6120D").toString();
 
-    zoneViewSortByName = settings->value("zoneview/sortbyname", true).toBool();
+    zoneViewGroupBy = settings->value("zoneview/groupby", 2).toInt();
-    zoneViewSortByType = settings->value("zoneview/sortbytype", true).toBool();
+    zoneViewSortBy = settings->value("zoneview/sortby", 1).toInt();
     zoneViewPileView = settings->value("zoneview/pileview", true).toBool();
 
     soundEnabled = settings->value("sound/enabled", false).toBool();
@@ -582,16 +582,16 @@ void SettingsCache::setChatHighlightColor(const QString &_chatHighlightColor)
     settings->setValue("chat/highlightcolor", chatHighlightColor);
 }
 
-void SettingsCache::setZoneViewSortByName(QT_STATE_CHANGED_T _zoneViewSortByName)
+void SettingsCache::setZoneViewGroupBy(int _zoneViewGroupBy)
 {
-    zoneViewSortByName = static_cast<bool>(_zoneViewSortByName);
+    zoneViewGroupBy = _zoneViewGroupBy;
-    settings->setValue("zoneview/sortbyname", zoneViewSortByName);
+    settings->setValue("zoneview/groupby", zoneViewGroupBy);
 }
 
-void SettingsCache::setZoneViewSortByType(QT_STATE_CHANGED_T _zoneViewSortByType)
+void SettingsCache::setZoneViewSortBy(int _zoneViewSortBy)
 {
-    zoneViewSortByType = static_cast<bool>(_zoneViewSortByType);
+    zoneViewSortBy = _zoneViewSortBy;
-    settings->setValue("zoneview/sortbytype", zoneViewSortByType);
+    settings->setValue("zoneview/sortby", zoneViewSortBy);
 }
 
 void SettingsCache::setZoneViewPileView(QT_STATE_CHANGED_T _zoneViewPileView)

cockatrice/src/settings/cache_settings.h

@@ -109,7 +109,8 @@ private:
     QString chatHighlightColor;
     bool chatMentionForeground;
     bool chatHighlightForeground;
-    bool zoneViewSortByName, zoneViewSortByType, zoneViewPileView;
+    int zoneViewSortBy, zoneViewGroupBy;
+    bool zoneViewPileView;
     bool soundEnabled;
     QString soundThemeName;
     bool ignoreUnregisteredUsers;
@@ -327,13 +328,13 @@ public:
     {
         return chatHighlightForeground;
     }
-    bool getZoneViewSortByName() const
+    int getZoneViewGroupBy() const
     {
-        return zoneViewSortByName;
+        return zoneViewGroupBy;
     }
-    bool getZoneViewSortByType() const
+    int getZoneViewSortBy() const
     {
-        return zoneViewSortByType;
+        return zoneViewSortBy;
     }
     /**
        Returns if the view should be sorted into pile view.
@@ -563,8 +564,8 @@ public slots:
     void setChatMentionCompleter(QT_STATE_CHANGED_T _chatMentionCompleter);
     void setChatMentionForeground(QT_STATE_CHANGED_T _chatMentionForeground);
     void setChatHighlightForeground(QT_STATE_CHANGED_T _chatHighlightForeground);
-    void setZoneViewSortByName(QT_STATE_CHANGED_T _zoneViewSortByName);
+    void setZoneViewGroupBy(const int _zoneViewGroupBy);
-    void setZoneViewSortByType(QT_STATE_CHANGED_T _zoneViewSortByType);
+    void setZoneViewSortBy(const int _zoneViewSortBy);
     void setZoneViewPileView(QT_STATE_CHANGED_T _zoneViewPileView);
     void setSoundEnabled(QT_STATE_CHANGED_T _soundEnabled);
     void setSoundThemeName(const QString &_soundThemeName);

dbconverter/src/mocks.cpp

@@ -184,10 +184,10 @@ void SettingsCache::setChatMentionColor(const QString & /* _chatMentionColor */)
 void SettingsCache::setChatHighlightColor(const QString & /* _chatHighlightColor */)
 {
 }
-void SettingsCache::setZoneViewSortByName(QT_STATE_CHANGED_T /* _zoneViewSortByName */)
+void SettingsCache::setZoneViewGroupBy(int /* _zoneViewSortByName */)
 {
 }
-void SettingsCache::setZoneViewSortByType(QT_STATE_CHANGED_T /* _zoneViewSortByType */)
+void SettingsCache::setZoneViewSortBy(int /* _zoneViewSortByType */)
 {
 }
 void SettingsCache::setZoneViewPileView(QT_STATE_CHANGED_T /* _zoneViewPileView */)

tests/carddatabase/mocks.cpp

@@ -188,10 +188,10 @@ void SettingsCache::setChatMentionColor(const QString & /* _chatMentionColor */)
 void SettingsCache::setChatHighlightColor(const QString & /* _chatHighlightColor */)
 {
 }
-void SettingsCache::setZoneViewSortByName(QT_STATE_CHANGED_T /* _zoneViewSortByName */)
+void SettingsCache::setZoneViewGroupBy(int /* _zoneViewGroupBy */)
 {
 }
-void SettingsCache::setZoneViewSortByType(QT_STATE_CHANGED_T /* _zoneViewSortByType */)
+void SettingsCache::setZoneViewSortBy(int /* _zoneViewSortBy */)
 {
 }
 void SettingsCache::setZoneViewPileView(QT_STATE_CHANGED_T /* _zoneViewPileView */)