[Models] Route group lookups around mirrored custom zones

Group lookups (createNodeIfNeeded, findCardNode) must not resolve a
mirrored custom zone that shares the group name. Introduce
findGroupChild to search only non-custom children, and make addCard
consult the deck tree before falling back to creating a top-level zone
so cards added to an un-mirrored custom zone land inside it.

mirrorCustomZones now flattens cards nested at any depth into the
mirrored zone so no card is left without a model row.

Add model behaviour tests (addCard routing, same-name group/zone
collision, removeRows guard, empty-zone survival, findCard inside a
custom zone) and fix the missing main() in the unit test binaries.
This commit is contained in:
Lukas Brübach 2026-08-30 22:15:58 +02:00 committed by GitHub
parent 80784d4e02
commit e221d8e1bb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 303 additions and 12 deletions

View file

@ -378,7 +378,8 @@ bool DeckListModel::removeRows(int row, int count, const QModelIndex &parent)
InnerDecklistNode *DeckListModel::createNodeIfNeeded(const QString &name, InnerDecklistNode *parent) InnerDecklistNode *DeckListModel::createNodeIfNeeded(const QString &name, InnerDecklistNode *parent)
{ {
auto *newNode = dynamic_cast<InnerDecklistNode *>(parent->findChild(name)); // Group lookups must not resolve a mirrored custom zone that shares the name.
auto *newNode = DeckListModelCustomZones::findGroupChild(parent, name);
if (!newNode) { if (!newNode) {
beginInsertRows(nodeToIndex(parent), parent->size(), parent->size()); beginInsertRows(nodeToIndex(parent), parent->size(), parent->size());
newNode = new InnerDecklistNode(name, parent); newNode = new InnerDecklistNode(name, parent);
@ -401,7 +402,7 @@ DecklistModelCardNode *DeckListModel::findCardNode(const QString &cardName,
// nested under the board. // nested under the board.
if (auto *zoneNode = dynamic_cast<InnerDecklistNode *>(root->findChild(zoneName))) { if (auto *zoneNode = dynamic_cast<InnerDecklistNode *>(root->findChild(zoneName))) {
QString groupCriteria = extractGroupCriteriaValue(info, activeGroupCriteria); QString groupCriteria = extractGroupCriteriaValue(info, activeGroupCriteria);
if (auto *groupNode = dynamic_cast<InnerDecklistNode *>(zoneNode->findChild(groupCriteria))) { if (auto *groupNode = DeckListModelCustomZones::findGroupChild(zoneNode, groupCriteria)) {
if (auto *card = dynamic_cast<DecklistModelCardNode *>( if (auto *card = dynamic_cast<DecklistModelCardNode *>(
groupNode->findCardChildByNameProviderIdAndNumber(cardName, providerId, cardNumber))) { groupNode->findCardChildByNameProviderIdAndNumber(cardName, providerId, cardNumber))) {
return card; return card;
@ -486,6 +487,26 @@ QModelIndex DeckListModel::addCard(const ExactCard &card, const QString &zoneNam
// Custom zone: cards live flat inside the zone. // Custom zone: cards live flat inside the zone.
cardParent = customZoneNode; cardParent = customZoneNode;
} else { } 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<InnerDecklistNode *>(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). // Unknown zone: create a top-level zone (legacy behavior).
QString groupCriteria = extractGroupCriteriaValue(cardInfo, activeGroupCriteria); QString groupCriteria = extractGroupCriteriaValue(cardInfo, activeGroupCriteria);
auto *newZone = createNodeIfNeeded(zoneName, root); auto *newZone = createNodeIfNeeded(zoneName, root);

View file

@ -14,26 +14,55 @@ bool isCustomZone(const AbstractDecklistNode *node)
return dynamic_cast<const DecklistModelSubZoneNode *>(node) != nullptr; return dynamic_cast<const DecklistModelSubZoneNode *>(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<DecklistCardNode *>(zone->at(k))) {
new DecklistModelCardNode(zoneCard, shadowZone);
} else if (auto *subZone = dynamic_cast<const InnerDecklistNode *>(zone->at(k))) {
flattenCards(subZone, shadowZone);
}
}
}
} // namespace
void mirrorCustomZones(const InnerDecklistNode *deckBoardZone, InnerDecklistNode *shadowBoardZone) void mirrorCustomZones(const InnerDecklistNode *deckBoardZone, InnerDecklistNode *shadowBoardZone)
{ {
for (int j = 0; j < deckBoardZone->size(); j++) { for (int j = 0; j < deckBoardZone->size(); j++) {
auto *customCard = dynamic_cast<DecklistCardNode *>(deckBoardZone->at(j));
if (customCard) {
continue;
}
auto *customZone = dynamic_cast<const InnerDecklistNode *>(deckBoardZone->at(j)); auto *customZone = dynamic_cast<const InnerDecklistNode *>(deckBoardZone->at(j));
if (!customZone) { if (!customZone) {
continue; continue;
} }
auto *shadowZone = new DecklistModelSubZoneNode(customZone->getName(), shadowBoardZone); auto *shadowZone = new DecklistModelSubZoneNode(customZone->getName(), shadowBoardZone);
for (int k = 0; k < customZone->size(); k++) { flattenCards(customZone, shadowZone);
if (auto *zoneCard = dynamic_cast<DecklistCardNode *>(customZone->at(k))) { }
new DecklistModelCardNode(zoneCard, 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<InnerDecklistNode *>(child);
if (group && group->getName() == name) {
return group;
} }
} }
return nullptr;
} }
DecklistModelSubZoneNode *findSubZoneByName(InnerDecklistNode *root, const QString &zoneName) DecklistModelSubZoneNode *findSubZoneByName(InnerDecklistNode *root, const QString &zoneName)

View file

@ -43,6 +43,21 @@ namespace DeckListModelCustomZones
*/ */
[[nodiscard]] bool isCustomZone(const AbstractDecklistNode *node); [[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. * @brief Mirrors the custom zones of a deck board zone into its shadow board node.
* *

View file

@ -1,4 +1,4 @@
add_executable(deck_list_model_custom_zones_test deck_list_model_custom_zones_test.cpp) add_executable(deck_list_model_custom_zones_test ${VERSION_STRING_CPP} deck_list_model_custom_zones_test.cpp)
if(NOT GTEST_FOUND) if(NOT GTEST_FOUND)
add_dependencies(deck_list_model_custom_zones_test gtest) add_dependencies(deck_list_model_custom_zones_test gtest)

View file

@ -129,6 +129,51 @@ TEST(DeckListModelCustomZones, MirrorCustomZonesWithNoCustomZonesIsNoop)
EXPECT_EQ(shadowBoard->size(), 0); 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<DecklistModelSubZoneNode *>(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<DecklistModelCardNode *>(shadowZone->at(0));
auto *shadowCard2 = dynamic_cast<DecklistModelCardNode *>(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 // sortWithCustomZonesLast
// ===================================================================================================================== // =====================================================================================================================
@ -223,3 +268,9 @@ TEST(DeckListModelCustomZones, SortPlainNodeDoesNotReorderCustomZones)
EXPECT_EQ(mapping[1].first, 0); EXPECT_EQ(mapping[1].first, 0);
EXPECT_EQ(mapping[1].second, 1); EXPECT_EQ(mapping[1].second, 1);
} }
int main(int argc, char **argv)
{
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}

View file

@ -1,4 +1,8 @@
#include <gtest/gtest.h> #include <gtest/gtest.h>
#include <libcockatrice/card/card_info.h>
#include <libcockatrice/card/database/card_database_manager.h>
#include <libcockatrice/card/game_specific_terms.h>
#include <libcockatrice/card/printing/exact_card.h>
#include <libcockatrice/deck_list/deck_list.h> #include <libcockatrice/deck_list/deck_list.h>
#include <libcockatrice/deck_list/deck_list_node_tree.h> #include <libcockatrice/deck_list/deck_list_node_tree.h>
#include <libcockatrice/deck_list/tree/deck_list_card_node.h> #include <libcockatrice/deck_list/tree/deck_list_card_node.h>
@ -25,6 +29,32 @@ int totalCustomZoneRows(const DeckListModel &model)
return count; 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 } // namespace
// The "Add to Zone" combobox/submenu lists getCustomZoneNames(), which reads the // The "Add to Zone" combobox/submenu lists getCustomZoneNames(), which reads the
@ -69,3 +99,148 @@ TEST(DeckListModelZoneIntegration, RebuildTreeMirrorsEachZoneOnce)
EXPECT_EQ(model.getCustomZoneNames(DECK_ZONE_MAIN), (QStringList{"Removal", "Utility"})); EXPECT_EQ(model.getCustomZoneNames(DECK_ZONE_MAIN), (QStringList{"Removal", "Utility"}));
EXPECT_EQ(totalCustomZoneRows(model), 2); EXPECT_EQ(totalCustomZoneRows(model), 2);
} }
// =====================================================================================================================
// Model behaviour: addCard routing, findCard lookup, removeRows guard, empty-zone survival.
// =====================================================================================================================
TEST(DeckListModelZoneIntegration, AddCardRoutesIntoMirroredCustomZone)
{
QSharedPointer<DeckList> 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<InnerDecklistNode *>(listRoot->at(i))) {
topLevelRemoval |= zone->getName() == "Removal";
}
}
EXPECT_FALSE(topLevelRemoval);
}
TEST(DeckListModelZoneIntegration, AddCardToUnmirroredCustomZoneRebuildsNotCreatesTopLevel)
{
QSharedPointer<DeckList> 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<DeckList> 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<DeckList> 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<DeckList> 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<DeckList> 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();
}