Address review findings

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<int> matchedServerIds;
+    QMap<QString, int> 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 = identityByLocal.value(it.key(), -1);
-        int serverId = -1;
+        if (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 {
             // 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());
     }

cockatrice/src/interface/deck_sync/deck_sync_manager.h

@@ -137,6 +137,7 @@ private:
     QString sidecarPath;
     QMap<QString, LocalFile> localFiles;
     QSet<QString> ensuredDirs;
+    QSet<QString> plannedLocalPaths; ///< Download targets reserved during this run, not yet on disk.
     QList<UploadOp> pendingUploads;
     QList<DownloadOp> pendingDownloads;
     int uploadedCount = 0;

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));
 }
 
 /**

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();
     ///@}

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)