From 26ffd1cfae1df0a75f4e9093394cfa1bae9b94b8 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 14:41:33 +0100 Subject: [PATCH] Address review findings --- .../interface/deck_sync/deck_sync_manager.cpp | 87 +++++++++++++------ .../interface/deck_sync/deck_sync_manager.h | 18 ++-- .../interface/deck_sync/deck_sync_sidecar.h | 9 +- 3 files changed, 77 insertions(+), 37 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 10bdd8a46..54447babe 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -73,6 +73,38 @@ QString localPathFor(const QString &dir, const QString &name) const QString path = segments.join('/'); return path.endsWith(".cod", Qt::CaseInsensitive) ? path : path + QStringLiteral(".cod"); } + +// The sidecar stores the first-sync verdict so the choice outlives the run that +// asked for it. An absent or unknown value means the default two-way sync, which +// keeps profiles written before the verdict was persisted usable. +DeckSyncMode modeForSidecar(const QString &mode) +{ + if (mode == DeckSyncSidecar::MODE_UPLOAD_ONLY) { + return DeckSyncMode::UploadOnly; + } + if (mode == DeckSyncSidecar::MODE_DOWNLOAD_ONLY) { + return DeckSyncMode::DownloadOnly; + } + if (mode == DeckSyncSidecar::MODE_SEPARATE) { + return DeckSyncMode::Separate; + } + return DeckSyncMode::TwoWay; +} + +const char *sidecarModeFor(DeckSyncMode mode) +{ + switch (mode) { + case DeckSyncMode::UploadOnly: + return DeckSyncSidecar::MODE_UPLOAD_ONLY; + case DeckSyncMode::DownloadOnly: + return DeckSyncSidecar::MODE_DOWNLOAD_ONLY; + case DeckSyncMode::Separate: + return DeckSyncSidecar::MODE_SEPARATE; + case DeckSyncMode::TwoWay: + break; + } + return DeckSyncSidecar::MODE_LINKED; +} } // namespace DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent) @@ -101,11 +133,6 @@ void DeckSyncManager::setConflictDefault(int _conflictDefault) conflictDefault = _conflictDefault; } -void DeckSyncManager::setSyncMode(DeckSyncMode mode) -{ - syncMode = mode; -} - bool DeckSyncManager::isSyncing() const { return syncing; @@ -144,7 +171,6 @@ void DeckSyncManager::syncNow() ensuredDirs.insert(QString()); localFiles.clear(); sessionDefault = -1; - syncMode = DeckSyncMode::TwoWay; firstSyncRun = false; emit syncStarted(); emit statusMessage(tr("Scanning local decks...")); @@ -155,7 +181,10 @@ void DeckSyncManager::syncNow() failSync(tr("Could not read sync state.")); return; } - if (sidecar.mode == DeckSyncSidecar::MODE_SEPARATE) { + // Apply the persisted first-sync verdict: the onboarding choice lasts beyond + // the run that asked for it, and a profile that never made one syncs both ways. + syncMode = modeForSidecar(sidecar.mode); + if (syncMode == DeckSyncMode::Separate) { failSync(tr("Sync is disabled for this server.")); return; } @@ -249,11 +278,16 @@ void DeckSyncManager::continueFirstSync(DeckSyncMode mode) return; } firstSyncRun = false; + DeckSyncSidecar sidecar(sidecarPath); + if (!sidecar.load()) { + failSync(tr("Could not read sync state.")); + return; + } + // Persist the verdict next to the separate marker so a later syncNow() uses + // the same direction instead of silently reverting to two-way. + sidecar.mode = sidecarModeFor(mode); + sidecar.save(); if (mode == DeckSyncMode::Separate) { - DeckSyncSidecar sidecar(sidecarPath); - sidecar.load(); - sidecar.mode = DeckSyncSidecar::MODE_SEPARATE; - sidecar.save(); stashedServerFiles.clear(); failSync(tr("Sync is disabled for this server.")); return; @@ -444,6 +478,14 @@ void DeckSyncManager::classify(const QMap &serverFiles) applyDownload(it.key(), server.id, server.guid, server.hash, server.lastModifiedSecs); continue; } + // Legacy row without a hash and an untouched local copy: upload once + // so the server stamps hash and last_modified. + if (!localChanged && !serverChanged) { + if (syncMode != DeckSyncMode::DownloadOnly) { + applyUpload(it.key(), server.path, server.id, server.guid); + } + continue; + } const qint64 localSecs = it.value().modified.toSecsSinceEpoch(); const int effective = sessionDefault >= 0 ? sessionDefault : conflictDefault; // A server row without last_modified predates sync: the local file wins. @@ -589,19 +631,9 @@ void DeckSyncManager::queueConflict(const QString &relativePath, const QString &serverHash, qint64 serverLastModifiedSecs) { - DeckSyncSidecar sidecar(sidecarPath); - sidecar.load(); - const QString guid = !serverGuid.isEmpty() ? serverGuid : sidecar.get(relativePath).guid; - if (readBaseSnapshot(guid).isEmpty()) { - emit statusMessage(tr("No base snapshot for %1, keeping the newer version.").arg(relativePath)); - const qint64 localSecs = localFiles.value(relativePath).modified.toSecsSinceEpoch(); - if (serverLastModifiedSecs == 0 || localSecs > serverLastModifiedSecs) { - applyUpload(relativePath, serverPath, deckId, serverGuid); - } else { - applyDownload(relativePath, deckId, serverGuid, serverHash, serverLastModifiedSecs); - } - return; - } + // No base snapshot (the usual first two-way sync) is not a reason to pick a + // winner silently: the dialog handles hasBase == false by disabling merge, + // and Keep both still works, so queue the conflict like any other. const ConflictDownload ctx{.serverPath = serverPath, .deckId = deckId, .guid = serverGuid, @@ -662,8 +694,11 @@ void DeckSyncManager::resolveConflict(const DeckSyncResolution &resolution) if (!syncing || !activeConflict.active) { return; } - if (resolution.rememberSession && (resolution.action == DeckSyncResolutionAction::KeepMine || - resolution.action == DeckSyncResolutionAction::KeepServer)) { + // "Always use this rule" also settles the conflicts already queued for this + // run, not just the setting for future ones. + const bool remember = resolution.rememberSession || resolution.rememberAlways; + if (remember && (resolution.action == DeckSyncResolutionAction::KeepMine || + resolution.action == DeckSyncResolutionAction::KeepServer)) { sessionDefault = resolution.action == DeckSyncResolutionAction::KeepMine ? DeckSyncConflictKeepMine : DeckSyncConflictKeepServer; } diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index e1c7c5807..61c5009c3 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -31,18 +31,19 @@ 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). 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. + * server decks download, and a deck changed on both sides pauses for a verdict, + * so nothing is overwritten silently. 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. * Both-sides-changed decks pause the queue for a verdict via conflictRequest / * resolveConflict (or the conflict default setting); the first sync of an * unknown profile pauses for an onboarding verdict via firstSyncDetected / - * continueFirstSync. No watcher and no auto triggers — those arrive later. + * continueFirstSync, and that verdict is stored in the sidecar so later runs keep + * the chosen direction. No watcher and no auto triggers — those arrive later. * * Deletes are tombstones, never silent: a locally deleted deck is pushed with * Command_DeckDel, a server-side delete is applied locally only after copying @@ -75,7 +76,6 @@ public: void setProfile(const DeckSyncProfile &_profile); void setDeckPath(const QString &path); void setConflictDefault(int conflictDefault); - void setSyncMode(DeckSyncMode mode); [[nodiscard]] bool isSyncing() const; public slots: @@ -217,7 +217,7 @@ private: QMap pendingConflicts; QMap stashedServerFiles; ActiveConflict activeConflict; - DeckSyncMode syncMode = DeckSyncMode::TwoWay; + DeckSyncMode syncMode = DeckSyncMode::TwoWay; ///< This run's direction, loaded from the sidecar. int conflictDefault = DeckSyncConflictAsk; int sessionDefault = -1; bool firstSyncRun = false; diff --git a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h index 2266922bc..404d08e8d 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h @@ -32,9 +32,14 @@ class DeckSyncSidecar public: static constexpr const char *MODE_LINKED = "linked"; static constexpr const char *MODE_SEPARATE = "separate"; + static constexpr const char *MODE_UPLOAD_ONLY = "uploadOnly"; + static constexpr const char *MODE_DOWNLOAD_ONLY = "downloadOnly"; - QString filePath; ///< Full path of the sidecar JSON file. - QString mode = MODE_LINKED; ///< Linked normally; separate keeps this profile out of sync. + QString filePath; ///< Full path of the sidecar JSON file. + /// How this profile syncs: linked is the default two-way mode, separate + /// keeps it out of sync, and the first-sync verdicts narrow it to one + /// direction. An unrecognised value falls back to the two-way sync. + QString mode = MODE_LINKED; explicit DeckSyncSidecar(const QString &_filePath);