[Installer][Client] Fix review findings: usable session reuse, loose decks, NSIS pics, symlinks

cmake/NSIS.template.in

@@ -22,6 +22,8 @@ Var LooseDecksBackupDone
 Var LooseDeckNoticeShown
 Var StaleFindHandle
 Var StaleFindName
+Var DataBackupFindHandle
+Var DataBackupFindName
 Var IsUpgrade
 Var AbortReason
 Var WaitPid
@@ -399,10 +401,28 @@ ${AndIf} ${FileExists} "$OldInstallDir\data"
     !insertmacro CockLog "pre-uninstall backup: copying $OldInstallDir\data (downloaded pics excluded)"
     DetailPrint "Backing up portable user data before removing previous version..."
     CreateDirectory "$DataBackupDir\data"
-    CopyFiles /SILENT "$OldInstallDir\data\*.*" "$DataBackupDir\data\"
-    ; Downloaded pictures are large and re-downloadable, so drop them from the
-    ; safety backup - but keep user-made custom pics (data\pics\CUSTOM).
-    RMDir /r "$DataBackupDir\data\pics"
+    ; Copy every top-level entry except pics (downloaded pictures are large and
+    ; re-downloadable): copying everything first and deleting pics afterwards
+    ; would duplicate gigabytes into %TEMP% just to throw them away.
+    FindFirst $DataBackupFindHandle $DataBackupFindName "$OldInstallDir\data\*.*"
+    ${If} $DataBackupFindHandle != ""
+    backup_data_loop:
+        ${If} $DataBackupFindName != "."
+        ${AndIf} $DataBackupFindName != ".."
+        ${AndIf} $DataBackupFindName != "pics"
+            ${If} ${FileExists} "$OldInstallDir\data\$DataBackupFindName\*.*"
+                CreateDirectory "$DataBackupDir\data\$DataBackupFindName"
+                CopyFiles /SILENT "$OldInstallDir\data\$DataBackupFindName\*.*" "$DataBackupDir\data\$DataBackupFindName\"
+            ${Else}
+                CopyFiles /SILENT "$OldInstallDir\data\$DataBackupFindName" "$DataBackupDir\data\"
+            ${EndIf}
+        ${EndIf}
+        ClearErrors
+        FindNext $DataBackupFindHandle $DataBackupFindName
+        IfErrors 0 backup_data_loop
+        FindClose $DataBackupFindHandle
+    ${EndIf}
+    ; Keep user-made custom pics (data\pics\CUSTOM).
     ${If} ${FileExists} "$OldInstallDir\data\pics\CUSTOM"
         CreateDirectory "$DataBackupDir\data\pics\CUSTOM"
         CopyFiles /SILENT "$OldInstallDir\data\pics\CUSTOM\*.*" "$DataBackupDir\data\pics\CUSTOM\"

cockatrice/src/interface/window_main.cpp

