[Client] Make DeckStateManager::decrementCard remove tokens from the token zone if the token is not in the given zone (#7588)

* Fixed decrementCard for tokens not using the token zone

* Removed dead function actDecrementCard

Updated documentation

* Fixed no-op, added documentation

* Removed stray blank line

* Allow finding card nodes even if no database entry exists for the given card

* Added DeckStateManager tests

* Added tokens on mainboard test

* Added if checks and sorted sources

Added if checks

* Library include fix

* Renamed zone to targetZone

* Revert "Allow finding card nodes even if no database entry exists for the given card"

This reverts commit 88578cd0d9.

* Combined loader_local_matching_test and deck_state_manager_test settings sources

* Added settings sandbox, refactor

* Format

* Renamed return variable

* Flattened if statement

* Style fix

cockatrice/src/interface/widgets/deck_editor/deck_editor_deck_dock_widget.cpp

@@ -762,18 +762,6 @@ void DeckEditorDeckDockWidget::actSwapSelection()
     update();
 }
 
-void DeckEditorDeckDockWidget::actDecrementCard(const ExactCard &card, QString zoneName)
-{
-    if (!card) {
-        return;
-    }
-    if (card.getInfo().getIsToken()) {
-        zoneName = DECK_ZONE_TOKENS;
-    }
-
-    deckStateManager->decrementCard(card, zoneName);
-}
-
 void DeckEditorDeckDockWidget::actDecrementSelection()
 {
     auto selectedRows = getSelectedCardNodeSourceIndices();

cockatrice/src/interface/widgets/deck_editor/deck_editor_deck_dock_widget.h

@@ -70,7 +70,6 @@ public slots:
     void syncDisplayWidgetsToModel();
     void actAddCard(const ExactCard &card, const QString &zoneName);
     void actIncrementSelection();
-    void actDecrementCard(const ExactCard &card, QString zoneName);
     void actDecrementSelection();
     void actSwapCard(const ExactCard &card, const QString &zoneName);
     void actSwapSelection();

cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp

@@ -211,15 +211,15 @@ QModelIndex DeckStateManager::addCard(const ExactCard &card, const QString &zone
         return {};
     }
 
-    QString zone = card.getInfo().getIsToken() ? DECK_ZONE_TOKENS : zoneName;
+    QString targetZone = card.getInfo().getIsToken() ? DECK_ZONE_TOKENS : zoneName;
 
     CardSetPtr set = card.getPrinting().getSet();
     QString setName = set ? set->getCorrectedShortName() : "";
 
     QString reason =
-        tr("Added (%1): %2 (%3) %4").arg(zone, card.getName(), setName, card.getPrinting().getProperty("num"));
+        tr("Added (%1): %2 (%3) %4").arg(targetZone, card.getName(), setName, card.getPrinting().getProperty("num"));
 
-    QModelIndex idx = modifyDeck(reason, [&card, &zone](auto model) { return model->addCard(card, zone); });
+    QModelIndex idx = modifyDeck(reason, [&card, &targetZone](auto model) { return model->addCard(card, targetZone); });
 
     if (idx.isValid()) {
         emit focusIndexChanged(idx, true);
@@ -234,10 +234,22 @@ QModelIndex DeckStateManager::decrementCard(const ExactCard &card, const QString
         return {};
     }
 
+    QString targetZone = zoneName;
+
     QString providerId = card.getPrinting().getUuid();
     QString collectorNumber = card.getPrinting().getProperty("num");
 
-    QModelIndex idx = deckListModel->findCard(card.getName(), zoneName, providerId, collectorNumber);
+    QModelIndex idx = deckListModel->findCard(card.getName(), targetZone, providerId, collectorNumber);
+
+    // Tokens are usually added to the token zone, but it is possible for imported/hand-edited
+    // decks to contain them in the main or sideboards. This is why we only try to remove the
+    // token from the token zone if it does not exist in the given zone, allowing us to still
+    // remove it from there.
+    if (!idx.isValid() && card.getInfo().getIsToken() && targetZone != DECK_ZONE_TOKENS) {
+        targetZone = DECK_ZONE_TOKENS;
+        idx = deckListModel->findCard(card.getName(), targetZone, providerId, collectorNumber);
+    }
+
     if (!idx.isValid()) {
         return {};
     }
@@ -249,7 +261,7 @@ QModelIndex DeckStateManager::decrementCard(const ExactCard &card, const QString
     }
 
     // old index is no longer safe since rows could have been removed
-    QModelIndex newIdx = deckListModel->findCard(card.getName(), zoneName, providerId, collectorNumber);
+    QModelIndex newIdx = deckListModel->findCard(card.getName(), targetZone, providerId, collectorNumber);
     if (newIdx.isValid()) {
         emit focusIndexChanged(newIdx, true);
     }

cockatrice/src/interface/widgets/deck_editor/deck_state_manager.h

@@ -188,17 +188,18 @@ public:
     ///@}
 
     /**
-     * @brief Adds the given card to the given zone.
+     * @brief Adds the given card to the given zone. Tokens always go to DECK_ZONE_TOKENS, ignoring the given zone.
      * Saves the operation to history if successful.
      *
      * @param card The card to add
-     * @param zoneName The zone to add the card to
+     * @param zoneName The zone to add the card to if the card is not a token
      * @return The index of the added card
      */
     QModelIndex addCard(const ExactCard &card, const QString &zoneName);
 
     /**
      * @brief Removes 1 copy of the given card from the given zone.
+     * If the card is a token and is not in the given zone, then it is removed from DECK_ZONE_TOKENS instead.
      * Saves the operation to history if successful.
      *
      * @param card The card to remove

tests/CMakeLists.txt

@@ -412,23 +412,6 @@ cockatrice_add_test(
   INCLUDE_DIRS
   ${CMAKE_SOURCE_DIR}/cockatrice/src
 )
-cockatrice_add_test(
-  NAME
-  loader_local_matching_test
-  SOURCES
-  ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp
-  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp
-  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp
-  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp
-  ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp
-  ${VERSION_STRING_CPP}
-  loader_local_matching_test.cpp
-  LIBS
-  libcockatrice_network
-  libcockatrice_settings
-  INCLUDE_DIRS
-  ${CMAKE_SOURCE_DIR}/cockatrice/src
-)
 cockatrice_add_test(
   NAME
   deck_hash_performance_test
@@ -548,3 +531,46 @@ endif()
 if(WITH_ORACLE OR WITH_CLIENT)
   add_subdirectory(picture_loader_benchmark)
 endif()
+
+# loader_local_matching_test and deck_state_manager_test links libcockatrice_settings,
+# which only exists when a client/UI-capable target is being built.
+if(WITH_ORACLE OR WITH_CLIENT)
+  set(SETTINGS_SOURCES
+      ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/client/release_channel.cpp
+      ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/cache_settings.cpp
+      ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/card_counter_settings.cpp
+      ${CMAKE_SOURCE_DIR}/cockatrice/src/client/settings/shortcuts_settings.cpp
+      ${VERSION_STRING_CPP}
+  )
+  set(SETTINGS_LIBS libcockatrice_network libcockatrice_settings)
+  set(SETTINGS_INCLUDE_DIRS ${CMAKE_SOURCE_DIR}/cockatrice/src)
+
+  cockatrice_add_test(
+    NAME
+    loader_local_matching_test
+    SOURCES
+    ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp
+    ${SETTINGS_SOURCES}
+    loader_local_matching_test.cpp
+    LIBS
+    ${SETTINGS_LIBS}
+    INCLUDE_DIRS
+    ${SETTINGS_INCLUDE_DIRS}
+  )
+  cockatrice_add_test(
+    NAME
+    deck_state_manager_test
+    SOURCES
+    ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp
+    ${SETTINGS_SOURCES}
+    deck_state_manager_test.cpp
+    LIBS
+    libcockatrice_card
+    libcockatrice_deck_list
+    libcockatrice_models
+    libcockatrice_utility
+    ${SETTINGS_LIBS}
+    INCLUDE_DIRS
+    ${SETTINGS_INCLUDE_DIRS}
+  )
+endif()

tests/deck_state_manager_test.cpp

@@ -0,0 +1,190 @@
+/** @file deck_state_manager_test.cpp
+ *  @brief Tests for DeckStateManager operations.
+ *  @ingroup Tests
+ */
+
+#include "libcockatrice/card/database/card_database.h"
+
+#include <QCoreApplication>
+#include <QDir>
+#include <QStandardPaths>
+#include <client/settings/cache_settings.h>
+#include <deck_list_model.h>
+#include <gtest/gtest.h>
+#include <interface/widgets/deck_editor/deck_state_manager.h>
+#include <libcockatrice/card/database/card_database_manager.h>
+#include <libcockatrice/card/printing/exact_card.h>
+#include <libcockatrice/deck_list/tree/inner_deck_list_node.h>
+
+namespace
+{
+
+/**
+ * @brief Assert that the DeckStateManager's deck list contains only the given zone,
+ * and that that zone only contains the given number of cards, all with the given card name.
+ *
+ * @param manager the DeckStateManager
+ * @param restrictToZones In which zones to look for the zone and cards. Empty means use the entire deck
+ * @param zone The only zone that should exist in the deck
+ * @param cardName The name of the only card that should exist in the deck
+ * @param cardNumber The number of cards that should be in the deck
+ */
+static void containsExactly(const DeckStateManager &manager,
+                            const QSet<QString> &restrictToZones,
+                            QString zone,
+                            QString cardName,
+                            int cardNumber)
+{
+    QList<const InnerDecklistNode *> zones = manager.getDeckList().getZoneNodes(restrictToZones);
+    ASSERT_EQ(zones.size(), 1);
+    ASSERT_EQ(zones[0]->getName(), zone);
+    ASSERT_EQ(zones[0]->size(), 1);
+    const DecklistCardNode *card = dynamic_cast<const DecklistCardNode *>((*zones[0])[0]);
+    ASSERT_NE(card, nullptr);
+    ASSERT_EQ(card->getName(), cardName);
+    ASSERT_EQ(card->getNumber(), cardNumber);
+}
+
+/** @brief Return CardInfo of a token with the given name. */
+static CardInfoPtr makeToken(QString name)
+{
+    return CardInfo::newInstance(name, "", true, {}, {}, {}, {}, {});
+}
+
+// =====================================================================================================================
+// addCard
+// =====================================================================================================================
+
+TEST(DeckStateManager, AddCards)
+{
+    DeckStateManager manager;
+    manager.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    containsExactly(manager, {}, DECK_ZONE_MAIN, "Lightning Bolt", 1);
+    manager.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    containsExactly(manager, {}, DECK_ZONE_MAIN, "Lightning Bolt", 2);
+}
+
+TEST(DeckStateManager, AddCardZones)
+{
+    DeckStateManager manager;
+    manager.addCard(ExactCard(CardInfo::newInstance("Grizzly Bears")), DECK_ZONE_MAIN);
+    manager.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_SIDE);
+    manager.addCard(ExactCard(CardInfo::newInstance("Swords to Plowshares")), DECK_ZONE_MAYBEBOARD);
+
+    // Check if the cards got put into the correct zones.
+    ASSERT_EQ(manager.getDeckList().getZoneNodes().size(), 3);
+    containsExactly(manager, {DECK_ZONE_MAIN}, DECK_ZONE_MAIN, "Grizzly Bears", 1);
+    containsExactly(manager, {DECK_ZONE_SIDE}, DECK_ZONE_SIDE, "Lightning Bolt", 1);
+    containsExactly(manager, {DECK_ZONE_MAYBEBOARD}, DECK_ZONE_MAYBEBOARD, "Swords to Plowshares", 1);
+}
+
+TEST(DeckStateManager, AddTokens)
+{
+    DeckStateManager manager;
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAIN);
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_SIDE);
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAYBEBOARD);
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_TOKENS);
+
+    // A token should always be put into the tokens zone.
+    containsExactly(manager, {}, DECK_ZONE_TOKENS, "Beast", 4);
+}
+
+// =====================================================================================================================
+// decrementCard
+// =====================================================================================================================
+
+TEST(DeckStateManager, DecrementCards)
+{
+    DeckStateManager manager;
+    manager.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    manager.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    containsExactly(manager, {}, DECK_ZONE_MAIN, "Lightning Bolt", 2);
+
+    // Removing from the sideboard should do nothing, as there is nothing in the side board.
+    manager.decrementCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_SIDE);
+    containsExactly(manager, {}, DECK_ZONE_MAIN, "Lightning Bolt", 2);
+
+    // Remove the first card.
+    manager.decrementCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    containsExactly(manager, {}, DECK_ZONE_MAIN, "Lightning Bolt", 1);
+
+    // Removing the second card should also remove the zone.
+    manager.decrementCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), DECK_ZONE_MAIN);
+    ASSERT_EQ(manager.getDeckList().getZoneNodes().size(), 0);
+}
+
+TEST(DeckStateManager, DecrementTokens)
+{
+    DeckStateManager manager;
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAIN);
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_SIDE);
+    manager.addCard(ExactCard(makeToken("Beast")), DECK_ZONE_TOKENS);
+    containsExactly(manager, {}, DECK_ZONE_TOKENS, "Beast", 3);
+
+    manager.decrementCard(ExactCard(makeToken("Beast")), DECK_ZONE_TOKENS);
+    containsExactly(manager, {}, DECK_ZONE_TOKENS, "Beast", 2);
+
+    // Removing tokens from a given zone should remove them from the
+    // token zone if the given zone does not contain the token.
+    manager.decrementCard(ExactCard(makeToken("Beast")), DECK_ZONE_SIDE);
+    containsExactly(manager, {}, DECK_ZONE_TOKENS, "Beast", 1);
+
+    manager.decrementCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAIN);
+    ASSERT_EQ(manager.getDeckList().getZoneNodes().size(), 0);
+}
+
+TEST(DeckStateManager, DecrementTokenMainboard)
+{
+    DeckStateManager manager;
+
+    // Add cards to the deck list directly so tokens are not
+    // automatically added to the token zone.
+    manager.getDeckListShared()->addCard("Beast", DECK_ZONE_MAIN);
+    manager.getDeckListShared()->addCard("Beast", DECK_ZONE_TOKENS);
+    ASSERT_EQ(manager.getDeckList().getZoneNodes().size(), 2);
+    containsExactly(manager, {DECK_ZONE_MAIN}, DECK_ZONE_MAIN, "Beast", 1);
+    containsExactly(manager, {DECK_ZONE_TOKENS}, DECK_ZONE_TOKENS, "Beast", 1);
+
+    // decrementCard uses the model to find the card, so we need to rebuild it.
+    manager.getModel()->rebuildTree();
+
+    // Tokens should be removed from the main zone first.
+    manager.decrementCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAIN);
+    ASSERT_EQ(manager.getDeckList().getZoneNodes({DECK_ZONE_MAIN}).size(), 0);
+    containsExactly(manager, {DECK_ZONE_TOKENS}, DECK_ZONE_TOKENS, "Beast", 1);
+
+    // If the main zone is empty they can be removed from the token zone.
+    manager.decrementCard(ExactCard(makeToken("Beast")), DECK_ZONE_MAIN);
+    ASSERT_EQ(manager.getDeckList().getZoneNodes().size(), 0);
+}
+} // namespace
+
+int main(int argc, char **argv)
+{
+    // Redirect SettingsCache reads/writes (app-data location) away from the real user profile.
+    QStandardPaths::setTestModeEnabled(true);
+    QCoreApplication app(argc, argv);
+    QCoreApplication::setOrganizationName(QStringLiteral("CockatriceTest"));
+    QCoreApplication::setApplicationName(
+        QStringLiteral("deck-state-manager-test-%1").arg(QCoreApplication::applicationPid()));
+
+    ::testing::InitGoogleTest(&argc, argv);
+
+    // Add cards to the card database
+    auto *db = CardDatabaseManager::getInstance();
+    db->addCard(CardInfo::newInstance("Lightning Bolt"));
+    db->addCard(CardInfo::newInstance("Grizzly Bears"));
+    db->addCard(CardInfo::newInstance("Swords to Plowshares"));
+    db->addCard(makeToken("Beast"));
+
+    int result = RUN_ALL_TESTS();
+
+    // Clean up the settings directory in ~/.qttest if the tests ran.
+    if (!::testing::GTEST_FLAG(list_tests)) {
+        QDir(SettingsCache::instance().getDataPath()).removeRecursively();
+        QDir(SettingsCache::instance().getCachePath()).removeRecursively();
+    }
+
+    return result;
+}