Address review findings

cockatrice/src/interface/deck_sync/deck_sync_manager.cpp

@@ -118,8 +118,10 @@ void DeckSyncManager::syncNow()
     syncing = true;
     uploadedCount = 0;
     downloadedCount = 0;
+    removedCount = 0;
     unchangedCount = 0;
     skippedCount = 0;
+    syncNote.clear();
     pendingUploads.clear();
     pendingDownloads.clear();
     pendingDeletes.clear();
@@ -215,6 +217,22 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
     DeckSyncSidecar sidecar(sidecarPath);
     sidecar.load();
     const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch();
+    // A listing that comes back empty while the sidecar still remembers rows that
+    // were uploaded means the server lost its deck data (a purge, or an account
+    // that was re-created), not that every deck was just deleted. Obeying that
+    // listing would remove the whole collection, so removals are held back.
+    bool sidecarHasRemoteRows = false;
+    // The records as they were before this run starts mutating them; only used to
+    // judge whether an empty listing is credible.
+    const QMap<QString, DeckSyncRecord> sidecarAtStart = sidecar.records();
+    for (auto rit = sidecarAtStart.begin(); rit != sidecarAtStart.end(); ++rit) {
+        if (rit.value().remoteId != -1) {
+            sidecarHasRemoteRows = true;
+            break;
+        }
+    }
+    const bool remoteStateLost = serverFiles.isEmpty() && sidecarHasRemoteRows;
+    bool heldBackRemovals = false;
     // 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) {
@@ -292,6 +310,15 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
             const bool unchangedSinceSync =
                 !sidecarRecord.baseHash.isEmpty() && it.value().hash == sidecarRecord.baseHash;
             if (knownToServer && unchangedSinceSync) {
+                if (remoteStateLost) {
+                    // The listing is empty although this deck was uploaded, so the
+                    // row is not known to be deleted, only missing. Keep the file
+                    // and the record; re-uploading it blind could duplicate the
+                    // deck once the real listing returns.
+                    heldBackRemovals = true;
+                    ++skippedCount;
+                    continue;
+                }
                 const QString absolutePath = deckPath + "/" + it.key();
                 const QString backupPath =
                     absolutePath + BACKUP_SUFFIX + QString::number(QDateTime::currentDateTime().toSecsSinceEpoch());
@@ -300,7 +327,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
                     continue;
                 }
                 sidecar.remove(it.key());
-                ++downloadedCount;
+                ++removedCount;
                 continue;
             }
             const QString serverPath = serverPathFor(it.key());
@@ -373,12 +400,30 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
             continue;
         }
         if (it.value().remoteId == -1) {
+            // Never uploaded, so there is no server row to remove.
             sidecar.remove(it.key());
             continue;
         }
+        if (matchedServerIds.contains(it.value().remoteId)) {
+            // Another local file (a rename or a copy) now owns this row: drop the
+            // stale record, but never tombstone the row it took over.
+            sidecar.remove(it.key());
+            continue;
+        }
+        if (remoteStateLost) {
+            // The whole listing is missing, not this one deck: pushing the delete
+            // would purge rows that are probably still there.
+            heldBackRemovals = true;
+            continue;
+        }
         pendingDeletes.append(it.key());
     }
 
+    if (heldBackRemovals) {
+        syncNote = tr("The server listed no decks although this collection was synced before. "
+                      "No deletions were applied this run; the decks were kept.");
+    }
+
     sidecar.save();
     processNext();
 }
@@ -530,7 +575,7 @@ void DeckSyncManager::onUploadFinished(const Response &response,
             // upload the deck as new instead of updating a dead id.
             const UploadOp retry{.relativePath = relativePath,
                                  .serverPath = context.value("serverPath").toString(),
-                                 .serverDir = {},
+                                 .serverDir = parentDir(context.value("serverPath").toString()),
                                  .deckId = -1,
                                  .guid = context.value("guid").toString()};
             pendingUploads.prepend(retry);
@@ -719,6 +764,7 @@ void DeckSyncManager::onDeleteFinished(const Response &response,
         sidecar.load();
         sidecar.remove(relativePath);
         sidecar.save();
+        ++removedCount;
     } else {
         ++skippedCount;
     }
@@ -757,11 +803,16 @@ void DeckSyncManager::finishSync()
 {
     syncing = false;
     const SyncSummary summary = currentSummary();
-    emit statusMessage(tr("Sync complete: %1 uploaded, %2 downloaded, %3 unchanged, %4 skipped.")
+    QString message = tr("Sync complete: %1 uploaded, %2 downloaded, %3 removed, %4 unchanged, %5 skipped.")
-                           .arg(summary.uploaded)
+                          .arg(summary.uploaded)
-                           .arg(summary.downloaded)
+                          .arg(summary.downloaded)
-                           .arg(summary.unchanged)
+                          .arg(summary.removed)
-                           .arg(summary.skipped));
+                          .arg(summary.unchanged)
+                          .arg(summary.skipped);
+    if (!summary.note.isEmpty()) {
+        message += QStringLiteral(" ") + summary.note;
+    }
+    emit statusMessage(message);
     emit syncFinished(summary, QString());
 }
 
@@ -770,7 +821,9 @@ DeckSyncManager::SyncSummary DeckSyncManager::currentSummary() const
     return SyncSummary{
         .uploaded = uploadedCount,
         .downloaded = downloadedCount,
+        .removed = removedCount,
         .unchanged = unchangedCount,
         .skipped = skippedCount,
+        .note = syncNote,
     };
 }

cockatrice/src/interface/deck_sync/deck_sync_manager.h

@@ -41,7 +41,10 @@ class ServerInfo_DeckStorage_Folder;
  *
  * 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.
+ * the file to a .sync-backup, and resurrection reuses the stored deck id. A
+ * listing that comes back empty while the sidecar still remembers uploaded rows
+ * is treated as lost server state rather than a collection-wide delete, so that
+ * run applies no removals and says so in its summary.
  */
 class DeckSyncManager : public QObject
 {
@@ -55,8 +58,10 @@ public:
     {
         int uploaded = 0;
         int downloaded = 0;
+        int removed = 0; ///< Decks deleted on the server, or locally to mirror a server delete.
         int unchanged = 0;
         int skipped = 0;
+        QString note; ///< Non-error caveat about this run, empty when there is none.
     };
 
     explicit DeckSyncManager(QObject *parent = nullptr);
@@ -149,8 +154,10 @@ private:
     QList<UploadOp> pendingUploads;
     QList<DownloadOp> pendingDownloads;
     QList<QString> pendingDeletes;
+    QString syncNote;
     int uploadedCount = 0;
     int downloadedCount = 0;
+    int removedCount = 0;
     int unchangedCount = 0;
     int skippedCount = 0;
 };

cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp

@@ -591,13 +591,19 @@ void TabDeckStorage::renderSyncStatus()
         case SyncStatusType::Failed:
             syncStatusLabel->setText(tr("Sync failed: %1").arg(syncStatusDetail));
             break;
-        case SyncStatusType::Finished:
+        case SyncStatusType::Finished: {
-            syncStatusLabel->setText(tr("Sync complete: %1 uploaded, %2 downloaded, %3 unchanged, %4 skipped.")
+            QString text = tr("Sync complete: %1 uploaded, %2 downloaded, %3 removed, %4 unchanged, %5 skipped.")
-                                         .arg(lastSyncSummary.uploaded)
+                               .arg(lastSyncSummary.uploaded)
-                                         .arg(lastSyncSummary.downloaded)
+                               .arg(lastSyncSummary.downloaded)
-                                         .arg(lastSyncSummary.unchanged)
+                               .arg(lastSyncSummary.removed)
-                                         .arg(lastSyncSummary.skipped));
+                               .arg(lastSyncSummary.unchanged)
+                               .arg(lastSyncSummary.skipped);
+            if (!lastSyncSummary.note.isEmpty()) {
+                text += QStringLiteral("\n") + lastSyncSummary.note;
+            }
+            syncStatusLabel->setText(text);
             break;
+        }
         case SyncStatusType::Idle:
             syncStatusLabel->setText(tr("Not synced yet"));
             break;