From 6eef7a6849bae24071c21b04d92d8e54bb64a531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= <44814898+BruebachL@users.noreply.github.com> Date: Sun, 8 Nov 2026 16:18:10 +0100 Subject: [PATCH] Address review findings --- .../interface/deck_sync/deck_sync_manager.cpp | 64 ++++++++++++++++--- .../interface/deck_sync/deck_sync_manager.h | 1 + .../libcockatrice/deck_list/deck_list.cpp | 16 +++-- .../libcockatrice/deck_list/deck_list.h | 20 ++++-- tests/deck_content_hash_test.cpp | 16 ++++- 5 files changed, 95 insertions(+), 22 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index b095575a9..e5748c512 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -116,6 +116,7 @@ void DeckSyncManager::syncNow() skippedCount = 0; pendingUploads.clear(); pendingDownloads.clear(); + plannedLocalPaths.clear(); ensuredDirs.clear(); ensuredDirs.insert(QString()); localFiles.clear(); @@ -188,6 +189,10 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont if (!syncing) { return; } + if (response.response_code() == Response::RespNotConnected) { + failSync(tr("Server connection lost.")); + return; + } if (response.response_code() != Response::RespOk) { failSync(tr("Could not read server decks.")); return; @@ -219,6 +224,24 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont } } QSet matchedServerIds; + QMap identityByLocal; + + // Pass 1: claim every row a sidecar names before any guess runs. In a single + // pass a content or path match could take the row another local file still + // identifies by id or guid, turning an unchanged deck into a duplicate. + for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { + const DeckSyncRecord sidecarRecord = sidecar.get(it.key()); + 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); + } + if (serverId != -1 && !matchedServerIds.contains(serverId)) { + matchedServerIds.insert(serverId); + identityByLocal.insert(it.key(), serverId); + } + } // 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 @@ -235,14 +258,10 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont return -1; }; + // Pass 2: resolve the remaining files by content, then path, and queue work. for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { - const DeckSyncRecord sidecarRecord = sidecar.get(it.key()); - 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 { + int serverId = identityByLocal.value(it.key(), -1); + if (serverId == -1) { // 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); @@ -310,6 +329,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont ++skippedCount; continue; } + plannedLocalPaths.insert(relativePath); pendingDownloads.append(DownloadOp{.relativePath = relativePath, .deckId = it.value().id, .guid = it.value().guid, @@ -381,6 +401,10 @@ void DeckSyncManager::onEnsureDirFinished(const Response &response, if (!syncing) { return; } + if (response.response_code() == Response::RespNotConnected) { + failSync(tr("Server connection lost.")); + return; + } if (response.response_code() != Response::RespOk) { failSync(tr("Could not create server folder.")); return; @@ -408,7 +432,9 @@ void DeckSyncManager::sendUpload(const UploadOp &op) processNext(); return; } - const QString deckString = loaded->deckList.writeToString_Native(); + // Collection sync compares and uploads the same timestamp-free string, so the + // server's contentHash() of what it stores matches getContentHash() locally. + const QString deckString = loaded->deckList.writeToString_Native(false); if (deckString.length() > MAX_FILE_LENGTH) { ++skippedCount; processNext(); @@ -439,6 +465,10 @@ void DeckSyncManager::onUploadFinished(const Response &response, if (!syncing) { return; } + if (response.response_code() == Response::RespNotConnected) { + failSync(tr("Server connection lost.")); + return; + } const QString relativePath = extraData.toString(); if (response.response_code() != Response::RespOk) { ++skippedCount; @@ -477,7 +507,12 @@ void DeckSyncManager::onUploadFinished(const Response &response, QString DeckSyncManager::uniqueLocalPath(const QString &relativePath) const { - if (!QFileInfo(deckPath + "/" + relativePath).exists()) { + // Planned paths are not on disk yet, so the set keeps two downloads from + // being queued into the same file during one run. + auto isTaken = [this](const QString &path) { + return plannedLocalPaths.contains(path) || QFileInfo(deckPath + "/" + path).exists(); + }; + if (!isTaken(relativePath)) { return relativePath; } QString base = relativePath; @@ -486,7 +521,7 @@ QString DeckSyncManager::uniqueLocalPath(const QString &relativePath) const } for (int i = 2; i < 1000; ++i) { const QString candidate = base + " (" + QString::number(i) + ").cod"; - if (!QFileInfo(deckPath + "/" + candidate).exists()) { + if (!isTaken(candidate)) { return candidate; } } @@ -512,12 +547,18 @@ void DeckSyncManager::onDownloadFinished(const Response &response, return; } const QString relativePath = extraData.toString(); + int deckId = -1; for (int i = 0; i < pendingDownloads.size(); ++i) { if (pendingDownloads.at(i).relativePath == relativePath) { + deckId = pendingDownloads.at(i).deckId; pendingDownloads.removeAt(i); break; } } + if (response.response_code() == Response::RespNotConnected) { + failSync(tr("Server connection lost.")); + return; + } if (response.response_code() != Response::RespOk) { ++skippedCount; processNext(); @@ -558,6 +599,9 @@ void DeckSyncManager::onDownloadFinished(const Response &response, DeckSyncSidecar sidecar(sidecarPath); sidecar.load(); DeckSyncRecord record = sidecar.get(relativePath); + if (deckId != -1) { + record.remoteId = deckId; + } if (!download.guid().empty()) { record.guid = QString::fromStdString(download.guid()); } diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index c891cbb58..30de5b35a 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -137,6 +137,7 @@ private: QString sidecarPath; QMap localFiles; QSet ensuredDirs; + QSet plannedLocalPaths; ///< Download targets reserved during this run, not yet on disk. QList pendingUploads; QList pendingDownloads; int uploadedCount = 0; diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp index 63cbbdc4e..08fc48796 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.cpp @@ -123,9 +123,11 @@ bool DeckList::Metadata::readElement(QXmlStreamReader *xml, const QString &child return true; } -void DeckList::Metadata::write(QXmlStreamWriter *xml) const +void DeckList::Metadata::write(QXmlStreamWriter *xml, bool includeLastLoadedTimestamp) const { - xml->writeTextElement("lastLoadedTimestamp", lastLoadedTimestamp); + if (includeLastLoadedTimestamp) { + xml->writeTextElement("lastLoadedTimestamp", lastLoadedTimestamp); + } xml->writeTextElement("deckname", name); xml->writeTextElement("format", gameFormat); xml->writeStartElement("bannerCard"); @@ -203,12 +205,12 @@ bool DeckList::readElement(QXmlStreamReader *xml) return true; } -void DeckList::write(QXmlStreamWriter *xml) const +void DeckList::write(QXmlStreamWriter *xml, bool includeLastLoadedTimestamp) const { xml->writeStartElement("cockatrice_deck"); xml->writeAttribute("version", "1"); - metadata.write(xml); + metadata.write(xml, includeLastLoadedTimestamp); // Write zones tree.write(xml); @@ -270,12 +272,12 @@ bool DeckList::loadFromString_Native(const QString &nativeString) return loadFromXml(&xml); } -QString DeckList::writeToString_Native() const +QString DeckList::writeToString_Native(bool includeLastLoadedTimestamp) const { QString result; QXmlStreamWriter xml(&result); xml.writeStartDocument(); - write(&xml); + write(&xml, includeLastLoadedTimestamp); xml.writeEndDocument(); return result; } @@ -486,7 +488,7 @@ QString DeckList::contentHash(const QString &nativeDeck) QString DeckList::getContentHash() const { - return contentHash(writeToString_Native()); + return contentHash(writeToString_Native(false)); } /** diff --git a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h index 01591bbd5..2875fb905 100644 --- a/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h +++ b/libcockatrice_deck_list/libcockatrice/deck_list/deck_list.h @@ -95,8 +95,10 @@ public: * @brief Writes the deck metadata section of a Cockatrice deck XML file. * * @param xml Writer to append the metadata elements to. + * @param includeLastLoadedTimestamp Whether to write lastLoadedTimestamp. Collection sync + * excludes it, since it changes every time a deck is opened. */ - void write(QXmlStreamWriter *xml) const; + void write(QXmlStreamWriter *xml, bool includeLastLoadedTimestamp = true) const; }; private: @@ -244,10 +246,15 @@ public: /** @name Serialization (XML) */ ///@{ bool readElement(QXmlStreamReader *xml); - void write(QXmlStreamWriter *xml) const; + void write(QXmlStreamWriter *xml, bool includeLastLoadedTimestamp = true) const; bool loadFromXml(QXmlStreamReader *xml); bool loadFromString_Native(const QString &nativeString); - QString writeToString_Native() const; + /** + * @brief Serialises the deck to a Cockatrice native XML string. + * @param includeLastLoadedTimestamp Whether to write lastLoadedTimestamp. Pass false for + * content comparisons, so opening a deck does not change its identity. + */ + QString writeToString_Native(bool includeLastLoadedTimestamp = true) const; bool loadFromFile_Native(QIODevice *device); bool saveToFile_Native(QIODevice *device) const; ///@} @@ -305,7 +312,12 @@ public: * peers compare the same bytes. */ static QString contentHash(const QString &nativeDeck); - /** @brief contentHash() of this deck's native serialisation. */ + /** + * @brief contentHash() of this deck's native serialisation, without lastLoadedTimestamp. + * + * Opening a deck rewrites that timestamp, so hashing it would make every played deck + * look modified to the server. + */ QString getContentHash() const; void refreshDeckHash(); ///@} diff --git a/tests/deck_content_hash_test.cpp b/tests/deck_content_hash_test.cpp index 192abb007..62c53aa6b 100644 --- a/tests/deck_content_hash_test.cpp +++ b/tests/deck_content_hash_test.cpp @@ -19,7 +19,21 @@ TEST(DeckContentHashTest, MatchesNativeSerialisation) { DeckList deck; ASSERT_TRUE(deck.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); - EXPECT_EQ(deck.getContentHash(), DeckList::contentHash(deck.writeToString_Native())); + // Content identity skips lastLoadedTimestamp: opening a deck rewrites it. + EXPECT_EQ(deck.getContentHash(), DeckList::contentHash(deck.writeToString_Native(false))); +} + +TEST(DeckContentHashTest, IgnoresLastLoadedTimestamp) +{ + DeckList first; + DeckList second; + ASSERT_TRUE(first.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + ASSERT_TRUE(second.loadFromString_Native(deckWithName(QStringLiteral("Burn")))); + first.setLastLoadedTimestamp(QStringLiteral("2026-01-01T00:00:00")); + second.setLastLoadedTimestamp(QStringLiteral("2026-11-08T12:34:56")); + + EXPECT_NE(first.writeToString_Native(), second.writeToString_Native()); + EXPECT_EQ(first.getContentHash(), second.getContentHash()); } TEST(DeckContentHashTest, IsStableAcrossLoads)