@@ -684,6 +684,29 @@ void MainWindow::warnAboutDataPathsInProgramDir()
         offenders.clear();
     } else {
         offenders = paths.pathsInsideDir(appDir);
+        // Loose decks sitting directly in the program folder are wiped by the old uninstaller
+        // just like configured paths. Mirror the installer backup set (DeckLoader extensions
+        // minus *.txt) so a manual run of the old uninstall.exe is covered too.
+        const QDir appDirHandle(appDir);
+        const QStringList topLevelFiles = appDirHandle.entryList(QDir::Files | QDir::Hidden | QDir::NoDotAndDotDot);
+        for (const QString &fileName : topLevelFiles) {
+            const QString suffix = QFileInfo(fileName).suffix().toLower();
+            if (suffix != QStringLiteral("cod") && suffix != QStringLiteral("dec") && suffix != QStringLiteral("dek") &&
+                suffix != QStringLiteral("mwdeck")) {
+                continue;
+            }
+            const QString looseDeck = QDir::cleanPath(appDirHandle.absoluteFilePath(fileName));
+            bool alreadyCovered = false;
+            for (const QString &existing : offenders) {
+                if (PathsSettings::isInsideDir(looseDeck, existing)) {
+                    alreadyCovered = true;
+                    break;
+                }
+            }
+            if (!alreadyCovered) {
+                offenders.append(looseDeck);
+            }
+        }
         if (offenders.isEmpty()) {
             return;
         }
@@ -717,21 +740,12 @@ void MainWindow::warnAboutDataPathsInProgramDir()
     bool backupReused = false;
     bool backupSkippedDueToSize = false;
     qint64 backupBytes = 0;
-    const QString recentSession = UserDataBackup::findRecentBackupSession(backupRoot, 24);
-    QStringList recentLocations;
-    bool recentUsable = false;
-    if (!recentSession.isEmpty()) {
-        recentLocations = UserDataBackup::listSessionBackupLocations(recentSession);
-        // An empty source set must never reuse a session: sessionCoversSources is
-        // vacuously true for it, which would claim an unrelated recent backup
-        // covers paths that were never backed up.
-        recentUsable = !sourcesToBackUp.isEmpty() && !recentLocations.isEmpty() &&
-                       UserDataBackup::isBackupSessionComplete(recentSession) &&
-                       UserDataBackup::sessionCoversSources(recentSession, sourcesToBackUp);
-    }
-    if (recentUsable) {
-        sessionDir = recentSession;
-        backupLocations = recentLocations;
+    // Search all recent sessions for a complete, covering one: a partial failure must not
+    // pin the newest slot and force a full re-copy on every later launch.
+    const QString reusableSession = UserDataBackup::findRecentUsableBackupSession(backupRoot, sourcesToBackUp, 24);
+    if (!reusableSession.isEmpty()) {
+        sessionDir = reusableSession;
+        backupLocations = UserDataBackup::listSessionBackupLocations(reusableSession);
         backupComplete = true;
         backupReused = true;
         qCInfo(MainWindowLog) << "Reusing recent at-risk user data backup at" << sessionDir;
@@ -753,13 +767,14 @@ void MainWindow::warnAboutDataPathsInProgramDir()
             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 - but
-                // only when it actually covers the current sources.
-                if (!recentSession.isEmpty() && UserDataBackup::sessionCoversSources(recentSession, sourcesToBackUp)) {
-                    sessionDir = recentSession;
-                    backupLocations = recentLocations;
+            if (backupLocations.isEmpty()) {
+                // Fresh copy failed entirely: fall back to the newest covering session so the
+                // dialog still points at something, flagged incomplete below.
+                const QString fallbackSession =
+                    UserDataBackup::findRecentCoveringBackupSession(backupRoot, sourcesToBackUp, 24);
+                if (!fallbackSession.isEmpty()) {
+                    sessionDir = fallbackSession;
+                    backupLocations = UserDataBackup::listSessionBackupLocations(fallbackSession);
                     backupComplete = false;
                 }
             }
@@ -823,6 +838,18 @@ void MainWindow::warnAboutDataPathsInProgramDir()
             if (!backupComplete) {
                 message += tr("\n\nThe backup may be incomplete; please back up these folders manually as well.");
             }
+            bool hasLooseDecks = false;
+            for (const QString &offender : offenders) {
+                const QString suffix = QFileInfo(offender).suffix().toLower();
+                if (suffix == QStringLiteral("cod") || suffix == QStringLiteral("dec") ||
+                    suffix == QStringLiteral("dek") || suffix == QStringLiteral("mwdeck")) {
+                    hasLooseDecks = true;
+                    break;
+                }
+            }
+            if (hasLooseDecks) {
+                message += tr("\n\nDeck files ending in .txt are not backed up - move those manually.");
+            }
         } else if (backupSkippedDueToSize) {
             message += tr("\n\nThe automatic safety backup was skipped because of its size: %1. "
                           "Please back up these folders manually.")

libcockatrice_utility/libcockatrice/utility/user_data_backup.cpp

@@ -80,7 +80,19 @@ bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir)
         it.next();
         QString entryRel = source.relativeFilePath(it.filePath());
         QString target = QDir(destDir).absoluteFilePath(entryRel);
-        if (it.fileInfo().isDir()) {
+        if (it.fileInfo().isSymLink()) {
+            // Copy the link itself best-effort without dereferencing it. QDirIterator does not
+            // follow symlinks here, so a symlink-to-directory must not take the isDir() branch
+            // below (which would create an empty dir and silently drop the contents).
+            QDir().mkpath(QFileInfo(target).absolutePath());
+            if (QFile::exists(target) && !QFile::remove(target)) {
+                ok = false;
+                continue;
+            }
+            if (!QFile::copy(it.filePath(), target)) {
+                ok = false;
+            }
+        } else if (it.fileInfo().isDir()) {
             if (!QDir().mkpath(target)) {
                 ok = false;
             }
@@ -284,10 +296,10 @@ QString findRecentBackupSession(const QString &backupRoot, int maxAgeHours)
         if (!info.exists()) {
             continue;
         }
-        // Sessions are newest-first, so once an entry is too old every later
-        // one is older as well and the search can stop.
+        // No early break: filename order is not guaranteed to match mtime order
+        // (clock adjustments, externally touched dirs), and session counts are tiny.
         if (info.lastModified().secsTo(now) >= static_cast<qint64>(maxAgeHours) * 3600) {
-            break;
+            continue;
         }
         // Skip empty sessions from failed attempts: they must never block a retry.
         if (listSessionBackupLocations(session).isEmpty()) {
@@ -298,6 +310,58 @@ QString findRecentBackupSession(const QString &backupRoot, int maxAgeHours)
     return {};
 }
 
+QString findRecentCoveringBackupSession(const QString &backupRoot, const QStringList &sourcePaths, int maxAgeHours)
+{
+    const QStringList sessions = listBackupSessions(backupRoot);
+    const QDateTime now = QDateTime::currentDateTime();
+    for (const QString &session : sessions) {
+        const QFileInfo info(session);
+        if (!info.exists()) {
+            continue;
+        }
+        if (info.lastModified().secsTo(now) >= static_cast<qint64>(maxAgeHours) * 3600) {
+            continue;
+        }
+        if (listSessionBackupLocations(session).isEmpty()) {
+            continue;
+        }
+        if (!sessionCoversSources(session, sourcePaths)) {
+            continue;
+        }
+        return session;
+    }
+    return {};
+}
+
+QString findRecentUsableBackupSession(const QString &backupRoot, const QStringList &sourcePaths, int maxAgeHours)
+{
+    if (sourcePaths.isEmpty()) {
+        return {};
+    }
+    const QStringList sessions = listBackupSessions(backupRoot);
+    const QDateTime now = QDateTime::currentDateTime();
+    for (const QString &session : sessions) {
+        const QFileInfo info(session);
+        if (!info.exists()) {
+            continue;
+        }
+        if (info.lastModified().secsTo(now) >= static_cast<qint64>(maxAgeHours) * 3600) {
+            continue;
+        }
+        if (listSessionBackupLocations(session).isEmpty()) {
+            continue;
+        }
+        if (!isBackupSessionComplete(session)) {
+            continue;
+        }
+        if (!sessionCoversSources(session, sourcePaths)) {
+            continue;
+        }
+        return session;
+    }
+    return {};
+}
+
 bool isBackupSessionComplete(const QString &sessionDir)
 {
     const QDir session(sessionDir);

libcockatrice_utility/libcockatrice/utility/user_data_backup.h

@@ -28,7 +28,8 @@ constexpr qint64 MAX_AUTO_BACKUP_BYTES = 500LL * 1024 * 1024;
 /**
  * @brief Recursively copies @p sourceDir into @p destDir, creating it as needed.
  * @returns true when every file was copied (or there was nothing to copy).
- * Symlinks are copied best-effort via QFile::copy; a dangling link fails that entry only.
+ * Symlinks (file or directory) are copied best-effort via QFile::copy without
+ * dereferencing them; a dangling link fails that entry only.
  */
 bool copyDirectoryRecursively(const QString &sourceDir, const QString &destDir);
 
@@ -96,11 +97,30 @@ QStringList listBackupSessions(const QString &backupRoot);
  *
  * Used to throttle startup safety backups so dismissing the warning without
  * "Don't show again" does not create a full duplicate on every launch.
- * Empty sessions (failed attempts) are skipped so they never block a retry,
- * and an expired newest entry stops the search since sessions are newest-first.
+ * Empty sessions (failed attempts) are skipped so they never block a retry.
+ * Every session is age-checked individually; no early exit assumes filename
+ * order matches mtime order (clock adjustments or externally touched dirs
+ * can break that assumption).
  */
 QString findRecentBackupSession(const QString &backupRoot, int maxAgeHours = 24);
 
+/**
+ * @brief Returns the newest session that is non-empty, complete and covering @p sourcePaths, or empty.
+ *
+ * Unlike findRecentBackupSession(), an incomplete newest session (partial failure) or one
+ * covering a different source set does not block reuse of an older good session.
+ */
+QString findRecentUsableBackupSession(const QString &backupRoot, const QStringList &sourcePaths, int maxAgeHours = 24);
+
+/**
+ * @brief Returns the newest non-empty session covering @p sourcePaths, regardless of completeness.
+ *
+ * Used as a fallback pointer when a fresh copy fails entirely, so the dialog can still
+ * point at salvaged content. Completeness is left to the caller to report.
+ */
+QString
+findRecentCoveringBackupSession(const QString &backupRoot, const QStringList &sourcePaths, int maxAgeHours = 24);
+
 /**
  * @brief Reports whether @p sessionDir holds a complete backup.
  * @returns true when the session carries the complete marker, or when it predates

tests/user_data_backup_test.cpp

@@ -447,6 +447,98 @@ TEST(UserDataBackup, ManifestIsHiddenFromSessionLocations)
     }
 }
 
+TEST(UserDataBackup, FindRecentUsableSkipsIncompleteNewest)
+{
+    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("<cockatrice_deck version=\"1\"/>\n");
+    deck.close();
+
+    QStringList locations;
+    QString oldSession;
+    QString newSession;
+    ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceDir.path()}, backupRoot.path(), &locations, &oldSession));
+    ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceDir.path()}, backupRoot.path(), &locations, &newSession));
+    ASSERT_NE(oldSession, newSession);
+
+    // Newest session becomes incomplete: usable search must fall back to the older complete one.
+    ASSERT_TRUE(QFile::remove(QDir(newSession).absoluteFilePath("backup.complete")));
+    QFile marker(QDir(newSession).absoluteFilePath("backup.incomplete"));
+    ASSERT_TRUE(marker.open(QIODevice::WriteOnly | QIODevice::Text));
+    marker.write("incomplete\n");
+    marker.close();
+
+    EXPECT_EQ(UserDataBackup::findRecentUsableBackupSession(backupRoot.path(), {sourceDir.path()}), oldSession);
+    // Covering search still finds the newest session even though it is incomplete.
+    EXPECT_EQ(UserDataBackup::findRecentCoveringBackupSession(backupRoot.path(), {sourceDir.path()}), newSession);
+}
+
+TEST(UserDataBackup, FindRecentUsableRequiresCoveringSet)
+{
+    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();
+    QFile fb(QDir(sourceB.path()).absoluteFilePath("b.cod"));
+    ASSERT_TRUE(fb.open(QIODevice::WriteOnly | QIODevice::Text));
+    fb.write("b");
+    fb.close();
+
+    QStringList locations;
+    QString oldSession;
+    QString newSession;
+    ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceA.path()}, backupRoot.path(), &locations, &oldSession));
+    ASSERT_TRUE(UserDataBackup::backupAtRiskPaths({sourceB.path()}, backupRoot.path(), &locations, &newSession));
+
+    // Newest covers B only, so a query for A must reuse the older session.
+    EXPECT_EQ(UserDataBackup::findRecentUsableBackupSession(backupRoot.path(), {sourceA.path()}), oldSession);
+    EXPECT_EQ(UserDataBackup::findRecentUsableBackupSession(backupRoot.path(), {sourceB.path()}), newSession);
+    EXPECT_TRUE(
+        UserDataBackup::findRecentUsableBackupSession(backupRoot.path(), {sourceA.path(), sourceB.path()}).isEmpty());
+    EXPECT_TRUE(UserDataBackup::findRecentUsableBackupSession(backupRoot.path(), {}).isEmpty());
+}
+
+TEST(UserDataBackup, CopyDirectoryDoesNotSilentlyEmptyDirSymlink)
+{
+    QTemporaryDir sourceDir;
+    QTemporaryDir destParent;
+    ASSERT_TRUE(sourceDir.isValid());
+    ASSERT_TRUE(destParent.isValid());
+    QDir source(sourceDir.path());
+    ASSERT_TRUE(source.mkpath("real"));
+    QFile inner(source.absoluteFilePath("real/card.cod"));
+    ASSERT_TRUE(inner.open(QIODevice::WriteOnly | QIODevice::Text));
+    inner.write("x");
+    inner.close();
+    if (!QFile::link(source.absoluteFilePath("real"), source.absoluteFilePath("link"))) {
+        GTEST_SKIP() << "Symlinks not permitted in this environment";
+    }
+
+    QString dest = QDir(destParent.path()).absoluteFilePath("copy");
+    const bool ok = UserDataBackup::copyDirectoryRecursively(sourceDir.path(), dest);
+    const QFileInfo linkCopy(QDir(dest).absoluteFilePath("link"));
+    // Old behavior created an empty plain dir and reported success, silently dropping contents.
+    // New behavior copies the link best-effort; if that fails it must report failure.
+    if (ok) {
+        EXPECT_TRUE(linkCopy.exists() || linkCopy.isSymLink());
+        if (linkCopy.isDir() && !linkCopy.isSymLink()) {
+            EXPECT_FALSE(QDir(linkCopy.absoluteFilePath()).isEmpty());
+        }
+    } else {
+        EXPECT_TRUE(linkCopy.isSymLink() || !linkCopy.exists() || !QDir(linkCopy.absoluteFilePath()).isEmpty());
+    }
+}
+
 TEST(UserDataBackup, SingularBackupClearsOutParamOnTotalFailure)
 {
     QTemporaryDir backupRoot;