From 473b255959dbd2901d3890bdc371ef8727f876f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 07:17:02 +0200 Subject: [PATCH] Fix ZeldaZach review for PR 7492: preserve printing in DeckDiff::merge, atomic sidecar save with QSaveFile --- .../interface/deck_sync/deck_sync_sidecar.cpp | 7 +- .../libcockatrice/deck_list/deck_diff.cpp | 81 +++++++++++++++---- .../libcockatrice/deck_list/deck_diff.h | 9 ++- 3 files changed, 76 insertions(+), 21 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp index bfea3a7fa..9e2785f41 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp @@ -5,6 +5,7 @@ #include #include #include +#include namespace { @@ -63,12 +64,12 @@ bool DeckSyncSidecar::save() const root.insert("records", records); QDir().mkpath(QFileInfo(filePath).absolutePath()); - QFile file(filePath); - if (!file.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + QSaveFile file(filePath); + if (!file.open(QIODevice::WriteOnly)) { return false; } file.write(QJsonDocument(root).toJson(QJsonDocument::Compact)); - return true; + return file.commit(); } bool DeckSyncSidecar::contains(const QString &relativePath) const diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp index 31a6b622b..effadb390 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp @@ -13,12 +13,36 @@ namespace { using ZoneCardKey = QPair; +struct ZoneCardInfo +{ + int count = 0; + QString cardSetShortName; + QString cardSetNumber; + QString cardProviderId; +}; + +QMap zoneCardInfos(const DeckList &deck) +{ + QMap infos; + deck.forEachCard([&infos](InnerDecklistNode *zone, DecklistCardNode *card) { + ZoneCardInfo &info = infos[{zone->getName(), card->getName()}]; + if (info.count == 0) { + info.cardSetShortName = card->getCardSetShortName(); + info.cardSetNumber = card->getCardCollectorNumber(); + info.cardProviderId = card->getCardProviderId(); + } + info.count += card->getNumber(); + }); + return infos; +} + QMap zoneCardCounts(const DeckList &deck) { QMap counts; - deck.forEachCard([&counts](InnerDecklistNode *zone, DecklistCardNode *card) { - counts[{zone->getName(), card->getName()}] += card->getNumber(); - }); + const QMap infos = zoneCardInfos(deck); + for (auto it = infos.begin(); it != infos.end(); ++it) { + counts.insert(it.key(), it.value().count); + } return counts; } @@ -34,17 +58,24 @@ bool changeLessThan(const DeckCardChange &a, const DeckCardChange &b) DeckDiffResult DeckDiff::diff(const DeckList &oldDeck, const DeckList &newDeck) { const QMap oldCounts = zoneCardCounts(oldDeck); - const QMap newCounts = zoneCardCounts(newDeck); + const QMap newInfos = zoneCardInfos(newDeck); QSet keys(oldCounts.keyBegin(), oldCounts.keyEnd()); - keys.unite(QSet(newCounts.keyBegin(), newCounts.keyEnd())); + keys.unite(QSet(newInfos.keyBegin(), newInfos.keyEnd())); DeckDiffResult result; for (const ZoneCardKey &key : keys) { const int oldCount = oldCounts.value(key, 0); - const int newCount = newCounts.value(key, 0); - if (oldCount != newCount) { - result.changes.append({key.first, key.second, oldCount, newCount}); + const ZoneCardInfo newInfo = newInfos.value(key, ZoneCardInfo()); + if (oldCount != newInfo.count) { + const DeckCardChange change{.zone = key.first, + .cardName = key.second, + .oldCount = oldCount, + .newCount = newInfo.count, + .cardSetShortName = newInfo.cardSetShortName, + .cardSetNumber = newInfo.cardSetNumber, + .cardProviderId = newInfo.cardProviderId}; + result.changes.append(change); } } std::sort(result.changes.begin(), result.changes.end(), changeLessThan); @@ -69,21 +100,41 @@ DeckList DeckDiff::merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) { QMap counts = zoneCardCounts(baseDeck); + QMap printings; for (const DeckCardChange &change : localChanges.changes) { counts[{change.zone, change.cardName}] = change.newCount; + printings[{change.zone, change.cardName}] = change; } for (const DeckCardChange &change : remoteChanges.changes) { counts[{change.zone, change.cardName}] = change.newCount; + printings[{change.zone, change.cardName}] = change; } - DeckList merged(baseDeck.getMetadata(), DecklistNodeTree(), baseDeck.getSideboardPlans()); - QList keys = counts.keys(); - std::sort(keys.begin(), keys.end()); - for (const ZoneCardKey &key : keys) { - const int count = qMax(0, counts.value(key)); - for (int i = 0; i < count; ++i) { - merged.addCard(key.second, key.first); + DeckList merged = baseDeck; + QMap> nodesByKey; + merged.forEachCard([&nodesByKey](InnerDecklistNode *zone, DecklistCardNode *card) { + nodesByKey[{zone->getName(), card->getName()}].append(card); + }); + + for (auto it = counts.begin(); it != counts.end(); ++it) { + const ZoneCardKey &key = it.key(); + const int target = qMax(0, it.value()); + QList nodes = nodesByKey.value(key); + if (target == 0) { + for (DecklistCardNode *node : nodes) { + merged.getTree()->deleteNode(node); + } + } else if (nodes.isEmpty()) { + const DeckCardChange printing = printings.value(key); + merged.getTree()->addCard(printing.cardName, target, printing.zone, -1, printing.cardSetShortName, + printing.cardSetNumber, printing.cardProviderId, true); + } else { + nodes.first()->setNumber(target); + for (int i = 1; i < nodes.size(); ++i) { + merged.getTree()->deleteNode(nodes.at(i)); + } } } + merged.refreshDeckHash(); return merged; } diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h index 2b4fb8732..372da5599 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h @@ -18,6 +18,9 @@ struct DeckCardChange QString cardName; ///< Card name. int oldCount = 0; ///< Copies in the old deck. int newCount = 0; ///< Copies in the new deck. + QString cardSetShortName; ///< Printing set carried from the new side, empty when unchanged. + QString cardSetNumber; ///< Printing collector number carried from the new side. + QString cardProviderId; ///< Printing provider id carried from the new side. /** @brief Copies added (positive) or removed (negative). */ [[nodiscard]] int delta() const { @@ -40,9 +43,9 @@ struct DeckDiffResult * @class DeckDiff * @brief Pure card-count diff and three-way merge for deck sync. * - * Compares card names and copy counts per zone only; printing information - * (set, collector number, provider) is not tracked, so a merged deck keeps - * base printings with merged counts. + * Compares card names and copy counts per zone only; a merged deck keeps + * base printings for existing cards and carries the changed side's printing + * for newly added cards. */ class DeckDiff {