From 2992bfdac1a8e0cfee37de6f7be2bc02426f22d0 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:31 +0100 Subject: [PATCH] Address review findings --- .../interface/deck_sync/deck_sync_manager.cpp | 32 +++++++++++-------- .../interface/deck_sync/deck_sync_manager.h | 2 ++ .../interface/deck_sync/deck_sync_sidecar.h | 2 +- .../widgets/tabs/tab_deck_storage.cpp | 8 ++++- servatrice/src/serversocketinterface.cpp | 2 +- 5 files changed, 29 insertions(+), 17 deletions(-) diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp index 215936fe5..80ef22687 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.cpp @@ -858,6 +858,16 @@ void DeckSyncManager::queueConflict(const QString &relativePath, .guid = serverGuid, .hash = serverHash, .lastModifiedSecs = serverLastModifiedSecs}; + if (autoRun) { + // An auto run has no user in front of it: opening the modal conflict + // dialog from a timer or a file-watcher event would pop up mid-game. + // Leave the path untouched so the next manual sync classifies it again + // and asks — and skip the server fetch, which would only be discarded + // and repeated on every poll. + ++conflictCount; + ++skippedCount; + return; + } pendingConflicts.insert(relativePath, ctx); conflictPaths.insert(relativePath); applyDownload(relativePath, deckId, serverGuid, serverHash, serverLastModifiedSecs); @@ -865,17 +875,10 @@ void DeckSyncManager::queueConflict(const QString &relativePath, void DeckSyncManager::handleConflictDownload(const QString &relativePath, const QString &serverDeckString) { + // Only a manual run fetches a conflicting server copy, so this point is + // never reached with autoRun set: queueConflict() records the conflict and + // leaves the path alone instead. const ConflictDownload ctx = pendingConflicts.take(relativePath); - if (autoRun) { - // An auto run has no user in front of it: opening the modal conflict - // dialog from a timer or a file-watcher event would pop up mid-game. - // Leave the path untouched so the next manual sync classifies it again - // and asks. - ++conflictCount; - ++skippedCount; - processNext(); - return; - } if (sessionDefault == DeckSyncConflictKeepMine) { applyUpload(relativePath, ctx.serverPath, ctx.deckId, ctx.guid); processNext(); @@ -1349,7 +1352,8 @@ void DeckSyncManager::onDeleteFinished(const Response &response, } } // Both outcomes converge the record: pushed deletes and server-side - // deletes (already tombstoned, answering NameNotFound) are done. + // deletes (already tombstoned, answering NameNotFound) are done. Either + // way the server tree the client is showing is stale, so both count. if (response.response_code() == Response::RespOk || response.response_code() == Response::RespNameNotFound) { DeckSyncSidecar sidecar(sidecarPath); sidecar.load(); @@ -1418,10 +1422,9 @@ void DeckSyncManager::finishSync() if (!summary.note.isEmpty()) { message += QStringLiteral(" ") + summary.note; } + // The conflict count travels in the summary: a statusMessage here would be + // overwritten by the syncFinished() summary the tab renders next. emit statusMessage(message); - if (conflictCount > 0) { - emit statusMessage(tr("%1 conflicting decks skipped. Press Sync now to resolve them.").arg(conflictCount)); - } emit syncFinished(summary, QString()); } @@ -1433,6 +1436,7 @@ DeckSyncManager::SyncSummary DeckSyncManager::currentSummary() const .removed = removedCount, .unchanged = unchangedCount, .skipped = skippedCount, + .conflicts = conflictCount, .note = syncNote, }; } diff --git a/cockatrice/src/interface/deck_sync/deck_sync_manager.h b/cockatrice/src/interface/deck_sync/deck_sync_manager.h index 8c20582fe..e3d748996 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_manager.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_manager.h @@ -70,6 +70,8 @@ public: int removed = 0; ///< Decks deleted on the server, or locally to mirror a server delete. int unchanged = 0; int skipped = 0; + /** @brief Both-sides-changed decks left untouched for a manual verdict. */ + int conflicts = 0; QString note; ///< Non-error caveat about this run, empty when there is none. }; diff --git a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h index 1f621b976..013500a4e 100644 --- a/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h +++ b/cockatrice/src/interface/deck_sync/deck_sync_sidecar.h @@ -40,7 +40,7 @@ public: /// 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; - qint64 lastSyncSecs = 0; ///< Last completed sync, seconds since epoch; 0 means never. + qint64 lastSyncSecs = 0; ///< Last completed sync, seconds since epoch; 0 means never. explicit DeckSyncSidecar(const QString &_filePath); diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp index 674f560ce..25a183c12 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp @@ -668,7 +668,9 @@ void TabDeckStorage::onSyncFinished(const SyncSummary &summary, const QString &e lastSyncSummary = summary; } renderSyncStatus(); - if (error.isEmpty()) { + // An idle poll changes nothing: refreshing the tree there would send a + // fresh Command_DeckList every interval for every connected client. + if (error.isEmpty() && summary.uploaded + summary.downloaded + summary.removed > 0) { serverDirView->refreshTree(); } } @@ -689,6 +691,10 @@ void TabDeckStorage::renderSyncStatus() .arg(lastSyncSummary.removed) .arg(lastSyncSummary.unchanged) .arg(lastSyncSummary.skipped); + if (lastSyncSummary.conflicts > 0) { + text += QStringLiteral(" ") + tr("%n conflicting deck(s) skipped. Press Sync now to resolve them.", "", + lastSyncSummary.conflicts); + } if (!lastSyncSummary.note.isEmpty()) { text += QStringLiteral("\n") + lastSyncSummary.note; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 595bd6d3d..1af6fc709 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -905,7 +905,7 @@ bool AbstractServerSocketInterface::syncCheckHelper(int userId, qint64 sinceSecs // their files linger as tombstones, so walking the live folder tree would // miss deletes pointing at nonexistent folders. The client only checks // changed_files_size(), so folder paths are not reconstructed here. - // last_modified is backfilled by the 37 -> 38 migration, so a plain range + // last_modified is backfilled by the 0040 -> 0041 migration, so a plain range // check is enough. QSqlQuery *query = sqlInterface->prepareQuery("select id, guid, hash, last_modified, deleted from " "{prefix}_decklist_files where id_user = :id_user "