From 53e842188557da8b29e3d74f52a0c3a28c73f9ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 13 Oct 2026 09:14:58 +0200 Subject: [PATCH] [Installer][Client] Fix review findings: sounds, session coverage, NSIS wording Client: back up data/sounds in portable mode and mention it in the warning; reuse a recent session only when complete and covering the current sources (source manifest), estimate size only for fresh copies, normalize dedup via isInsideDir. Helper: source manifest per session with sessionCoversSources(), clear out-params on all failure paths. NSIS: document defensive portable branch, merge backup over surviving data/ to repair partial deletes, copy (not move) wording in relocate, note .txt exclusion in deck notice. --- cmake/NSIS.template.in | 29 ++++-- cockatrice/src/interface/window_main.cpp | 90 +++++++++++-------- .../utility/user_data_backup.cpp | 64 ++++++++++++- .../libcockatrice/utility/user_data_backup.h | 12 ++- tests/user_data_backup_test.cpp | 64 +++++++++++++ 5 files changed, 209 insertions(+), 50 deletions(-) diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 54e3eb689..3359b9803 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -368,6 +368,13 @@ FunctionEnd ; The backup is intentionally left in %TEMP% afterwards as a second safety copy. ; Non-portable data paths beyond loose decks are covered by the client's ; startup safety backup (warnAboutDataPathsInProgramDir). +; +; The portable-data branch below is defensive: both callers (AutoUninstallIfNeeded +; on /R, componentsPagePre interactively) only run for non-portable installs, and +; portable installs never write the registry uninstall keys that trigger them. +; It fires only when $OldInstallDir nevertheless contains portable.dat - e.g. a +; stale registry InstallLocation pointing at a portable dir, or portable.dat added +; to a normal install dir. The loose-decks branch is the live one for normal installs. Function BackupUserDataBeforeUninstall ; $OldInstallDir is the directory being uninstalled: on the /R path it equals ; $INSTDIR (updater passes /D=), on the interactive path componentsPagePre @@ -431,8 +438,11 @@ ${AndIf} ${FileExists} "$DataBackupDir\data" ${If} ${FileExists} "$OldInstallDir\data" ; Existence-only: a partially-deleted data dir (locked files during the old ; uninstall) looks the same as a survived one, so the backup is always kept. - !insertmacro CockLog "post-uninstall restore: $OldInstallDir\data still exists, backup kept" - DetailPrint "Previous user data is still in place; pre-uninstall backup kept at $DataBackupDir\data." + ; Merge the backup back over it to repair partial deletes; CopyFiles only + ; adds/overwrites, so a fully-survived dir is unaffected. + !insertmacro CockLog "post-uninstall restore: $OldInstallDir\data still exists, merging backup and keeping it" + CopyFiles /SILENT "$DataBackupDir\data\*.*" "$OldInstallDir\data\" + DetailPrint "Previous user data is still in place; pre-uninstall backup merged back and kept at $DataBackupDir\data." ${Else} !insertmacro CockLog "post-uninstall restore: restoring $OldInstallDir\data" CreateDirectory "$OldInstallDir\data" @@ -454,7 +464,7 @@ ${AndIf} ${FileExists} "$DataBackupDir\loose_decks" ${IfNot} ${Silent} ${AndIf} $LooseDeckNoticeShown != "1" StrCpy $LooseDeckNoticeShown "1" - MessageBox MB_OK|MB_ICONINFORMATION "Your decks were stored inside the Cockatrice install folder ($OldInstallDir), which the previous version's uninstaller deletes.$\n$\nThey have been restored from a backup to that folder. If you install to a different folder, they are moved there automatically. Please move them out of the install folder afterwards (Settings -> General -> Paths) so a future uninstall cannot delete them again." + MessageBox MB_OK|MB_ICONINFORMATION "Your decks were stored inside the Cockatrice install folder ($OldInstallDir), which the previous version's uninstaller deletes.$\n$\nThey have been restored from a backup to that folder (deck files ending in .txt are not backed up - move those manually). If you install to a different folder, they are moved there automatically. Please move them out of the install folder afterwards (Settings -> General -> Paths) so a future uninstall cannot delete them again." ${EndIf} ${EndIf} done: @@ -466,30 +476,33 @@ FunctionEnd ; directory page, so $OldInstallDir (registry InstallLocation) is authoritative ; there while $INSTDIR is still the default. When the user then picks a different ; target directory, the restored user data would otherwise stay orphaned in the -; old directory while the new install starts empty. Copy it across when they differ. +; old directory while the new install starts empty. Copy it across when they differ +; (a copy, not a move: the source is user data and is deliberately left in place). +; StrCmp is case-sensitive while Windows paths are not; a casing-only difference +; triggers a harmless redundant copy onto itself. Function RelocateRestoredDataToInstDir StrCmp $DataBackupDir "" done StrCmp $OldInstallDir "" done StrCmp $OldInstallDir $INSTDIR done ${If} $DataBackupDone == "1" ${AndIf} ${FileExists} "$OldInstallDir\data\*.*" - !insertmacro CockLog "relocate: moving restored data $OldInstallDir\data -> $INSTDIR\data" + !insertmacro CockLog "relocate: copying restored data $OldInstallDir\data -> $INSTDIR\data" CreateDirectory "$INSTDIR\data" CopyFiles /SILENT "$OldInstallDir\data\*.*" "$INSTDIR\data\" - DetailPrint "Moved restored user data to the new install folder." + DetailPrint "Copied restored user data to the new install folder." ${EndIf} ${If} $LooseDecksBackupDone == "1" ${If} ${FileExists} "$OldInstallDir\*.cod" ${OrIf} ${FileExists} "$OldInstallDir\*.dec" ${OrIf} ${FileExists} "$OldInstallDir\*.dek" ${OrIf} ${FileExists} "$OldInstallDir\*.mwDeck" - !insertmacro CockLog "relocate: moving restored decks $OldInstallDir -> $INSTDIR" + !insertmacro CockLog "relocate: copying restored decks $OldInstallDir -> $INSTDIR" CreateDirectory "$INSTDIR" CopyFiles /SILENT "$OldInstallDir\*.cod" "$INSTDIR\" CopyFiles /SILENT "$OldInstallDir\*.dec" "$INSTDIR\" CopyFiles /SILENT "$OldInstallDir\*.dek" "$INSTDIR\" CopyFiles /SILENT "$OldInstallDir\*.mwDeck" "$INSTDIR\" - DetailPrint "Moved restored deck files to the new install folder." + DetailPrint "Copied restored deck files to the new install folder." ${EndIf} ${EndIf} done: diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index e950cde30..c8c782b99 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -636,8 +636,9 @@ void MainWindow::warnAboutDataPathsInProgramDir() const QString dataDir = SettingsCache::instance().getDataPath(); for (const QString &sub : QStringList{QStringLiteral("decks"), QStringLiteral("customsets"), QStringLiteral("settings"), - QStringLiteral("filters"), QStringLiteral("themes"), QStringLiteral("pics/CUSTOM")}) { - const QString candidate = QDir(dataDir).absoluteFilePath(sub); + QStringLiteral("filters"), QStringLiteral("themes"), QStringLiteral("sounds"), + QStringLiteral("pics/CUSTOM")}) { + const QString candidate = QDir::cleanPath(QDir(dataDir).absoluteFilePath(sub)); if (QFileInfo::exists(candidate)) { backupSources.append(candidate); } @@ -651,9 +652,11 @@ void MainWindow::warnAboutDataPathsInProgramDir() // already-collected sources it supersedes them. offenders = paths.pathsInsideDir(appDir); for (const QString &offender : offenders) { - if (!QFileInfo::exists(offender) || backupSources.contains(offender)) { + if (!QFileInfo::exists(offender)) { continue; } + // isInsideDir is equality-aware and case-insensitive on Windows/macOS, + // so it also covers exact duplicates with different casing or separators. bool alreadyCovered = false; for (const QString &existing : backupSources) { if (PathsSettings::isInsideDir(offender, existing)) { @@ -698,49 +701,59 @@ void MainWindow::warnAboutDataPathsInProgramDir() } } } - // The copy runs synchronously on the UI thread at startup, so cap it: a - // pictures folder pointed into the program dir can be gigabytes, and freezing - // the client to duplicate it is worse than asking for a manual copy. - // Portable sources are small by construction (downloaded pictures and replays - // are excluded above); this guard bites for non-portable offenders. + // Throttle: dismissing the dialog without "Don't show again" must not create + // a full duplicate on every launch. A recent session is reused only when it is + // complete and its source manifest covers the current source set; otherwise a + // fresh copy is taken so the dialog never claims an uncovered path is backed up. + // The size estimate runs only when a fresh copy is actually going to happen, + // keeping a synchronous tree walk off the reuse fast path at startup. + QStringList backupLocations; + QString sessionDir; + bool backupComplete = true; + bool backupReused = false; bool backupSkippedDueToSize = false; qint64 backupBytes = 0; - if (!sourcesToBackUp.isEmpty()) { + const QString recentSession = UserDataBackup::findRecentBackupSession(backupRoot, 24); + QStringList recentLocations; + bool recentUsable = false; + if (!recentSession.isEmpty()) { + recentLocations = UserDataBackup::listSessionBackupLocations(recentSession); + recentUsable = !recentLocations.isEmpty() && UserDataBackup::isBackupSessionComplete(recentSession) && + UserDataBackup::sessionCoversSources(recentSession, sourcesToBackUp); + } + if (recentUsable) { + sessionDir = recentSession; + backupLocations = recentLocations; + backupComplete = true; + backupReused = true; + qCInfo(MainWindowLog) << "Reusing recent at-risk user data backup at" << sessionDir; + } else if (!sourcesToBackUp.isEmpty()) { + // The copy runs synchronously on the UI thread at startup, so cap it: a + // pictures folder pointed into the program dir can be gigabytes, and freezing + // the client to duplicate it is worse than asking for a manual copy. + // Portable sources are small by construction (downloaded pictures and replays + // are excluded above); this guard bites for non-portable offenders. backupBytes = UserDataBackup::estimateBackupSize(sourcesToBackUp); if (backupBytes >= UserDataBackup::MAX_AUTO_BACKUP_BYTES) { backupSkippedDueToSize = true; qCWarning(MainWindowLog) << "At-risk user data backup skipped: estimated" << backupBytes << "bytes exceeds automatic cap"; + } else { + // One timestamped session directory for all sources, so a second boundary + // never splits a single startup check across two backup folders and + // same-named sources cannot overwrite each other. + backupComplete = + UserDataBackup::backupAtRiskPaths(sourcesToBackUp, backupRoot, &backupLocations, &sessionDir); + UserDataBackup::pruneOldBackupSessions(backupRoot, 5); + if (backupLocations.isEmpty() && !recentLocations.isEmpty()) { + // Fresh copy failed entirely: fall back to the recent session so the + // dialog still points at something, flagged incomplete below. + sessionDir = recentSession; + backupLocations = recentLocations; + backupComplete = false; + } } } - // Throttle: dismissing the dialog without "Don't show again" must not create - // a full duplicate on every launch. Reuse a recent session instead, and keep - // only a handful of sessions so Documents cannot grow without bound. - QStringList backupLocations; - QString sessionDir; - bool backupComplete = true; - bool backupReused = false; - const QString recentSession = UserDataBackup::findRecentBackupSession(backupRoot, 24); - if (!recentSession.isEmpty()) { - const QStringList reused = UserDataBackup::listSessionBackupLocations(recentSession); - if (!reused.isEmpty()) { - sessionDir = recentSession; - backupLocations = reused; - // A reused session keeps the completeness it was recorded with, so a - // previously partial backup is still presented with the manual-copy - // warning instead of as a success. - backupComplete = UserDataBackup::isBackupSessionComplete(recentSession); - backupReused = true; - qCInfo(MainWindowLog) << "Reusing recent at-risk user data backup at" << sessionDir; - } - } - if (!backupReused && !backupSkippedDueToSize && !sourcesToBackUp.isEmpty()) { - // One timestamped session directory for all sources, so a second boundary - // never splits a single startup check across two backup folders and - // same-named sources cannot overwrite each other. - backupComplete = UserDataBackup::backupAtRiskPaths(sourcesToBackUp, backupRoot, &backupLocations, &sessionDir); - UserDataBackup::pruneOldBackupSessions(backupRoot, 5); - } for (const QString &backupLocation : backupLocations) { qCInfo(MainWindowLog) << "Backed up at-risk user data to" << backupLocation; } @@ -757,7 +770,8 @@ void MainWindow::warnAboutDataPathsInProgramDir() if (!backupLocations.isEmpty()) { message = tr("This portable install keeps its data inside the program directory, where the " "uninstallers shipped in Cockatrice 3.1.0/3.1.1 delete everything on uninstall or " - "update.\n\nA safety backup of your decks, custom sets, settings, filters, themes and custom " + "update.\n\nA safety backup of your decks, custom sets, settings, filters, themes, sounds and " + "custom " "pictures %1 " "at:\n%2\n\nReplays and downloaded pictures are not included in this backup. Uninstalling " "with the old uninstaller still deletes the program " diff --git a/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp b/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp index d07ec86df..4885fb4fc 100644 --- a/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp +++ b/libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp @@ -15,10 +15,20 @@ namespace const QString COMPLETE_MARKER = QStringLiteral("backup.complete"); const QString INCOMPLETE_MARKER = QStringLiteral("backup.incomplete"); +const QString SOURCES_MANIFEST = QStringLiteral("backup.sources"); bool isMarkerFile(const QString &fileName) { - return fileName == COMPLETE_MARKER || fileName == INCOMPLETE_MARKER; + return fileName == COMPLETE_MARKER || fileName == INCOMPLETE_MARKER || fileName == SOURCES_MANIFEST; +} + +bool pathsEqual(const QString &a, const QString &b) +{ +#if defined(Q_OS_WIN) || defined(Q_OS_MACOS) + return QDir::cleanPath(a).compare(QDir::cleanPath(b), Qt::CaseInsensitive) == 0; +#else + return QDir::cleanPath(a) == QDir::cleanPath(b); +#endif } void writeCompletionMarker(const QString &sessionDir, bool complete) @@ -114,8 +124,12 @@ bool backupAtRiskPath(const QString &sourcePath, const QString &backupRoot, QStr { QStringList locations; const bool ok = backupAtRiskPaths({sourcePath}, backupRoot, &locations, nullptr); - if (!locations.isEmpty() && backupLocationOut) { - *backupLocationOut = locations.first(); + if (backupLocationOut) { + if (!locations.isEmpty()) { + *backupLocationOut = locations.first(); + } else { + backupLocationOut->clear(); + } } return ok; } @@ -143,6 +157,12 @@ bool backupAtRiskPaths(const QStringList &sourcePaths, QString sessionDir; if (!createBackupSessionDir(backupRoot, &sessionDir)) { + if (backupLocationsOut) { + backupLocationsOut->clear(); + } + if (sessionDirOut) { + sessionDirOut->clear(); + } return false; } if (sessionDirOut) { @@ -184,6 +204,14 @@ bool backupAtRiskPaths(const QStringList &sourcePaths, return false; } writeCompletionMarker(sessionDir, allOk); + // Record which sources this session covers so callers can reuse it only + // when it actually covers the current source set (see sessionCoversSources). + QFile manifest(QDir(sessionDir).absoluteFilePath(SOURCES_MANIFEST)); + if (manifest.open(QIODevice::WriteOnly | QIODevice::Text)) { + for (const QString &sourcePath : existing) { + manifest.write(QDir::cleanPath(sourcePath).toUtf8() + "\n"); + } + } if (backupLocationsOut) { *backupLocationsOut = locations; } @@ -325,4 +353,34 @@ QStringList listSessionBackupLocations(const QString &sessionDir) return locations; } +bool sessionCoversSources(const QString &sessionDir, const QStringList &sourcePaths) +{ + QFile manifest(QDir(sessionDir).absoluteFilePath(SOURCES_MANIFEST)); + if (!manifest.open(QIODevice::ReadOnly | QIODevice::Text)) { + // Legacy sessions predate the manifest: coverage cannot be verified, + // so report not-covered and let the caller take a fresh copy. + return false; + } + QStringList recorded; + while (!manifest.atEnd()) { + const QString line = QString::fromUtf8(manifest.readLine()).trimmed(); + if (!line.isEmpty()) { + recorded.append(line); + } + } + for (const QString &source : sourcePaths) { + bool found = false; + for (const QString &entry : recorded) { + if (pathsEqual(source, entry)) { + found = true; + break; + } + } + if (!found) { + return false; + } + } + return true; +} + } // namespace UserDataBackup diff --git a/libcockatrice_utility/libcockatrice/utility/user_data_backup.h b/libcockatrice_utility/libcockatrice/utility/user_data_backup.h index 9de7515d3..ed3fda7d1 100644 --- a/libcockatrice_utility/libcockatrice/utility/user_data_backup.h +++ b/libcockatrice_utility/libcockatrice/utility/user_data_backup.h @@ -47,6 +47,7 @@ bool createBackupSessionDir(const QString &backupRoot, QString *sessionDirOut); * @param backupRoot directory OUTSIDE the install tree that will hold timestamped backups. * @param backupLocationOut receives the created backup file or directory whenever a target * was created, even when the copy was only partial (i.e. also on failure with a partial copy). + * Cleared when nothing was salvaged. * @returns true when the whole source was copied, false when the source is missing or * any entry failed. A false return with a non-empty @p backupLocationOut means a partial * backup exists at that location. @@ -118,10 +119,19 @@ void pruneOldBackupSessions(const QString &backupRoot, int keepCount = 5); /** * @brief Lists the backup targets inside an existing session directory. * @returns absolute paths of the entries, for reuse messages without a new copy. - * Completeness marker files are internal bookkeeping and are never listed. + * Completeness marker and source manifest files are internal bookkeeping and are never listed. */ QStringList listSessionBackupLocations(const QString &sessionDir); +/** + * @brief Reports whether @p sessionDir was recorded as covering all of @p sourcePaths. + * @returns true when the session's source manifest lists every entry of @p sourcePaths + * (normalized via QDir::cleanPath, case-insensitive on Windows/macOS). Legacy sessions + * without a manifest return false so a changed source set always triggers a fresh copy + * instead of reusing a backup that may not cover it. + */ +bool sessionCoversSources(const QString &sessionDir, const QStringList &sourcePaths); + } // namespace UserDataBackup #endif // USER_DATA_BACKUP_H diff --git a/tests/user_data_backup_test.cpp b/tests/user_data_backup_test.cpp index 85ccb70ca..192a95e28 100644 --- a/tests/user_data_backup_test.cpp +++ b/tests/user_data_backup_test.cpp @@ -393,6 +393,70 @@ TEST(UserDataBackup, EstimateBackupSize) EXPECT_GE(UserDataBackup::estimateBackupSize({sourceDir.path()}, 1), 1); } +TEST(UserDataBackup, SessionCoversSourcesRequiresManifestMatch) +{ + QTemporaryDir sourceA; + QTemporaryDir sourceB; + QTemporaryDir backupRoot; + ASSERT_TRUE(sourceA.isValid()); + ASSERT_TRUE(sourceB.isValid()); + ASSERT_TRUE(backupRoot.isValid()); + QFile fa(QDir(sourceA.path()).absoluteFilePath("a.cod")); + ASSERT_TRUE(fa.open(QIODevice::WriteOnly | QIODevice::Text)); + fa.write("a"); + fa.close(); + + QStringList locations; + QString sessionDir; + ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceA.path()}, backupRoot.path(), &locations, &sessionDir)); + EXPECT_TRUE(UserDataBackup::sessionCoversSources(sessionDir, {sourceA.path()})); + EXPECT_FALSE(UserDataBackup::sessionCoversSources(sessionDir, {sourceA.path(), sourceB.path()})); + EXPECT_FALSE(UserDataBackup::sessionCoversSources(sessionDir, {sourceB.path()})); +} + +TEST(UserDataBackup, LegacySessionWithoutManifestIsNotCovering) +{ + QTemporaryDir backupRoot; + ASSERT_TRUE(backupRoot.isValid()); + QString sessionDir; + ASSERT_TRUE(UserDataBackup::createBackupSessionDir(backupRoot.path(), &sessionDir)); + ASSERT_TRUE(QDir(sessionDir).mkpath("decks")); + QFile deck(QDir(sessionDir).absoluteFilePath("decks/mydeck.cod")); + ASSERT_TRUE(deck.open(QIODevice::WriteOnly | QIODevice::Text)); + deck.write("\n"); + deck.close(); + EXPECT_FALSE(UserDataBackup::sessionCoversSources(sessionDir, {QDir(sessionDir).absoluteFilePath("decks")})); +} + +TEST(UserDataBackup, ManifestIsHiddenFromSessionLocations) +{ + QTemporaryDir sourceDir; + QTemporaryDir backupRoot; + ASSERT_TRUE(sourceDir.isValid()); + ASSERT_TRUE(backupRoot.isValid()); + QFile deck(QDir(sourceDir.path()).absoluteFilePath("mydeck.cod")); + ASSERT_TRUE(deck.open(QIODevice::WriteOnly | QIODevice::Text)); + deck.write("x"); + deck.close(); + + QStringList locations; + QString sessionDir; + ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceDir.path()}, backupRoot.path(), &locations, &sessionDir)); + for (const QString &location : UserDataBackup::listSessionBackupLocations(sessionDir)) { + EXPECT_FALSE(location.endsWith(QStringLiteral("backup.sources"))); + } +} + +TEST(UserDataBackup, SingularBackupClearsOutParamOnTotalFailure) +{ + QTemporaryDir backupRoot; + ASSERT_TRUE(backupRoot.isValid()); + QString location = QStringLiteral("stale"); + const QString missing = QDir(backupRoot.path()).absoluteFilePath("does-not-exist.cod"); + EXPECT_FALSE(UserDataBackup::backupAtRiskPath(missing, backupRoot.path(), &location)); + EXPECT_TRUE(location.isEmpty()); +} + } // namespace int main(int argc, char **argv)