mirror of
https://github.com/Cockatrice/Cockatrice.git
synced 2026-09-28 00:42:19 -07:00
[Client] Exit deterministically when launching the update installer (#7309)
* [Client] Exit deterministically when launching the update installer * [Client] Only quit when the main window close is accepted Wait for MainWindow::close() to be accepted before quitting after the update installer is launched. When the close is vetoed (a running card DB update, open games, or an unsaved deck), keep running and tell the user the installer is already waiting, instead of exiting over their answer. Also fix the comment so it does not claim settings are saved on the vetoed path. * [Client] Only quit on update when the shutdown actually ran MainWindow::closeEvent has a static re-entrancy guard that returns early on a second close event, leaving it in its default accepted state. DlgUpdate reached from a nested event loop while a shutdown prompt was up could then get close() == true with no shutdown work done (no settings flush, no tab shutdown) and tear down the process behind an unanswered prompt. Add MainWindow::closeForUpdate(), which reports false when a close is already in progress or was vetoed, and gate the update-exit on it. * [Client] Use QCoreApplication::exit() to leave on update QCoreApplication::quit() only asks the application to quit and can be interrupted if a top-level window refuses its close event. On the update path the process must actually leave so its Qt DLLs are unlocked before the installer starts replacing files - apply the exit(0) variant both on the accepted-close path and the fallback where no MainWindow was ever closed. * [Client] Warn that the update installer force-closes within a minute Telling the user Cockatrice just 'stays open for now' is misleading: the installer launched by #7308 waits only about 60 s for a graceful close before it force-terminates the process, which would lose unsaved work. State the deadline and tell the user to save and close before then. * [Client] Warn that a vetoed update is cancelled, not forced The warning on the veto path promised the opposite of what the installer does: WaitForAppToClose in #7308 never force-closes cockatrice.exe ($AllowForceClose is 0), it aborts the update after 60 s so no unsaved data is lost. A user who is told Cockatrice will be terminated in a minute hurries to close it, only to find the update already given up. State the real outcome instead: the installer waits about a minute for a close, and the update is cancelled if that does not happen. * [Client] Warn about a waiting installer without blocking it CloseMatchingApps in #7308 sends the WM_CLOSE once and the wait loop only polls IsAppRunning afterwards, but QMessageBox::warning is application modal and QGuiApplicationPrivate::processCloseEvent drops spontaneous close events for windows blocked by a modal widget. The veto path therefore swallowed the installer's one and only request, so even a user who resolved the blocker and closed the app still ended in the 60 s timeout. Move the warning into warnInstallerIsWaiting() and show it modeless, so the main window stays able to answer the installer while the message is up. * [Client] Close the parent on the no-MainWindow update fallback The fallback branch exits the process without closing anything, so it skips the settings flush and tab shutdown this change exists to get in front of the installer - the opposite of the fix if a call site ever stops passing a MainWindow. Close the parent widget first, like the code before this change did, and only then leave unconditionally: the parent is the one that knows how to shut down gracefully, and the deterministic exit is what the installer needs. * Add override. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
This commit is contained in:
parent
28d684f8aa
commit
fa8220df3c
4 changed files with 80 additions and 3 deletions
|
|
@ -5,15 +5,25 @@
|
||||||
#include "../client/network/update/client/release_channel.h"
|
#include "../client/network/update/client/release_channel.h"
|
||||||
#include "../interface/window_main.h"
|
#include "../interface/window_main.h"
|
||||||
|
|
||||||
|
#include <QCoreApplication>
|
||||||
#include <QDesktopServices>
|
#include <QDesktopServices>
|
||||||
|
#include <QDir>
|
||||||
|
#include <QFileInfo>
|
||||||
#include <QLabel>
|
#include <QLabel>
|
||||||
#include <QMessageBox>
|
#include <QMessageBox>
|
||||||
#include <QProgressBar>
|
#include <QProgressBar>
|
||||||
#include <QPushButton>
|
#include <QPushButton>
|
||||||
|
#include <QTimer>
|
||||||
#include <QVBoxLayout>
|
#include <QVBoxLayout>
|
||||||
#include <QtNetwork>
|
#include <QtNetwork>
|
||||||
#include <version_string.h>
|
#include <version_string.h>
|
||||||
|
|
||||||
|
// Executable that, when it sits next to the downloaded update installer, is installed instead of
|
||||||
|
// it. A packager shipping a custom build - or someone testing one - can drop the file there and
|
||||||
|
// have Cockatrice run it rather than the official installer, without the release channel having to
|
||||||
|
// host an installer for that build. It is a drop-in and is run with the same arguments.
|
||||||
|
static const QString UPDATE_INSTALLER_OVERRIDE = "Cockatrice-Update-Override.exe";
|
||||||
|
|
||||||
DlgUpdate::DlgUpdate(QWidget *parent) : QDialog(parent)
|
DlgUpdate::DlgUpdate(QWidget *parent) : QDialog(parent)
|
||||||
{
|
{
|
||||||
|
|
||||||
|
|
@ -203,6 +213,24 @@ void DlgUpdate::setLabel(const QString &newText)
|
||||||
statusLabel->setText(newText);
|
statusLabel->setText(newText);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void DlgUpdate::warnInstallerIsWaiting()
|
||||||
|
{
|
||||||
|
// Modeless on purpose: the installer asks the application to close exactly once (see
|
||||||
|
// CloseMatchingApps in NSIS.template.in) and only polls afterwards, while Qt drops spontaneous
|
||||||
|
// close events - a WM_CLOSE from the installer - for windows that are blocked by a modal
|
||||||
|
// widget (QGuiApplicationPrivate::processCloseEvent). An application modal message box here
|
||||||
|
// would therefore swallow the installer's one and only request for the rest of its wait.
|
||||||
|
auto *notification = new QMessageBox(QMessageBox::Warning, tr("Update"),
|
||||||
|
tr("The update installer is already running and waits about a "
|
||||||
|
"minute for Cockatrice to close. Cockatrice is still busy, so "
|
||||||
|
"save your work and close it before then. Otherwise the "
|
||||||
|
"installer gives up and the update is cancelled."),
|
||||||
|
QMessageBox::Ok, parentWidget());
|
||||||
|
notification->setWindowModality(Qt::NonModal);
|
||||||
|
notification->setAttribute(Qt::WA_DeleteOnClose);
|
||||||
|
notification->show();
|
||||||
|
}
|
||||||
|
|
||||||
void DlgUpdate::updateCheckError(const QString &errorString)
|
void DlgUpdate::updateCheckError(const QString &errorString)
|
||||||
{
|
{
|
||||||
setLabel(tr("Error"));
|
setLabel(tr("Error"));
|
||||||
|
|
@ -224,6 +252,12 @@ void DlgUpdate::downloadSuccessful(const QUrl &filepath)
|
||||||
|
|
||||||
QString installerPath = filepath.toLocalFile();
|
QString installerPath = filepath.toLocalFile();
|
||||||
|
|
||||||
|
const QString overridePath = QDir(QFileInfo(installerPath).absolutePath()).filePath(UPDATE_INSTALLER_OVERRIDE);
|
||||||
|
if (QFileInfo::exists(overridePath)) {
|
||||||
|
qCInfo(DlgUpdateLog) << "Installing the update installer override instead of the download:" << overridePath;
|
||||||
|
installerPath = overridePath;
|
||||||
|
}
|
||||||
|
|
||||||
QString appDir = QDir::toNativeSeparators(QCoreApplication::applicationDirPath());
|
QString appDir = QDir::toNativeSeparators(QCoreApplication::applicationDirPath());
|
||||||
QProcess process;
|
QProcess process;
|
||||||
process.setProgram(installerPath);
|
process.setProgram(installerPath);
|
||||||
|
|
@ -240,8 +274,31 @@ void DlgUpdate::downloadSuccessful(const QUrl &filepath)
|
||||||
|
|
||||||
// Try to open the installer. If it opens, quit Cockatrice
|
// Try to open the installer. If it opens, quit Cockatrice
|
||||||
if (process.startDetached()) {
|
if (process.startDetached()) {
|
||||||
QMetaObject::invokeMethod(static_cast<MainWindow *>(parent()), "close", Qt::QueuedConnection);
|
qCInfo(DlgUpdateLog) << "Opened update installer successfully - closing Cockatrice";
|
||||||
qCInfo(DlgUpdateLog) << "Opened downloaded update file successfully - closing Cockatrice";
|
// Close the main window synchronously so file locks are released before the NSIS installer
|
||||||
|
// (already launched) starts replacing files. This also flushes settings and shuts down the
|
||||||
|
// tabs, but only when the close is actually accepted: MainWindow may veto it for a running
|
||||||
|
// card DB update, an open game, or an unsaved deck, and closeForUpdate() also reports a
|
||||||
|
// close already in progress (reached from a nested event loop while a shutdown prompt is
|
||||||
|
// up). Only quit when the shutdown really ran - otherwise keep running so the user can
|
||||||
|
// resolve the blocker, and warn them that the installer only waits about a minute
|
||||||
|
// before it gives up and cancels the update.
|
||||||
|
if (auto *window = qobject_cast<MainWindow *>(parent())) {
|
||||||
|
if (window->closeForUpdate()) {
|
||||||
|
QTimer::singleShot(0, qApp, [] { QCoreApplication::exit(0); });
|
||||||
|
} else {
|
||||||
|
warnInstallerIsWaiting();
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// Not a MainWindow, so no faithful close can be requested - but the parent still gets
|
||||||
|
// its close() call, which is what the code before this change did and lets it flush
|
||||||
|
// settings and shut down its tabs. Leaving is then unconditional: the installer is
|
||||||
|
// already running against a process that still holds locks on the files it replaces.
|
||||||
|
if (auto *widget = parentWidget()) {
|
||||||
|
widget->close();
|
||||||
|
}
|
||||||
|
QTimer::singleShot(0, qApp, [] { QCoreApplication::exit(0); });
|
||||||
|
}
|
||||||
close();
|
close();
|
||||||
} else {
|
} else {
|
||||||
setLabel(tr("Error"));
|
setLabel(tr("Error"));
|
||||||
|
|
|
||||||
|
|
@ -42,6 +42,7 @@ private:
|
||||||
void addStopDownloadAndRemoveOthers(bool enable);
|
void addStopDownloadAndRemoveOthers(bool enable);
|
||||||
void beginUpdateCheck();
|
void beginUpdateCheck();
|
||||||
void setLabel(const QString &text);
|
void setLabel(const QString &text);
|
||||||
|
void warnInstallerIsWaiting();
|
||||||
QLabel *statusLabel, *descriptionLabel;
|
QLabel *statusLabel, *descriptionLabel;
|
||||||
QProgressBar *progress;
|
QProgressBar *progress;
|
||||||
QPushButton *manualDownload, *gotoDownload, *ok, *stopDownload;
|
QPushButton *manualDownload, *gotoDownload, *ok, *stopDownload;
|
||||||
|
|
|
||||||
|
|
@ -871,7 +871,6 @@ void MainWindow::actShow()
|
||||||
void MainWindow::closeEvent(QCloseEvent *event)
|
void MainWindow::closeEvent(QCloseEvent *event)
|
||||||
{
|
{
|
||||||
// workaround Qt bug where closeEvent gets called twice
|
// workaround Qt bug where closeEvent gets called twice
|
||||||
static bool bClosingDown = false;
|
|
||||||
if (bClosingDown) {
|
if (bClosingDown) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
@ -900,6 +899,18 @@ void MainWindow::closeEvent(QCloseEvent *event)
|
||||||
tabSupervisor->deleteLater();
|
tabSupervisor->deleteLater();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
bool MainWindow::closeForUpdate()
|
||||||
|
{
|
||||||
|
// A shutdown is already being handled (e.g. this is reached from a nested event loop while
|
||||||
|
// closeEvent() is blocked on a user prompt). close() would hit the re-entrancy guard and return
|
||||||
|
// true without any shutdown having happened, so report faithfully instead: the caller must keep
|
||||||
|
// the process alive until the user resolves whatever is blocking the close.
|
||||||
|
if (bClosingDown) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
return close();
|
||||||
|
}
|
||||||
|
|
||||||
void MainWindow::changeEvent(QEvent *event)
|
void MainWindow::changeEvent(QEvent *event)
|
||||||
{
|
{
|
||||||
if (event->type() == QEvent::LanguageChange) {
|
if (event->type() == QEvent::LanguageChange) {
|
||||||
|
|
|
||||||
|
|
@ -167,6 +167,7 @@ private:
|
||||||
LagMonitor lagMonitor; ///< watches the main thread for event loop stalls
|
LagMonitor lagMonitor; ///< watches the main thread for event loop stalls
|
||||||
LatencyStatusWidget *latencyStatus = nullptr; ///< status bar widget with live round-trip stats and history graph
|
LatencyStatusWidget *latencyStatus = nullptr; ///< status bar widget with live round-trip stats and history graph
|
||||||
bool bHasActivated, askedForDbUpdater;
|
bool bHasActivated, askedForDbUpdater;
|
||||||
|
bool bClosingDown = false; ///< guards closeEvent() against re-entrancy
|
||||||
bool skipStartupAutoConnect = false;
|
bool skipStartupAutoConnect = false;
|
||||||
bool startupAutoConnectAttempted = false;
|
bool startupAutoConnectAttempted = false;
|
||||||
bool firstRunWizardActive = false;
|
bool firstRunWizardActive = false;
|
||||||
|
|
@ -205,6 +206,13 @@ public:
|
||||||
return tabSupervisor;
|
return tabSupervisor;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @brief Closes the window so an update installer can replace the running binaries.
|
||||||
|
* Returns true only if the shutdown actually ran (settings flushed, tabs shut down);
|
||||||
|
* false if the close was vetoed by the user or is already in progress.
|
||||||
|
*/
|
||||||
|
bool closeForUpdate();
|
||||||
|
|
||||||
protected:
|
protected:
|
||||||
void closeEvent(QCloseEvent *event) override;
|
void closeEvent(QCloseEvent *event) override;
|
||||||
void changeEvent(QEvent *event) override;
|
void changeEvent(QEvent *event) override;
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue