From 1e97ab086bfee9fbadccf718fb7113c7e5deaa13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 11 Oct 2026 14:26:12 +0200 Subject: [PATCH 01/16] [Client] DeckSyncProfile + sidecar + DeckDiff with tests --- cockatrice/CMakeLists.txt | 2 + .../interface/deck_sync/deck_sync_profile.cpp | 31 ++++ .../interface/deck_sync/deck_sync_profile.h | 42 ++++++ .../interface/deck_sync/deck_sync_sidecar.cpp | 97 +++++++++++++ .../interface/deck_sync/deck_sync_sidecar.h | 52 +++++++ libcockatrice_deck_list/CMakeLists.txt | 2 + .../libcockatrice/deck_list/deck_diff.cpp | 89 ++++++++++++ .../libcockatrice/deck_list/deck_diff.h | 71 +++++++++ tests/CMakeLists.txt | 28 ++++ tests/deck_diff_test.cpp | 137 ++++++++++++++++++ tests/deck_sync_profile_test.cpp | 63 ++++++++ tests/deck_sync_sidecar_test.cpp | 126 ++++++++++++++++ 12 files changed, 740 insertions(+) create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_profile.cpp create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_profile.h create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_sidecar.h create mode 100644 libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp create mode 100644 libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h create mode 100644 tests/deck_diff_test.cpp create mode 100644 tests/deck_sync_profile_test.cpp create mode 100644 tests/deck_sync_sidecar_test.cpp diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index ce7ad5712..d76d89211 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -26,6 +26,8 @@ set(cockatrice_SOURCES src/client/settings/shortcuts_settings.cpp src/interface/deck_loader/card_node_function.cpp src/interface/deck_loader/deck_loader.cpp + src/interface/deck_sync/deck_sync_profile.cpp + src/interface/deck_sync/deck_sync_sidecar.cpp src/interface/widgets/dialogs/dlg_connect.cpp src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp src/interface/widgets/dialogs/dlg_create_game.cpp diff --git a/cockatrice/src/interface/deck_sync/deck_sync_profile.cpp b/cockatrice/src/interface/deck_sync/deck_sync_profile.cpp new file mode 100644 index 000000000..a886128a7 --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_profile.cpp @@ -0,0 +1,31 @@ +#include "deck_sync_profile.h" + +#include + +DeckSyncProfile::DeckSyncProfile(const QString &_host, int _port, const QString &_username) + : host(_host), port(_port), username(_username) +{ +} + +bool DeckSyncProfile::isValid() const +{ + return !host.isEmpty() && port > 0 && !username.isEmpty(); +} + +QString DeckSyncProfile::key() const +{ + if (!isValid()) { + return {}; + } + // NUL separators keep the triple injective: neither IPv6 hosts nor + // usernames containing ':' can collide two profiles into one file. + // Lowercasing is safe on both parts: DNS names are case-insensitive, and + // Servatrice usernames are unique under a case-insensitive collation + // (cockatrice_users uses utf8mb4_unicode_ci), so both spellings are one account. + QByteArray blob = host.trimmed().toLower().toUtf8(); + blob.append('\0'); + blob.append(QByteArray::number(port)); + blob.append('\0'); + blob.append(username.trimmed().toLower().toUtf8()); + return QCryptographicHash::hash(blob, QCryptographicHash::Sha1).toHex(); +} diff --git a/cockatrice/src/interface/deck_sync/deck_sync_profile.h b/cockatrice/src/interface/deck_sync/deck_sync_profile.h new file mode 100644 index 000000000..ac63ab913 --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_profile.h @@ -0,0 +1,42 @@ +/** + * @file deck_sync_profile.h + * @brief Identity of one sync target (server + user) for deck collection sync. + */ + +#ifndef DECK_SYNC_PROFILE_H +#define DECK_SYNC_PROFILE_H + +#include + +/** + * @struct DeckSyncProfile + * @brief Identifies which server and login a sync sidecar belongs to. + * + * A local deck folder can sync against several servers. Every server login + * gets its own profile, and every profile gets its own sidecar file, so + * switching servers never mixes up remote deck ids. + */ +struct DeckSyncProfile +{ + QString host; ///< Server hostname as entered by the user. + int port = 0; ///< Server port. + QString username; ///< Login name on that server. + + DeckSyncProfile() = default; + DeckSyncProfile(const QString &_host, int _port, const QString &_username); + + /** @brief False when any of host, port or username is missing. */ + [[nodiscard]] bool isValid() const; + /** + * @brief Stable file-system-safe key for this profile. + * + * SHA1 hex over the NUL-separated lowercased "host", port and "username" + * triple, so the same server reached with different hostname + * capitalisation maps to one file, while IPv6 hosts and usernames + * containing ':' can never collide two profiles. Empty for invalid + * profiles, so no two half-configured profiles share a file. + */ + [[nodiscard]] QString key() const; +}; + +#endif // DECK_SYNC_PROFILE_H diff --git a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp new file mode 100644 index 000000000..bfea3a7fa --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp @@ -0,0 +1,97 @@ +#include "deck_sync_sidecar.h" + +#include +#include +#include +#include +#include + +namespace +{ +constexpr int SIDECAR_FORMAT_VERSION = 1; +} // namespace + +DeckSyncSidecar::DeckSyncSidecar(const QString &_filePath) : filePath(_filePath) +{ +} + +bool DeckSyncSidecar::load() +{ + recordMap.clear(); + + QFile file(filePath); + if (!file.exists()) { + return true; + } + if (!file.open(QIODevice::ReadOnly)) { + return false; + } + const QJsonDocument document = QJsonDocument::fromJson(file.readAll()); + if (!document.isObject() || !document.object().value("records").isObject()) { + return false; + } + const QJsonObject records = document.object().value("records").toObject(); + for (auto it = records.begin(); it != records.end(); ++it) { + if (!it.value().isObject()) { + continue; + } + const QJsonObject entry = it.value().toObject(); + const DeckSyncRecord record{.guid = entry.value("guid").toString(), + .remoteId = entry.value("remoteId").toInt(-1), + .baseHash = entry.value("baseHash").toString(), + .lastSyncSecs = static_cast(entry.value("lastSync").toDouble()), + .deleted = entry.value("deleted").toBool(false)}; + recordMap.insert(it.key(), record); + } + return true; +} + +bool DeckSyncSidecar::save() const +{ + QJsonObject records; + for (auto it = recordMap.begin(); it != recordMap.end(); ++it) { + QJsonObject entry; + entry.insert("guid", it.value().guid); + entry.insert("remoteId", it.value().remoteId); + entry.insert("baseHash", it.value().baseHash); + entry.insert("lastSync", static_cast(it.value().lastSyncSecs)); + entry.insert("deleted", it.value().deleted); + records.insert(it.key(), entry); + } + QJsonObject root; + root.insert("version", SIDECAR_FORMAT_VERSION); + root.insert("records", records); + + QDir().mkpath(QFileInfo(filePath).absolutePath()); + QFile file(filePath); + if (!file.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + return false; + } + file.write(QJsonDocument(root).toJson(QJsonDocument::Compact)); + return true; +} + +bool DeckSyncSidecar::contains(const QString &relativePath) const +{ + return recordMap.contains(relativePath); +} + +DeckSyncRecord DeckSyncSidecar::get(const QString &relativePath) const +{ + return recordMap.value(relativePath, DeckSyncRecord()); +} + +void DeckSyncSidecar::set(const QString &relativePath, const DeckSyncRecord &record) +{ + recordMap.insert(relativePath, record); +} + +void DeckSyncSidecar::remove(const QString &relativePath) +{ + recordMap.remove(relativePath); +} + +QMap DeckSyncSidecar::records() const +{ + return recordMap; +} diff --git a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h new file mode 100644 index 000000000..7eb2db7ae --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h @@ -0,0 +1,52 @@ +/** + * @file deck_sync_sidecar.h + * @brief Per-profile local bookkeeping for deck collection sync. + */ + +#ifndef DECK_SYNC_SIDECAR_H +#define DECK_SYNC_SIDECAR_H + +#include +#include + +/** @brief Sync bookkeeping for one local deck file. */ +struct DeckSyncRecord +{ + QString guid; ///< Stable deck identity shared with the server. + int remoteId = -1; ///< Server row id, -1 when never uploaded. + QString baseHash; ///< Deck hash at the last successful sync. + qint64 lastSyncSecs = 0; ///< Last sync time, seconds since Unix epoch. + bool deleted = false; ///< Local tombstone waiting to be pushed. +}; + +/** + * @class DeckSyncSidecar + * @brief Loads and stores sync records as JSON next to the deck folder. + * + * One sidecar file per DeckSyncProfile (named by profile key), mapping paths + * relative to the deck folder to their last-synced state. Comparing the live + * file hash against baseHash tells clean apart from locally modified. + */ +class DeckSyncSidecar +{ +public: + QString filePath; ///< Full path of the sidecar JSON file. + + explicit DeckSyncSidecar(const QString &_filePath); + + /** @brief Loads records; a missing file loads as empty and returns true. */ + bool load(); + /** @brief Stores records, creating parent folders as needed. */ + bool save() const; + [[nodiscard]] bool contains(const QString &relativePath) const; + /** @brief Returns the record, or a default record when unknown. */ + [[nodiscard]] DeckSyncRecord get(const QString &relativePath) const; + void set(const QString &relativePath, const DeckSyncRecord &record); + void remove(const QString &relativePath); + [[nodiscard]] QMap records() const; + +private: + QMap recordMap; +}; + +#endif // DECK_SYNC_SIDECAR_H diff --git a/libcockatrice_deck_list/CMakeLists.txt b/libcockatrice_deck_list/CMakeLists.txt index ffd4c28a1..4a59d615a 100644 --- a/libcockatrice_deck_list/CMakeLists.txt +++ b/libcockatrice_deck_list/CMakeLists.txt @@ -7,6 +7,7 @@ set(HEADERS libcockatrice/deck_list/tree/abstract_deck_list_node.h libcockatrice/deck_list/tree/deck_list_card_node.h libcockatrice/deck_list/tree/inner_deck_list_node.h + libcockatrice/deck_list/deck_diff.h libcockatrice/deck_list/deck_file_format.h libcockatrice/deck_list/deck_list.h libcockatrice/deck_list/deck_list_history_manager.h @@ -25,6 +26,7 @@ add_library( libcockatrice/deck_list/tree/abstract_deck_list_node.cpp libcockatrice/deck_list/tree/deck_list_card_node.cpp libcockatrice/deck_list/tree/inner_deck_list_node.cpp + libcockatrice/deck_list/deck_diff.cpp libcockatrice/deck_list/deck_file_format.cpp libcockatrice/deck_list/deck_list.cpp libcockatrice/deck_list/deck_list_history_manager.cpp diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp new file mode 100644 index 000000000..31a6b622b --- /dev/null +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp @@ -0,0 +1,89 @@ +#include "deck_diff.h" + +#include "deck_list.h" +#include "tree/deck_list_card_node.h" +#include "tree/inner_deck_list_node.h" + +#include +#include +#include +#include + +namespace +{ +using ZoneCardKey = QPair; + +QMap zoneCardCounts(const DeckList &deck) +{ + QMap counts; + deck.forEachCard([&counts](InnerDecklistNode *zone, DecklistCardNode *card) { + counts[{zone->getName(), card->getName()}] += card->getNumber(); + }); + return counts; +} + +bool changeLessThan(const DeckCardChange &a, const DeckCardChange &b) +{ + if (a.zone != b.zone) { + return a.zone < b.zone; + } + return a.cardName < b.cardName; +} +} // namespace + +DeckDiffResult DeckDiff::diff(const DeckList &oldDeck, const DeckList &newDeck) +{ + const QMap oldCounts = zoneCardCounts(oldDeck); + const QMap newCounts = zoneCardCounts(newDeck); + + QSet keys(oldCounts.keyBegin(), oldCounts.keyEnd()); + keys.unite(QSet(newCounts.keyBegin(), newCounts.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}); + } + } + std::sort(result.changes.begin(), result.changes.end(), changeLessThan); + return result; +} + +bool DeckDiff::canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) +{ + QSet localKeys; + for (const DeckCardChange &change : localChanges.changes) { + localKeys.insert({change.zone, change.cardName}); + } + for (const DeckCardChange &change : remoteChanges.changes) { + if (localKeys.contains({change.zone, change.cardName})) { + return false; + } + } + return true; +} + +DeckList +DeckDiff::merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) +{ + QMap counts = zoneCardCounts(baseDeck); + for (const DeckCardChange &change : localChanges.changes) { + counts[{change.zone, change.cardName}] = change.newCount; + } + for (const DeckCardChange &change : remoteChanges.changes) { + counts[{change.zone, change.cardName}] = change.newCount; + } + + 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); + } + } + return merged; +} diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h new file mode 100644 index 000000000..2b4fb8732 --- /dev/null +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h @@ -0,0 +1,71 @@ +/** + * @file deck_diff.h + * @brief Card-level diff and three-way merge for deck collection sync. + */ + +#ifndef DECK_DIFF_H +#define DECK_DIFF_H + +#include +#include + +class DeckList; + +/** @brief A single card-count change within one zone. */ +struct DeckCardChange +{ + QString zone; ///< Zone name, e.g. "main". + QString cardName; ///< Card name. + int oldCount = 0; ///< Copies in the old deck. + int newCount = 0; ///< Copies in the new deck. + /** @brief Copies added (positive) or removed (negative). */ + [[nodiscard]] int delta() const + { + return newCount - oldCount; + } +}; + +/** @brief Card-level difference between two decks, sorted by zone then card name. */ +struct DeckDiffResult +{ + QList changes; + /** @brief True when both decks hold the same cards in the same zones. */ + [[nodiscard]] bool isEmpty() const + { + return changes.isEmpty(); + } +}; + +/** + * @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. + */ +class DeckDiff +{ +public: + /** @brief Diffs oldDeck against newDeck. */ + static DeckDiffResult diff(const DeckList &oldDeck, const DeckList &newDeck); + /** + * @brief True when both change sets touch disjoint (zone, card) keys and a merge needs no arbitration. + */ + static bool canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); + /** + * @brief Applies both change sets onto a copy of baseDeck. + * + * @precondition localChanges and remoteChanges were both computed with + * diff() against baseDeck. Stale or foreign change sets silently + * resurrect or delete cards. + * + * Counts are clamped at zero. On overlapping keys the remote change wins; + * callers offer this result as "Use Merge" only when canAutoMerge is true. + * Metadata and sideboard plans come from baseDeck. + */ + static DeckList + merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); +}; + +#endif // DECK_DIFF_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c8d3fbd59..32ee9b7f1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -407,6 +407,34 @@ cockatrice_add_test( bench ) +cockatrice_add_test( + NAME + deck_diff_test + SOURCES + deck_diff_test.cpp + LIBS + libcockatrice_deck_list + libcockatrice_utility +) +cockatrice_add_test( + NAME + deck_sync_profile_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/deck_sync/deck_sync_profile.cpp + deck_sync_profile_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/deck_sync +) +cockatrice_add_test( + NAME + deck_sync_sidecar_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/deck_sync/deck_sync_sidecar.cpp + deck_sync_sidecar_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src/interface/deck_sync +) + # The translation loader test compiles a .qm fixture, which needs lrelease from Qt Linguist. if(TARGET Qt6::lrelease) set(TRANSLATION_LOADER_TEST_QM "${CMAKE_CURRENT_BINARY_DIR}/translation_loader_test_de.qm") diff --git a/tests/deck_diff_test.cpp b/tests/deck_diff_test.cpp new file mode 100644 index 000000000..4ff0b556b --- /dev/null +++ b/tests/deck_diff_test.cpp @@ -0,0 +1,137 @@ +#include "gtest/gtest.h" +#include +#include +#include +#include +#include +#include + +namespace +{ +DeckList makeDeck(std::initializer_list> mainCards, + std::initializer_list> sideCards = {}) +{ + DeckList deck; + for (const auto &[name, count] : mainCards) { + for (int i = 0; i < count; ++i) { + deck.addCard(name, "main"); + } + } + for (const auto &[name, count] : sideCards) { + for (int i = 0; i < count; ++i) { + deck.addCard(name, "side"); + } + } + return deck; +} + +int countOf(const DeckList &deck, const char *zone, const char *name) +{ + int count = 0; + deck.forEachCard([&](InnerDecklistNode *zoneNode, DecklistCardNode *card) { + if (zoneNode->getName() == zone && card->getName() == name) { + count += card->getNumber(); + } + }); + return count; +} +} // namespace + +TEST(DeckDiffTest, IdenticalDecksHaveEmptyDiff) +{ + DeckList first = makeDeck({{"Lightning Bolt", 4}}); + DeckList second = makeDeck({{"Lightning Bolt", 4}}); + EXPECT_TRUE(DeckDiff::diff(first, second).isEmpty()); +} + +TEST(DeckDiffTest, ChangesAreSortedByZoneThenName) +{ + DeckList oldDeck = makeDeck({{"Mountain", 20}, {"Lightning Bolt", 4}}, {{"Shock", 2}}); + DeckList newDeck = makeDeck({{"Mountain", 19}, {"Shock", 5}}, {{"Shock", 1}, {"Teferi", 1}}); + + DeckDiffResult result = DeckDiff::diff(oldDeck, newDeck); + ASSERT_EQ(result.changes.size(), 5); + EXPECT_EQ(result.changes.at(0).zone, "main"); + EXPECT_EQ(result.changes.at(0).cardName, "Lightning Bolt"); + EXPECT_EQ(result.changes.at(1).cardName, "Mountain"); + EXPECT_EQ(result.changes.at(2).cardName, "Shock"); + EXPECT_EQ(result.changes.at(3).zone, "side"); + EXPECT_EQ(result.changes.at(3).cardName, "Shock"); + EXPECT_EQ(result.changes.at(4).cardName, "Teferi"); +} + +TEST(DeckDiffTest, ReportsAddedAndRemovedCopies) +{ + DeckList oldDeck = makeDeck({{"Lightning Bolt", 4}, {"Mountain", 20}}); + DeckList newDeck = makeDeck({{"Lightning Bolt", 2}, {"Shock", 4}, {"Mountain", 20}}); + + DeckDiffResult result = DeckDiff::diff(oldDeck, newDeck); + ASSERT_EQ(result.changes.size(), 2); + EXPECT_EQ(result.changes.at(0).cardName, "Lightning Bolt"); + EXPECT_EQ(result.changes.at(0).oldCount, 4); + EXPECT_EQ(result.changes.at(0).newCount, 2); + EXPECT_EQ(result.changes.at(0).delta(), -2); + EXPECT_EQ(result.changes.at(1).cardName, "Shock"); + EXPECT_EQ(result.changes.at(1).oldCount, 0); + EXPECT_EQ(result.changes.at(1).newCount, 4); +} + +TEST(DeckDiffTest, ZoneMoveIsTwoChanges) +{ + DeckList oldDeck = makeDeck({{"Teferi", 1}}); + DeckList newDeck = makeDeck({}, {{"Teferi", 1}}); + + DeckDiffResult result = DeckDiff::diff(oldDeck, newDeck); + ASSERT_EQ(result.changes.size(), 2); + EXPECT_EQ(result.changes.at(0).zone, "main"); + EXPECT_EQ(result.changes.at(0).newCount, 0); + EXPECT_EQ(result.changes.at(1).zone, "side"); + EXPECT_EQ(result.changes.at(1).oldCount, 0); +} + +TEST(DeckDiffTest, DisjointChangesCanAutoMerge) +{ + DeckList base = makeDeck({{"Lightning Bolt", 4}, {"Mountain", 20}}); + DeckList local = makeDeck({{"Lightning Bolt", 4}, {"Mountain", 18}}); + DeckList remote = makeDeck({{"Shock", 2}, {"Lightning Bolt", 4}, {"Mountain", 20}}); + + DeckDiffResult localChanges = DeckDiff::diff(base, local); + DeckDiffResult remoteChanges = DeckDiff::diff(base, remote); + EXPECT_TRUE(DeckDiff::canAutoMerge(localChanges, remoteChanges)); + + DeckList merged = DeckDiff::merge(base, localChanges, remoteChanges); + EXPECT_EQ(countOf(merged, "main", "Mountain"), 18); + EXPECT_EQ(countOf(merged, "main", "Shock"), 2); + EXPECT_EQ(countOf(merged, "main", "Lightning Bolt"), 4); +} + +TEST(DeckDiffTest, OverlappingChangesCannotAutoMergeAndRemoteWins) +{ + DeckList base = makeDeck({{"Lightning Bolt", 4}}); + DeckList local = makeDeck({{"Lightning Bolt", 3}}); + DeckList remote = makeDeck({{"Lightning Bolt", 1}}); + + DeckDiffResult localChanges = DeckDiff::diff(base, local); + DeckDiffResult remoteChanges = DeckDiff::diff(base, remote); + EXPECT_FALSE(DeckDiff::canAutoMerge(localChanges, remoteChanges)); + + DeckList merged = DeckDiff::merge(base, localChanges, remoteChanges); + EXPECT_EQ(countOf(merged, "main", "Lightning Bolt"), 1); +} + +TEST(DeckDiffTest, MergeKeepsBaseMetadata) +{ + DeckList base = makeDeck({{"Lightning Bolt", 4}}); + base.setName("Affinity v3"); + DeckList remote = makeDeck({{"Lightning Bolt", 4}, {"Shock", 1}}); + + DeckList merged = DeckDiff::merge(base, DeckDiff::diff(base, base), DeckDiff::diff(base, remote)); + EXPECT_EQ(merged.getName(), "Affinity v3"); + EXPECT_EQ(countOf(merged, "main", "Shock"), 1); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/deck_sync_profile_test.cpp b/tests/deck_sync_profile_test.cpp new file mode 100644 index 000000000..fc6917559 --- /dev/null +++ b/tests/deck_sync_profile_test.cpp @@ -0,0 +1,63 @@ +#include "gtest/gtest.h" +#include +#include + +TEST(DeckSyncProfileTest, KeyIsStable) +{ + DeckSyncProfile first("woogerworks.com", 4747, "Skog7"); + DeckSyncProfile second("woogerworks.com", 4747, "Skog7"); + EXPECT_EQ(first.key(), second.key()); + EXPECT_FALSE(first.key().isEmpty()); +} + +TEST(DeckSyncProfileTest, KeyIgnoresCase) +{ + DeckSyncProfile lower("woogerworks.com", 4747, "skog7"); + DeckSyncProfile upper("WOOGERWORKS.COM", 4747, "SKOG7"); + EXPECT_EQ(lower.key(), upper.key()); +} + +TEST(DeckSyncProfileTest, KeyDiffersPerServerAndUser) +{ + DeckSyncProfile base("woogerworks.com", 4747, "skog7"); + EXPECT_NE(base.key(), DeckSyncProfile("other.server", 4747, "skog7").key()); + EXPECT_NE(base.key(), DeckSyncProfile("woogerworks.com", 4748, "skog7").key()); + EXPECT_NE(base.key(), DeckSyncProfile("woogerworks.com", 4747, "other").key()); +} + +TEST(DeckSyncProfileTest, KeyIsUnambiguousAcrossSeparators) +{ + DeckSyncProfile left("a:1", 2, "b"); + DeckSyncProfile right("a", 1, "2:b"); + EXPECT_NE(left.key(), right.key()); + EXPECT_NE(DeckSyncProfile("::1", 4747, "skog7").key(), DeckSyncProfile("::2", 4747, "skog7").key()); +} + +TEST(DeckSyncProfileTest, InvalidProfilesHaveNoKey) +{ + EXPECT_TRUE(DeckSyncProfile().key().isEmpty()); + EXPECT_TRUE(DeckSyncProfile("", 4747, "skog7").key().isEmpty()); + EXPECT_TRUE(DeckSyncProfile("woogerworks.com", 0, "skog7").key().isEmpty()); +} + +TEST(DeckSyncProfileTest, KeyIgnoresSurroundingWhitespace) +{ + DeckSyncProfile plain("woogerworks.com", 4747, "skog7"); + DeckSyncProfile padded(" woogerworks.com ", 4747, " skog7 "); + EXPECT_EQ(plain.key(), padded.key()); +} + +TEST(DeckSyncProfileTest, ValidityRequiresAllFields) +{ + EXPECT_TRUE(DeckSyncProfile("woogerworks.com", 4747, "skog7").isValid()); + EXPECT_FALSE(DeckSyncProfile("", 4747, "skog7").isValid()); + EXPECT_FALSE(DeckSyncProfile("woogerworks.com", 0, "skog7").isValid()); + EXPECT_FALSE(DeckSyncProfile("woogerworks.com", 4747, "").isValid()); + EXPECT_FALSE(DeckSyncProfile().isValid()); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/deck_sync_sidecar_test.cpp b/tests/deck_sync_sidecar_test.cpp new file mode 100644 index 000000000..4bd04be33 --- /dev/null +++ b/tests/deck_sync_sidecar_test.cpp @@ -0,0 +1,126 @@ +#include "gtest/gtest.h" +#include +#include +#include +#include +#include +#include + +namespace +{ +QString sidecarPath(QTemporaryDir &dir) +{ + return dir.filePath(".sync/profilekey.json"); +} +} // namespace + +TEST(DeckSyncSidecarTest, MissingFileLoadsEmpty) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + DeckSyncSidecar sidecar(sidecarPath(dir)); + EXPECT_TRUE(sidecar.load()); + EXPECT_TRUE(sidecar.records().isEmpty()); + EXPECT_FALSE(sidecar.contains("affinity.cod")); +} + +TEST(DeckSyncSidecarTest, RoundTripsRecords) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + DeckSyncSidecar sidecar(sidecarPath(dir)); + + DeckSyncRecord record; + record.guid = "123e4567-e89b-12d3-a456-426614174000"; + record.remoteId = 42; + record.baseHash = "5cac19qm"; + record.lastSyncSecs = 1791739200; + record.deleted = true; + sidecar.set("decks/affinity.cod", record); + EXPECT_TRUE(sidecar.save()); + + DeckSyncSidecar reloaded(sidecarPath(dir)); + EXPECT_TRUE(reloaded.load()); + EXPECT_TRUE(reloaded.contains("decks/affinity.cod")); + DeckSyncRecord back = reloaded.get("decks/affinity.cod"); + EXPECT_EQ(back.guid, record.guid); + EXPECT_EQ(back.remoteId, 42); + EXPECT_EQ(back.baseHash, "5cac19qm"); + EXPECT_EQ(back.lastSyncSecs, 1791739200); + EXPECT_TRUE(back.deleted); +} + +TEST(DeckSyncSidecarTest, UnknownPathReturnsDefaultRecord) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + DeckSyncSidecar sidecar(sidecarPath(dir)); + DeckSyncRecord back = sidecar.get("missing.cod"); + EXPECT_EQ(back.remoteId, -1); + EXPECT_TRUE(back.guid.isEmpty()); +} + +namespace +{ +bool writeRaw(const QString &path, const QByteArray &bytes) +{ + QDir().mkpath(QFileInfo(path).absolutePath()); + QFile file(path); + return file.open(QIODevice::WriteOnly | QIODevice::Truncate) && file.write(bytes) == bytes.size(); +} +} // namespace + +TEST(DeckSyncSidecarTest, GarbageFileFailsToLoad) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_TRUE(writeRaw(sidecarPath(dir), "not json at all")); + DeckSyncSidecar sidecar(sidecarPath(dir)); + EXPECT_FALSE(sidecar.load()); +} + +TEST(DeckSyncSidecarTest, WrongRecordsShapeFailsToLoad) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_TRUE(writeRaw(sidecarPath(dir), R"({"records":["a.cod"]})")); + DeckSyncSidecar missing(sidecarPath(dir)); + EXPECT_FALSE(missing.load()); + + ASSERT_TRUE(writeRaw(sidecarPath(dir), R"({"foo":1})")); + DeckSyncSidecar other(sidecarPath(dir)); + EXPECT_FALSE(other.load()); +} + +TEST(DeckSyncSidecarTest, NonObjectEntriesAreSkipped) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_TRUE(writeRaw(sidecarPath(dir), R"({"records":{"a.cod":42,"b.cod":{"guid":"g"}}})")); + DeckSyncSidecar sidecar(sidecarPath(dir)); + EXPECT_TRUE(sidecar.load()); + EXPECT_FALSE(sidecar.contains("a.cod")); + EXPECT_TRUE(sidecar.contains("b.cod")); + EXPECT_EQ(sidecar.get("b.cod").guid, "g"); +} + +TEST(DeckSyncSidecarTest, RemoveDropsRecord) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + DeckSyncSidecar sidecar(sidecarPath(dir)); + sidecar.set("a.cod", DeckSyncRecord()); + sidecar.remove("a.cod"); + EXPECT_FALSE(sidecar.contains("a.cod")); + EXPECT_TRUE(sidecar.save()); + + DeckSyncSidecar reloaded(sidecarPath(dir)); + EXPECT_TRUE(reloaded.load()); + EXPECT_FALSE(reloaded.contains("a.cod")); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} 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 02/16] 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 { From 02ccad237437ffde40f377348899e1e47c47091b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 07:22:47 +0200 Subject: [PATCH 03/16] Fixup: clang-format alignment in deck_diff.h --- .../libcockatrice/deck_list/deck_diff.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h index 372da5599..563b54b70 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h @@ -14,10 +14,10 @@ class DeckList; /** @brief A single card-count change within one zone. */ struct DeckCardChange { - QString zone; ///< Zone name, e.g. "main". - QString cardName; ///< Card name. - int oldCount = 0; ///< Copies in the old deck. - int newCount = 0; ///< Copies in the new deck. + QString zone; ///< Zone name, e.g. "main". + 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. From 507a80f0b28cd43abf82143fba915cfe8ba11abb Mon Sep 17 00:00:00 2001 From: BruebachL Date: Tue, 13 Oct 2026 11:46:49 +0200 Subject: [PATCH 04/16] [DeckDiff] Use namespace instead of static-only class per review --- .../libcockatrice/deck_list/deck_diff.cpp | 10 +++-- .../libcockatrice/deck_list/deck_diff.h | 44 +++++++++---------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp index effadb390..5024010b2 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.cpp @@ -55,7 +55,9 @@ bool changeLessThan(const DeckCardChange &a, const DeckCardChange &b) } } // namespace -DeckDiffResult DeckDiff::diff(const DeckList &oldDeck, const DeckList &newDeck) +namespace DeckDiff +{ +DeckDiffResult diff(const DeckList &oldDeck, const DeckList &newDeck) { const QMap oldCounts = zoneCardCounts(oldDeck); const QMap newInfos = zoneCardInfos(newDeck); @@ -82,7 +84,7 @@ DeckDiffResult DeckDiff::diff(const DeckList &oldDeck, const DeckList &newDeck) return result; } -bool DeckDiff::canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) +bool canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) { QSet localKeys; for (const DeckCardChange &change : localChanges.changes) { @@ -96,8 +98,7 @@ bool DeckDiff::canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffRe return true; } -DeckList -DeckDiff::merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) +DeckList merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges) { QMap counts = zoneCardCounts(baseDeck); QMap printings; @@ -138,3 +139,4 @@ DeckDiff::merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, co merged.refreshDeckHash(); return merged; } +} // namespace DeckDiff diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h index 563b54b70..4a6562292 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_diff.h @@ -40,35 +40,33 @@ struct DeckDiffResult }; /** - * @class DeckDiff + * @namespace DeckDiff * @brief Pure card-count diff and three-way merge for deck sync. * * 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 +namespace DeckDiff { -public: - /** @brief Diffs oldDeck against newDeck. */ - static DeckDiffResult diff(const DeckList &oldDeck, const DeckList &newDeck); - /** - * @brief True when both change sets touch disjoint (zone, card) keys and a merge needs no arbitration. - */ - static bool canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); - /** - * @brief Applies both change sets onto a copy of baseDeck. - * - * @precondition localChanges and remoteChanges were both computed with - * diff() against baseDeck. Stale or foreign change sets silently - * resurrect or delete cards. - * - * Counts are clamped at zero. On overlapping keys the remote change wins; - * callers offer this result as "Use Merge" only when canAutoMerge is true. - * Metadata and sideboard plans come from baseDeck. - */ - static DeckList - merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); -}; +/** @brief Diffs oldDeck against newDeck. */ +DeckDiffResult diff(const DeckList &oldDeck, const DeckList &newDeck); +/** + * @brief True when both change sets touch disjoint (zone, card) keys and a merge needs no arbitration. + */ +bool canAutoMerge(const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); +/** + * @brief Applies both change sets onto a copy of baseDeck. + * + * @precondition localChanges and remoteChanges were both computed with + * diff() against baseDeck. Stale or foreign change sets silently + * resurrect or delete cards. + * + * Counts are clamped at zero. On overlapping keys the remote change wins; + * callers offer this result as "Use Merge" only when canAutoMerge is true. + * Metadata and sideboard plans come from baseDeck. + */ +DeckList merge(const DeckList &baseDeck, const DeckDiffResult &localChanges, const DeckDiffResult &remoteChanges); +} // namespace DeckDiff #endif // DECK_DIFF_H From 0c6d64c565efcfb209e9dd7fd51def8e561c98d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 1 Nov 2026 05:22:59 +0100 Subject: [PATCH 05/16] Cover merge printing preservation with a test Base deck with 4 Mountain (DOM 250), local adds 2 Shock (M19 156), remote adds an unrelated card. Asserts the merge keeps one Mountain node with its printing and adds a single Shock node carrying M19/156. --- tests/deck_diff_test.cpp | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/deck_diff_test.cpp b/tests/deck_diff_test.cpp index 4ff0b556b..7fdc48c8b 100644 --- a/tests/deck_diff_test.cpp +++ b/tests/deck_diff_test.cpp @@ -35,6 +35,17 @@ int countOf(const DeckList &deck, const char *zone, const char *name) }); return count; } + +QList nodesOf(const DeckList &deck, const char *zone, const char *name) +{ + QList nodes; + deck.forEachCard([&](InnerDecklistNode *zoneNode, DecklistCardNode *card) { + if (zoneNode->getName() == zone && card->getName() == name) { + nodes.append(card); + } + }); + return nodes; +} } // namespace TEST(DeckDiffTest, IdenticalDecksHaveEmptyDiff) @@ -130,6 +141,39 @@ TEST(DeckDiffTest, MergeKeepsBaseMetadata) EXPECT_EQ(countOf(merged, "main", "Shock"), 1); } +TEST(DeckDiffTest, MergePreservesBasePrintingAndCollapsesNodes) +{ + DeckList base; + base.addCard("Mountain", "main", -1, "DOM", "250")->setNumber(4); + base.refreshDeckHash(); + + DeckList local = base; + local.addCard("Shock", "main", -1, "M19", "156")->setNumber(2); + local.refreshDeckHash(); + + DeckList remote = base; + remote.addCard("Lightning Bolt", "main", -1, "M10", "146")->setNumber(4); + remote.refreshDeckHash(); + + const DeckDiffResult localChanges = DeckDiff::diff(base, local); + const DeckDiffResult remoteChanges = DeckDiff::diff(base, remote); + ASSERT_TRUE(DeckDiff::canAutoMerge(localChanges, remoteChanges)); + + const DeckList merged = DeckDiff::merge(base, localChanges, remoteChanges); + + const QList mountains = nodesOf(merged, "main", "Mountain"); + ASSERT_EQ(mountains.size(), 1); + EXPECT_EQ(mountains.first()->getNumber(), 4); + EXPECT_EQ(mountains.first()->getCardSetShortName(), "DOM"); + EXPECT_EQ(mountains.first()->getCardCollectorNumber(), "250"); + + const QList shocks = nodesOf(merged, "main", "Shock"); + ASSERT_EQ(shocks.size(), 1); + EXPECT_EQ(shocks.first()->getNumber(), 2); + EXPECT_EQ(shocks.first()->getCardSetShortName(), "M19"); + EXPECT_EQ(shocks.first()->getCardCollectorNumber(), "156"); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); From 9c5ddc889c474cd58b512a7394647d3a78495b17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 11 Oct 2026 14:37:01 +0200 Subject: [PATCH 06/16] [Client] Manual deck collection sync with Sync Now action --- cockatrice/CMakeLists.txt | 1 + .../interface/deck_sync/deck_sync_manager.cpp | 510 ++++++++++++++++++ .../interface/deck_sync/deck_sync_manager.h | 133 +++++ .../widgets/tabs/tab_deck_storage.cpp | 80 +++ .../interface/widgets/tabs/tab_deck_storage.h | 29 + 5 files changed, 753 insertions(+) create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_manager.cpp create mode 100644 cockatrice/src/interface/deck_sync/deck_sync_manager.h diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index d76d89211..5519d2f3b 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -28,6 +28,7 @@ set(cockatrice_SOURCES src/interface/deck_loader/deck_loader.cpp src/interface/deck_sync/deck_sync_profile.cpp src/interface/deck_sync/deck_sync_sidecar.cpp + src/interface/deck_sync/deck_sync_manager.cpp src/interface/widgets/dialogs/dlg_connect.cpp src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp src/interface/widgets/dialogs/dlg_create_game.cpp diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp new file mode 100644 index 000000000..9fe76445b --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -0,0 +1,510 @@ +#include "deck_sync_manager.h" + +#include "../deck_loader/deck_loader.h" +#include "deck_sync_sidecar.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace +{ +constexpr const char *SYNC_META_DIR = ".sync"; +constexpr const char *BACKUP_SUFFIX = ".sync-backup-"; + +QString serverPathFor(const QString &relativePath) +{ + QString path = relativePath; + if (path.endsWith(".cod", Qt::CaseInsensitive)) { + path.chop(4); + } + return path; +} +} // namespace + +DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent) +{ +} + +void DeckSyncManager::setClient(AbstractClient *_client) +{ + client = _client; +} + +void DeckSyncManager::setProfile(const DeckSyncProfile &_profile) +{ + profile = _profile; +} + +void DeckSyncManager::setDeckPath(const QString &path) +{ + deckPath = path; +} + +void DeckSyncManager::setColorIdentityProvider(ColorIdentityProvider provider) +{ + colorIdentityProvider = std::move(provider); +} + +bool DeckSyncManager::isSyncing() const +{ + return syncing; +} + +void DeckSyncManager::syncNow() +{ + if (syncing) { + return; + } + if (client == nullptr || !profile.isValid() || deckPath.isEmpty()) { + emit syncFinished(0, 0, 0, 0, tr("Not connected to a server.")); + return; + } + QDir deckDir(deckPath); + if (!deckDir.exists()) { + emit syncFinished(0, 0, 0, 0, tr("Deck folder not found.")); + return; + } + + syncing = true; + uploadedCount = 0; + downloadedCount = 0; + unchangedCount = 0; + skippedCount = 0; + pendingUploads.clear(); + pendingDownloads.clear(); + ensuredDirs.clear(); + ensuredDirs.insert(QString()); + localFiles.clear(); + emit syncStarted(); + emit statusMessage(tr("Scanning local decks...")); + + sidecarPath = deckDir.filePath(SYNC_META_DIR + QString("/") + profile.key() + ".json"); + DeckSyncSidecar sidecar(sidecarPath); + if (!sidecar.load()) { + failSync(tr("Could not read sync state.")); + return; + } + + QDirIterator it(deckPath, QDir::Files | QDir::NoDotAndDotDot, QDirIterator::Subdirectories); + while (it.hasNext()) { + const QString absolutePath = it.next(); + const QString relativePath = deckDir.relativeFilePath(absolutePath); + if (relativePath.startsWith(SYNC_META_DIR + QString("/")) || + !absolutePath.endsWith(".cod", Qt::CaseInsensitive)) { + continue; + } + std::optional loaded = DeckLoader::loadFromFile(absolutePath, DeckFileFormat::Cockatrice); + if (!loaded) { + ++skippedCount; + continue; + } + LocalFile file; + file.absolutePath = absolutePath; + file.hash = loaded->deckList.getDeckHash(); + file.modified = QFileInfo(absolutePath).lastModified(); + localFiles.insert(relativePath, file); + } + + Command_DeckList cmd; + PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); + connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onListFinished); + client->sendCommand(pend); +} + +void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, + const QString &prefix, + QMap &files, + QSet &dirs) +{ + for (const auto &item : folder.items()) { + const QString name = QString::fromStdString(item.name()); + if (item.has_folder()) { + const QString path = prefix.isEmpty() ? name : prefix + "/" + name; + dirs.insert(path); + collectServerFiles(item.folder(), path, files, dirs); + } else if (item.has_file()) { + const QString path = prefix.isEmpty() ? name : prefix + "/" + name; + const ServerInfo_DeckStorage_File &file = item.file(); + ServerFile entry; + entry.id = item.id(); + entry.guid = QString::fromStdString(file.guid()); + entry.hash = QString::fromStdString(file.hash()); + entry.lastModifiedSecs = file.has_last_modified() ? static_cast(file.last_modified()) : 0; + files.insert(path, entry); + } + } +} + +void DeckSyncManager::onListFinished(const Response &response, const CommandContainer & /*commandContainer*/) +{ + if (!syncing) { + return; + } + if (response.response_code() != Response::RespOk) { + failSync(tr("Could not read server decks.")); + return; + } + + QMap serverFiles; + QSet serverDirs; + const Response_DeckList &list = response.GetExtension(Response_DeckList::ext); + collectServerFiles(list.root(), QString(), serverFiles, serverDirs); + ensuredDirs.unite(serverDirs); + emit statusMessage(tr("Comparing %1 local and %2 server decks...").arg(localFiles.size()).arg(serverFiles.size())); + + DeckSyncSidecar sidecar(sidecarPath); + sidecar.load(); + const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); + + for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { + const QString serverPath = serverPathFor(it.key()); + auto serverIt = serverFiles.find(serverPath); + if (serverIt == serverFiles.end()) { + UploadOp op; + op.relativePath = it.key(); + op.serverPath = serverPath; + op.deckId = -1; + op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() + ? sidecar.get(it.key()).guid + : QUuid::createUuid().toString(QUuid::WithoutBraces); + pendingUploads.append(op); + continue; + } + if (it.value().hash == serverIt.value().hash) { + DeckSyncRecord record = sidecar.get(it.key()); + if (record.guid.isEmpty()) { + record.guid = serverIt.value().guid; + } + record.remoteId = serverIt.value().id; + record.baseHash = it.value().hash; + record.lastSyncSecs = nowSecs; + record.deleted = false; + sidecar.set(it.key(), record); + ++unchangedCount; + continue; + } + const qint64 localSecs = it.value().modified.toSecsSinceEpoch(); + // A server row without last_modified predates sync: the local file wins. + // Otherwise strictly newer wins; ties go to the server. + if (serverIt.value().lastModifiedSecs == 0 || localSecs > serverIt.value().lastModifiedSecs) { + UploadOp op; + op.relativePath = it.key(); + op.serverPath = serverPath; + op.deckId = serverIt.value().id; + op.guid = serverIt.value().guid; + pendingUploads.append(op); + } else { + DownloadOp op; + op.relativePath = it.key(); + op.deckId = serverIt.value().id; + op.guid = serverIt.value().guid; + op.hash = serverIt.value().hash; + op.lastModifiedSecs = serverIt.value().lastModifiedSecs; + pendingDownloads.append(op); + } + } + + for (auto it = serverFiles.begin(); it != serverFiles.end(); ++it) { + const QString wantedPath = it.key().endsWith(".cod", Qt::CaseInsensitive) ? it.key() : it.key() + ".cod"; + if (localFiles.contains(wantedPath) || QFileInfo(deckPath + "/" + wantedPath).exists()) { + continue; + } + const QString relativePath = uniqueLocalPath(wantedPath); + if (relativePath.isEmpty()) { + ++skippedCount; + continue; + } + DownloadOp op; + op.relativePath = relativePath; + op.deckId = it.value().id; + op.guid = it.value().guid; + op.hash = it.value().hash; + op.lastModifiedSecs = it.value().lastModifiedSecs; + pendingDownloads.append(op); + } + + sidecar.save(); + processNext(); +} + +QString DeckSyncManager::parentDir(const QString &path) +{ + const int slash = path.lastIndexOf('/'); + return slash == -1 ? QString() : path.left(slash); +} + +void DeckSyncManager::processNext() +{ + if (!syncing) { + return; + } + if (!pendingUploads.isEmpty()) { + processNextUpload(); + } else if (!pendingDownloads.isEmpty()) { + sendDownload(pendingDownloads.first()); + } else { + finishSync(); + } +} + +void DeckSyncManager::processNextUpload() +{ + const UploadOp op = pendingUploads.first(); + const QString dir = parentDir(op.serverPath); + if (!ensuredDirs.contains(dir)) { + sendMkDirFor(dir); + return; + } + pendingUploads.removeFirst(); + sendUpload(op); +} + +void DeckSyncManager::sendMkDirFor(const QString &dir) +{ + QStringList parts = dir.split('/'); + QString made; + for (const QString &part : parts) { + const QString candidate = made.isEmpty() ? part : made + "/" + part; + if (!ensuredDirs.contains(candidate)) { + Command_DeckNewDir cmd; + cmd.set_path(made.toStdString()); + cmd.set_dir_name(part.toStdString()); + PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); + pend->setExtraData(dir); + connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onEnsureDirFinished); + client->sendCommand(pend); + return; + } + made = candidate; + } + processNextUpload(); +} + +void DeckSyncManager::onEnsureDirFinished(const Response &response, + const CommandContainer & /*commandContainer*/, + const QVariant &extraData) +{ + if (!syncing) { + return; + } + if (response.response_code() != Response::RespOk) { + failSync(tr("Could not create server folder.")); + return; + } + const QString dir = extraData.toString(); + QStringList parts = dir.split('/'); + QString made; + for (const QString &part : parts) { + made = made.isEmpty() ? part : made + "/" + part; + if (!ensuredDirs.contains(made)) { + ensuredDirs.insert(made); + break; + } + } + processNextUpload(); +} + +void DeckSyncManager::sendUpload(const UploadOp &op) +{ + emit statusMessage(tr("Uploading %1...").arg(op.relativePath)); + std::optional loaded = + DeckLoader::loadFromFile(localFiles.value(op.relativePath).absolutePath, DeckFileFormat::Cockatrice); + if (!loaded) { + ++skippedCount; + processNext(); + return; + } + const QString deckString = loaded->deckList.writeToString_Native(); + if (deckString.length() > MAX_FILE_LENGTH) { + ++skippedCount; + processNext(); + return; + } + + Command_DeckUpload cmd; + if (op.deckId == -1) { + cmd.set_path(parentDir(op.serverPath).toStdString()); + } else { + cmd.set_deck_id(op.deckId); + } + cmd.set_deck_list(deckString.toStdString()); + cmd.set_guid(op.guid.toStdString()); + if (colorIdentityProvider) { + cmd.set_color_identity(colorIdentityProvider(loaded->deckList).toStdString()); + } + PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); + pend->setExtraData(op.relativePath); + connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onUploadFinished); + client->sendCommand(pend); +} + +void DeckSyncManager::onUploadFinished(const Response &response, + const CommandContainer & /*commandContainer*/, + const QVariant &extraData) +{ + if (!syncing) { + return; + } + const QString relativePath = extraData.toString(); + if (response.response_code() != Response::RespOk) { + ++skippedCount; + processNext(); + return; + } + + const Response_DeckUpload &upload = response.GetExtension(Response_DeckUpload::ext); + DeckSyncSidecar sidecar(sidecarPath); + sidecar.load(); + DeckSyncRecord record = sidecar.get(relativePath); + if (upload.has_new_file()) { + const auto &file = upload.new_file(); + if (!file.file().guid().empty()) { + record.guid = QString::fromStdString(file.file().guid()); + } + if (!file.file().hash().empty()) { + record.baseHash = QString::fromStdString(file.file().hash()); + } + if (file.file().has_last_modified()) { + record.lastSyncSecs = static_cast(file.file().last_modified()); + } else { + record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); + } + record.remoteId = file.id(); + } else if (localFiles.contains(relativePath)) { + record.baseHash = localFiles.value(relativePath).hash; + record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); + } + record.deleted = false; + sidecar.set(relativePath, record); + sidecar.save(); + ++uploadedCount; + processNext(); +} + +QString DeckSyncManager::uniqueLocalPath(const QString &relativePath) const +{ + if (!QFileInfo(deckPath + "/" + relativePath).exists()) { + return relativePath; + } + QString base = relativePath; + if (base.endsWith(".cod", Qt::CaseInsensitive)) { + base.chop(4); + } + for (int i = 2; i < 1000; ++i) { + const QString candidate = base + " (" + QString::number(i) + ").cod"; + if (!QFileInfo(deckPath + "/" + candidate).exists()) { + return candidate; + } + } + return QString(); +} + +void DeckSyncManager::sendDownload(const DownloadOp &op) +{ + emit statusMessage(tr("Downloading %1...").arg(op.relativePath)); + Command_DeckDownload cmd; + cmd.set_deck_id(op.deckId); + PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); + pend->setExtraData(op.relativePath); + connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onDownloadFinished); + client->sendCommand(pend); +} + +void DeckSyncManager::onDownloadFinished(const Response &response, + const CommandContainer & /*commandContainer*/, + const QVariant &extraData) +{ + if (!syncing) { + return; + } + const QString relativePath = extraData.toString(); + for (int i = 0; i < pendingDownloads.size(); ++i) { + if (pendingDownloads.at(i).relativePath == relativePath) { + pendingDownloads.removeAt(i); + break; + } + } + if (response.response_code() != Response::RespOk) { + ++skippedCount; + processNext(); + return; + } + + const Response_DeckDownload &download = response.GetExtension(Response_DeckDownload::ext); + DeckList deck; + if (!deck.loadFromString_Native(QString::fromStdString(download.deck()))) { + ++skippedCount; + processNext(); + return; + } + + const QString absolutePath = deckPath + "/" + relativePath; + if (localFiles.contains(relativePath)) { + const QString backupPath = + absolutePath + BACKUP_SUFFIX + QString::number(QDateTime::currentDateTime().toSecsSinceEpoch()); + if (!QFile::copy(absolutePath, backupPath)) { + ++skippedCount; + processNext(); + return; + } + } + QDir().mkpath(QFileInfo(absolutePath).absolutePath()); + if (!DeckLoader::saveToFile(deck, absolutePath, DeckFileFormat::Cockatrice)) { + ++skippedCount; + processNext(); + return; + } + + DeckSyncSidecar sidecar(sidecarPath); + sidecar.load(); + DeckSyncRecord record = sidecar.get(relativePath); + if (!download.guid().empty()) { + record.guid = QString::fromStdString(download.guid()); + } + if (!download.hash().empty()) { + record.baseHash = QString::fromStdString(download.hash()); + } else { + record.baseHash = deck.getDeckHash(); + } + record.lastSyncSecs = download.has_last_modified() ? static_cast(download.last_modified()) + : QDateTime::currentDateTime().toSecsSinceEpoch(); + record.deleted = false; + sidecar.set(relativePath, record); + sidecar.save(); + ++downloadedCount; + processNext(); +} + +void DeckSyncManager::failSync(const QString &error) +{ + syncing = false; + emit syncFinished(uploadedCount, downloadedCount, unchangedCount, skippedCount, error); +} + +void DeckSyncManager::finishSync() +{ + syncing = false; + emit statusMessage(tr("Sync complete: %1 uploaded, %2 downloaded, %3 unchanged, %4 skipped.") + .arg(uploadedCount) + .arg(downloadedCount) + .arg(unchangedCount) + .arg(skippedCount)); + emit syncFinished(uploadedCount, downloadedCount, unchangedCount, skippedCount, QString()); +} diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h new file mode 100644 index 000000000..99f2040fe --- /dev/null +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -0,0 +1,133 @@ +/** + * @file deck_sync_manager.h + * @brief One-shot two-way deck collection sync between local files and the server. + */ + +#ifndef DECK_SYNC_MANAGER_H +#define DECK_SYNC_MANAGER_H + +#include "deck_sync_profile.h" + +#include +#include +#include +#include +#include +#include +#include +#include + +class AbstractClient; +class CommandContainer; +class DeckList; +class Response; +class ServerInfo_DeckStorage_Folder; + +/** + * @class DeckSyncManager + * @brief Runs a manual whole-collection sync: local *.cod files vs server deck storage. + * + * Each syncNow() run loads the profile sidecar, scans the deck folder, fetches + * the server tree and mirrors both sides: unknown local decks upload, unknown + * server decks download, and decks changed on both sides resolve newer-wins + * (ties go to the server). Matching is path-based; a rename looks like a + * delete plus an add. Before a download overwrites a locally modified file, the + * local content is copied to a .sync-backup file next to it. + * + * Ownership: fire-and-forget per run, driven by PendingCommand round trips. + * No watcher, no auto triggers, no conflict UI — those arrive in later layers. + */ +class DeckSyncManager : public QObject +{ + Q_OBJECT +public: + /** @brief Computes the color identity string uploaded alongside a deck. May be empty. */ + using ColorIdentityProvider = std::function; + + explicit DeckSyncManager(QObject *parent = nullptr); + + void setClient(AbstractClient *_client); + void setProfile(const DeckSyncProfile &_profile); + void setDeckPath(const QString &path); + void setColorIdentityProvider(ColorIdentityProvider provider); + [[nodiscard]] bool isSyncing() const; + +public slots: + void syncNow(); + +signals: + void syncStarted(); + void statusMessage(const QString &message); + void syncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error); + +private slots: + void onListFinished(const Response &response, const CommandContainer &commandContainer); + void + onEnsureDirFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); + void + onUploadFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); + void + onDownloadFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); + +private: + struct ServerFile + { + int id = -1; + QString guid; + QString hash; + qint64 lastModifiedSecs = 0; + }; + struct LocalFile + { + QString absolutePath; + QString hash; + QDateTime modified; + }; + struct UploadOp + { + QString relativePath; + QString serverPath; + int deckId = -1; + QString guid; + }; + struct DownloadOp + { + QString relativePath; + int deckId = -1; + QString guid; + QString hash; + qint64 lastModifiedSecs = 0; + }; + + void failSync(const QString &error); + void finishSync(); + void collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, + const QString &prefix, + QMap &files, + QSet &dirs); + void processNext(); + void processNextUpload(); + void sendMkDirFor(const QString &dir); + void sendUpload(const UploadOp &op); + void sendDownload(const DownloadOp &op); + static QString parentDir(const QString &path); + QString uniqueLocalPath(const QString &relativePath) const; + + AbstractClient *client = nullptr; + DeckSyncProfile profile; + QString deckPath; + ColorIdentityProvider colorIdentityProvider; + bool syncing = false; + + QString sidecarPath; + QMap localFiles; + QSet ensuredDirs; + QList pendingUploads; + QList pendingDownloads; + int uploadedCount = 0; + int downloadedCount = 0; + int unchangedCount = 0; + int skippedCount = 0; +}; + +#endif // DECK_SYNC_MANAGER_H diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp index 3dfdc956b..90cd24290 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp @@ -2,6 +2,7 @@ #include "../../../client/settings/cache_settings.h" #include "../../deck_loader/deck_loader.h" +#include "../../deck_sync/deck_sync_manager.h" #include "../../pixel_map_generator.h" #include "../cards/additional_info/deck_color_identity.h" #include "../deck_share/deck_share_utils.h" @@ -29,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -177,6 +179,9 @@ TabDeckStorage::TabDeckStorage(TabSupervisor *_tabSupervisor, rightVbox->addWidget(shareBar); rightVbox->addWidget(serverDirView); rightVbox->addLayout(rightToolBarLayout); + syncStatusLabel = new QLabel(this); + syncStatusLabel->setWordWrap(true); + rightVbox->addWidget(syncStatusLabel); rightGroupBox = new QGroupBox; rightGroupBox->setLayout(rightVbox); @@ -228,6 +233,10 @@ TabDeckStorage::TabDeckStorage(TabSupervisor *_tabSupervisor, aPublishDeck->setIcon(QPixmap("theme:icons/lock")); connect(aPublishDeck, &QAction::triggered, this, &TabDeckStorage::actPublishDeck); + aSyncNow = new QAction(this); + aSyncNow->setIcon(qApp->style()->standardIcon(QStyle::SP_BrowserReload)); + connect(aSyncNow, &QAction::triggered, this, &TabDeckStorage::actSyncNow); + // Add actions to toolbars leftToolBar->addAction(aOpenLocalDeck); leftToolBar->addAction(aRenameLocal); @@ -239,11 +248,22 @@ TabDeckStorage::TabDeckStorage(TabSupervisor *_tabSupervisor, rightToolBar->addAction(aOpenRemoteDeck); rightToolBar->addAction(aDownload); + rightToolBar->addAction(aSyncNow); rightToolBar->addAction(aShareDecks); rightToolBar->addAction(aPublishDeck); rightToolBar->addAction(aNewFolder); rightToolBar->addAction(aDeleteRemoteDeck); + syncManager = new DeckSyncManager(this); + syncManager->setClient(client); + syncManager->setColorIdentityProvider( + [](const DeckList &deck) { return getDeckColorIdentity(deck, CardDatabaseManager::query()); }); + connect(syncManager, &DeckSyncManager::statusMessage, this, &TabDeckStorage::onSyncStatusMessage); + connect(syncManager, &DeckSyncManager::syncFinished, this, &TabDeckStorage::onSyncFinished); + if (currentUserInfo != nullptr) { + syncUsername = QString::fromStdString(currentUserInfo->name()); + } + retranslateUi(); QWidget *mainWidget = new QWidget(this); @@ -272,6 +292,8 @@ void TabDeckStorage::retranslateUi() aShareDecks->setText(tr("Share decks")); aPublishDeck->setText(tr("Publish/unpublish deck")); aOpenDecksFolder->setText(tr("Open decks folder")); + aSyncNow->setText(tr("Sync now")); + renderSyncStatus(); shareBar->retranslateUi(); if (shareBar->isVisible()) { onServerSelectionChanged(); @@ -299,6 +321,7 @@ QString TabDeckStorage::getTargetPath() const void TabDeckStorage::handleConnected(const ServerInfo_User &userInfo) { + syncUsername = QString::fromStdString(userInfo.name()); setRemoteEnabled(userInfo.user_level() & ServerInfo_User::IsRegistered); } @@ -316,6 +339,7 @@ void TabDeckStorage::handleConnectionChanged(ClientStatus status) void TabDeckStorage::setRemoteEnabled(bool enabled) { + remoteEnabled = enabled; aUpload->setEnabled(enabled); aOpenRemoteDeck->setEnabled(enabled); aDownload->setEnabled(enabled); @@ -323,6 +347,7 @@ void TabDeckStorage::setRemoteEnabled(bool enabled) aPublishDeck->setEnabled(enabled); aNewFolder->setEnabled(enabled); aDeleteRemoteDeck->setEnabled(enabled); + aSyncNow->setEnabled(enabled && !syncManager->isSyncing()); if (enabled) { serverDirView->refreshTree(); @@ -526,6 +551,61 @@ void TabDeckStorage::actOpenDecksFolder() QtUtils::openPathInFileManager(dir); } +void TabDeckStorage::actSyncNow() +{ + DeckSyncProfile profile(client->serverName(), client->serverPort(), syncUsername); + syncManager->setProfile(profile); + syncManager->setDeckPath(SettingsCache::instance().paths().getDeckPath()); + aSyncNow->setEnabled(false); + syncManager->syncNow(); +} + +void TabDeckStorage::onSyncStatusMessage(const QString &message) +{ + syncStatusType = SyncStatusType::Working; + syncStatusDetail = message; + renderSyncStatus(); +} + +void TabDeckStorage::onSyncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error) +{ + aSyncNow->setEnabled(remoteEnabled); + if (!error.isEmpty()) { + syncStatusType = SyncStatusType::Failed; + syncStatusDetail = error; + } else { + syncStatusType = SyncStatusType::Finished; + syncStatusDetail.clear(); + lastSyncSummary = {uploaded, downloaded, unchanged, skipped}; + } + renderSyncStatus(); + if (error.isEmpty()) { + serverDirView->refreshTree(); + } +} + +void TabDeckStorage::renderSyncStatus() +{ + switch (syncStatusType) { + case SyncStatusType::Working: + syncStatusLabel->setText(syncStatusDetail); + break; + case SyncStatusType::Failed: + syncStatusLabel->setText(tr("Sync failed: %1").arg(syncStatusDetail)); + break; + case SyncStatusType::Finished: + syncStatusLabel->setText(tr("Sync complete: %1 uploaded, %2 downloaded, %3 unchanged, %4 skipped.") + .arg(lastSyncSummary.uploaded) + .arg(lastSyncSummary.downloaded) + .arg(lastSyncSummary.unchanged) + .arg(lastSyncSummary.skipped)); + break; + case SyncStatusType::Idle: + syncStatusLabel->setText(tr("Not synced yet")); + break; + } +} + void TabDeckStorage::actRemoteDoubleClick(const QModelIndex &curRight) { if (dynamic_cast(serverDirView->getNode(curRight))) { diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h index 35a4649c7..f24f07ac7 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h @@ -25,9 +25,11 @@ class QFileSystemModel; class QToolBar; class QGroupBox; class QTimer; +class QLabel; class CommandContainer; class Response; class ShareBarWidget; +class DeckSyncManager; class QAction; class QModelIndex; class TabSupervisor; @@ -50,6 +52,29 @@ private: QAction *aOpenLocalDeck, *aRenameLocal, *aUpload, *aNewLocalFolder, *aDeleteLocalDeck; QAction *aOpenDecksFolder; QAction *aOpenRemoteDeck, *aDownload, *aShareDecks, *aPublishDeck, *aNewFolder, *aDeleteRemoteDeck; + QAction *aSyncNow; + QLabel *syncStatusLabel; + DeckSyncManager *syncManager; + QString syncUsername; + bool remoteEnabled = false; + enum class SyncStatusType + { + Idle, + Working, + Finished, + Failed + }; + struct SyncSummary + { + int uploaded = 0; + int downloaded = 0; + int unchanged = 0; + int skipped = 0; + }; + SyncStatusType syncStatusType = SyncStatusType::Idle; + QString syncStatusDetail; + SyncSummary lastSyncSummary; + void renderSyncStatus(); bool visibilityRefreshStarted = false; QTimer *visibilityRefreshTimer; QStringList visibilityFailures; @@ -108,6 +133,10 @@ private slots: void deleteFolderFinished(const Response &response, const CommandContainer &commandContainer); void deleteDeckFinished(const Response &response, const CommandContainer &commandContainer); + void actSyncNow(); + void onSyncStatusMessage(const QString &message); + void onSyncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error); + public: TabDeckStorage(TabSupervisor *_tabSupervisor, AbstractClient *_client, const ServerInfo_User *currentUserInfo); void retranslateUi() override; From c0aa30f75f8e6fd1ed293b1ad5b0ce95228c7b4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 07:18:43 +0200 Subject: [PATCH 07/16] Fix ZeldaZach review for PR 7493: match uploads by sidecar remoteId/guid, validate server paths --- .../interface/deck_sync/deck_sync_manager.cpp | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 9fe76445b..941126e48 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -135,6 +135,9 @@ void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &fo { for (const auto &item : folder.items()) { const QString name = QString::fromStdString(item.name()); + if (name.isEmpty() || name == "." || name == ".." || name.contains('/') || name.contains('\\')) { + continue; + } if (item.has_folder()) { const QString path = prefix.isEmpty() ? name : prefix + "/" + name; dirs.insert(path); @@ -173,8 +176,37 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont sidecar.load(); const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); + QMap serverPathById; + QMap serverPathByGuid; + for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) { + serverPathById.insert(sit.value().id, sit.key()); + if (!sit.value().guid.isEmpty()) { + serverPathByGuid.insert(sit.value().guid, sit.key()); + } + } + QSet matchedServerPaths; + for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { - const QString serverPath = serverPathFor(it.key()); + const DeckSyncRecord sidecarRecord = sidecar.get(it.key()); + QString serverPath; + if (sidecarRecord.remoteId != -1 && serverPathById.contains(sidecarRecord.remoteId)) { + serverPath = serverPathById.value(sidecarRecord.remoteId); + } else if (!sidecarRecord.guid.isEmpty() && serverPathByGuid.contains(sidecarRecord.guid)) { + serverPath = serverPathByGuid.value(sidecarRecord.guid); + } else { + serverPath = serverPathFor(it.key()); + if (!serverFiles.contains(serverPath)) { + UploadOp op; + op.relativePath = it.key(); + op.serverPath = serverPath; + op.deckId = -1; + op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() + ? sidecar.get(it.key()).guid + : QUuid::createUuid().toString(QUuid::WithoutBraces); + pendingUploads.append(op); + continue; + } + } auto serverIt = serverFiles.find(serverPath); if (serverIt == serverFiles.end()) { UploadOp op; @@ -187,6 +219,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont pendingUploads.append(op); continue; } + matchedServerPaths.insert(serverIt.key()); if (it.value().hash == serverIt.value().hash) { DeckSyncRecord record = sidecar.get(it.key()); if (record.guid.isEmpty()) { @@ -222,6 +255,9 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont } for (auto it = serverFiles.begin(); it != serverFiles.end(); ++it) { + if (matchedServerPaths.contains(it.key())) { + continue; + } const QString wantedPath = it.key().endsWith(".cod", Qt::CaseInsensitive) ? it.key() : it.key() + ".cod"; if (localFiles.contains(wantedPath) || QFileInfo(deckPath + "/" + wantedPath).exists()) { continue; @@ -455,7 +491,13 @@ void DeckSyncManager::onDownloadFinished(const Response &response, return; } - const QString absolutePath = deckPath + "/" + relativePath; + const QString cleanDeckPath = QDir::cleanPath(deckPath); + const QString absolutePath = QDir::cleanPath(deckPath + "/" + relativePath); + if (absolutePath != cleanDeckPath && !absolutePath.startsWith(cleanDeckPath + "/")) { + ++skippedCount; + processNext(); + return; + } if (localFiles.contains(relativePath)) { const QString backupPath = absolutePath + BACKUP_SUFFIX + QString::number(QDateTime::currentDateTime().toSecsSinceEpoch()); From aa5cadd4ebb8213b31450dc008805c918bd58364 Mon Sep 17 00:00:00 2001 From: BruebachL Date: Tue, 13 Oct 2026 11:45:29 +0200 Subject: [PATCH 08/16] [DeckSync] Move SyncSummary into DeckSyncManager per review --- cockatrice/src/interface/deck_sync/deck_sync_manager.h | 9 +++++++++ .../src/interface/widgets/tabs/tab_deck_storage.h | 10 ++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 99f2040fe..7f759c313 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -44,6 +44,15 @@ public: /** @brief Computes the color identity string uploaded alongside a deck. May be empty. */ using ColorIdentityProvider = std::function; + /** @brief Outcome counts for a finished sync run; mirrors syncFinished() parameters. */ + struct SyncSummary + { + int uploaded = 0; + int downloaded = 0; + int unchanged = 0; + int skipped = 0; + }; + explicit DeckSyncManager(QObject *parent = nullptr); void setClient(AbstractClient *_client); diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h index f24f07ac7..bd53d5073 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h @@ -11,6 +11,7 @@ #include "../interface/widgets/server/remote/remote_decklist_tree_widget.h" #include "tab.h" +#include "../deck_sync/deck_sync_manager.h" #include #include #include @@ -29,7 +30,6 @@ class QLabel; class CommandContainer; class Response; class ShareBarWidget; -class DeckSyncManager; class QAction; class QModelIndex; class TabSupervisor; @@ -64,13 +64,7 @@ private: Finished, Failed }; - struct SyncSummary - { - int uploaded = 0; - int downloaded = 0; - int unchanged = 0; - int skipped = 0; - }; + using SyncSummary = DeckSyncManager::SyncSummary; SyncStatusType syncStatusType = SyncStatusType::Idle; QString syncStatusDetail; SyncSummary lastSyncSummary; From 5b51fc8810bdb31bc8672d8f9b4b035f4dbe29a6 Mon Sep 17 00:00:00 2001 From: BruebachL Date: Tue, 13 Oct 2026 11:45:44 +0200 Subject: [PATCH 09/16] [DeckSync] Prefer ctor params for set-once client/provider per review --- cockatrice/src/interface/deck_sync/deck_sync_manager.cpp | 5 +++++ cockatrice/src/interface/deck_sync/deck_sync_manager.h | 1 + cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp | 6 ++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 941126e48..464849eec 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -42,6 +42,11 @@ DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent) { } +DeckSyncManager::DeckSyncManager(AbstractClient *_client, ColorIdentityProvider provider, QObject *parent) + : QObject(parent), client(_client), colorIdentityProvider(std::move(provider)) +{ +} + void DeckSyncManager::setClient(AbstractClient *_client) { client = _client; diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 7f759c313..2bf12f72f 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -54,6 +54,7 @@ public: }; explicit DeckSyncManager(QObject *parent = nullptr); + DeckSyncManager(AbstractClient *client, ColorIdentityProvider provider, QObject *parent = nullptr); void setClient(AbstractClient *_client); void setProfile(const DeckSyncProfile &_profile); diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp index 90cd24290..74154f3da 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp @@ -254,10 +254,8 @@ TabDeckStorage::TabDeckStorage(TabSupervisor *_tabSupervisor, rightToolBar->addAction(aNewFolder); rightToolBar->addAction(aDeleteRemoteDeck); - syncManager = new DeckSyncManager(this); - syncManager->setClient(client); - syncManager->setColorIdentityProvider( - [](const DeckList &deck) { return getDeckColorIdentity(deck, CardDatabaseManager::query()); }); + syncManager = new DeckSyncManager( + client, [](const DeckList &deck) { return getDeckColorIdentity(deck, CardDatabaseManager::query()); }, this); connect(syncManager, &DeckSyncManager::statusMessage, this, &TabDeckStorage::onSyncStatusMessage); connect(syncManager, &DeckSyncManager::syncFinished, this, &TabDeckStorage::onSyncFinished); if (currentUserInfo != nullptr) { From 8846a6f5cadc187635386b9da43294432cb06e5c Mon Sep 17 00:00:00 2001 From: BruebachL Date: Tue, 13 Oct 2026 11:45:54 +0200 Subject: [PATCH 10/16] [DeckSync] Use designated initializer for ServerFile per review --- .../src/interface/deck_sync/deck_sync_manager.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 464849eec..49be7fac0 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -150,11 +150,12 @@ void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &fo } else if (item.has_file()) { const QString path = prefix.isEmpty() ? name : prefix + "/" + name; const ServerInfo_DeckStorage_File &file = item.file(); - ServerFile entry; - entry.id = item.id(); - entry.guid = QString::fromStdString(file.guid()); - entry.hash = QString::fromStdString(file.hash()); - entry.lastModifiedSecs = file.has_last_modified() ? static_cast(file.last_modified()) : 0; + ServerFile entry{ + .id = item.id(), + .guid = QString::fromStdString(file.guid()), + .hash = QString::fromStdString(file.hash()), + .lastModifiedSecs = file.has_last_modified() ? static_cast(file.last_modified()) : 0, + }; files.insert(path, entry); } } From 8367deefaf211fa5e7e47b01e6f01f99ab30a044 Mon Sep 17 00:00:00 2001 From: BruebachL Date: Tue, 13 Oct 2026 11:50:40 +0200 Subject: [PATCH 11/16] [Format] Fix include order in tab_deck_storage.h --- cockatrice/src/interface/widgets/tabs/tab_deck_storage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h index bd53d5073..ade10e951 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h @@ -8,10 +8,10 @@ #ifndef TAB_DECK_STORAGE_H #define TAB_DECK_STORAGE_H +#include "../deck_sync/deck_sync_manager.h" #include "../interface/widgets/server/remote/remote_decklist_tree_widget.h" #include "tab.h" -#include "../deck_sync/deck_sync_manager.h" #include #include #include From aa0ab333f668018019af3979eb0332b43147bc3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 1 Nov 2026 12:41:00 +0100 Subject: [PATCH 12/16] [DeckSync] Pass the sync summary through the signal and tidy the manager API - Emit SyncSummary from syncFinished() instead of four positional ints, and build it from a single currentSummary() helper, so the counts cannot drift between the status message and the signal. - Use a designated initializer for LocalFile, like the other records. - Drop setClient() and setColorIdentityProvider(): both fields are already constructor parameters and the setters had no callers. setProfile() and setDeckPath() stay, since the tab re-reads them before every run. - Fix the include paths in tab_deck_storage.h and the narrowing conversion of the tree item id, so the branch builds again. --- .../interface/deck_sync/deck_sync_manager.cpp | 47 +++++++++---------- .../interface/deck_sync/deck_sync_manager.h | 7 ++- .../widgets/tabs/tab_deck_storage.cpp | 4 +- .../interface/widgets/tabs/tab_deck_storage.h | 6 +-- 4 files changed, 31 insertions(+), 33 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 49be7fac0..0ff162ee1 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -47,11 +47,6 @@ DeckSyncManager::DeckSyncManager(AbstractClient *_client, ColorIdentityProvider { } -void DeckSyncManager::setClient(AbstractClient *_client) -{ - client = _client; -} - void DeckSyncManager::setProfile(const DeckSyncProfile &_profile) { profile = _profile; @@ -62,11 +57,6 @@ void DeckSyncManager::setDeckPath(const QString &path) deckPath = path; } -void DeckSyncManager::setColorIdentityProvider(ColorIdentityProvider provider) -{ - colorIdentityProvider = std::move(provider); -} - bool DeckSyncManager::isSyncing() const { return syncing; @@ -78,12 +68,12 @@ void DeckSyncManager::syncNow() return; } if (client == nullptr || !profile.isValid() || deckPath.isEmpty()) { - emit syncFinished(0, 0, 0, 0, tr("Not connected to a server.")); + emit syncFinished(SyncSummary{}, tr("Not connected to a server.")); return; } QDir deckDir(deckPath); if (!deckDir.exists()) { - emit syncFinished(0, 0, 0, 0, tr("Deck folder not found.")); + emit syncFinished(SyncSummary{}, tr("Deck folder not found.")); return; } @@ -120,11 +110,9 @@ void DeckSyncManager::syncNow() ++skippedCount; continue; } - LocalFile file; - file.absolutePath = absolutePath; - file.hash = loaded->deckList.getDeckHash(); - file.modified = QFileInfo(absolutePath).lastModified(); - localFiles.insert(relativePath, file); + localFiles.insert(relativePath, LocalFile{.absolutePath = absolutePath, + .hash = loaded->deckList.getDeckHash(), + .modified = QFileInfo(absolutePath).lastModified()}); } Command_DeckList cmd; @@ -151,7 +139,7 @@ void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &fo const QString path = prefix.isEmpty() ? name : prefix + "/" + name; const ServerInfo_DeckStorage_File &file = item.file(); ServerFile entry{ - .id = item.id(), + .id = static_cast(item.id()), .guid = QString::fromStdString(file.guid()), .hash = QString::fromStdString(file.hash()), .lastModifiedSecs = file.has_last_modified() ? static_cast(file.last_modified()) : 0, @@ -543,16 +531,27 @@ void DeckSyncManager::onDownloadFinished(const Response &response, void DeckSyncManager::failSync(const QString &error) { syncing = false; - emit syncFinished(uploadedCount, downloadedCount, unchangedCount, skippedCount, error); + emit syncFinished(currentSummary(), error); } void DeckSyncManager::finishSync() { syncing = false; + const SyncSummary summary = currentSummary(); emit statusMessage(tr("Sync complete: %1 uploaded, %2 downloaded, %3 unchanged, %4 skipped.") - .arg(uploadedCount) - .arg(downloadedCount) - .arg(unchangedCount) - .arg(skippedCount)); - emit syncFinished(uploadedCount, downloadedCount, unchangedCount, skippedCount, QString()); + .arg(summary.uploaded) + .arg(summary.downloaded) + .arg(summary.unchanged) + .arg(summary.skipped)); + emit syncFinished(summary, QString()); +} + +DeckSyncManager::SyncSummary DeckSyncManager::currentSummary() const +{ + return SyncSummary{ + .uploaded = uploadedCount, + .downloaded = downloadedCount, + .unchanged = unchangedCount, + .skipped = skippedCount, + }; } diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 2bf12f72f..8ecbaf74c 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -44,7 +44,7 @@ public: /** @brief Computes the color identity string uploaded alongside a deck. May be empty. */ using ColorIdentityProvider = std::function; - /** @brief Outcome counts for a finished sync run; mirrors syncFinished() parameters. */ + /** @brief Outcome counts for a finished sync run, carried by syncFinished(). */ struct SyncSummary { int uploaded = 0; @@ -56,10 +56,8 @@ public: explicit DeckSyncManager(QObject *parent = nullptr); DeckSyncManager(AbstractClient *client, ColorIdentityProvider provider, QObject *parent = nullptr); - void setClient(AbstractClient *_client); void setProfile(const DeckSyncProfile &_profile); void setDeckPath(const QString &path); - void setColorIdentityProvider(ColorIdentityProvider provider); [[nodiscard]] bool isSyncing() const; public slots: @@ -68,7 +66,7 @@ public slots: signals: void syncStarted(); void statusMessage(const QString &message); - void syncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error); + void syncFinished(const SyncSummary &summary, const QString &error); private slots: void onListFinished(const Response &response, const CommandContainer &commandContainer); @@ -111,6 +109,7 @@ private: void failSync(const QString &error); void finishSync(); + [[nodiscard]] SyncSummary currentSummary() const; void collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, const QString &prefix, QMap &files, diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp index 74154f3da..2ca46d4f4 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp @@ -565,7 +565,7 @@ void TabDeckStorage::onSyncStatusMessage(const QString &message) renderSyncStatus(); } -void TabDeckStorage::onSyncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error) +void TabDeckStorage::onSyncFinished(const SyncSummary &summary, const QString &error) { aSyncNow->setEnabled(remoteEnabled); if (!error.isEmpty()) { @@ -574,7 +574,7 @@ void TabDeckStorage::onSyncFinished(int uploaded, int downloaded, int unchanged, } else { syncStatusType = SyncStatusType::Finished; syncStatusDetail.clear(); - lastSyncSummary = {uploaded, downloaded, unchanged, skipped}; + lastSyncSummary = summary; } renderSyncStatus(); if (error.isEmpty()) { diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h index ade10e951..c7456877e 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h @@ -8,8 +8,8 @@ #ifndef TAB_DECK_STORAGE_H #define TAB_DECK_STORAGE_H -#include "../deck_sync/deck_sync_manager.h" -#include "../interface/widgets/server/remote/remote_decklist_tree_widget.h" +#include "../../deck_sync/deck_sync_manager.h" +#include "../server/remote/remote_decklist_tree_widget.h" #include "tab.h" #include @@ -129,7 +129,7 @@ private slots: void actSyncNow(); void onSyncStatusMessage(const QString &message); - void onSyncFinished(int uploaded, int downloaded, int unchanged, int skipped, const QString &error); + void onSyncFinished(const SyncSummary &summary, const QString &error); public: TabDeckStorage(TabSupervisor *_tabSupervisor, AbstractClient *_client, const ServerInfo_User *currentUserInfo); From 34e22df5baf1ff8758109e7ab0832716c702abd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 1 Nov 2026 12:43:02 +0100 Subject: [PATCH 13/16] [DeckSync] Match server rows by identity and hash the whole deck Sync keyed local files to server rows by path, but the server names a row after the deck, not after the file, so any deck whose file name and deck name differ uploaded again on every run. A second pass then brought the server copy back as an extra local file. - Match by the sidecar's row id, then guid, then content, and only then by the server path, so renaming either side no longer duplicates a deck. - Key the server tree by row id: the server allows several rows with the same name in one folder, and keying by path silently dropped all but one. - Stop discarding rows whose name contains '/' ("U/W Control"). They stay matchable, and only the download file name is sanitised. - Build the download path from the folder plus a sanitised row name, including every folder segment, so a hostile server name cannot escape the deck folder. - Download a colliding server row under a unique name and count it, instead of skipping it without a trace. - Hash the whole deck instead of the card hash: SHA-1 over the native deck string covers name, printings, banner, tokens and sideboard plans. The client and the server hash the exact bytes they exchange, so both agree. Adds deck_content_hash_test for the client/server hashing invariant and for the name-only change the card hash cannot see. --- .../interface/deck_sync/deck_sync_manager.cpp | 195 +++++++++++------- .../interface/deck_sync/deck_sync_manager.h | 16 +- .../libcockatrice/deck_list/deck_list.cpp | 11 + .../libcockatrice/deck_list/deck_list.h | 10 + servatrice/src/serversocketinterface.cpp | 4 +- tests/CMakeLists.txt | 9 + tests/deck_content_hash_test.cpp | 51 +++++ 7 files changed, 215 insertions(+), 81 deletions(-) create mode 100644 tests/deck_content_hash_test.cpp diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 0ff162ee1..dd4dd6f69 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,35 @@ QString serverPathFor(const QString &relativePath) } return path; } + +// Rows are named after the deck, not after the file, so a row name may contain +// characters a file name cannot ("U/W Control"). Downloads therefore rebuild the +// local path from the folder plus a sanitised row name, and never from the raw +// server-supplied path. +QString sanitizedSegment(const QString &segment) +{ + QString safe = segment; + safe.replace('/', '-'); + safe.replace('\\', '-'); + safe = safe.trimmed(); + if (safe.isEmpty() || safe == QStringLiteral(".") || safe == QStringLiteral("..")) { + return QStringLiteral("deck"); + } + return safe; +} + +QString localPathFor(const QString &dir, const QString &name) +{ + // Every segment is server-supplied, so each one is sanitised before it can + // become part of a local path. + QStringList segments; + for (const QString &segment : dir.split('/', Qt::SkipEmptyParts)) { + segments << sanitizedSegment(segment); + } + segments << sanitizedSegment(name); + const QString path = segments.join('/'); + return path.endsWith(".cod", Qt::CaseInsensitive) ? path : path + QStringLiteral(".cod"); +} } // namespace DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent) @@ -111,7 +141,7 @@ void DeckSyncManager::syncNow() continue; } localFiles.insert(relativePath, LocalFile{.absolutePath = absolutePath, - .hash = loaded->deckList.getDeckHash(), + .hash = loaded->deckList.getContentHash(), .modified = QFileInfo(absolutePath).lastModified()}); } @@ -123,28 +153,30 @@ void DeckSyncManager::syncNow() void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, const QString &prefix, - QMap &files, + QMap &files, QSet &dirs) { for (const auto &item : folder.items()) { const QString name = QString::fromStdString(item.name()); - if (name.isEmpty() || name == "." || name == ".." || name.contains('/') || name.contains('\\')) { - continue; - } + const QString path = prefix.isEmpty() ? name : prefix + "/" + name; if (item.has_folder()) { - const QString path = prefix.isEmpty() ? name : prefix + "/" + name; dirs.insert(path); collectServerFiles(item.folder(), path, files, dirs); } else if (item.has_file()) { - const QString path = prefix.isEmpty() ? name : prefix + "/" + name; const ServerInfo_DeckStorage_File &file = item.file(); + const int id = static_cast(item.id()); ServerFile entry{ - .id = static_cast(item.id()), + .id = id, + .path = path, + .dir = prefix, + .name = name, .guid = QString::fromStdString(file.guid()), .hash = QString::fromStdString(file.hash()), .lastModifiedSecs = file.has_last_modified() ? static_cast(file.last_modified()) : 0, }; - files.insert(path, entry); + // Keyed by id: the server allows several rows with the same name in a + // folder, and keying by path silently dropped all but one of them. + files.insert(id, entry); } } } @@ -159,7 +191,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont return; } - QMap serverFiles; + QMap serverFiles; QSet serverDirs; const Response_DeckList &list = response.GetExtension(Response_DeckList::ext); collectServerFiles(list.root(), QString(), serverFiles, serverDirs); @@ -169,57 +201,78 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont DeckSyncSidecar sidecar(sidecarPath); sidecar.load(); const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); + // Reuse the sidecar's guid so the server keeps recognizing the row, and mint + // one only for a deck that has never been synced. + auto guidFor = [&sidecar](const QString &relativePath) { + const DeckSyncRecord record = sidecar.get(relativePath); + return record.guid.isEmpty() ? QUuid::createUuid().toString(QUuid::WithoutBraces) : record.guid; + }; - QMap serverPathById; - QMap serverPathByGuid; + QMap serverIdByPath; + QMap serverIdByGuid; for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) { - serverPathById.insert(sit.value().id, sit.key()); + serverIdByPath.insert(sit.value().path, sit.key()); if (!sit.value().guid.isEmpty()) { - serverPathByGuid.insert(sit.value().guid, sit.key()); + serverIdByGuid.insert(sit.value().guid, sit.key()); } } - QSet matchedServerPaths; + QSet matchedServerIds; + + // A local file without a usable sidecar record still usually has a server + // counterpart: rows are named after the deck, so compare content rather than + // file names, and only upload when no unmatched row has the same content. + auto unmatchedByHash = [&serverFiles, &matchedServerIds](const QString &hash) { + if (hash.isEmpty()) { + return -1; + } + for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) { + if (!matchedServerIds.contains(sit.key()) && sit.value().hash == hash) { + return sit.key(); + } + } + return -1; + }; for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { const DeckSyncRecord sidecarRecord = sidecar.get(it.key()); - QString serverPath; - if (sidecarRecord.remoteId != -1 && serverPathById.contains(sidecarRecord.remoteId)) { - serverPath = serverPathById.value(sidecarRecord.remoteId); - } else if (!sidecarRecord.guid.isEmpty() && serverPathByGuid.contains(sidecarRecord.guid)) { - serverPath = serverPathByGuid.value(sidecarRecord.guid); + int serverId = -1; + if (sidecarRecord.remoteId != -1 && serverFiles.contains(sidecarRecord.remoteId)) { + serverId = sidecarRecord.remoteId; + } else if (!sidecarRecord.guid.isEmpty() && serverIdByGuid.contains(sidecarRecord.guid)) { + serverId = serverIdByGuid.value(sidecarRecord.guid); } else { - serverPath = serverPathFor(it.key()); - if (!serverFiles.contains(serverPath)) { - UploadOp op; - op.relativePath = it.key(); - op.serverPath = serverPath; - op.deckId = -1; - op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() - ? sidecar.get(it.key()).guid - : QUuid::createUuid().toString(QUuid::WithoutBraces); - pendingUploads.append(op); - continue; + // Content first: a row with the same deck is unambiguous, while a + // matching file name may belong to a different deck entirely. + serverId = unmatchedByHash(it.value().hash); + if (serverId == -1) { + const QString wantedPath = serverPathFor(it.key()); + if (serverIdByPath.contains(wantedPath)) { + const int pathId = serverIdByPath.value(wantedPath); + if (!matchedServerIds.contains(pathId)) { + serverId = pathId; + } + } } } - auto serverIt = serverFiles.find(serverPath); - if (serverIt == serverFiles.end()) { - UploadOp op; - op.relativePath = it.key(); - op.serverPath = serverPath; - op.deckId = -1; - op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() - ? sidecar.get(it.key()).guid - : QUuid::createUuid().toString(QUuid::WithoutBraces); - pendingUploads.append(op); + + if (serverId == -1) { + const QString serverPath = serverPathFor(it.key()); + pendingUploads.append(UploadOp{.relativePath = it.key(), + .serverPath = serverPath, + .serverDir = parentDir(serverPath), + .deckId = -1, + .guid = guidFor(it.key())}); continue; } - matchedServerPaths.insert(serverIt.key()); - if (it.value().hash == serverIt.value().hash) { + + const ServerFile &server = serverFiles.value(serverId); + matchedServerIds.insert(serverId); + if (it.value().hash == server.hash) { DeckSyncRecord record = sidecar.get(it.key()); if (record.guid.isEmpty()) { - record.guid = serverIt.value().guid; + record.guid = server.guid; } - record.remoteId = serverIt.value().id; + record.remoteId = server.id; record.baseHash = it.value().hash; record.lastSyncSecs = nowSecs; record.deleted = false; @@ -230,44 +283,36 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont const qint64 localSecs = it.value().modified.toSecsSinceEpoch(); // A server row without last_modified predates sync: the local file wins. // Otherwise strictly newer wins; ties go to the server. - if (serverIt.value().lastModifiedSecs == 0 || localSecs > serverIt.value().lastModifiedSecs) { - UploadOp op; - op.relativePath = it.key(); - op.serverPath = serverPath; - op.deckId = serverIt.value().id; - op.guid = serverIt.value().guid; - pendingUploads.append(op); + if (server.lastModifiedSecs == 0 || localSecs > server.lastModifiedSecs) { + pendingUploads.append(UploadOp{.relativePath = it.key(), + .serverPath = server.path, + .serverDir = server.dir, + .deckId = server.id, + .guid = server.guid}); } else { - DownloadOp op; - op.relativePath = it.key(); - op.deckId = serverIt.value().id; - op.guid = serverIt.value().guid; - op.hash = serverIt.value().hash; - op.lastModifiedSecs = serverIt.value().lastModifiedSecs; - pendingDownloads.append(op); + pendingDownloads.append(DownloadOp{.relativePath = it.key(), + .deckId = server.id, + .guid = server.guid, + .hash = server.hash, + .lastModifiedSecs = server.lastModifiedSecs}); } } for (auto it = serverFiles.begin(); it != serverFiles.end(); ++it) { - if (matchedServerPaths.contains(it.key())) { - continue; - } - const QString wantedPath = it.key().endsWith(".cod", Qt::CaseInsensitive) ? it.key() : it.key() + ".cod"; - if (localFiles.contains(wantedPath) || QFileInfo(deckPath + "/" + wantedPath).exists()) { + if (matchedServerIds.contains(it.key())) { continue; } + const QString wantedPath = localPathFor(it.value().dir, it.value().name); const QString relativePath = uniqueLocalPath(wantedPath); if (relativePath.isEmpty()) { ++skippedCount; continue; } - DownloadOp op; - op.relativePath = relativePath; - op.deckId = it.value().id; - op.guid = it.value().guid; - op.hash = it.value().hash; - op.lastModifiedSecs = it.value().lastModifiedSecs; - pendingDownloads.append(op); + pendingDownloads.append(DownloadOp{.relativePath = relativePath, + .deckId = it.value().id, + .guid = it.value().guid, + .hash = it.value().hash, + .lastModifiedSecs = it.value().lastModifiedSecs}); } sidecar.save(); @@ -297,7 +342,7 @@ void DeckSyncManager::processNext() void DeckSyncManager::processNextUpload() { const UploadOp op = pendingUploads.first(); - const QString dir = parentDir(op.serverPath); + const QString dir = op.serverDir; if (!ensuredDirs.contains(dir)) { sendMkDirFor(dir); return; @@ -370,7 +415,7 @@ void DeckSyncManager::sendUpload(const UploadOp &op) Command_DeckUpload cmd; if (op.deckId == -1) { - cmd.set_path(parentDir(op.serverPath).toStdString()); + cmd.set_path(op.serverDir.toStdString()); } else { cmd.set_deck_id(op.deckId); } @@ -416,7 +461,7 @@ void DeckSyncManager::onUploadFinished(const Response &response, } else { record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); } - record.remoteId = file.id(); + record.remoteId = static_cast(file.id()); } else if (localFiles.contains(relativePath)) { record.baseHash = localFiles.value(relativePath).hash; record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); @@ -517,7 +562,7 @@ void DeckSyncManager::onDownloadFinished(const Response &response, if (!download.hash().empty()) { record.baseHash = QString::fromStdString(download.hash()); } else { - record.baseHash = deck.getDeckHash(); + record.baseHash = deck.getContentHash(); } record.lastSyncSecs = download.has_last_modified() ? static_cast(download.last_modified()) : QDateTime::currentDateTime().toSecsSinceEpoch(); diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 8ecbaf74c..c891cbb58 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -30,9 +30,11 @@ class ServerInfo_DeckStorage_Folder; * Each syncNow() run loads the profile sidecar, scans the deck folder, fetches * the server tree and mirrors both sides: unknown local decks upload, unknown * server decks download, and decks changed on both sides resolve newer-wins - * (ties go to the server). Matching is path-based; a rename looks like a - * delete plus an add. Before a download overwrites a locally modified file, the - * local content is copied to a .sync-backup file next to it. + * (ties go to the server). A local file is matched to its server row by the + * sidecar's row id, then guid, then the server path, and finally by content, so + * renaming either side does not duplicate a deck. Before a download overwrites a + * locally modified file, the local content is copied to a .sync-backup file next + * to it. * * Ownership: fire-and-forget per run, driven by PendingCommand round trips. * No watcher, no auto triggers, no conflict UI — those arrive in later layers. @@ -81,6 +83,9 @@ private: struct ServerFile { int id = -1; + QString path; ///< Server-side address: folder prefix plus row name. + QString dir; ///< Folder prefix, empty in the root. + QString name; ///< Row name exactly as the server stores it. QString guid; QString hash; qint64 lastModifiedSecs = 0; @@ -94,7 +99,8 @@ private: struct UploadOp { QString relativePath; - QString serverPath; + QString serverPath; ///< Target address of a brand-new row. + QString serverDir; ///< Folder that must exist before the upload. int deckId = -1; QString guid; }; @@ -112,7 +118,7 @@ private: [[nodiscard]] SyncSummary currentSummary() const; void collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, const QString &prefix, - QMap &files, + QMap &files, QSet &dirs); void processNext(); void processNextUpload(); diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp index 821b53dff..6f1a358f2 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp @@ -9,6 +9,7 @@ #include "tree/inner_deck_list_node.h" #include +#include #include #include #include @@ -453,6 +454,16 @@ void DeckList::refreshDeckHash() cachedDeckHash = QString(); } +QString DeckList::contentHash(const QString &nativeDeck) +{ + return QString::fromLatin1(QCryptographicHash::hash(nativeDeck.toUtf8(), QCryptographicHash::Sha1).toHex()); +} + +QString DeckList::getContentHash() const +{ + return contentHash(writeToString_Native()); +} + /** * Calls a given function on each card in the deck. */ diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h index 6796e9c33..9a8b54c24 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h @@ -289,6 +289,16 @@ public: /** @name Deck identity */ ///@{ QString getDeckHash() const; + /** + * @brief SHA-1 of a native deck string, covering everything the card hash + * ignores (name, printings, banner, tokens, comments, sideboard plans). + * + * Deck-collection sync hashes the exact string it sends or receives, so both + * peers compare the same bytes. + */ + static QString contentHash(const QString &nativeDeck); + /** @brief contentHash() of this deck's native serialisation. */ + QString getContentHash() const; void refreshDeckHash(); ///@} diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index e21b3b10f..f9e602f82 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -1114,7 +1114,9 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckUpload(const Comman const QString bannerCardProvider = deck.getBannerCard().providerId.left(32); const QString tagsJson = serializeDeckTags(deck.getTags()); const QString colorIdentity = sanitizeColorIdentity(nameFromStdString(cmd.color_identity())); - const QString deckHash = deck.getDeckHash(); + // Hash the exact bytes the client sent: both peers must compare the same + // string, not each side's re-serialisation of it. + const QString deckHash = DeckList::contentHash(deckStr); QString guid = cmd.has_guid() ? nameFromStdString(cmd.guid()).left(36) : QString(); if (QUuid::fromString(guid).isNull()) { guid = QUuid::createUuid().toString(QUuid::WithoutBraces); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 32ee9b7f1..5163db067 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -416,6 +416,15 @@ cockatrice_add_test( libcockatrice_deck_list libcockatrice_utility ) +cockatrice_add_test( + NAME + deck_content_hash_test + SOURCES + deck_content_hash_test.cpp + LIBS + libcockatrice_deck_list + libcockatrice_utility +) cockatrice_add_test( NAME deck_sync_profile_test diff --git a/tests/deck_content_hash_test.cpp b/tests/deck_content_hash_test.cpp new file mode 100644 index 000000000..192abb007 --- /dev/null +++ b/tests/deck_content_hash_test.cpp @@ -0,0 +1,51 @@ +#include "gtest/gtest.h" +#include +#include + +namespace +{ +QString deckWithName(const QString &name) +{ + return QStringLiteral(R"()" + R"(%1)" + R"()" + R"()" + R"()") + .arg(name); +} +} // namespace + +TEST(DeckContentHashTest, MatchesNativeSerialisation) +{ + DeckList deck; + ASSERT_TRUE(deck.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + EXPECT_EQ(deck.getContentHash(), DeckList::contentHash(deck.writeToString_Native())); +} + +TEST(DeckContentHashTest, IsStableAcrossLoads) +{ + DeckList first; + DeckList second; + ASSERT_TRUE(first.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + ASSERT_TRUE(second.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + EXPECT_EQ(first.getContentHash(), second.getContentHash()); +} + +TEST(DeckContentHashTest, CoversDeckName) +{ + DeckList burn; + DeckList renamed; + ASSERT_TRUE(burn.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + ASSERT_TRUE(renamed.loadFromString_Native(deckWithName(QStringLiteral("Burn v2")))); + + // The card hash only looks at card names and counts, so it cannot tell the + // two decks apart; the content hash must. + EXPECT_EQ(burn.getDeckHash(), renamed.getDeckHash()); + EXPECT_NE(burn.getContentHash(), renamed.getContentHash()); +} + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From 4e5c4403b2097fdb6ac33c00bb5c55faa2b7b085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 11 Oct 2026 16:05:22 +0200 Subject: [PATCH 14/16] [Server+Client] Tombstone deck deletes with purge and base snapshots --- .../interface/deck_sync/deck_sync_manager.cpp | 117 +++++++++++++++++- .../interface/deck_sync/deck_sync_manager.h | 9 ++ servatrice/servatrice.ini.example | 7 ++ servatrice/src/servatrice.cpp | 11 ++ servatrice/src/servatrice.h | 2 + .../src/servatrice_database_interface.cpp | 17 ++- .../src/servatrice_database_interface.h | 5 + servatrice/src/serversocketinterface.cpp | 33 ++--- 8 files changed, 180 insertions(+), 21 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index dd4dd6f69..7771c050a 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -114,6 +115,7 @@ void DeckSyncManager::syncNow() skippedCount = 0; pendingUploads.clear(); pendingDownloads.clear(); + pendingDeletes.clear(); ensuredDirs.clear(); ensuredDirs.insert(QString()); localFiles.clear(); @@ -254,8 +256,21 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont } } } - if (serverId == -1) { + if (sidecarRecord.remoteId != -1) { + // The server no longer lists this deck: it was deleted elsewhere. + // Apply the delete locally, keeping a backup of the file. + const QString absolutePath = deckPath + "/" + it.key(); + const QString backupPath = + absolutePath + BACKUP_SUFFIX + QString::number(QDateTime::currentDateTime().toSecsSinceEpoch()); + if (!QFile::copy(absolutePath, backupPath) || !QFile::remove(absolutePath)) { + ++skippedCount; + continue; + } + sidecar.remove(it.key()); + ++downloadedCount; + continue; + } const QString serverPath = serverPathFor(it.key()); pendingUploads.append(UploadOp{.relativePath = it.key(), .serverPath = serverPath, @@ -277,6 +292,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont record.lastSyncSecs = nowSecs; record.deleted = false; sidecar.set(it.key(), record); + snapshotLocalFile(it.key(), record.guid); ++unchangedCount; continue; } @@ -315,6 +331,18 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont .lastModifiedSecs = it.value().lastModifiedSecs}); } + const QMap knownRecords = sidecar.records(); + for (auto it = knownRecords.begin(); it != knownRecords.end(); ++it) { + if (localFiles.contains(it.key())) { + continue; + } + if (it.value().remoteId == -1) { + sidecar.remove(it.key()); + continue; + } + pendingDeletes.append(it.key()); + } + sidecar.save(); processNext(); } @@ -330,7 +358,12 @@ void DeckSyncManager::processNext() if (!syncing) { return; } - if (!pendingUploads.isEmpty()) { + if (!pendingDeletes.isEmpty()) { + const QString relativePath = pendingDeletes.first(); + DeckSyncSidecar sidecar(sidecarPath); + sidecar.load(); + sendDelete(relativePath, sidecar.get(relativePath).remoteId); + } else if (!pendingUploads.isEmpty()) { processNextUpload(); } else if (!pendingDownloads.isEmpty()) { sendDownload(pendingDownloads.first()); @@ -425,7 +458,12 @@ void DeckSyncManager::sendUpload(const UploadOp &op) cmd.set_color_identity(colorIdentityProvider(loaded->deckList).toStdString()); } PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); - pend->setExtraData(op.relativePath); + QVariantMap context; + context.insert("path", op.relativePath); + context.insert("serverPath", op.serverPath); + context.insert("deckId", op.deckId); + context.insert("guid", op.guid); + pend->setExtraData(context); connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onUploadFinished); client->sendCommand(pend); } @@ -437,8 +475,22 @@ void DeckSyncManager::onUploadFinished(const Response &response, if (!syncing) { return; } - const QString relativePath = extraData.toString(); + const QVariantMap context = extraData.toMap(); + const QString relativePath = context.value("path").toString(); + const int deckId = context.value("deckId", -1).toInt(); if (response.response_code() != Response::RespOk) { + if (response.response_code() == Response::RespNameNotFound && deckId != -1) { + // The server row is gone (purged tombstone or removed elsewhere): + // upload the deck as new instead of updating a dead id. + UploadOp retry; + retry.relativePath = relativePath; + retry.serverPath = context.value("serverPath").toString(); + retry.deckId = -1; + retry.guid = context.value("guid").toString(); + pendingUploads.prepend(retry); + processNext(); + return; + } ++skippedCount; processNext(); return; @@ -469,6 +521,7 @@ void DeckSyncManager::onUploadFinished(const Response &response, record.deleted = false; sidecar.set(relativePath, record); sidecar.save(); + snapshotLocalFile(relativePath, record.guid); ++uploadedCount; processNext(); } @@ -569,10 +622,66 @@ void DeckSyncManager::onDownloadFinished(const Response &response, record.deleted = false; sidecar.set(relativePath, record); sidecar.save(); + snapshotLocalFile(relativePath, record.guid); ++downloadedCount; processNext(); } +void DeckSyncManager::sendDelete(const QString &relativePath, int deckId) +{ + emit statusMessage(tr("Deleting %1 from server...").arg(relativePath)); + Command_DeckDel cmd; + cmd.set_deck_id(deckId); + PendingCommand *pend = AbstractClient::prepareSessionCommand(cmd); + pend->setExtraData(relativePath); + connect(pend, &PendingCommand::finished, this, &DeckSyncManager::onDeleteFinished); + client->sendCommand(pend); +} + +void DeckSyncManager::onDeleteFinished(const Response &response, + const CommandContainer & /*commandContainer*/, + const QVariant &extraData) +{ + if (!syncing) { + return; + } + const QString relativePath = extraData.toString(); + for (int i = 0; i < pendingDeletes.size(); ++i) { + if (pendingDeletes.at(i) == relativePath) { + pendingDeletes.removeAt(i); + break; + } + } + // Both outcomes converge the record: pushed deletes and server-side + // deletes (already tombstoned, answering NameNotFound) are done. + if (response.response_code() == Response::RespOk || response.response_code() == Response::RespNameNotFound) { + DeckSyncSidecar sidecar(sidecarPath); + sidecar.load(); + sidecar.remove(relativePath); + sidecar.save(); + } else { + ++skippedCount; + } + processNext(); +} + +void DeckSyncManager::snapshotLocalFile(const QString &relativePath, const QString &guid) +{ + if (guid.isEmpty()) { + return; + } + QFile source(deckPath + "/" + relativePath); + if (!source.open(QIODevice::ReadOnly)) { + return; + } + QDir().mkpath(deckPath + "/" + SYNC_META_DIR + "/bases"); + QFile snapshot(deckPath + "/" + SYNC_META_DIR + "/bases/" + guid + ".cod"); + if (!snapshot.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + return; + } + snapshot.write(source.readAll()); +} + void DeckSyncManager::failSync(const QString &error) { syncing = false; diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index c891cbb58..4d61c0d0a 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -38,6 +38,10 @@ class ServerInfo_DeckStorage_Folder; * * Ownership: fire-and-forget per run, driven by PendingCommand round trips. * No watcher, no auto triggers, no conflict UI — those arrive in later layers. + * + * Deletes are tombstones, never silent: a locally deleted deck is pushed with + * Command_DeckDel, a server-side delete is applied locally only after copying + * the file to a .sync-backup, and resurrection reuses the stored deck id. */ class DeckSyncManager : public QObject { @@ -78,6 +82,8 @@ private slots: onUploadFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); void onDownloadFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); + void + onDeleteFinished(const Response &response, const CommandContainer &commandContainer, const QVariant &extraData); private: struct ServerFile @@ -125,6 +131,8 @@ private: void sendMkDirFor(const QString &dir); void sendUpload(const UploadOp &op); void sendDownload(const DownloadOp &op); + void sendDelete(const QString &relativePath, int deckId); + void snapshotLocalFile(const QString &relativePath, const QString &guid); static QString parentDir(const QString &path); QString uniqueLocalPath(const QString &relativePath) const; @@ -139,6 +147,7 @@ private: QSet ensuredDirs; QList pendingUploads; QList pendingDownloads; + QList pendingDeletes; int uploadedCount = 0; int downloadedCount = 0; int unchangedCount = 0; diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index 4a26ba1d9..08046a489 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -598,3 +598,10 @@ max_shares_per_day=50 ; Minimum enforced: 1. ; Default: 20 max_versions=20 + +; How many days a deleted deck stays restorable as a tombstone before the +; cleanup clock purges it (with its versions) for good. Re-uploading the +; same deck within this window resurrects it instead of creating a copy. +; A value of 0 purges tombstones on the next cleanup run. +; Default: 30 +tombstone_days=30 diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index feb178b08..f5a1f0e93 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -622,6 +622,7 @@ bool Servatrice::initServer() deckShareCleanupClock = new QTimer(this); connect(deckShareCleanupClock, SIGNAL(timeout()), this, SLOT(cleanupExpiredDeckShares())); + connect(deckShareCleanupClock, SIGNAL(timeout()), this, SLOT(purgeDeckTombstones())); const int deckShareCleanupInterval = getDeckShareCleanupInterval(); if (deckShareCleanupInterval > 0) { qDebug() << "Starting deck share cleanup clock, interval" << deckShareCleanupInterval << "ms"; @@ -856,6 +857,11 @@ void Servatrice::cleanupExpiredDeckShares() servatriceDatabaseInterface->cleanupExpiredDeckShares(); } +void Servatrice::purgeDeckTombstones() +{ + servatriceDatabaseInterface->purgeDeckTombstones(getDeckSyncTombstoneDays()); +} + void Servatrice::statusUpdate() { if (!servatriceDatabaseInterface->checkSql()) { @@ -1401,6 +1407,11 @@ int Servatrice::getDeckSyncMaxVersions() const return qMax(1, settingsCache->value("deck_sync/max_versions", 20).toInt()); } +int Servatrice::getDeckSyncTombstoneDays() const +{ + return qMax(0, settingsCache->value("deck_sync/tombstone_days", 30).toInt()); +} + int Servatrice::getNumberOfTCPPools() const { return settingsCache->value("server/number_pools", 1).toInt(); diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 3be364aa3..f925c0984 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -166,6 +166,7 @@ private slots: void sweepLegacyBackups(); void shutdownTimeout(); void cleanupExpiredDeckShares(); + void purgeDeckTombstones(); protected: void doSendIslMessage(const IslMessage &msg, int _serverId) override; @@ -321,6 +322,7 @@ public: int getDeckShareMaxDecksPerShare() const; int getDeckShareMaxSharesPerDay() const; int getDeckSyncMaxVersions() const; + int getDeckSyncTombstoneDays() const; int getMaxUsersPerAddress() const; /// Raw security/trusted_sources list; match addresses against it with isTrustedSource(). QString getTrustedSources() const; diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index b5c45065c..8d5459a88 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -1370,8 +1370,8 @@ DeckList *Servatrice_DatabaseInterface::getDeckFromDatabase(int deckId, int user { checkSql(); - QSqlQuery *query = - prepareQuery("select content from {prefix}_decklist_files where id = :id and id_user = :id_user"); + QSqlQuery *query = prepareQuery( + "select content from {prefix}_decklist_files where id = :id and id_user = :id_user and deleted = 0"); query->bindValue(":id", deckId); query->bindValue(":id_user", userId); execSqlQuery(query); @@ -1522,6 +1522,19 @@ void Servatrice_DatabaseInterface::cleanupExpiredDeckShares() execSqlQuery(query); } +void Servatrice_DatabaseInterface::purgeDeckTombstones(int olderThanDays) +{ + checkSql(); + + // DB-native clock: tombstones are stamped with NOW() on this same + // database, so the grace window is immune to app/db clock skew. + // The null arm only catches manually flagged rows. + QSqlQuery *query = prepareQuery("delete from {prefix}_decklist_files where deleted = 1 and (last_modified < " + "DATE_SUB(NOW(), INTERVAL :days DAY) or last_modified is null)"); + query->bindValue(":days", qMax(0, olderThanDays)); + execSqlQuery(query); +} + bool Servatrice_DatabaseInterface::getDeckSharesForUser(int userId, QList &shares) { checkSql(); diff --git a/servatrice/src/servatrice_database_interface.h b/servatrice/src/servatrice_database_interface.h index d0fd8bd12..04005e160 100644 --- a/servatrice/src/servatrice_database_interface.h +++ b/servatrice/src/servatrice_database_interface.h @@ -152,6 +152,11 @@ public: bool getDeckShareItem(const QString &token, int itemId, QString &content); /** @brief Deletes all expired share bundles (cascades to their items). */ void cleanupExpiredDeckShares(); + /** + * @brief Hard-deletes tombstoned decks older than the grace period (cascades to versions). + * @param olderThanDays Tombstones older than this many days are purged; 0 purges all tombstones. + */ + void purgeDeckTombstones(int olderThanDays); int getNextGameId() override; int getNextReplayId() override; diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index f9e602f82..36099e394 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -709,7 +709,8 @@ bool AbstractServerSocketInterface::deckListHelper(int folderId, query = sqlInterface->prepareQuery("select id, name, upload_time, is_public, banner_card_name, " "banner_card_provider, color_identity, tags, guid, hash, last_modified from " - "{prefix}_decklist_files where id_folder = :id_folder and id_user = :id_user"); + "{prefix}_decklist_files where id_folder = :id_folder and id_user = :id_user " + "and deleted = 0"); query->bindValue(":id_folder", folderId); query->bindValue(":id_user", userId); if (!sqlInterface->execSqlQuery(query)) { @@ -854,8 +855,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckSetVisibility(const sqlInterface->checkSql(); if (cmd.has_deck_id()) { - QSqlQuery *query = - sqlInterface->prepareQuery("select 1 from {prefix}_decklist_files where id = :id and id_user = :id_user"); + QSqlQuery *query = sqlInterface->prepareQuery( + "select 1 from {prefix}_decklist_files where id = :id and id_user = :id_user and deleted = 0"); query->bindValue(":id", cmd.deck_id()); query->bindValue(":id_user", userInfo->id()); sqlInterface->execSqlQuery(query); @@ -965,7 +966,8 @@ void AbstractServerSocketInterface::deckDelDirHelper(int basePathId) deckDelDirHelper(query->value(0).toInt()); } - query = sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id_folder = :id_folder"); + query = sqlInterface->prepareQuery("update {prefix}_decklist_files set deleted = 1, last_modified = NOW() " + "where id_folder = :id_folder and deleted = 0"); query->bindValue(":id_folder", basePathId); sqlInterface->execSqlQuery(query); @@ -1016,18 +1018,17 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckDel(const Command_D sqlInterface->checkSql(); QSqlQuery *query = - sqlInterface->prepareQuery("select id from {prefix}_decklist_files where id = :id and id_user = :id_user"); + sqlInterface->prepareQuery("update {prefix}_decklist_files set deleted = 1, last_modified = NOW() where id = " + ":id and id_user = :id_user and deleted = 0"); query->bindValue(":id", cmd.deck_id()); query->bindValue(":id_user", userInfo->id()); - sqlInterface->execSqlQuery(query); - if (!query->next()) { + if (!sqlInterface->execSqlQuery(query)) { + return Response::RespContextError; + } + if (query->numRowsAffected() == 0) { return Response::RespNameNotFound; } - query = sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id = :id"); - query->bindValue(":id", cmd.deck_id()); - sqlInterface->execSqlQuery(query); - return Response::RespOk; } @@ -1197,9 +1198,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckUpload(const Comman } const QString oldHash = currentQuery->value(1).toString(); - QString updateQuery = "update {prefix}_decklist_files set name=:name, upload_time=NOW(), content=:content, " - "banner_card_name=:banner_card_name, banner_card_provider=:banner_card_provider, " - "color_identity=:color_identity, tags=:tags, guid=:guid, hash=:hash, last_modified=NOW()"; + QString updateQuery = + "update {prefix}_decklist_files set name=:name, upload_time=NOW(), content=:content, " + "banner_card_name=:banner_card_name, banner_card_provider=:banner_card_provider, " + "color_identity=:color_identity, tags=:tags, guid=:guid, hash=:hash, last_modified=NOW(), " + "deleted=0"; if (cmd.has_is_public()) { updateQuery += ", is_public=:is_public"; } @@ -1401,7 +1404,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckShareCreate(const C // issues its own query on the same cached statement set. QSqlQuery *query = sqlInterface->prepareQuery("select id, color_identity from {prefix}_decklist_files where id_folder = " - ":id_folder and id_user = :id_user"); + ":id_folder and id_user = :id_user and deleted = 0"); query->bindValue(":id_folder", folderId); query->bindValue(":id_user", userInfo->id()); if (!sqlInterface->execSqlQuery(query)) { From f8cd3126f97b095ab1d855315226ccff7c13e565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 07:19:32 +0200 Subject: [PATCH 15/16] Fix ZeldaZach review for PR 7494: match by remoteId/guid before remote-delete, only tombstone when file truly missing --- cockatrice/src/interface/deck_sync/deck_sync_manager.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 7771c050a..43a0c10e4 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -336,6 +336,9 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont if (localFiles.contains(it.key())) { continue; } + if (QFileInfo::exists(deckPath + "/" + it.key())) { + continue; + } if (it.value().remoteId == -1) { sidecar.remove(it.key()); continue; From a94db4171bbbed38e9d0a483b40c5cb141d95c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 1 Nov 2026 13:21:05 +0100 Subject: [PATCH 16/16] [DeckSync] Address tombstone review: remote deletes, revival and purge clock - Treat a record whose server row is gone as a remote delete only when the local file still matches the sidecar's base hash. An edit made after the remote delete is newer than the delete, so it uploads and revives the row instead of being removed. - Revive a tombstone with the same user and guid in cmdDeckUpload instead of inserting a second row next to it, so the deck keeps its id and version history and the promise in servatrice.ini.example holds. - Run tombstone purging on its own clock, driven by the new [deck_sync] purge_interval option, so disabling share cleanup no longer disables purging silently. - Write the base snapshot through QSaveFile and log a failed commit, so a crash or a full disk cannot leave a truncated base for DeckDiff::merge(). --- .../interface/deck_sync/deck_sync_manager.cpp | 22 ++++- servatrice/servatrice.ini.example | 12 ++- servatrice/src/servatrice.cpp | 17 +++- servatrice/src/servatrice.h | 2 + servatrice/src/serversocketinterface.cpp | 93 ++++++++++++++----- 5 files changed, 112 insertions(+), 34 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 43a0c10e4..126bbca77 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -3,10 +3,12 @@ #include "../deck_loader/deck_loader.h" #include "deck_sync_sidecar.h" +#include #include #include #include #include +#include #include #include #include @@ -257,9 +259,14 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont } } if (serverId == -1) { - if (sidecarRecord.remoteId != -1) { - // The server no longer lists this deck: it was deleted elsewhere. - // Apply the delete locally, keeping a backup of the file. + // The sidecar knows a server row that the listing no longer has: it was + // deleted elsewhere. Only mirror that delete when the local file is + // still exactly what was synced - an edit made after the remote delete + // is newer than the delete and gets uploaded again. + const bool knownToServer = sidecarRecord.remoteId != -1 || !sidecarRecord.guid.isEmpty(); + const bool unchangedSinceSync = + !sidecarRecord.baseHash.isEmpty() && it.value().hash == sidecarRecord.baseHash; + if (knownToServer && unchangedSinceSync) { const QString absolutePath = deckPath + "/" + it.key(); const QString backupPath = absolutePath + BACKUP_SUFFIX + QString::number(QDateTime::currentDateTime().toSecsSinceEpoch()); @@ -678,11 +685,16 @@ void DeckSyncManager::snapshotLocalFile(const QString &relativePath, const QStri return; } QDir().mkpath(deckPath + "/" + SYNC_META_DIR + "/bases"); - QFile snapshot(deckPath + "/" + SYNC_META_DIR + "/bases/" + guid + ".cod"); - if (!snapshot.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + // QSaveFile: a crash or a full disk must not leave a truncated base behind, + // because DeckDiff::merge() against a partial base re-adds or drops cards. + QSaveFile snapshot(deckPath + "/" + SYNC_META_DIR + "/bases/" + guid + ".cod"); + if (!snapshot.open(QIODevice::WriteOnly)) { return; } snapshot.write(source.readAll()); + if (!snapshot.commit()) { + qWarning() << "DeckSync: could not write the base snapshot for" << relativePath; + } } void DeckSyncManager::failSync(const QString &error) diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index 08046a489..5edb38695 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -600,8 +600,14 @@ max_shares_per_day=50 max_versions=20 ; How many days a deleted deck stays restorable as a tombstone before the -; cleanup clock purges it (with its versions) for good. Re-uploading the -; same deck within this window resurrects it instead of creating a copy. -; A value of 0 purges tombstones on the next cleanup run. +; purge clock removes it (with its versions) for good. Re-uploading the +; same deck within this window revives the tombstone instead of creating a copy. +; A value of 0 purges tombstones on the next purge run. ; Default: 30 tombstone_days=30 + +; How often, in minutes, the tombstone purge runs. This is independent of +; [deck_share] cleanup_interval, so disabling share cleanup does not disable +; purging. A value of 0 disables purging entirely. +; Default: 60 +purge_interval=60 diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index f5a1f0e93..34e5573c0 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -622,7 +622,6 @@ bool Servatrice::initServer() deckShareCleanupClock = new QTimer(this); connect(deckShareCleanupClock, SIGNAL(timeout()), this, SLOT(cleanupExpiredDeckShares())); - connect(deckShareCleanupClock, SIGNAL(timeout()), this, SLOT(purgeDeckTombstones())); const int deckShareCleanupInterval = getDeckShareCleanupInterval(); if (deckShareCleanupInterval > 0) { qDebug() << "Starting deck share cleanup clock, interval" << deckShareCleanupInterval << "ms"; @@ -633,6 +632,16 @@ bool Servatrice::initServer() connect(legacyBackupSweepClock, &QTimer::timeout, this, &Servatrice::sweepLegacyBackups); legacyBackupSweepClock->start(kLegacyBackupSweepIntervalMs); + // Tombstone purging runs on its own clock: turning share cleanup off must not + // silently disable it as well. + deckTombstoneClock = new QTimer(this); + connect(deckTombstoneClock, SIGNAL(timeout()), this, SLOT(purgeDeckTombstones())); + const int deckSyncPurgeInterval = getDeckSyncPurgeInterval(); + if (deckSyncPurgeInterval > 0) { + qDebug() << "Starting deck tombstone purge clock, interval" << deckSyncPurgeInterval << "ms"; + deckTombstoneClock->start(deckSyncPurgeInterval); + } + // SOCKET SERVER if (getNumberOfTCPPools() > 0) { gameServer = @@ -1412,6 +1421,12 @@ int Servatrice::getDeckSyncTombstoneDays() const return qMax(0, settingsCache->value("deck_sync/tombstone_days", 30).toInt()); } +int Servatrice::getDeckSyncPurgeInterval() const +{ + // default: every 60 minutes, independent of deck_share/cleanup_interval + return qMax(0, settingsCache->value("deck_sync/purge_interval", 60).toInt()) * 60000; +} + int Servatrice::getNumberOfTCPPools() const { return settingsCache->value("server/number_pools", 1).toInt(); diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index f925c0984..ed8d82ddb 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -182,6 +182,7 @@ private: QTimer *pingClock, *statusUpdateClock; QTimer *deckShareCleanupClock; QTimer *legacyBackupSweepClock; + QTimer *deckTombstoneClock; Servatrice_GameServer *gameServer; Servatrice_WebsocketGameServer *websocketGameServer; Servatrice_IslServer *islServer; @@ -323,6 +324,7 @@ public: int getDeckShareMaxSharesPerDay() const; int getDeckSyncMaxVersions() const; int getDeckSyncTombstoneDays() const; + int getDeckSyncPurgeInterval() const; int getMaxUsersPerAddress() const; /// Raw security/trusted_sources list; match addresses against it with isTrustedSource(). QString getTrustedSources() const; diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 36099e394..e88c74527 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -1129,31 +1129,71 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckUpload(const Comman return Response::RespNameNotFound; } - QSqlQuery *query = - sqlInterface->prepareQuery("insert into {prefix}_decklist_files (id_folder, id_user, name, upload_time, " - "content, is_public, banner_card_name, banner_card_provider, color_identity, " - "tags, guid, hash, last_modified) values(:id_folder, :id_user, :name, NOW(), " - ":content, :is_public, :banner_card_name, :banner_card_provider, " - ":color_identity, :tags, :guid, :hash, NOW())"); - query->bindValue(":id_folder", folderId); - query->bindValue(":id_user", userInfo->id()); - query->bindValue(":name", deckName); - query->bindValue(":content", deckStr); - query->bindValue(":is_public", cmd.has_is_public() && cmd.is_public() ? 1 : 0); - query->bindValue(":banner_card_name", bannerCardName); - query->bindValue(":banner_card_provider", bannerCardProvider); - query->bindValue(":color_identity", colorIdentity); - query->bindValue(":tags", tagsJson); - query->bindValue(":guid", guid); - query->bindValue(":hash", deckHash); - if (!sqlInterface->execSqlQuery(query)) { + // A deck that was deleted elsewhere leaves a tombstone behind. Revive it + // instead of inserting a second row with the same guid, which would split + // the deck's version history and leave the tombstone to be purged. + int deckId = -1; + bool inserted = false; + QSqlQuery *tombstoneQuery = + sqlInterface->prepareQuery("select id from {prefix}_decklist_files where id_user = :id_user and guid = " + ":guid and deleted = 1 order by last_modified desc limit 1"); + tombstoneQuery->bindValue(":id_user", userInfo->id()); + tombstoneQuery->bindValue(":guid", guid); + if (!sqlInterface->execSqlQuery(tombstoneQuery)) { return Response::RespContextError; } - const int deckId = query->lastInsertId().toInt(); + if (tombstoneQuery->next()) { + deckId = tombstoneQuery->value(0).toInt(); + QSqlQuery *reviveQuery = sqlInterface->prepareQuery( + "update {prefix}_decklist_files set id_folder=:id_folder, name=:name, upload_time=NOW(), " + "content=:content, is_public=:is_public, banner_card_name=:banner_card_name, " + "banner_card_provider=:banner_card_provider, color_identity=:color_identity, tags=:tags, " + "hash=:hash, last_modified=NOW(), deleted=0 where id=:id and id_user=:id_user"); + reviveQuery->bindValue(":id_folder", folderId); + reviveQuery->bindValue(":id_user", userInfo->id()); + reviveQuery->bindValue(":id", deckId); + reviveQuery->bindValue(":name", deckName); + reviveQuery->bindValue(":content", deckStr); + reviveQuery->bindValue(":is_public", cmd.has_is_public() && cmd.is_public() ? 1 : 0); + reviveQuery->bindValue(":banner_card_name", bannerCardName); + reviveQuery->bindValue(":banner_card_provider", bannerCardProvider); + reviveQuery->bindValue(":color_identity", colorIdentity); + reviveQuery->bindValue(":tags", tagsJson); + reviveQuery->bindValue(":hash", deckHash); + if (!sqlInterface->execSqlQuery(reviveQuery)) { + return Response::RespContextError; + } + } else { + QSqlQuery *query = sqlInterface->prepareQuery( + "insert into {prefix}_decklist_files (id_folder, id_user, name, upload_time, " + "content, is_public, banner_card_name, banner_card_provider, color_identity, " + "tags, guid, hash, last_modified) values(:id_folder, :id_user, :name, NOW(), " + ":content, :is_public, :banner_card_name, :banner_card_provider, " + ":color_identity, :tags, :guid, :hash, NOW())"); + query->bindValue(":id_folder", folderId); + query->bindValue(":id_user", userInfo->id()); + query->bindValue(":name", deckName); + query->bindValue(":content", deckStr); + query->bindValue(":is_public", cmd.has_is_public() && cmd.is_public() ? 1 : 0); + query->bindValue(":banner_card_name", bannerCardName); + query->bindValue(":banner_card_provider", bannerCardProvider); + query->bindValue(":color_identity", colorIdentity); + query->bindValue(":tags", tagsJson); + query->bindValue(":guid", guid); + query->bindValue(":hash", deckHash); + if (!sqlInterface->execSqlQuery(query)) { + return Response::RespContextError; + } + deckId = query->lastInsertId().toInt(); + inserted = true; + } if (!storeDeckVersion(deckId, deckStr, deckHash)) { - QSqlQuery *cleanupQuery = sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id = :id"); - cleanupQuery->bindValue(":id", deckId); - sqlInterface->execSqlQuery(cleanupQuery); + if (inserted) { + QSqlQuery *cleanupQuery = + sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id = :id"); + cleanupQuery->bindValue(":id", deckId); + sqlInterface->execSqlQuery(cleanupQuery); + } return Response::RespContextError; } @@ -1163,9 +1203,12 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckUpload(const Comman stampQuery->bindValue(":id", deckId); stampQuery->bindValue(":id_user", userInfo->id()); if (!sqlInterface->execSqlQuery(stampQuery) || !stampQuery->next()) { - QSqlQuery *cleanupQuery = sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id = :id"); - cleanupQuery->bindValue(":id", deckId); - sqlInterface->execSqlQuery(cleanupQuery); + if (inserted) { + QSqlQuery *cleanupQuery = + sqlInterface->prepareQuery("delete from {prefix}_decklist_files where id = :id"); + cleanupQuery->bindValue(":id", deckId); + sqlInterface->execSqlQuery(cleanupQuery); + } return Response::RespContextError; }