diff --git a/cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp b/cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp index e007e6dd0..b60685d20 100644 --- a/cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp +++ b/cockatrice/src/interface/widgets/deck_editor/deck_state_manager.cpp @@ -470,13 +470,20 @@ QString DeckStateManager::validateNewZoneName(const QString &zoneName) const const auto *tree = deckList->getTree(); - // Reuse the tree's own uniqueness contract: any top-level zone and any - // custom zone on *every* board claims the name (hasZoneName also reserves - // the standard board names, which we already rejected with a dedicated - // message above). Scanning only the standard boards here would miss a - // custom zone an imported deck carries under `tokens`. - if (tree->hasZoneName(trimmedZoneName)) { - return tr("A zone with this name already exists."); + // Top-level zones (boards and legacy zones) claim their names too. + for (int i = 0; i < tree->getRoot()->size(); i++) { + if (tree->getRoot()->at(i)->getName() == trimmedZoneName) { + return tr("A zone with this name already exists."); + } + } + + // Custom zone names are unique across the whole deck. + for (const QString &board : InnerDecklistNode::boardZoneNames()) { + for (const auto *customZone : tree->getCustomZones(board)) { + if (customZone->getName() == trimmedZoneName) { + return tr("A zone with this name already exists."); + } + } } return {}; @@ -551,7 +558,6 @@ bool DeckStateManager::modifyTree(const QString &reason, const std::functionsave(memento); deckListModel->rebuildTree(); deckList->refreshDeckHash(); - emit deckListModel->deckHashChanged(); doCardModified(); } diff --git a/cockatrice/src/interface/widgets/deck_editor/deck_zone_dialog.cpp b/cockatrice/src/interface/widgets/deck_editor/deck_zone_dialog.cpp index 9a0be2570..a14beec0e 100644 --- a/cockatrice/src/interface/widgets/deck_editor/deck_zone_dialog.cpp +++ b/cockatrice/src/interface/widgets/deck_editor/deck_zone_dialog.cpp @@ -50,9 +50,6 @@ DeckZoneDialog::DeckZoneDialog(QWidget *parent, if (allowBoardSelection) { layout->addWidget(boardLabel); layout->addWidget(boardCombo); - } else { - boardLabel->hide(); - boardCombo->hide(); } layout->addWidget(buttonBox); diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_node_tree.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_node_tree.h index 3c88d6030..af1193f26 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_node_tree.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list_node_tree.h @@ -115,15 +115,6 @@ public: */ QList getCustomZones(const QString &boardZoneName) const; - /** - * @brief Checks whether a zone name is taken anywhere in the deck. - * - * Covers the standard board names and any top-level or nested custom zone. - * @param zoneName The checked name. - * @return true if the name is reserved or already in use. - */ - bool hasZoneName(const QString &zoneName) const; - /** * @brief Applies a function to every card in the deck tree. This can modify the cards. * @@ -138,6 +129,7 @@ private: InnerDecklistNode *findBoardZone(const QString &boardZoneName) const; InnerDecklistNode *findOrCreateBoardZone(const QString &boardZoneName); InnerDecklistNode *findCustomZoneByName(const QString &zoneName) const; + bool hasZoneName(const QString &zoneName) const; }; #endif // COCKATRICE_DECKLIST_NODE_TREE_H diff --git a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model.cpp b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model.cpp index 5e9fb6b4a..ad278c8bf 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model.cpp +++ b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model.cpp @@ -378,8 +378,7 @@ bool DeckListModel::removeRows(int row, int count, const QModelIndex &parent) InnerDecklistNode *DeckListModel::createNodeIfNeeded(const QString &name, InnerDecklistNode *parent) { - // Group lookups must not resolve a mirrored custom zone that shares the name. - auto *newNode = DeckListModelCustomZones::findGroupChild(parent, name); + auto *newNode = dynamic_cast(parent->findChild(name)); if (!newNode) { beginInsertRows(nodeToIndex(parent), parent->size(), parent->size()); newNode = new InnerDecklistNode(name, parent); @@ -402,7 +401,7 @@ DecklistModelCardNode *DeckListModel::findCardNode(const QString &cardName, // nested under the board. if (auto *zoneNode = dynamic_cast(root->findChild(zoneName))) { QString groupCriteria = extractGroupCriteriaValue(info, activeGroupCriteria); - if (auto *groupNode = DeckListModelCustomZones::findGroupChild(zoneNode, groupCriteria)) { + if (auto *groupNode = dynamic_cast(zoneNode->findChild(groupCriteria))) { if (auto *card = dynamic_cast( groupNode->findCardChildByNameProviderIdAndNumber(cardName, providerId, cardNumber))) { return card; @@ -487,26 +486,6 @@ QModelIndex DeckListModel::addCard(const ExactCard &card, const QString &zoneNam // Custom zone: cards live flat inside the zone. cardParent = customZoneNode; } else { - // Not present in the shadow tree. The deck tree may still hold a custom - // zone that has not been mirrored (callers can add a zone and then a - // card without a rebuild). Check before falling back to creating a - // top-level zone the deck does not actually have. - auto *listRoot = deckList->getTree()->getRoot(); - bool hasDeckZone = false; - for (int i = 0; i < listRoot->size(); ++i) { - if (auto *boardZone = dynamic_cast(listRoot->at(i))) { - if (boardZone->findChild(zoneName)) { - hasDeckZone = true; - break; - } - } - } - - if (hasDeckZone) { - rebuildTree(); - return addCard(card, zoneName); - } - // Unknown zone: create a top-level zone (legacy behavior). QString groupCriteria = extractGroupCriteriaValue(cardInfo, activeGroupCriteria); auto *newZone = createNodeIfNeeded(zoneName, root); diff --git a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.cpp b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.cpp index 1dc745e63..f10ee1a72 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.cpp +++ b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.cpp @@ -14,55 +14,26 @@ bool isCustomZone(const AbstractDecklistNode *node) return dynamic_cast(node) != nullptr; } -namespace -{ - -/** - * @brief Flattens every card under @p zone into @p shadowZone, preserving order. - * - * Custom zones mirror as a single row level: cards nested in sub-zones of any - * depth are added as direct children of the mirrored zone so no card is left - * without a model row. - */ -void flattenCards(const InnerDecklistNode *zone, InnerDecklistNode *shadowZone) -{ - for (int k = 0; k < zone->size(); k++) { - if (auto *zoneCard = dynamic_cast(zone->at(k))) { - new DecklistModelCardNode(zoneCard, shadowZone); - } else if (auto *subZone = dynamic_cast(zone->at(k))) { - flattenCards(subZone, shadowZone); - } - } -} - -} // namespace - void mirrorCustomZones(const InnerDecklistNode *deckBoardZone, InnerDecklistNode *shadowBoardZone) { for (int j = 0; j < deckBoardZone->size(); j++) { + auto *customCard = dynamic_cast(deckBoardZone->at(j)); + if (customCard) { + continue; + } + auto *customZone = dynamic_cast(deckBoardZone->at(j)); if (!customZone) { continue; } auto *shadowZone = new DecklistModelSubZoneNode(customZone->getName(), shadowBoardZone); - flattenCards(customZone, shadowZone); - } -} - -InnerDecklistNode *findGroupChild(InnerDecklistNode *parent, const QString &name) -{ - for (int i = 0; i < parent->size(); i++) { - AbstractDecklistNode *child = parent->at(i); - if (isCustomZone(child)) { - continue; - } - auto *group = dynamic_cast(child); - if (group && group->getName() == name) { - return group; + for (int k = 0; k < customZone->size(); k++) { + if (auto *zoneCard = dynamic_cast(customZone->at(k))) { + new DecklistModelCardNode(zoneCard, shadowZone); + } } } - return nullptr; } DecklistModelSubZoneNode *findSubZoneByName(InnerDecklistNode *root, const QString &zoneName) diff --git a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.h b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.h index 518a9e1d2..a973b127e 100644 --- a/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.h +++ b/libcockatrice_models/libcockatrice/models/deck_list/deck_list_model_custom_zones.h @@ -43,21 +43,6 @@ namespace DeckListModelCustomZones */ [[nodiscard]] bool isCustomZone(const AbstractDecklistNode *node); -/** - * @brief Finds a criteria-group child of @p parent by name, skipping custom zones. - * - * The shadow tree keeps criteria groups and mirrored custom zones as siblings - * under a board zone, and `InnerDecklistNode::findChild` matches both by name. - * Group lookups must not resolve a custom zone that happens to share the group - * name (e.g. a zone called "Creature"), so this searches only non-custom - * children. - * - * @param parent The shadow node whose children are searched. - * @param name The group name to find. - * @return The matching group node, or nullptr if none exists. - */ -[[nodiscard]] InnerDecklistNode *findGroupChild(InnerDecklistNode *parent, const QString &name); - /** * @brief Mirrors the custom zones of a deck board zone into its shadow board node. * diff --git a/tests/deck_list_model/CMakeLists.txt b/tests/deck_list_model/CMakeLists.txt index e3096c559..8f2cfcc2c 100644 --- a/tests/deck_list_model/CMakeLists.txt +++ b/tests/deck_list_model/CMakeLists.txt @@ -1,4 +1,4 @@ -add_executable(deck_list_model_custom_zones_test ${VERSION_STRING_CPP} deck_list_model_custom_zones_test.cpp) +add_executable(deck_list_model_custom_zones_test deck_list_model_custom_zones_test.cpp) if(NOT GTEST_FOUND) add_dependencies(deck_list_model_custom_zones_test gtest) diff --git a/tests/deck_list_model/deck_list_model_custom_zones_test.cpp b/tests/deck_list_model/deck_list_model_custom_zones_test.cpp index 7f4ab81e1..c51c72d8f 100644 --- a/tests/deck_list_model/deck_list_model_custom_zones_test.cpp +++ b/tests/deck_list_model/deck_list_model_custom_zones_test.cpp @@ -129,51 +129,6 @@ TEST(DeckListModelCustomZones, MirrorCustomZonesWithNoCustomZonesIsNoop) EXPECT_EQ(shadowBoard->size(), 0); } -TEST(DeckListModelCustomZones, MirrorCustomZonesFlattensNestedSubzones) -{ - // Cards deeper than one level under a custom zone still get a model row. - auto *deckBoard = new InnerDecklistNode(DECK_ZONE_MAIN); - auto *deckZone = new InnerDecklistNode("Removal", deckBoard); - auto *deckCard1 = new DecklistCardNode("Bolt", 1, deckZone); - auto *deeper = new InnerDecklistNode("Deeper", deckZone); - auto *deckCard2 = new DecklistCardNode("Swords", 1, deeper); - - InnerDecklistNode shadowRoot; - auto *shadowBoard = new InnerDecklistNode(DECK_ZONE_MAIN, &shadowRoot); - - DeckListModelCustomZones::mirrorCustomZones(deckBoard, shadowBoard); - - ASSERT_EQ(shadowBoard->size(), 1); - auto *shadowZone = dynamic_cast(shadowBoard->at(0)); - ASSERT_NE(shadowZone, nullptr); - EXPECT_EQ(shadowZone->getName(), QString("Removal")); - - // Both cards are flattened into the mirrored zone, preserving order. - ASSERT_EQ(shadowZone->size(), 2); - auto *shadowCard1 = dynamic_cast(shadowZone->at(0)); - auto *shadowCard2 = dynamic_cast(shadowZone->at(1)); - ASSERT_NE(shadowCard1, nullptr); - ASSERT_NE(shadowCard2, nullptr); - EXPECT_EQ(shadowCard1->getDataNode(), deckCard1); - EXPECT_EQ(shadowCard2->getDataNode(), deckCard2); -} - -// ===================================================================================================================== -// findGroupChild -// ===================================================================================================================== - -TEST(DeckListModelCustomZones, FindGroupChildSkipsCustomZones) -{ - InnerDecklistNode root; - auto *board = new InnerDecklistNode(DECK_ZONE_MAIN, &root); - auto *group = new InnerDecklistNode("Creature", board); - new DecklistModelSubZoneNode("Creature", board); - - EXPECT_EQ(DeckListModelCustomZones::findGroupChild(board, "Creature"), group); - EXPECT_EQ(DeckListModelCustomZones::findGroupChild(board, "Missing"), nullptr); - EXPECT_EQ(DeckListModelCustomZones::findGroupChild(&root, DECK_ZONE_MAIN), board); -} - // ===================================================================================================================== // sortWithCustomZonesLast // ===================================================================================================================== @@ -268,9 +223,3 @@ TEST(DeckListModelCustomZones, SortPlainNodeDoesNotReorderCustomZones) EXPECT_EQ(mapping[1].first, 0); EXPECT_EQ(mapping[1].second, 1); } - -int main(int argc, char **argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/tests/deck_list_model/deck_list_model_zone_integration_test.cpp b/tests/deck_list_model/deck_list_model_zone_integration_test.cpp index d49d1b443..530e1c351 100644 --- a/tests/deck_list_model/deck_list_model_zone_integration_test.cpp +++ b/tests/deck_list_model/deck_list_model_zone_integration_test.cpp @@ -1,8 +1,4 @@ #include -#include -#include -#include -#include #include #include #include @@ -29,32 +25,6 @@ int totalCustomZoneRows(const DeckListModel &model) return count; } -QModelIndex findBoardIndex(const DeckListModel &model, const QString &boardName) -{ - for (int r = 0; r < model.rowCount(QModelIndex()); ++r) { - const QModelIndex idx = model.index(r, 0, QModelIndex()); - if (idx.data(DeckRoles::IsCardRole).toBool()) { - continue; - } - const QString name = idx.sibling(idx.row(), DeckListModelColumns::CARD_NAME).data(Qt::EditRole).toString(); - if (name == boardName) { - return idx; - } - } - return {}; -} - -QModelIndex findZoneRow(const DeckListModel &model, const QModelIndex &board) -{ - for (int r = 0; r < model.rowCount(board); ++r) { - const QModelIndex child = model.index(r, 0, board); - if (child.data(DeckRoles::IsCustomZoneRole).toBool()) { - return child; - } - } - return {}; -} - } // namespace // The "Add to Zone" combobox/submenu lists getCustomZoneNames(), which reads the @@ -99,148 +69,3 @@ TEST(DeckListModelZoneIntegration, RebuildTreeMirrorsEachZoneOnce) EXPECT_EQ(model.getCustomZoneNames(DECK_ZONE_MAIN), (QStringList{"Removal", "Utility"})); EXPECT_EQ(totalCustomZoneRows(model), 2); } - -// ===================================================================================================================== -// Model behaviour: addCard routing, findCard lookup, removeRows guard, empty-zone survival. -// ===================================================================================================================== - -TEST(DeckListModelZoneIntegration, AddCardRoutesIntoMirroredCustomZone) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr); - model.rebuildTree(); - - QModelIndex added = model.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), "Removal"); - ASSERT_TRUE(added.isValid()); - - // The card is a direct child of the mirrored custom zone, not a new top-level zone. - const QModelIndex zoneParent = added.parent(); - ASSERT_TRUE(zoneParent.isValid()); - EXPECT_TRUE(zoneParent.data(DeckRoles::IsCustomZoneRole).toBool()); - EXPECT_EQ(zoneParent.sibling(zoneParent.row(), DeckListModelColumns::CARD_NAME).data(Qt::DisplayRole).toString(), - QString("Removal")); - - // No "Removal" top-level zone appeared in the deck tree. - auto *listRoot = tree->getRoot(); - bool topLevelRemoval = false; - for (int i = 0; i < listRoot->size(); ++i) { - if (auto *zone = dynamic_cast(listRoot->at(i))) { - topLevelRemoval |= zone->getName() == "Removal"; - } - } - EXPECT_FALSE(topLevelRemoval); -} - -TEST(DeckListModelZoneIntegration, AddCardToUnmirroredCustomZoneRebuildsNotCreatesTopLevel) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - // The zone exists on the deck tree but the shadow tree has never mirrored it. - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr); - - QModelIndex added = model.addCard(ExactCard(CardInfo::newInstance("Lightning Bolt")), "Removal"); - ASSERT_TRUE(added.isValid()); - - const QModelIndex zoneParent = added.parent(); - ASSERT_TRUE(zoneParent.isValid()); - EXPECT_TRUE(zoneParent.data(DeckRoles::IsCustomZoneRole).toBool()); - EXPECT_EQ(zoneParent.sibling(zoneParent.row(), DeckListModelColumns::CARD_NAME).data(Qt::DisplayRole).toString(), - QString("Removal")); -} - -TEST(DeckListModelZoneIntegration, AddCardCreatesGroupSeparatelyFromSameNamedZone) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - // A custom zone named exactly like a grouping criterion. - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Creature"), nullptr); - model.rebuildTree(); - - CardInfoPtr bear = CardInfo::newInstance("Grizzly Bears"); - bear->setProperty(Mtg::MainCardType, "Creature"); - - QModelIndex added = model.addCard(ExactCard(bear), DECK_ZONE_MAIN); - ASSERT_TRUE(added.isValid()); - - // The card lands in a *group* node called "Creature", not swallowed by the custom zone. - const QModelIndex groupParent = added.parent(); - ASSERT_TRUE(groupParent.isValid()); - EXPECT_FALSE(groupParent.data(DeckRoles::IsCustomZoneRole).toBool()); - EXPECT_EQ(groupParent.sibling(groupParent.row(), DeckListModelColumns::CARD_NAME).data(Qt::DisplayRole).toString(), - QString("Creature")); - - // The board keeps both rows: the "Creature" group and the "Creature" custom zone. - const QModelIndex boardIndex = groupParent.parent(); - ASSERT_TRUE(boardIndex.isValid()); - EXPECT_EQ(model.rowCount(boardIndex), 2); -} - -TEST(DeckListModelZoneIntegration, FindCardResolvesCardInsideCustomZone) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr); - model.rebuildTree(); - - // findCard resolves through the card database; register the card we add. - const QString cardName = "Swords to Plowshares"; - CardInfoPtr info = CardInfo::newInstance(cardName); - CardDatabaseManager::getInstance()->addCard(info); - - QModelIndex added = model.addCard(ExactCard(info), "Removal"); - ASSERT_TRUE(added.isValid()); - - QModelIndex found = model.findCard(cardName, "Removal"); - EXPECT_TRUE(found.isValid()); - EXPECT_EQ(found, added); -} - -TEST(DeckListModelZoneIntegration, RemoveRowsRefusesCustomZoneRow) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr); - tree->addCard("Lightning Bolt", 2, DECK_ZONE_MAIN, -1); - model.rebuildTree(); - - const QModelIndex mainIndex = findBoardIndex(model, DECK_ZONE_MAIN); - ASSERT_TRUE(mainIndex.isValid()); - const QModelIndex zoneRow = findZoneRow(model, mainIndex); - ASSERT_TRUE(zoneRow.isValid()); - - EXPECT_FALSE(model.removeRow(zoneRow.row(), zoneRow.parent())); - EXPECT_EQ(model.rowCount(mainIndex), 2); // the zone survives, alongside the card group -} - -TEST(DeckListModelZoneIntegration, EmptyCustomZoneSurvivesMirrorAndPruning) -{ - QSharedPointer deck(new DeckList()); - DeckListModel model(nullptr, deck); - auto *tree = deck->getTree(); - - // An empty custom zone must be mirrored (the stack deliberately keeps it alive). - ASSERT_NE(tree->addCustomZone(DECK_ZONE_MAIN, "Removal"), nullptr); - model.rebuildTree(); - - const QModelIndex mainIndex = findBoardIndex(model, DECK_ZONE_MAIN); - ASSERT_TRUE(mainIndex.isValid()); - EXPECT_EQ(model.rowCount(mainIndex), 1); - EXPECT_TRUE(findZoneRow(model, mainIndex).isValid()); -} - -int main(int argc, char **argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -}