From 3aba1ef9296aafe53570ef570e1891f49ad4bd73 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sat, 3 Oct 2026 14:59:58 +0200 Subject: [PATCH] [Settings] Back up the settings folder before migrating (#7386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Settings] Back up the settings folder before migrating The per-file settings migration only preserved global.ini, as global.ini.old. Everything else the migrations write was overwritten in place with no way back: the legacy migration and the 17 per-domain migrateXxxSettings functions all write straight into the per-domain .ini files, so any of them that got a key mapping wrong silently replaced a working file. The re-migration path is the sharpest case, because migrateSettingsFromGlobalIni() runs again whenever an older build wrote non-sentinel keys back into global.ini, and those values then land on top of per-domain files the user had already edited. Snapshot the whole settings folder before either migration runs, into a timestamped sibling settings-backup-. A sibling rather than a child, so a backup is never copied into the next backup and the "Open Settings Folder" action keeps showing only live files. Keep every snapshot; settings files are kilobytes, so growth is not a concern. SettingsMigration::isMigrationPending() gates the copy so a normal start creates nothing. It reuses the same condition migrateSettingsFromGlobalIni() uses to decide whether it has work, factored into a shared helper so the two cannot drift, and it ignores the legacy migration on an empty profile where that migration only writes its sentinel. The backup itself skips a folder whose files are all empty, which covers a fresh profile whose global.ini was created but never written. A failed copy logs a warning and returns an empty path rather than aborting the migration, since a full disk or an unwritable data folder must not leave the client unable to start. global.ini.old still works as the single-file escape hatch. Document the folder layout, the automatic backups, and the fact that migrations only run forwards, so users know to copy the settings folder before downgrading to a version that cannot read the per-group files. * [Client] Match the desktop file name case for the portal app ID QGuiApplication::setDesktopFileName() was given "Cockatrice"/"Oracle" while the installed desktop entries are cockatrice.desktop and oracle.desktop. Linux paths are case-sensitive, so xdg-desktop-portal failed the app-info lookup for "Cockatrice" and Qt warned on every QDesktopServices call that it could not register with the host portal. The same string becomes the xdg-shell appId, so the window icon lookup failed on Wayland for the same reason. --------- Co-authored-by: Lukas Brübach --- .../src/client/settings/cache_settings.cpp | 10 + cockatrice/src/main.cpp | 7 +- .../extra-pages/user_documentation/index.md | 4 + .../settings/settings_folder.md | 43 ++++ .../settings/settings_migration.cpp | 119 +++++++++-- .../settings/settings_migration.h | 13 ++ oracle/src/main.cpp | 5 +- tests/settings/settings_migration_test.cpp | 187 +++++++++++++++++- 8 files changed, 372 insertions(+), 16 deletions(-) create mode 100644 doc/doxygen/extra-pages/user_documentation/settings/settings_folder.md diff --git a/cockatrice/src/client/settings/cache_settings.cpp b/cockatrice/src/client/settings/cache_settings.cpp index eaa5d96ac..0f8693f2d 100644 --- a/cockatrice/src/client/settings/cache_settings.cpp +++ b/cockatrice/src/client/settings/cache_settings.cpp @@ -110,6 +110,16 @@ SettingsCache::SettingsCache() settings = new QSettings(settingsPath + "global.ini", QSettings::IniFormat, this); + // Both migrations below overwrite per-domain files in place, so snapshot the whole settings + // folder first: global.ini is preserved as global.ini.old, but pre-existing per-domain files + // (and a re-migration after a downgrade) would otherwise have no way back. + if (SettingsMigration::isMigrationPending(settingsPath)) { + const QString backupPath = SettingsMigration::backupSettingsDirectory(settingsPath); + if (!backupPath.isEmpty()) { + qCInfo(SettingsCacheLog) << "[SettingsCache] Settings backed up before migration:" << backupPath; + } + } + // Migrate from legacy NativeFormat settings to per-class INI files (runs at most once) if (!isPortableBuild) { SettingsMigration::migrateLegacySettings(settingsPath); diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index b9c30d1ad..5050ff8be 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -390,8 +390,11 @@ int main(int argc, char *argv[]) qCInfo(MainLog) << "MainWindow constructor finished"; ui.setWindowIcon(themePixmap(QStringLiteral("cockatrice"))); - // Set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("Cockatrice"); + // Base name of the desktop file installed next to the binary, without the ".desktop" + // extension. Wayland reads the window icon from it, and xdg-desktop-portal looks up + // org.freedesktop.host.portal.Registry by this app ID, so it has to match the file name + // exactly, down to the case. + QGuiApplication::setDesktopFileName("cockatrice"); SettingsCache::instance().network().setClientID(generateClientID()); diff --git a/doc/doxygen/extra-pages/user_documentation/index.md b/doc/doxygen/extra-pages/user_documentation/index.md index b55d00fcd..f4ecbe8a1 100644 --- a/doc/doxygen/extra-pages/user_documentation/index.md +++ b/doc/doxygen/extra-pages/user_documentation/index.md @@ -11,6 +11,10 @@ - @subpage beta_release +## Settings + +- @subpage settings_folder + ## Card Pictures - @subpage custom_card_pictures diff --git a/doc/doxygen/extra-pages/user_documentation/settings/settings_folder.md b/doc/doxygen/extra-pages/user_documentation/settings/settings_folder.md new file mode 100644 index 000000000..5b3426e45 --- /dev/null +++ b/doc/doxygen/extra-pages/user_documentation/settings/settings_folder.md @@ -0,0 +1,43 @@ +@page settings_folder Settings Folder and Backups + +Cockatrice keeps all of its preferences in a settings folder on your computer, one `.ini` file per +group of settings (appearance, sound, decks, and so on). Open it from the main window via +Help → Open Settings Folder. + +# The Per-File Migration + +Older versions of Cockatrice stored every setting in a single `global.ini` file. The settings were +split into per-group `.ini` files so that unrelated groups no longer share one file. The first time +you start a version that uses the new layout, your settings are moved into the new files +automatically and nothing is lost. The old `global.ini` is kept next to the new files as +`global.ini.old`. + +# Automatic Backups + +Both migrations overwrite files in the settings folder in place, so before either one runs, +Cockatrice copies the whole folder to a timestamped backup folder: + +``` +/settings-backup-20261002-193000/ +``` + +The backup folder sits next to the settings folder, not inside it. Every backup is kept, so if +settings ever end up wrong you can compare against, or copy files back from, the earlier snapshot. +Backups are only created when a migration actually has work to do, so a normal start creates +nothing. + +# Downgrading + +Settings migrations only run forwards. If you ever go back to an older version of Cockatrice, that +version does not read the new per-group files, so it will start with default settings. + +Before downgrading, copy your settings folder somewhere safe, for example: + +- Linux: `~/.local/share/Cockatrice/settings/` +- macOS: `~/Library/Application Support/Cockatrice/settings/` +- Windows: `%LOCALAPPDATA%\Cockatrice\settings\` + +Portable installs keep the settings folder in the `data` folder next to the executable. + +Copying that folder back after downgrading restores your settings. The automatic backup folder from +the migration is there as a second copy of the same data if you need it. \ No newline at end of file diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp index f6e107dad..c22bd3c02 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp +++ b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp @@ -1,12 +1,44 @@ #include "settings_migration.h" +#include +#include +#include #include +#include +#include #include #include #include +#include + +inline Q_LOGGING_CATEGORY(SettingsMigrationLog, "settings_migration"); static const QString MIGRATION_SENTINEL_KEY = QStringLiteral("migration/perfile_complete"); +/** + * True when global.ini holds keys that migrateSettingsFromGlobalIni() would still move, i.e. the + * exact condition that function uses to decide whether it has work to do. If a user runs an older + * Cockatrice build that writes to global.ini (non-sentinel keys) and then upgrades back, those new + * keys would otherwise be silently ignored, so they are re-migrated instead. + */ +static bool hasPendingGlobalIniSettings(const QString &settingsPath) +{ + if (!QFile::exists(settingsPath + "global.ini")) { + return false; + } + + QSettings globalIni(settingsPath + "global.ini", QSettings::IniFormat); + if (!globalIni.value(MIGRATION_SENTINEL_KEY, false).toBool()) { + return true; + } + + // The sentinel is set, so any key besides it was written after the migration, i.e. by an + // older build sharing this file. Those are exactly the keys to re-migrate. + QStringList otherKeys = globalIni.allKeys(); + otherKeys.removeAll(MIGRATION_SENTINEL_KEY); + return !otherKeys.isEmpty(); +} + static void migrateTabsSettings(const QString &settingsPath, QSettings &globalIni) { if (!globalIni.contains("tabs/visualDeckStorage") && !globalIni.contains("tabs/server") && @@ -658,21 +690,11 @@ bool SettingsMigration::migrateLegacySettings(const QString &settingsPath) bool SettingsMigration::migrateSettingsFromGlobalIni(const QString &settingsPath) { - if (!QFile::exists(settingsPath + "global.ini")) { + if (!hasPendingGlobalIniSettings(settingsPath)) { return false; } QSettings globalIni(settingsPath + "global.ini", QSettings::IniFormat); - if (globalIni.value(MIGRATION_SENTINEL_KEY, false).toBool()) { - // If a user runs an older Cockatrice build that writes to global.ini (non-sentinel keys) - // and then upgrades back, those new keys will be silently ignored. Re-migrate them. - globalIni.sync(); - auto allKeys = globalIni.allKeys(); - allKeys.removeAll(MIGRATION_SENTINEL_KEY); - if (allKeys.isEmpty()) { - return false; - } - } migrateTabsSettings(settingsPath, globalIni); migrateSoundSettings(settingsPath, globalIni); @@ -700,3 +722,78 @@ bool SettingsMigration::migrateSettingsFromGlobalIni(const QString &settingsPath return true; } + +bool SettingsMigration::isMigrationPending(const QString &settingsPath) +{ + if (hasPendingGlobalIniSettings(settingsPath)) { + return true; + } + + QSettings personalIni(settingsPath + "personal.ini", QSettings::IniFormat); + if (personalIni.value("migration/legacy_complete", false).toBool()) { + return false; + } + + // migrateLegacySettings() also runs on an empty profile, where it only writes its sentinel. + // Reporting that as pending would leave a backup folder behind on every fresh install, so + // only count it when there really are legacy keys left to move. + return !QSettings().allKeys().isEmpty(); +} + +QString SettingsMigration::backupSettingsDirectory(const QString &settingsPath) +{ + QDir settingsDir(QDir::cleanPath(settingsPath)); + if (!settingsDir.exists()) { + return QString(); + } + + const QDir::Filters fileFilters = QDir::Files | QDir::Hidden | QDir::System | QDir::NoDotAndDotDot; + + // A profile holding nothing but empty files has nothing to lose, and backing it up would only + // leave clutter next to the settings folder. + const QFileInfoList files = settingsDir.entryInfoList(fileFilters); + const bool hasContent = + std::any_of(files.cbegin(), files.cend(), [](const QFileInfo &file) { return file.size() > 0; }); + if (!hasContent) { + return QString(); + } + + // Keep the backup a sibling of the settings folder rather than a child: a folder that is never + // copied into its own backup, and one the "Open settings folder" action does not have to skip. + QDir backupParentDir(settingsDir.absolutePath()); + if (!backupParentDir.cdUp()) { + qCWarning(SettingsMigrationLog) << "[SettingsMigration] Could not resolve the parent of the settings folder:" + << settingsDir.absolutePath(); + return QString(); + } + + const QString backupName = + QStringLiteral("settings-backup-") + QDateTime::currentDateTime().toString("yyyyMMdd-HHmmss"); + QString backupPath = backupParentDir.absoluteFilePath(backupName); + // Two migrations inside the same second must not merge into a single folder. + for (int suffix = 1; QFileInfo::exists(backupPath); ++suffix) { + backupPath = backupParentDir.absoluteFilePath(backupName + '-' + QString::number(suffix)); + } + + if (!QDir().mkpath(backupPath)) { + qCWarning(SettingsMigrationLog) << "[SettingsMigration] Could not create the settings backup folder:" + << backupPath; + return QString(); + } + + QDir backupDir(backupPath); + QDirIterator it(settingsDir.absolutePath(), fileFilters, QDirIterator::Subdirectories); + while (it.hasNext()) { + it.next(); + + const QString target = backupDir.absoluteFilePath(settingsDir.relativeFilePath(it.filePath())); + QDir().mkpath(QFileInfo(target).absolutePath()); + if (!QFile::copy(it.filePath(), target)) { + // A partially copied backup still beats none: report it and let the migration run. + qCWarning(SettingsMigrationLog) << "[SettingsMigration] Could not back up settings file:" << it.filePath(); + } + } + + qCInfo(SettingsMigrationLog) << "[SettingsMigration] Settings backed up to" << backupPath; + return backupPath; +} diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.h b/libcockatrice_settings/libcockatrice/settings/settings_migration.h index f596dba75..78f9c8a3f 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.h +++ b/libcockatrice_settings/libcockatrice/settings/settings_migration.h @@ -8,6 +8,19 @@ class SettingsMigration public: static bool migrateSettingsFromGlobalIni(const QString &settingsPath); static bool migrateLegacySettings(const QString &settingsPath); + + /** + * Returns true when at least one of the two migrations would still do work, i.e. when a + * backup of the settings directory is worth taking before they run. + */ + static bool isMigrationPending(const QString &settingsPath); + + /** + * Copies the whole settings directory to a timestamped sibling folder, so a migration that + * overwrites per-domain files can be undone by hand. Returns the backup path, or an empty + * string when there was nothing to back up or the copy failed. Never aborts the migration. + */ + static QString backupSettingsDirectory(const QString &settingsPath); }; #endif // SETTINGS_MIGRATION_H diff --git a/oracle/src/main.cpp b/oracle/src/main.cpp index 0f962f9be..1d3983461 100644 --- a/oracle/src/main.cpp +++ b/oracle/src/main.cpp @@ -82,8 +82,9 @@ int main(int argc, char *argv[]) QIcon icon("theme:appicon.svg"); wizard.setWindowIcon(icon); - // set name of the app desktop file; used by wayland to load the window icon - QGuiApplication::setDesktopFileName("Oracle"); + // Base name of the installed oracle.desktop; wayland reads the window icon from it and + // xdg-desktop-portal registers it as the app ID, so the case has to match the file name. + QGuiApplication::setDesktopFileName("oracle"); wizard.show(); diff --git a/tests/settings/settings_migration_test.cpp b/tests/settings/settings_migration_test.cpp index 4fa843abf..84b948d2e 100644 --- a/tests/settings/settings_migration_test.cpp +++ b/tests/settings/settings_migration_test.cpp @@ -20,7 +20,11 @@ protected: void SetUp() override { - settingsPath = tempDir.path() + "/"; + // Mirror the production layout: the settings folder lives inside the data folder, so + // migration backups (siblings of settings/) stay isolated inside tempDir instead of + // landing in the shared system temp directory. + settingsPath = tempDir.path() + "/settings/"; + QDir().mkpath(settingsPath); // Isolate the settings used by the legacy migration tests inside the temporary // directory so the tests never read or write the real user config (registry on @@ -64,6 +68,25 @@ protected: } return false; } + + // The migration backups are timestamped, so tests address them by index rather than by name. + QStringList backupFolderNames() const + { + return QDir(tempDir.path()) + .entryList(QDir::Dirs | QDir::NoDotAndDotDot, QDir::Name) + .filter(QStringLiteral("settings-backup-")); + } + + QVariant readFromBackup(const QString &folderName, const QString &fileName, const QString &key) const + { + QSettings ini(QDir::cleanPath(tempDir.path() + '/' + folderName + '/' + fileName), QSettings::IniFormat); + return ini.value(key); + } + + bool backupFileExists(const QString &folderName, const QString &fileName) const + { + return QFile::exists(QDir::cleanPath(tempDir.path() + '/' + folderName + '/' + fileName)); + } }; TEST_F(SettingsMigrationTest, NoGlobalIniDoesNothing) @@ -725,6 +748,168 @@ TEST_F(SettingsMigrationTest, RenameFailureWithExistingBackup) ASSERT_FALSE(SettingsMigration::migrateSettingsFromGlobalIni(settingsPath)); } +// --- pre-migration backup of the settings folder --- + +TEST_F(SettingsMigrationTest, BackupHoldsPreMigrationStateOfPerDomainFiles) +{ + // A user already migrated once, changed a setting, then ran an older build that wrote to + // global.ini again. Re-migrating clobbers the per-domain file, so the backup has to hold the + // pre-migration value. + { + QSettings tabs(settingsPath + "tabs.ini", QSettings::IniFormat); + tabs.setValue("tabs/visualDeckStorage", false); + tabs.sync(); + } + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("tabs/visualDeckStorage", true); + g.sync(); + } + + const QString backupPath = SettingsMigration::backupSettingsDirectory(settingsPath); + ASSERT_FALSE(backupPath.isEmpty()); + ASSERT_TRUE(SettingsMigration::migrateSettingsFromGlobalIni(settingsPath)); + + // The migration overwrote the user's change ... + ASSERT_EQ(readFromIni("tabs.ini", "tabs/visualDeckStorage"), QVariant(true)); + + // ... but the backup still holds it, so it can be restored by hand. + const QStringList backups = backupFolderNames(); + ASSERT_EQ(backups.size(), 1); + ASSERT_EQ(readFromBackup(backups.at(0), "tabs.ini", "tabs/visualDeckStorage"), QVariant(false)); + ASSERT_EQ(readFromBackup(backups.at(0), "global.ini", "tabs/visualDeckStorage"), QVariant(true)); +} + +TEST_F(SettingsMigrationTest, BackupIsASiblingOfTheSettingsFolder) +{ + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("sound/enabled", true); + g.sync(); + } + + const QString backupPath = SettingsMigration::backupSettingsDirectory(settingsPath); + ASSERT_FALSE(backupPath.isEmpty()); + + const QDir backupDir(backupPath); + ASSERT_TRUE(backupDir.dirName().startsWith("settings-backup-")); + // Same parent as the settings folder, and not inside it, so a backup is never copied into + // another backup and "Open settings folder" shows only live files. + ASSERT_EQ(QDir::cleanPath(backupDir.absolutePath() + "/.."), QDir::cleanPath(tempDir.path())); + ASSERT_FALSE(backupDir.absolutePath().startsWith(QDir::cleanPath(settingsPath) + '/')); + ASSERT_TRUE(backupFileExists(backupDir.dirName(), "global.ini")); +} + +TEST_F(SettingsMigrationTest, BackupCopiesNestedFolders) +{ + ASSERT_TRUE(QDir().mkpath(settingsPath + "nested")); + { + QFile f(settingsPath + "nested/deep.ini"); + ASSERT_TRUE(f.open(QIODevice::WriteOnly)); + f.write("[visualDeckStorage]\nfirstDeck=1234\n"); + } + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("sound/enabled", true); + g.sync(); + } + + const QString backupPath = SettingsMigration::backupSettingsDirectory(settingsPath); + ASSERT_FALSE(backupPath.isEmpty()); + + const QStringList backups = backupFolderNames(); + ASSERT_EQ(backups.size(), 1); + ASSERT_TRUE(backupFileExists(backups.at(0), "nested/deep.ini")); + ASSERT_EQ(readFromBackup(backups.at(0), "nested/deep.ini", "visualDeckStorage/firstDeck"), QVariant("1234")); +} + +TEST_F(SettingsMigrationTest, NoBackupWhenSettingsFolderIsEmpty) +{ + ASSERT_TRUE(SettingsMigration::backupSettingsDirectory(settingsPath).isEmpty()); + ASSERT_TRUE(backupFolderNames().isEmpty()); +} + +TEST_F(SettingsMigrationTest, NoBackupWhenEverySettingsFileIsEmpty) +{ + // A fresh profile: QSettings has created global.ini, but it holds nothing worth preserving. + { + QFile f(settingsPath + "global.ini"); + ASSERT_TRUE(f.open(QIODevice::WriteOnly)); + f.close(); + } + + ASSERT_TRUE(SettingsMigration::backupSettingsDirectory(settingsPath).isEmpty()); + ASSERT_TRUE(backupFolderNames().isEmpty()); +} + +TEST_F(SettingsMigrationTest, BackupFailsGracefullyOnMissingSettingsFolder) +{ + ASSERT_TRUE(SettingsMigration::backupSettingsDirectory(tempDir.path() + "/does-not-exist/").isEmpty()); +} + +TEST_F(SettingsMigrationTest, BackupKeepsEveryPreviousSnapshot) +{ + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("sound/enabled", true); + g.sync(); + } + + ASSERT_FALSE(SettingsMigration::backupSettingsDirectory(settingsPath).isEmpty()); + ASSERT_FALSE(SettingsMigration::backupSettingsDirectory(settingsPath).isEmpty()); + + // A second snapshot in the same second must not merge into the first one. + const QStringList backups = backupFolderNames(); + ASSERT_EQ(backups.size(), 2); + ASSERT_NE(backups.at(0), backups.at(1)); +} + +TEST_F(SettingsMigrationTest, IsMigrationPendingWhenGlobalIniHasKeys) +{ + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("sound/enabled", true); + g.sync(); + } + + ASSERT_TRUE(SettingsMigration::isMigrationPending(settingsPath)); +} + +TEST_F(SettingsMigrationTest, IsNotMigrationPendingAfterSentinelIsSet) +{ + { + QSettings g(settingsPath + "global.ini", QSettings::IniFormat); + g.setValue("sound/enabled", true); + g.sync(); + } + ASSERT_TRUE(SettingsMigration::migrateSettingsFromGlobalIni(settingsPath)); + + // The sentinel-only global.ini and the legacy sentinel leave nothing to migrate. + { + QSettings personal(settingsPath + "personal.ini", QSettings::IniFormat); + personal.setValue("migration/legacy_complete", true); + personal.sync(); + } + ASSERT_FALSE(SettingsMigration::isMigrationPending(settingsPath)); +} + +TEST_F(SettingsMigrationTest, IsMigrationPendingWhenOnlyLegacySettingsRemain) +{ + QSettings().setValue("server/host", "example.com"); + QSettings().sync(); + + ASSERT_TRUE(SettingsMigration::isMigrationPending(settingsPath)); +} + +TEST_F(SettingsMigrationTest, IsNotMigrationPendingWithoutLegacyData) +{ + // Nothing to migrate: a fresh profile must not leave a backup folder behind on every start. + QSettings().clear(); + QSettings().sync(); + + ASSERT_FALSE(SettingsMigration::isMigrationPending(settingsPath)); +} + } // namespace int main(int argc, char **argv)