From 0de92c06f870d7e7175967db616657399c69bf49 Mon Sep 17 00:00:00 2001 From: Bryce! Date: Wed, 4 Nov 2026 12:24:19 -0500 Subject: [PATCH] [Spoilers] Preserve spoiler files through update and reload failures (#7707) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Preserve spoiler files and serialize update reloads * Surface spoiler removal reload failures and allow disabled-state retry * Address review feedback on spoiler update lifecycle Only remove the spoiler file when the user unchecks the setting; opening Settings no longer queues an unrequested reload for a leftover file. Remove the spoilersUpdatedSuccessfully signal, which had no receiver. * Fix spoiler settings state after reopen and while disabled Reopen Settings while an application-owned update is running: the new page now observes that updater, so the button reads Updating... and re-enables on completion instead of offering a second update that cancels the first. Observation does not take ownership: only updaters the page creates are retired with deleteLater, because the launch check is stack-allocated. When spoilers are off, the hint now describes what Reload Card Database actually does, stays readable, and wraps instead of clipping. --------- Co-authored-by: Lukas Brübach --- cockatrice/CMakeLists.txt | 1 + .../spoiler_background_updater.cpp | 448 ++++++++++-------- .../card_spoiler/spoiler_background_updater.h | 46 +- .../update/card_spoiler/spoiler_file.cpp | 109 +++++ .../update/card_spoiler/spoiler_file.h | 24 + .../deck_editor_settings_page.cpp | 86 +++- .../settings_page/deck_editor_settings_page.h | 5 + tests/CMakeLists.txt | 9 + tests/spoiler_file_test.cpp | 149 ++++++ 9 files changed, 648 insertions(+), 229 deletions(-) create mode 100644 cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp create mode 100644 cockatrice/src/client/network/update/card_spoiler/spoiler_file.h create mode 100644 tests/spoiler_file_test.cpp diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index b18846079..b76783167 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -15,6 +15,7 @@ set(cockatrice_SOURCES src/client/network/update/client/client_update_checker.cpp src/client/network/update/client/release_channel.cpp src/client/network/update/card_spoiler/spoiler_background_updater.cpp + src/client/network/update/card_spoiler/spoiler_file.cpp src/client/crash/crash_reporter.cpp src/client/latency_graph_widget.cpp src/client/latency_status_widget.cpp diff --git a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp index 62668c4d9..d74414d4a 100644 --- a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp +++ b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.cpp @@ -2,253 +2,305 @@ #include "../../../../main.h" #include "../../../settings/cache_settings.h" +#include "spoiler_file.h" -#include -#include -#include +#include #include -#include #include #include #include -#include -#include -#include -#include #include #include #include -#include #include -#include +#include #include #include -#include -#include +#include #include #include #include #include #include -#include -#include #include -#define SPOILERS_STATUS_URL "https://raw.githubusercontent.com/Cockatrice/Magic-Spoiler/files/SpoilerSeasonEnabled" -#define SPOILERS_URL "https://raw.githubusercontent.com/Cockatrice/Magic-Spoiler/files/spoiler.xml" - -SpoilerBackgroundUpdater::SpoilerBackgroundUpdater(QObject *apParent) : QObject(apParent), cardUpdateProcess(nullptr) +namespace { - isSpoilerDownloadEnabled = SettingsCache::instance().downloads().getDownloadSpoilersStatus(); - if (isSpoilerDownloadEnabled) { - // Start the process of checking if we're in spoiler season - // File exists means we're in spoiler season - startSpoilerDownloadProcess(SPOILERS_STATUS_URL, false); +const QUrl SPOILERS_STATUS_URL( + QStringLiteral("https://raw.githubusercontent.com/Cockatrice/Magic-Spoiler/files/SpoilerSeasonEnabled")); +const QUrl SPOILERS_URL(QStringLiteral("https://raw.githubusercontent.com/Cockatrice/Magic-Spoiler/files/spoiler.xml")); +// Request coordination stays on the GUI thread; reload workers never access it. +QPointer currentUpdater; +QPointer> activeReload; +quint64 newestOperation = 0; + +bool removeSpoilerFile(const QString &path) +{ + QFile file(path); + if (!file.exists()) { + return false; + } + if (!file.remove()) { + qCWarning(SpoilerBackgroundUpdaterLog) << "Could not remove spoiler database" << path << file.errorString(); + return false; + } + return true; +} +} // namespace + +SpoilerBackgroundUpdater::SpoilerBackgroundUpdater(QObject *parent, bool forceReload) + : QObject(parent), enabledAtStart(SettingsCache::instance().downloads().getDownloadSpoilersStatus()), + forceReload(forceReload), targetPath(SettingsCache::instance().paths().getSpoilerCardDatabasePath()), + operationId(++newestOperation), manager(NetworkManagerFactory::create(this)), notificationIcon(trayIcon) +{ + connect(&SettingsCache::instance().downloads(), &DownloadSettings::downloadSpoilerStatusChanged, this, + &SpoilerBackgroundUpdater::cancelIfSettingsChanged); + connect(&SettingsCache::instance().paths(), &PathsSettings::cardDatabasePathChanged, this, + &SpoilerBackgroundUpdater::cancelIfSettingsChanged); + // Even a disabled/no-op operation completes after its caller can connect signals. + QTimer::singleShot(0, this, &SpoilerBackgroundUpdater::begin); +} + +void SpoilerBackgroundUpdater::begin() +{ + if (completed) { + return; + } + if (cancelled || operationId != newestOperation || + (reloadOnly ? SettingsCache::instance().downloads().getDownloadSpoilersStatus() + : (!enabledAtStart || !SettingsCache::instance().downloads().getDownloadSpoilersStatus())) || + targetPath != SettingsCache::instance().paths().getSpoilerCardDatabasePath()) { + finish(); + return; + } + const auto previous = currentUpdater; + if (activeReload) { + // The existing database API stores its final load status after its parser + // mutex is released. Do not overlap updater-owned calls to that API. + connect(activeReload, &QFutureWatcher::finished, this, &SpoilerBackgroundUpdater::begin, + static_cast(Qt::QueuedConnection | Qt::SingleShotConnection)); + if (previous && previous != this) { + previous->cancel(); + } + return; + } + currentUpdater = this; + QPointer guard(this); + if (previous && previous != this) { + previous->cancel(); + } + if (!guard) { + return; + } + if (!isCurrent()) { + finish(); + } else if (reloadOnly) { + if (QFileInfo::exists(targetPath) && !removeSpoilerFile(targetPath)) { + finish(tr("The spoiler database could not be removed. Check its file permissions and use Reload Card " + "Database to retry.")); + return; + } + reloadDatabase(false); } else { - qCInfo(SpoilerBackgroundUpdaterLog) << "Spoilers Disabled"; + downloadFromURL(SPOILERS_STATUS_URL, false); } } -void SpoilerBackgroundUpdater::startSpoilerDownloadProcess(QString url, bool saveResults) +bool SpoilerBackgroundUpdater::isCurrent() const { - auto spoilerURL = QUrl(url); - downloadFromURL(spoilerURL, saveResults); + return !completed && !cancelled && currentUpdater == this && operationId == newestOperation && + (reloadOnly != SettingsCache::instance().downloads().getDownloadSpoilersStatus()) && + targetPath == SettingsCache::instance().paths().getSpoilerCardDatabasePath(); } -void SpoilerBackgroundUpdater::downloadFromURL(QUrl url, bool saveResults) +void SpoilerBackgroundUpdater::cancelIfSettingsChanged() { - auto *nam = NetworkManagerFactory::create(this); - auto request = QNetworkRequest(url); - request.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); - QNetworkReply *reply = nam->get(request); + if (reloadOnly == SettingsCache::instance().downloads().getDownloadSpoilersStatus() || + targetPath != SettingsCache::instance().paths().getSpoilerCardDatabasePath()) { + cancel(); + } +} +void SpoilerBackgroundUpdater::cancel() +{ + if (completed) { + return; + } + cancelled = true; + const auto reply = activeReply; + activeReply.clear(); + QPointer guard(this); + if (reply) { + reply->abort(); + if (reply) { + reply->deleteLater(); + } + } + // A running reload owns no updater state. Allow its completion to retire its + // watcher before a heap-owned updater is released by the caller. + if (guard && !reloading) { + finish(); + } +} + +void SpoilerBackgroundUpdater::downloadFromURL(const QUrl &url, bool saveResults) +{ + QNetworkRequest request(url); + request.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); + request.setTransferTimeout(60000); + activeReply = manager->get(request); if (saveResults) { - // This will write out to the file (used for spoiler.xml) - connect(reply, &QNetworkReply::finished, this, &SpoilerBackgroundUpdater::actDownloadFinishedSpoilersFile); + connect(activeReply, &QNetworkReply::finished, this, + &SpoilerBackgroundUpdater::actDownloadFinishedSpoilersFile); } else { - // This will check the status (used to see if we're in spoiler season or not) - connect(reply, &QNetworkReply::finished, this, &SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled); + connect(activeReply, &QNetworkReply::finished, this, &SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled); + } +} + +void SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled() +{ + auto *reply = qobject_cast(sender()); + if (!reply || reply != activeReply) { + return; + } + activeReply.clear(); + reply->deleteLater(); + if (!isCurrent()) { + finish(); + return; + } + if (reply->error() == QNetworkReply::NoError) { + downloadFromURL(SPOILERS_URL, true); + } else if (reply->error() == QNetworkReply::ContentNotFoundError) { + const bool existed = QFileInfo::exists(targetPath); + seasonEnded = existed; + if (existed && !removeSpoilerFile(targetPath)) { + finish(tr("The spoiler season has ended, but the old spoiler database could not be removed.")); + return; + } + if (existed || forceReload) { + reloadDatabase(false); + } else { + finish(); + } + } else { + finish(tr("Could not check spoiler availability: %1").arg(reply->errorString())); } } void SpoilerBackgroundUpdater::actDownloadFinishedSpoilersFile() { - // Check for server reply - auto *reply = dynamic_cast(sender()); - QNetworkReply::NetworkError errorCode = reply->error(); - - if (errorCode == QNetworkReply::NoError) { - spoilerData = reply->readAll(); - - // Save the spoiler.xml file to the disk - saveDownloadedFile(spoilerData); - - reply->deleteLater(); - emit spoilerCheckerDone(); + auto *reply = qobject_cast(sender()); + if (!reply || reply != activeReply) { + return; + } + activeReply.clear(); + reply->deleteLater(); + if (!isCurrent()) { + finish(); + return; + } + if (reply->error() != QNetworkReply::NoError) { + finish(tr("Could not download spoilers: %1").arg(reply->errorString())); + return; + } + spoilerData = reply->readAll(); + QString error; + const auto result = SpoilerFile::save(targetPath, spoilerData, [this] { return isCurrent(); }, error); + if (result == SpoilerFile::SaveResult::Failed) { + finish(isCurrent() ? error : QString()); + } else if (result == SpoilerFile::SaveResult::Written || forceReload) { + reloadDatabase(result == SpoilerFile::SaveResult::Written); } else { - qCWarning(SpoilerBackgroundUpdaterLog) << "Error downloading spoilers file" << errorCode; - emit spoilerCheckerDone(); + finish(); } } -bool SpoilerBackgroundUpdater::deleteSpoilerFile() +void SpoilerBackgroundUpdater::reloadDatabase(bool updated) { - QString fileName = SettingsCache::instance().paths().getSpoilerCardDatabasePath(); - QFileInfo fi(fileName); - QDir fileDir(fi.path()); - QFile file(fileName); - - // Delete the spoiler.xml file - if (file.exists() && file.remove()) { - qCInfo(SpoilerBackgroundUpdaterLog) << "Deleting spoiler.xml"; - return true; - } - - qCInfo(SpoilerBackgroundUpdaterLog) << "Error: Spoiler.xml not found or not deleted"; - return false; -} - -void SpoilerBackgroundUpdater::actCheckIfSpoilerSeasonEnabled() -{ - auto *response = dynamic_cast(sender()); - QNetworkReply::NetworkError errorCode = response->error(); - - if (errorCode == QNetworkReply::ContentNotFoundError) { - // Spoiler season is offline at this point, so the spoiler.xml file can be safely deleted - // The user should run Oracle to get the latest card information - if (deleteSpoilerFile() && trayIcon) { - trayIcon->showMessage(tr("Spoilers season has ended"), tr("Deleting spoiler.xml. Please run Oracle")); + reloading = true; + auto *database = CardDatabaseManager::getInstance(); + // The job outlives an individual request, so destroying that request cannot + // let a newer updater overlap the worker that is still completing. + auto *watcher = new QFutureWatcher(QCoreApplication::instance()); + activeReload = watcher; + connect(watcher, &QFutureWatcher::finished, watcher, [watcher] { + if (activeReload == watcher) { + activeReload.clear(); } - - qCInfo(SpoilerBackgroundUpdaterLog) << "Spoiler Season Offline"; - emit spoilerCheckerDone(); - } else if (errorCode == QNetworkReply::NoError) { - qCInfo(SpoilerBackgroundUpdaterLog) << "Spoiler Service Online"; - startSpoilerDownloadProcess(SPOILERS_URL, true); - } else if (errorCode == QNetworkReply::HostNotFoundError) { - if (trayIcon) { - trayIcon->showMessage(tr("Spoilers download failed"), tr("No internet connection")); - } - - qCWarning(SpoilerBackgroundUpdaterLog) << "Spoiler download failed due to no internet connection"; - emit spoilerCheckerDone(); - } else { - if (trayIcon) { - trayIcon->showMessage(tr("Spoilers download failed"), tr("Error") + " " + (short)errorCode); - } - - qCWarning(SpoilerBackgroundUpdaterLog) << "Spoiler download failed with reason" << errorCode; - emit spoilerCheckerDone(); - } -} - -bool SpoilerBackgroundUpdater::saveDownloadedFile(QByteArray data) -{ - QString fileName = SettingsCache::instance().paths().getSpoilerCardDatabasePath(); - QFileInfo fi(fileName); - QDir fileDir(fi.path()); - - if (!fileDir.exists() && !fileDir.mkpath(fileDir.absolutePath())) { - return false; - } - - // Check if the data matches. If it does, then spoilers are up to date. - if (getHash(fileName) == getHash(data)) { - if (trayIcon) { - trayIcon->showMessage(tr("Spoilers already up to date"), tr("No new spoilers added")); - } - - qCInfo(SpoilerBackgroundUpdaterLog) << "Spoilers Up to Date"; - return false; - } - - QFile file(fileName); - if (!file.open(QIODevice::WriteOnly)) { - qCWarning(SpoilerBackgroundUpdaterLog) << "Spoiler Service Error: File open (w) failed for" << fileName; - file.close(); - return false; - } - - if (file.write(data) == -1) { - qCWarning(SpoilerBackgroundUpdaterLog) << "Spoiler Service Error: File write (w) failed for" << fileName; - file.close(); - return false; - } - - file.close(); - - // Data written, so reload the card database - qCInfo(SpoilerBackgroundUpdaterLog) << "Spoiler Service Data Written"; - // Spoilers can introduce sets the config has never seen; the loader never - // runs checkUnknownSets() itself, so run it once the finished snapshot has - // been swapped in. - auto *watcher = new QFutureWatcher(this); - connect(watcher, &QFutureWatcher::finished, this, [watcher] { watcher->deleteLater(); - CardDatabaseManager::getInstance()->checkUnknownSets(); }); - watcher->setFuture(QtConcurrent::run([] { CardDatabaseManager::getInstance()->loadCardDatabases(); })); - - // If the user has notifications enabled, let them know - // when the database was last updated - if (trayIcon) { - QList lines = data.split('\n'); - - for (const QByteArray &line : lines) { - if (line.contains("Created At:")) { - QString timeStamp = QString(line).replace("Created At:", "").trimmed(); - timeStamp.chop(6); // Remove " (UTC)" - - auto utcTime = QLocale().toDateTime(timeStamp, "ddd, MMM dd yyyy, hh:mm:ss"); -#if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0) - utcTime.setTimeZone(QTimeZone::UTC); -#else - utcTime.setTimeSpec(Qt::UTC); -#endif - - QString localTime = utcTime.toLocalTime().toString("MMM d, hh:mm"); - - trayIcon->showMessage(tr("Spoilers have been updated!"), tr("Last change:") + " " + localTime); - emit spoilersUpdatedSuccessfully(); - return true; - } + connect(watcher, &QFutureWatcher::finished, this, [this, watcher, database, updated] { + bool loaded = false; + try { + loaded = watcher->result() && database->getLoadStatus() == Ok; + } catch (const std::exception &error) { + qCWarning(SpoilerBackgroundUpdaterLog) << "Spoiler database reload failed:" << error.what(); } - } - - return true; + reloading = false; + if (!isCurrent()) { + finish(); + } else if (!loaded) { + finish(reloadOnly + ? tr("Could not reload card data after disabling spoilers. Use Reload Card Database to retry.") + : tr("Could not reload card data after updating spoilers. Use Update Spoilers to retry the " + "reload.")); + } else { + finish({}, updated); + } + }); + watcher->setFuture(QtConcurrent::run([database] { + database->reloadCardDatabasesAndNotify(); + return true; + })); } -QByteArray SpoilerBackgroundUpdater::getHash(const QString fileName) +void SpoilerBackgroundUpdater::finish(const QString &error, bool updated) { - QFile file(fileName); - - if (file.open(QFile::ReadOnly)) { - // Only read the first 512 bytes (enough to get the "created" tag) - const QByteArray bytes = file.read(512); - - QCryptographicHash hash(QCryptographicHash::Algorithm::Md5); - hash.addData(bytes); - - qCInfo(SpoilerBackgroundUpdaterLog) << "File Hash =" << hash.result(); - - file.close(); - return hash.result(); - } else { - qCWarning(SpoilerBackgroundUpdaterLog) << "getHash ReadOnly failed!"; - file.close(); - return QByteArray(); + if (completed) { + return; + } + completed = true; + spoilerData = QByteArray(); + if (currentUpdater == this) { + currentUpdater.clear(); + } + QPointer guard(this); + if (!error.isEmpty() && !cancelled) { + qCWarning(SpoilerBackgroundUpdaterLog) << error; + if (notificationIcon) { + notificationIcon->showMessage(tr("Spoiler update failed"), error); + } + if (!guard) { + return; + } + emit spoilerUpdateFailed(error); + } else if (updated && !cancelled) { + if (notificationIcon) { + notificationIcon->showMessage(tr("Spoilers have been updated!"), + tr("The card database now includes the downloaded spoilers.")); + } + } else if (seasonEnded && !cancelled && notificationIcon) { + notificationIcon->showMessage(tr("Spoiler season has ended"), + tr("The spoiler database was removed. Please run Oracle to get released cards.")); + } + if (guard) { + emit spoilerCheckerDone(); } } -QByteArray SpoilerBackgroundUpdater::getHash(QByteArray data) +SpoilerBackgroundUpdater *SpoilerBackgroundUpdater::current() { - // Only read the first 512 bytes (enough to get the "created" tag) - const QByteArray bytes = data.left(512); - - QCryptographicHash hash(QCryptographicHash::Algorithm::Md5); - hash.addData(bytes); - - qCInfo(SpoilerBackgroundUpdaterLog) << "Data Hash =" << hash.result(); - - return hash.result(); + return currentUpdater; +} + +SpoilerBackgroundUpdater *SpoilerBackgroundUpdater::deleteSpoilerFile() +{ + // Deletion and the following reload share the operation lifetime. Callers can + // observe either failure before the queued operation starts, and an explicit + // retry still reloads when the file has already been removed. + auto *updater = new SpoilerBackgroundUpdater(QCoreApplication::instance(), true); + updater->reloadOnly = true; + connect(updater, &SpoilerBackgroundUpdater::spoilerCheckerDone, updater, &QObject::deleteLater); + return updater; } diff --git a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.h b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.h index a6c4ed00f..fd90eb688 100644 --- a/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.h +++ b/cockatrice/src/client/network/update/card_spoiler/spoiler_background_updater.h @@ -2,7 +2,6 @@ * @file spoiler_background_updater.h * @ingroup Client */ -//! \todo Document this file. #ifndef COCKATRICE_SPOILER_DOWNLOADER_H #define COCKATRICE_SPOILER_DOWNLOADER_H @@ -10,10 +9,13 @@ #include #include #include +#include #include #include -class QProcess; +class QNetworkAccessManager; +class QNetworkReply; +class QSystemTrayIcon; class QUrl; inline Q_LOGGING_CATEGORY(SpoilerBackgroundUpdaterLog, "spoiler_background_updater"); @@ -22,30 +24,48 @@ class SpoilerBackgroundUpdater : public QObject { Q_OBJECT public: - explicit SpoilerBackgroundUpdater(QObject *apParent = nullptr); + explicit SpoilerBackgroundUpdater(QObject *parent = nullptr, bool forceReload = false); inline QString getCardUpdaterBinaryName() { return "oracle"; } - QByteArray getHash(const QString fileName); - QByteArray getHash(QByteArray data); - static bool deleteSpoilerFile(); + // Queues removal and reload; the application owns the returned updater and + // deletes it on completion. Callers can connect its failure/completion signals. + static SpoilerBackgroundUpdater *deleteSpoilerFile(); + // The operation currently running, if any. The updater outlives the page that + // started it, so a reopened page uses this to observe the in-flight update. + static SpoilerBackgroundUpdater *current(); private slots: + void begin(); void actDownloadFinishedSpoilersFile(); void actCheckIfSpoilerSeasonEnabled(); + void cancelIfSettingsChanged(); private: - bool isSpoilerDownloadEnabled; - QProcess *cardUpdateProcess; + const bool enabledAtStart; + const bool forceReload; + const QString targetPath; + const quint64 operationId; + QNetworkAccessManager *manager; + QPointer activeReply; + QPointer notificationIcon; QByteArray spoilerData; - void startSpoilerDownloadProcess(QString url, bool saveResults); - void downloadFromURL(QUrl url, bool saveResults); - bool saveDownloadedFile(QByteArray data); + bool completed = false; + bool cancelled = false; + bool reloading = false; + bool seasonEnded = false; + bool reloadOnly = false; + + bool isCurrent() const; + void cancel(); + void finish(const QString &error = {}, bool updated = false); + void downloadFromURL(const QUrl &url, bool saveResults); + void reloadDatabase(bool updated); signals: - void spoilersUpdatedSuccessfully(); void spoilerCheckerDone(); + void spoilerUpdateFailed(const QString &message); }; -#endif // COCKATRICE_SPOILER_DOWNLOADER_H \ No newline at end of file +#endif // COCKATRICE_SPOILER_DOWNLOADER_H diff --git a/cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp b/cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp new file mode 100644 index 000000000..554ea1108 --- /dev/null +++ b/cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp @@ -0,0 +1,109 @@ +#include "spoiler_file.h" + +#include +#include +#include +#include +#include +#include +#include + +namespace +{ +QByteArray hashFile(const QString &path) +{ + QFile file(path); + if (!file.open(QIODevice::ReadOnly)) { + return {}; + } + QCryptographicHash hash(QCryptographicHash::Sha256); + return hash.addData(&file) ? hash.result() : QByteArray(); +} +} // namespace + +namespace SpoilerFile +{ +QString validate(const QByteArray &data) +{ + QXmlStreamReader xml(data); + if (!xml.readNextStartElement() || xml.name() != QStringLiteral("cockatrice_carddatabase")) { + return QCoreApplication::translate( + "SpoilerBackgroundUpdater", + "The download is not a Cockatrice card database. The previous spoiler file was preserved."); + } + bool versionOk = false; + const int version = xml.attributes().value(QStringLiteral("version")).toInt(&versionOk); + if (!versionOk || (version != 3 && version != 4)) { + return QCoreApplication::translate( + "SpoilerBackgroundUpdater", + "The spoiler database uses an unsupported format. The previous spoiler file was preserved."); + } + bool hasCards = false; + int depth = 1; + while (!xml.atEnd()) { + const auto token = xml.readNext(); + if (token == QXmlStreamReader::StartElement) { + ++depth; + if (depth == 2 && xml.name() == QStringLiteral("cards")) { + hasCards = true; + } + } else if (token == QXmlStreamReader::EndElement) { + --depth; + } + } + if (xml.hasError() || !hasCards) { + return QCoreApplication::translate( + "SpoilerBackgroundUpdater", + "The spoiler database is incomplete or invalid. The previous spoiler file was preserved."); + } + return {}; +} + +SaveResult +save(const QString &targetPath, const QByteArray &data, const std::function &isCurrent, QString &error) +{ + error = validate(data); + if (!error.isEmpty()) { + return SaveResult::Failed; + } + if (!isCurrent()) { + error = QCoreApplication::translate("SpoilerBackgroundUpdater", + "The spoiler update was cancelled because its settings changed."); + return SaveResult::Failed; + } + if (hashFile(targetPath) == QCryptographicHash::hash(data, QCryptographicHash::Sha256)) { + return SaveResult::Unchanged; + } + const QDir directory = QFileInfo(targetPath).dir(); + if (!directory.exists() && !directory.mkpath(QStringLiteral("."))) { + error = QCoreApplication::translate("SpoilerBackgroundUpdater", + "Could not create the spoiler database directory: %1") + .arg(directory.absolutePath()); + return SaveResult::Failed; + } + QSaveFile file(targetPath); + file.setDirectWriteFallback(false); + if (!file.open(QIODevice::WriteOnly) || file.write(data) != data.size() || !file.flush()) { + error = QCoreApplication::translate( + "SpoilerBackgroundUpdater", + "Could not save the spoiler database to %1: %2. The previous file was preserved.") + .arg(targetPath, file.errorString()); + return SaveResult::Failed; + } + if (!isCurrent()) { + file.cancelWriting(); + error = QCoreApplication::translate("SpoilerBackgroundUpdater", + "The spoiler update was cancelled because its settings changed."); + return SaveResult::Failed; + } + if (!file.commit()) { + error = QCoreApplication::translate( + "SpoilerBackgroundUpdater", + "Could not replace the spoiler database at %1: %2. The previous file was preserved.") + .arg(targetPath, file.errorString()); + return SaveResult::Failed; + } + return SaveResult::Written; +} + +} // namespace SpoilerFile diff --git a/cockatrice/src/client/network/update/card_spoiler/spoiler_file.h b/cockatrice/src/client/network/update/card_spoiler/spoiler_file.h new file mode 100644 index 000000000..3a138cdea --- /dev/null +++ b/cockatrice/src/client/network/update/card_spoiler/spoiler_file.h @@ -0,0 +1,24 @@ +#ifndef COCKATRICE_SPOILER_FILE_H +#define COCKATRICE_SPOILER_FILE_H + +#include +#include +#include + +namespace SpoilerFile +{ +enum class SaveResult +{ + Failed, + Unchanged, + Written +}; + +QString validate(const QByteArray &data); +// Validate before opening the destination, compare complete files, and replace +// atomically. Check operation ownership both before writing and before committing. +SaveResult +save(const QString &targetPath, const QByteArray &data, const std::function &isCurrent, QString &error); +} // namespace SpoilerFile + +#endif // COCKATRICE_SPOILER_FILE_H diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index 31178ba4c..79370f512 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -61,7 +62,7 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() connect(mpSpoilerPathButton, &QPushButton::clicked, this, &DeckEditorSettingsPage::spoilerPathButtonClicked); updateNowButton = new QPushButton; - updateNowButton->setFixedWidth(150); + updateNowButton->setMinimumWidth(150); connect(updateNowButton, &QPushButton::clicked, this, &DeckEditorSettingsPage::updateSpoilers); // Update the GUI depending on if the box is ticked or not @@ -120,12 +121,19 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() lpSpoilerGrid->addWidget(&lastUpdatedLabel, 2, 0); lpSpoilerGrid->addWidget(updateNowButton, 2, 1); lpSpoilerGrid->addWidget(&infoOnSpoilersLabel, 3, 0, 1, 3, Qt::AlignTop); + infoOnSpoilersLabel.setWordWrap(true); // On a change to the checkbox, hide/un-hide the other fields connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, &SettingsCache::instance().downloads(), &DownloadSettings::setDownloadSpoilerStatus); connect(&mcDownloadSpoilersCheckBox, &QCheckBox::toggled, this, &DeckEditorSettingsPage::setSpoilersEnabled); + // The updater is application-owned and outlives this page, so a Settings page + // reopened mid-update must observe the operation that is still running. + if (auto *runningUpdater = SpoilerBackgroundUpdater::current()) { + observeSpoilerUpdater(runningUpdater); + } + mpGeneralGroupBox = new QGroupBox; mpGeneralGroupBox->setLayout(lpGeneralGrid); @@ -328,20 +336,60 @@ void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const void DeckEditorSettingsPage::updateSpoilers() { - // Disable the button so the user can only press it once at a time - updateNowButton->setDisabled(true); - updateNowButton->setText(tr("Updating...")); + if (spoilerUpdater) { + return; + } + // Application ownership lets either operation finish after settings closes. + // When disabled, the same button retries file removal and the local reload. + SpoilerBackgroundUpdater *sbu; + if (SettingsCache::instance().downloads().getDownloadSpoilersStatus()) { + sbu = new SpoilerBackgroundUpdater(QCoreApplication::instance(), true); + // Only an updater this page creates is retired by deleteLater: observation + // may attach to the stack-allocated launch updater, which must not be deleted. + connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, sbu, &QObject::deleteLater); + } else { + // deleteSpoilerFile() wires its own deleteLater. + sbu = SpoilerBackgroundUpdater::deleteSpoilerFile(); + } + observeSpoilerUpdater(sbu); +} - // Create a new SBU that will act as if the client was just reloaded - auto *sbu = new SpoilerBackgroundUpdater(); - connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, this, &DeckEditorSettingsPage::unlockSettings); - connect(sbu, &SpoilerBackgroundUpdater::spoilersUpdatedSuccessfully, this, &DeckEditorSettingsPage::unlockSettings); +void DeckEditorSettingsPage::observeSpoilerUpdater(SpoilerBackgroundUpdater *sbu) +{ + spoilerUpdater = sbu; + unlockSettings(); + connect(sbu, &SpoilerBackgroundUpdater::spoilerCheckerDone, this, [this, sbu] { + if (spoilerUpdater == sbu) { + spoilerUpdater.clear(); + unlockSettings(); + } + }); + connect(sbu, &SpoilerBackgroundUpdater::spoilerUpdateFailed, this, [this, sbu](const QString &message) { + if (spoilerUpdater != sbu) { + return; + } + auto *notice = new QMessageBox(QMessageBox::Warning, tr("Spoiler update"), message, QMessageBox::Ok, this); + notice->setTextFormat(Qt::PlainText); + notice->setAttribute(Qt::WA_DeleteOnClose); + notice->open(); + }); } void DeckEditorSettingsPage::unlockSettings() { - updateNowButton->setDisabled(false); - updateNowButton->setText(tr("Update Spoilers")); + updateNowButton->setEnabled(!spoilerUpdater); + updateNowButton->setText(spoilerUpdater ? tr("Updating...") + : SettingsCache::instance().downloads().getDownloadSpoilersStatus() + ? tr("Update Spoilers") + : tr("Reload Card Database")); + lastUpdatedLabel.setText(tr("Last Change") + ": " + getLastUpdateTime()); + infoOnSpoilersLabel.setText( + SettingsCache::instance().downloads().getDownloadSpoilersStatus() + ? tr("Spoilers download automatically on launch") + "\n" + + tr("Press the button to manually update without relaunching") + "\n\n" + + tr("Updates continue if you close settings") + : tr("Spoilers are off. Reload Card Database removes any downloaded spoiler file and reloads your " + "card data.")); } QString DeckEditorSettingsPage::getLastUpdateTime() @@ -379,12 +427,17 @@ void DeckEditorSettingsPage::setSpoilersEnabled(bool anInput) mpSpoilerSavePathLineEdit->setEnabled(anInput); mpSpoilerPathButton->setEnabled(anInput); lastUpdatedLabel.setEnabled(anInput); - updateNowButton->setEnabled(anInput); - infoOnSpoilersLabel.setEnabled(anInput); + // The hint explains the button in both states, especially the disabled-state + // Reload Card Database action, so keep it readable while spoilers are off. + infoOnSpoilersLabel.setEnabled(true); - if (!anInput) { - SpoilerBackgroundUpdater::deleteSpoilerFile(); + // Only react to the user disabling the setting. The constructor calls this to + // initialize the controls, and a leftover spoiler file is recovered through the + // Reload Card Database button rather than an unrequested reload on page open. + if (!anInput && sender() == &mcDownloadSpoilersCheckBox) { + observeSpoilerUpdater(SpoilerBackgroundUpdater::deleteSpoilerFile()); } + unlockSettings(); } void DeckEditorSettingsPage::retranslateUi() @@ -394,13 +447,10 @@ void DeckEditorSettingsPage::retranslateUi() mcDownloadSpoilersCheckBox.setText(tr("Download Spoilers Automatically")); mcSpoilerSaveLabel.setText(tr("Spoiler Location:")); lastUpdatedLabel.setText(tr("Last Change") + ": " + getLastUpdateTime()); - infoOnSpoilersLabel.setText(tr("Spoilers download automatically on launch") + "\n" + - tr("Press the button to manually update without relaunching") + "\n\n" + - tr("Do not close settings until manual update is complete")); picDownloadCheckBox.setText(tr("Download card pictures on the fly")); urlLinkLabel.setText(QString("%2").arg(WIKI_CUSTOM_PIC_URL).arg(tr("How to add a custom URL"))); resetDownloadURLs.setText(tr("Reset Download URLs")); - updateNowButton->setText(tr("Update Spoilers")); + unlockSettings(); aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 47f840191..8e33e697d 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,7 @@ class QLineEdit; class QListWidget; class QListWidgetItem; class QModelIndex; +class SpoilerBackgroundUpdater; class DeckEditorSettingsPage : public AbstractSettingsPage { @@ -54,6 +56,7 @@ private: QLabel infoOnSpoilersLabel; QPushButton *mpSpoilerPathButton; QPushButton *updateNowButton; + QPointer spoilerUpdater; /** @brief Adds a list item for the given URL, storing the raw URL alongside its displayed label. */ QListWidgetItem *addUrlItem(const QString &url); @@ -66,6 +69,8 @@ private: /** @brief Refreshes the displayed label of every URL item after limits or settings change. */ void refreshUrlItems(); + + void observeSpoilerUpdater(SpoilerBackgroundUpdater *updater); }; #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e25390ba0..5b7e06c20 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -190,6 +190,15 @@ cockatrice_add_test( ${CMAKE_SOURCE_DIR}/cockatrice/src ${CMAKE_SOURCE_DIR}/libcockatrice_network ) +cockatrice_add_test( + NAME + spoiler_file_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/network/update/card_spoiler/spoiler_file.cpp + spoiler_file_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src +) # The installer's holder probe is Windows-only, but the wording it produces and the # question which programs are the payload's own are plain C++ and are what a stuck user # has to act on, so they are tested everywhere. diff --git a/tests/spoiler_file_test.cpp b/tests/spoiler_file_test.cpp new file mode 100644 index 000000000..9944f95bd --- /dev/null +++ b/tests/spoiler_file_test.cpp @@ -0,0 +1,149 @@ +#include "client/network/update/card_spoiler/spoiler_file.h" + +#include +#include +#include +#include +#include + +#ifdef Q_OS_UNIX +#include +#include +#include +#endif + +namespace +{ +class SpoilerFileTest : public ::testing::Test +{ +protected: + QTemporaryDir directory; + QString path; + QString error; + const QByteArray original = ""; + + void SetUp() override + { + ASSERT_TRUE(directory.isValid()); + path = directory.filePath("spoiler.xml"); + QFile file(path); + ASSERT_TRUE(file.open(QIODevice::WriteOnly)); + ASSERT_EQ(file.write(original), original.size()); + } + + QByteArray read() + { + QFile file(path); + EXPECT_TRUE(file.open(QIODevice::ReadOnly)); + return file.readAll(); + } + + SpoilerFile::SaveResult save(const QByteArray &data) + { + return SpoilerFile::save(path, data, [] { return true; }, error); + } +}; + +TEST_F(SpoilerFileTest, InvalidDocumentsPreserveExistingFile) +{ + for (const QByteArray &invalid : + {QByteArray(), QByteArray(""), + QByteArray(""), + QByteArray(""), + QByteArray(""), + QByteArray(""), + QByteArray("")}) { + EXPECT_EQ(save(invalid), SpoilerFile::SaveResult::Failed); + EXPECT_FALSE(error.isEmpty()); + EXPECT_EQ(read(), original); + } +} + +TEST_F(SpoilerFileTest, SupportedVersionsAreAccepted) +{ + EXPECT_TRUE(SpoilerFile::validate(original).isEmpty()); + EXPECT_TRUE( + SpoilerFile::validate("").isEmpty()); +} + +TEST_F(SpoilerFileTest, IdenticalFileIsUnchanged) +{ + EXPECT_EQ(save(original), SpoilerFile::SaveResult::Unchanged); + EXPECT_TRUE(error.isEmpty()); + EXPECT_EQ(read(), original); +} + +TEST_F(SpoilerFileTest, CompleteFileHashDetectsChangesAfterLargeCommonPrefix) +{ + QByteArray changed = ""; + ASSERT_EQ(save(changed), SpoilerFile::SaveResult::Written); + changed.replace("", ""); + EXPECT_EQ(save(changed), SpoilerFile::SaveResult::Written); + EXPECT_EQ(read(), changed); + EXPECT_TRUE(error.isEmpty()); +} + +TEST_F(SpoilerFileTest, CancellationBeforeWritePreservesExistingFile) +{ + EXPECT_EQ(SpoilerFile::save(path, original, [] { return false; }, error), SpoilerFile::SaveResult::Failed); + EXPECT_FALSE(error.isEmpty()); + EXPECT_EQ(read(), original); +} + +TEST_F(SpoilerFileTest, CancellationBeforeCommitPreservesExistingFile) +{ + int checks = 0; + EXPECT_EQ(SpoilerFile::save( + path, original + "\n", [&checks] { return ++checks == 1; }, error), + SpoilerFile::SaveResult::Failed); + EXPECT_EQ(checks, 2); + EXPECT_FALSE(error.isEmpty()); + EXPECT_EQ(read(), original); + EXPECT_EQ(QDir(directory.path()).entryList(QDir::Files), QStringList{"spoiler.xml"}); +} + +TEST_F(SpoilerFileTest, MissingParentIsCreated) +{ + path = directory.filePath("nested/spoiler.xml"); + EXPECT_EQ(save(original), SpoilerFile::SaveResult::Written); + EXPECT_EQ(read(), original); +} + +TEST_F(SpoilerFileTest, FileInParentPathReportsFailureWithoutModifyingIt) +{ + EXPECT_EQ(SpoilerFile::save( + QDir(path).filePath("spoiler.xml"), original, [] { return true; }, error), + SpoilerFile::SaveResult::Failed); + EXPECT_FALSE(error.isEmpty()); + EXPECT_EQ(read(), original); +} + +#ifdef Q_OS_UNIX +TEST_F(SpoilerFileTest, ShortWriteLeavesExistingFileIntact) +{ + // Limit only the child process: a real short write exercises QSaveFile's + // buffered flush failure without depending on disk capacity or file ownership. + const QByteArray changed = ""; + EXPECT_EXIT( + { + struct rlimit limit; + if (getrlimit(RLIMIT_FSIZE, &limit) != 0) { + _exit(2); + } + limit.rlim_cur = 128; + if (setrlimit(RLIMIT_FSIZE, &limit) != 0) { + _exit(3); + } + std::signal(SIGXFSZ, SIG_IGN); + const auto result = save(changed); + _exit(result == SpoilerFile::SaveResult::Failed && !error.isEmpty() ? 0 : 1); + }, + ::testing::ExitedWithCode(0), ""); + EXPECT_EQ(read(), original); + EXPECT_EQ(QDir(directory.path()).entryList(QDir::Files), QStringList{"spoiler.xml"}); +} +#endif +} // namespace