[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

@@ -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=<appDir>), 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"
+        ; Merge the backup back over it to repair partial deletes; CopyFiles only
-        DetailPrint "Previous user data is still in place; pre-uninstall backup kept at $DataBackupDir\data."
+        ; 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:

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")}) {
+                         QStringLiteral("filters"), QStringLiteral("themes"), QStringLiteral("sounds"),
-            const QString candidate = QDir(dataDir).absoluteFilePath(sub);
+                         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
+    // Throttle: dismissing the dialog without "Don't show again" must not create
-    // pictures folder pointed into the program dir can be gigabytes, and freezing
+    // a full duplicate on every launch. A recent session is reused only when it is
-    // the client to duplicate it is worse than asking for a manual copy.
+    // complete and its source manifest covers the current source set; otherwise a
-    // Portable sources are small by construction (downloaded pictures and replays
+    // fresh copy is taken so the dialog never claims an uncovered path is backed up.
-    // are excluded above); this guard bites for non-portable offenders.
+    // 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 "

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) {
+    if (backupLocationOut) {
-        *backupLocationOut = locations.first();
+        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

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

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("<cockatrice_deck version=\"1\"/>\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)