From 677692500b7cdacad6033ffce8b3d4c3e5dc2731 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Thu, 15 Oct 2026 11:35:03 +0200 Subject: [PATCH 1/4] [Game] Raise rubber band above count label and collapse multi-selection on click (#7451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Game] Raise rubber band above count label and collapse multi-selection on click Raise rubber band on start and collapse multi-selection to clicked card when left-clicking an already-selected card with no modifiers. * [Game] Collapse multi-selection on release instead of press Collapsing at press time dropped the selection before the drag started, which broke dragging multiple cards. Keep the selection on press and collapse it on release when the pointer stayed within the drag distance. * [Game] Defer selection collapse until the double-click window passes A release without drag collapsed the multi-selection immediately, so a double-click that followed (playSelected acts on scene()->selectedItems()) only ever saw the single clicked card. When the card handles double-clicks on the selection, arm the collapse on a single-shot timer for QApplication::doubleClickInterval() instead and cancel it from the double-click handler; a new press disarms the other cards and a selection-count guard covers rubber-band/ctrl changes meanwhile. * [Game] Keep sideboard multi-selection on double-click DeckViewCard overrides doubleClickActsOnSelection to preserve the selection, restoring master behavior where double-clicking moves all selected cards between main and side. DeckView cancels any pending collapse before reading the selection. * [Game] Remove duplicated inline selection-collapse declarations * [Game] Fix DeckView collapse call visibility and multi-selection collapse Make cancelPendingSelectionCollapse() public so DeckView can call it on selected DeckViewCards. Always arm the deferred collapse on release instead of skipping cards whose double-click acts on the selection; the double-click-interval timer plus the cancel calls in CardItem::mouseDoubleClickEvent and DeckView::mouseDoubleClickEvent already preserve the selection for double-clicks. --------- Co-authored-by: Lukas Brübach --- .../board/abstract_card_item.cpp | 55 ++++++++++++++++++- .../game_graphics/board/abstract_card_item.h | 14 +++++ .../src/game_graphics/board/card_item.cpp | 8 +++ .../src/game_graphics/board/card_item.h | 1 + .../src/game_graphics/deckview/deck_view.cpp | 9 +++ .../src/game_graphics/deckview/deck_view.h | 4 ++ cockatrice/src/game_graphics/game_view.cpp | 1 + 7 files changed, 91 insertions(+), 1 deletion(-) diff --git a/cockatrice/src/game_graphics/board/abstract_card_item.cpp b/cockatrice/src/game_graphics/board/abstract_card_item.cpp index a79c5873e..cc121cfb2 100644 --- a/cockatrice/src/game_graphics/board/abstract_card_item.cpp +++ b/cockatrice/src/game_graphics/board/abstract_card_item.cpp @@ -12,6 +12,7 @@ #include "libcockatrice/card/printing/exact_card.h" #include "libcockatrice/utility/card_ref.h" +#include #include #include #include @@ -28,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -42,7 +44,7 @@ class QSizeF; AbstractCardItem::AbstractCardItem(QGraphicsItem *parent, const CardRef &cardRef, PlayerLogic *_owner, int _id) : ArrowTarget(_owner, parent), id(_id), cardRef(cardRef), tapped(false), facedown(false), tapAngle(0), - bgColor(Qt::transparent), isHovered(false), realZValue(0) + bgColor(Qt::transparent), isHovered(false), realZValue(0), pendingSelectionCollapse(false) { setCursor(Qt::OpenHandCursor); setFlag(ItemIsSelectable); @@ -344,6 +346,16 @@ void AbstractCardItem::setFaceDown(bool _facedown) void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) { + cancelPendingSelectionCollapse(); + if (scene() != nullptr) { + // A new press starts a new gesture: no card may collapse the selection out from under it afterwards. + for (QGraphicsItem *selectedItem : scene()->selectedItems()) { + auto *selectedCard = qgraphicsitem_cast(selectedItem); + if (selectedCard != nullptr && selectedCard != this) { + selectedCard->cancelPendingSelectionCollapse(); + } + } + } if ((event->modifiers() & Qt::AltModifier) && event->button() == Qt::LeftButton) { emit cardShiftClicked(cardRef.name); } else if ((event->modifiers() & Qt::ControlModifier)) { @@ -351,6 +363,10 @@ void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) } else if (!isSelected()) { scene()->clearSelection(); setSelected(true); + } else if (event->button() == Qt::LeftButton && scene()->selectedItems().count() > 1) { + // Part of a multi-selection; collapse only if this turns out to be a click rather than a drag, + // so that dragging the whole selection keeps it intact. + pendingSelectionCollapse = true; } if (event->button() == Qt::LeftButton) { setCursor(Qt::ClosedHandCursor); @@ -362,6 +378,32 @@ void AbstractCardItem::mousePressEvent(QGraphicsSceneMouseEvent *event) void AbstractCardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) { + if (pendingSelectionCollapse) { + pendingSelectionCollapse = false; + bool dragged = (event->screenPos() - event->buttonDownScreenPos(event->button())).manhattanLength() >= + 2 * QApplication::startDragDistance(); + if (!dragged) { + if (doubleClickActsOnSelection()) { + // A double-click may still arrive and must act on the whole multi-selection, so collapse only + // once the double-click window has passed without one. The generation guard (and the receiver + // context) drops the collapse if the item is gone, deselected, or a double-click cancelled it; + // the count guard drops it if the selection changed meanwhile (rubber band, ctrl-toggle). + const int generation = ++selectionCollapseGeneration; + const int armedSelectionCount = scene()->selectedItems().count(); + QTimer::singleShot(QApplication::doubleClickInterval(), this, [this, generation, armedSelectionCount] { + if (generation == selectionCollapseGeneration && scene() != nullptr && isSelected() && + scene()->selectedItems().count() == armedSelectionCount && armedSelectionCount > 1) { + scene()->clearSelection(); + setSelected(true); + } + }); + } else { + scene()->clearSelection(); + setSelected(true); + } + } + } + if (event->button() == Qt::MiddleButton) { emit deleteCardInfoPopup(cardRef.name); } @@ -370,6 +412,17 @@ void AbstractCardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) event->accept(); } +void AbstractCardItem::cancelPendingSelectionCollapse() +{ + pendingSelectionCollapse = false; + ++selectionCollapseGeneration; +} + +bool AbstractCardItem::doubleClickActsOnSelection() const +{ + return false; +} + void AbstractCardItem::processHoverEvent() { emit hovered(this); diff --git a/cockatrice/src/game_graphics/board/abstract_card_item.h b/cockatrice/src/game_graphics/board/abstract_card_item.h index 385f9c32f..e2b150ce8 100644 --- a/cockatrice/src/game_graphics/board/abstract_card_item.h +++ b/cockatrice/src/game_graphics/board/abstract_card_item.h @@ -41,6 +41,8 @@ protected: private: bool isHovered; qreal realZValue; + bool pendingSelectionCollapse; + int selectionCollapseGeneration = 0; ///< Guards the deferred collapse timer against stale firings private slots: void pixmapUpdated(); @@ -130,6 +132,8 @@ public: } void setFaceDown(bool _facedown); void processHoverEvent(); + /** @brief Drop a pending collapse and disarm any deferred collapse timer. */ + void cancelPendingSelectionCollapse(); void deleteCardInfoPopup() { emit deleteCardInfoPopup(cardRef.name); @@ -144,6 +148,16 @@ protected: void mouseReleaseEvent(QGraphicsSceneMouseEvent *event) override; QVariant itemChange(QGraphicsItem::GraphicsItemChange change, const QVariant &value) override; void cacheBgColor(); + /** + * @brief Whether a double-click on this item acts on the whole selection. + * + * When true, a release without drag defers the multi-selection collapse + * until the double-click window passes, so a following double-click still + * sees every selected card. Subclasses that handle double-clicks on the + * selection must return true here and call + * cancelPendingSelectionCollapse() from their double-click handler. + */ + virtual bool doubleClickActsOnSelection() const; }; #endif diff --git a/cockatrice/src/game_graphics/board/card_item.cpp b/cockatrice/src/game_graphics/board/card_item.cpp index a04d86c15..1901a634f 100644 --- a/cockatrice/src/game_graphics/board/card_item.cpp +++ b/cockatrice/src/game_graphics/board/card_item.cpp @@ -518,6 +518,9 @@ void CardItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event) void CardItem::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) { + // The preceding release deferred its multi-selection collapse for exactly this: the double-click plays + // every selected card, so the collapse must not run. + cancelPendingSelectionCollapse(); if ((event->modifiers() != Qt::AltModifier) && (event->buttons() == Qt::LeftButton) && (SettingsCache::instance().userInterface().getDoubleClickToPlay())) { handleClickedToPlay(event->modifiers().testFlag(Qt::ShiftModifier)); @@ -525,6 +528,11 @@ void CardItem::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) event->accept(); } +bool CardItem::doubleClickActsOnSelection() const +{ + return SettingsCache::instance().userInterface().getDoubleClickToPlay(); +} + bool CardItem::animationEvent() { int rotation = ROTATION_DEGREES_PER_FRAME; diff --git a/cockatrice/src/game_graphics/board/card_item.h b/cockatrice/src/game_graphics/board/card_item.h index df79ef378..0737d7f9c 100644 --- a/cockatrice/src/game_graphics/board/card_item.h +++ b/cockatrice/src/game_graphics/board/card_item.h @@ -175,6 +175,7 @@ protected: void mouseReleaseEvent(QGraphicsSceneMouseEvent *event) override; void mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event) override; QVariant itemChange(GraphicsItemChange change, const QVariant &value) override; + bool doubleClickActsOnSelection() const override; }; #endif diff --git a/cockatrice/src/game_graphics/deckview/deck_view.cpp b/cockatrice/src/game_graphics/deckview/deck_view.cpp index c86bc9925..630530792 100644 --- a/cockatrice/src/game_graphics/deckview/deck_view.cpp +++ b/cockatrice/src/game_graphics/deckview/deck_view.cpp @@ -182,6 +182,15 @@ void DeckView::mouseDoubleClickEvent(QMouseEvent *event) QList result; QList sel = scene()->selectedItems(); + // The double-click goes to the view, not the card, so cancel any pending + // selection collapse on the cards before reading the selection. + for (auto *item : sel) { + if (auto *c = dynamic_cast(item)) { + c->cancelPendingSelectionCollapse(); + } + } + sel = scene()->selectedItems(); + for (int i = 0; i < sel.size(); i++) { auto *c = static_cast(sel.at(i)); auto *zone = static_cast(c->parentItem()); diff --git a/cockatrice/src/game_graphics/deckview/deck_view.h b/cockatrice/src/game_graphics/deckview/deck_view.h index 3b5b4b6f4..31cb0f87f 100644 --- a/cockatrice/src/game_graphics/deckview/deck_view.h +++ b/cockatrice/src/game_graphics/deckview/deck_view.h @@ -49,6 +49,10 @@ public: { return originZone; } + bool doubleClickActsOnSelection() const override + { + return true; + } protected: void mouseMoveEvent(QGraphicsSceneMouseEvent *event) override; diff --git a/cockatrice/src/game_graphics/game_view.cpp b/cockatrice/src/game_graphics/game_view.cpp index 1c92d74ae..cb59b3d7d 100644 --- a/cockatrice/src/game_graphics/game_view.cpp +++ b/cockatrice/src/game_graphics/game_view.cpp @@ -127,6 +127,7 @@ void GameView::startRubberBand(const QPointF &_selectionOrigin) selectionOrigin = _selectionOrigin; previousBandRect = QRect(); rubberBand->setGeometry(QRect(mapFromScene(selectionOrigin), QSize(0, 0))); + rubberBand->raise(); rubberBand->show(); } From fec693ca2a8274054eb9fb1518dd878eb6cdc148 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Thu, 15 Oct 2026 12:21:40 +0200 Subject: [PATCH 2/4] [Tests] Scope loader matcher settings to the process to fix parallel-run flakes (#7504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Tests] Scope loader matcher settings to the process to fix parallel-run flakes * [Tests] Clean up per-process settings sandbox after loader matcher run * [Tests] Skip loader matcher sandbox cleanup during gtest discovery --------- Co-authored-by: Lukas Brübach --- tests/loader_local_matching_test.cpp | 38 +++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/tests/loader_local_matching_test.cpp b/tests/loader_local_matching_test.cpp index 13d3e5bb3..7632e60d4 100644 --- a/tests/loader_local_matching_test.cpp +++ b/tests/loader_local_matching_test.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -223,8 +224,43 @@ int main(int argc, char **argv) } QCoreApplication app(argc, argv); + + // Every TEST() runs as its own process and CI runs them in parallel (ctest -j4), + // while the picture paths round-trip through the process-global SettingsCache + // file. Parallel cases must therefore never share one settings directory: on + // platforms where QStandardPaths ignores the $HOME override above (macOS + // resolves the home directory via system APIs), every case would otherwise read + // and write the same paths.ini, and a sibling's setPicsPath can land between our + // own set and the loader's get — so the loader searches a foreign (or empty) + // directory and flakily returns a null image. Scope the settings to this process + // with a pid-unique application name; PIDs are never reused by live processes. + QCoreApplication::setOrganizationName(QStringLiteral("CockatriceTest")); + QCoreApplication::setApplicationName( + QStringLiteral("loader-local-matching-test-%1").arg(QCoreApplication::applicationPid())); + QLoggingCategory::setFilterRules("card_picture_loader.*=false\nsettings_cache.*=false"); ::testing::InitGoogleTest(&argc, argv); + + // CMake's gtest discovery runs this binary with --gtest_list_tests, which lists + // without running any test body. Arming the cleanup guard here would make it the + // sole SettingsCache::instance() call in that mode, instantiating the + // Q_GLOBAL_STATIC singleton (and its QNetworkAccessManagers) just to tear down at + // exit — which LeakSanitizer reports as leaks and fails discovery on the sanitizer + // leg. Skip the sandbox cleanup entirely when only listing. + if (::testing::GTEST_FLAG(list_tests)) { + return RUN_ALL_TESTS(); + } + + // Best-effort cleanup of this process's settings sandbox. QStandardPaths test + // mode is never auto-cleaned, and on macOS the $HOME override above is ignored, + // so without this every run litters ~/.qttest with one pid-unique directory. + // Safe under parallel runs: the pid-unique application name above guarantees + // this directory belongs to this process alone. + const auto cleanupSandbox = qScopeGuard([] { + QDir(SettingsCache::instance().getDataPath()).removeRecursively(); + QDir(SettingsCache::instance().getCachePath()).removeRecursively(); + }); + return RUN_ALL_TESTS(); -} \ No newline at end of file +} From bbf0425655b3ed4b58effb024a87c9d0569130f4 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Thu, 15 Oct 2026 12:41:01 +0200 Subject: [PATCH 3/4] [Style] Confirm broken Windows styles with a dialog offering Fusion (#7507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Style] Confirm broken Windows styles with a dialog offering Fusion * [Style] Address review nits on broken style dialog Extract Fusion index lookup into findFusionIndex helper and add all message box buttons before choosing the default button. --------- Co-authored-by: Lukas Brübach --- .../appearance_settings_page.cpp | 98 +++++++++++++++++-- .../settings_page/appearance_settings_page.h | 2 + 2 files changed, 90 insertions(+), 10 deletions(-) diff --git a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp index 4c38459c6..be2d5735e 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -84,10 +85,9 @@ AppearanceSettingsPage::AppearanceSettingsPage() const QString currentStyle = cfg.styleName; const int styleSeedIdx = currentStyle.isEmpty() ? 0 : styleCombo.findData(currentStyle); styleCombo.setCurrentIndex(styleSeedIdx >= 0 ? styleSeedIdx : 0); + previousStyle = styleCombo.currentData().toString(); - connect(&styleCombo, &QComboBox::currentIndexChanged, this, - [this] { themeManager->setStyleName(styleCombo.currentData().toString()); }); - connect(&styleCombo, &QComboBox::currentIndexChanged, this, &AppearanceSettingsPage::updateStyleWarning); + connect(&styleCombo, &QComboBox::currentIndexChanged, this, &AppearanceSettingsPage::styleComboChanged); connect(themeManager, &ThemeManager::themeChanged, this, [this, dirPath] { const QString newDir = themeManager->getAvailableThemes().value(SettingsCache::instance().getThemeName()); @@ -104,6 +104,7 @@ AppearanceSettingsPage::AppearanceSettingsPage() const int styleIdx = currentStyle.isEmpty() ? 0 : styleCombo.findData(currentStyle); styleCombo.setCurrentIndex(styleIdx >= 0 ? styleIdx : 0); styleCombo.blockSignals(false); + previousStyle = styleCombo.currentData().toString(); updateStyleWarning(); }); @@ -467,12 +468,88 @@ void AppearanceSettingsPage::updateHomeTabSettingsVisibility() homeTabDisplayCardNameCheckBox.setVisible(visible); } +static bool isBrokenStyle(const QString &style) +{ + return style.compare("windows11", Qt::CaseInsensitive) == 0 || + style.compare("windowsvista", Qt::CaseInsensitive) == 0; +} + +static int findFusionIndex(const QComboBox &combo) +{ + int fusionIdx = combo.findData(QStringLiteral("Fusion")); + if (fusionIdx >= 0) { + return fusionIdx; + } + for (int i = 0; i < combo.count(); ++i) { + if (combo.itemData(i).toString().compare(QStringLiteral("Fusion"), Qt::CaseInsensitive) == 0) { + return i; + } + } + return -1; +} + void AppearanceSettingsPage::updateStyleWarning() { - const QString style = styleCombo.currentData().toString(); - const bool broken = - style.compare("windows11", Qt::CaseInsensitive) == 0 || style.compare("windowsvista", Qt::CaseInsensitive) == 0; - styleWarningLabel.setVisible(broken); + styleWarningLabel.setVisible(isBrokenStyle(styleCombo.currentData().toString())); +} + +void AppearanceSettingsPage::styleComboChanged(int index) +{ + if (index < 0) { + return; + } + + const QString newStyle = styleCombo.itemData(index).toString(); + if (!isBrokenStyle(newStyle)) { + themeManager->setStyleName(newStyle); + previousStyle = newStyle; + updateStyleWarning(); + return; + } + + QMessageBox msgBox(this); + msgBox.setWindowTitle(tr("Broken style")); + msgBox.setIcon(QMessageBox::Warning); + msgBox.setText(tr("The \"%1\" style is broken and can break the game view: dragging cards across zones can " + "shrink the board to a tiny grey window that can only be fixed by rejoining the game.") + .arg(newStyle)); + msgBox.setInformativeText( + tr("It can also render controls such as combo boxes unreadable, especially in dark mode.\n\n" + "Fusion is the recommended style on every platform. Switch to Fusion instead?")); + + int fusionIdx = findFusionIndex(styleCombo); + QPushButton *fusionButton = nullptr; + if (fusionIdx >= 0) { + fusionButton = msgBox.addButton(tr("Use Fusion instead"), QMessageBox::AcceptRole); + } + QPushButton *keepButton = msgBox.addButton(tr("Use %1 anyway").arg(newStyle), QMessageBox::DestructiveRole); + msgBox.addButton(QMessageBox::Cancel); + if (fusionButton != nullptr) { + msgBox.setDefaultButton(fusionButton); + } else { + msgBox.setDefaultButton(keepButton); + } + + msgBox.exec(); + + const QAbstractButton *clicked = msgBox.clickedButton(); + if (fusionButton != nullptr && clicked == fusionButton) { + styleCombo.setCurrentIndex(fusionIdx); + return; + } + if (clicked == keepButton) { + themeManager->setStyleName(newStyle); + previousStyle = newStyle; + updateStyleWarning(); + return; + } + + // Cancel: revert to the previously applied style without re-prompting + styleCombo.blockSignals(true); + const int previousIdx = styleCombo.findData(previousStyle); + styleCombo.setCurrentIndex(previousIdx >= 0 ? previousIdx : 0); + styleCombo.blockSignals(false); + updateStyleWarning(); } void AppearanceSettingsPage::showShortcutsChanged(QT_STATE_CHANGED_T value) @@ -538,9 +615,10 @@ void AppearanceSettingsPage::retranslateUi() styleComboLabel.setText(tr("Active theme style:")); styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"System\" lets the application decide)")); styleWarningLabel.setText( - tr("Warning: the Windows 11 and Windows Vista styles are buggy and can render controls such as combo boxes " - "unreadable, especially in dark mode. Fusion is the recommended style on every platform: it is Qt's " - "cross-platform style and the one Cockatrice is developed against.")); + tr("Warning: the Windows 11 and Windows Vista styles are broken and can break the game view (dragging " + "cards can shrink the board to a tiny grey window that requires rejoining to fix), and can render " + "controls such as combo boxes unreadable, especially in dark mode. Fusion is the recommended style on " + "every platform: it is Qt's cross-platform style and the one Cockatrice is developed against.")); updateStyleWarning(); editPaletteButton.setText(tr("Edit theme palette")); diff --git a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h index 8afd326a3..f919a2a2f 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.h @@ -23,6 +23,7 @@ private slots: void editPalette(); void updateHomeTabSettingsVisibility(); void updateStyleWarning(); + void styleComboChanged(int index); void showShortcutsChanged(QT_STATE_CHANGED_T enabled); void overrideAllCardArtWithPersonalPreferenceToggled(QT_STATE_CHANGED_T enabled); @@ -40,6 +41,7 @@ private: QComboBox styleCombo; QLabel styleWarningLabel; QPushButton editPaletteButton; + QString previousStyle; QLabel homeTabBackgroundSourceLabel; QComboBox homeTabBackgroundSourceBox; From 96b0888a7365744ee26e845fe0d389a98132d0b4 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:21:07 +0200 Subject: [PATCH 4/4] [Oracle] Don't report a cancelled card database update as successful (#7082) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Oracle] Don't report a cancelled card database update as successful (#3470) Took 3 minutes * [Oracle] Only report cancel when nothing was saved yet Rejecting the wizard after SaveSetsPage or a download page already wrote the database to disk still replaced cards.xml on disk, but wasCancelled() forced exit code 1 so Cockatrice skipped the reload. Track successful saves with OracleWizard::markDatabaseSaved() and only treat a cancel as a failure when nothing was saved. * [Oracle] Restore updater stderr in card update error dialog --------- Co-authored-by: Lukas Brübach --- cockatrice/src/interface/window_main.cpp | 20 ++++++++++++-------- cockatrice/src/interface/window_main.h | 2 +- oracle/src/main.cpp | 5 ++++- oracle/src/oraclewizard.cpp | 8 ++++++++ oracle/src/oraclewizard.h | 15 +++++++++++++++ oracle/src/pages.cpp | 5 +++++ oracle/src/pagetemplates.cpp | 5 +++++ 7 files changed, 50 insertions(+), 10 deletions(-) diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 815cec1d2..57d6e8b15 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -1253,7 +1253,7 @@ void MainWindow::createCardUpdateProcess(bool background) << dir.absoluteFilePath(binaryName); QMessageBox::warning(this, tr("Error"), tr("Unable to run the card database updater: ") + dir.absoluteFilePath(binaryName)); - exitCardDatabaseUpdate(); + exitCardDatabaseUpdate(false); emit cardDatabaseUpdateFinished(false); return; } @@ -1330,7 +1330,7 @@ void MainWindow::cardUpdateStandardError() } } -void MainWindow::exitCardDatabaseUpdate() +void MainWindow::exitCardDatabaseUpdate(bool doReload) { if (!cardUpdateProcess) { return; @@ -1339,9 +1339,11 @@ void MainWindow::exitCardDatabaseUpdate() cardUpdateProcess = nullptr; statusBar()->clearMessage(); - // This can be the first successful load of the session (e.g. the onboarding - // download), so it must own the set check for that load. - reloadCardDatabaseAndCheckSets(); + if (doReload) { + // This can be the first successful load of the session (e.g. the onboarding + // download), so it must own the set check for that load. + reloadCardDatabaseAndCheckSets(); + } } void MainWindow::reloadCardDatabaseAndCheckSets() @@ -1387,7 +1389,7 @@ void MainWindow::cardUpdateError(QProcess::ProcessError err) } qCWarning(CardDatabaseUpdateLog) << "The card database updater exited with an error:" << error; - exitCardDatabaseUpdate(); + exitCardDatabaseUpdate(false); QMessageBox::warning(this, tr("Error"), tr("The card database updater exited with an error:\n%1").arg(error)); emit cardDatabaseUpdateFinished(false); } @@ -1400,12 +1402,14 @@ void MainWindow::cardUpdateFinished(int exitCode, QProcess::ExitStatus exitStatu cardUpdateProgressOutput(); // drain any progress lines not yet parsed const bool success = (exitStatus == QProcess::NormalExit) && (exitCode == 0); - if (exitStatus == QProcess::NormalExit) { + if (success) { SettingsCache::instance().updates().setLastCardUpdateCheck(QDateTime::currentDateTime().date()); + } else if (exitStatus == QProcess::NormalExit) { + statusBar()->showMessage(tr("Card database update cancelled."), 10000); } qCInfo(CardDatabaseUpdateLog) << "The card database updater finished with exit code" << exitCode << "exit status" << static_cast(exitStatus) << "success:" << success; - exitCardDatabaseUpdate(); + exitCardDatabaseUpdate(success); notifyCardUpdateFinishedFromTray(success); emit cardDatabaseUpdateFinished(success); } diff --git a/cockatrice/src/interface/window_main.h b/cockatrice/src/interface/window_main.h index ee8873000..2b6ed5633 100644 --- a/cockatrice/src/interface/window_main.h +++ b/cockatrice/src/interface/window_main.h @@ -157,7 +157,7 @@ private: return "oracle"; } void createCardUpdateProcess(bool background = false); - void exitCardDatabaseUpdate(); + void exitCardDatabaseUpdate(bool doReload); /** * @brief Reloads the card database on a worker thread and runs checkUnknownSets() once the finished * snapshot has been swapped in. diff --git a/oracle/src/main.cpp b/oracle/src/main.cpp index cae2ee37d..bf56df5c9 100644 --- a/oracle/src/main.cpp +++ b/oracle/src/main.cpp @@ -85,5 +85,8 @@ int main(int argc, char *argv[]) QTimer::singleShot(0, &wizard, [&wizard]() { wizard.runInBackground(); }); } - return app.exec(); + const int execResult = app.exec(); + // A cancel only counts when nothing was written yet: closing the wizard + // after the card database was already saved on disk is still an update. + return (wizard.wasCancelled() && !wizard.hasSavedDatabase()) ? 1 : execResult; } diff --git a/oracle/src/oraclewizard.cpp b/oracle/src/oraclewizard.cpp index 05f08ce64..d2b3a71a4 100644 --- a/oracle/src/oraclewizard.cpp +++ b/oracle/src/oraclewizard.cpp @@ -128,12 +128,20 @@ void OracleWizard::accept() void OracleWizard::reject() { + cancelled = true; + // The wizard is being closed while a page may still run a worker on the // importer. Ask it to stop before the wizard (and the importer child) is // destroyed, so the worker thread never touches freed memory. if (auto *active = dynamic_cast(currentPage())) { active->cancelWork(); } + + const auto replies = nam->findChildren(); + for (QNetworkReply *reply : replies) { + reply->abort(); + } + QWizard::reject(); } diff --git a/oracle/src/oraclewizard.h b/oracle/src/oraclewizard.h index ea652bdc5..b1dc5eab2 100644 --- a/oracle/src/oraclewizard.h +++ b/oracle/src/oraclewizard.h @@ -20,6 +20,19 @@ public: explicit OracleWizard(QWidget *parent = nullptr); void accept() override; void reject() override; + bool wasCancelled() + { + return cancelled; + } + /** @brief Records that the card database was written to disk, so a later cancel still counts as an update. */ + void markDatabaseSaved() + { + databaseSaved = true; + } + bool hasSavedDatabase() + { + return databaseSaved; + } void enableButtons(); void disableButtons(); void retranslateUi(); @@ -67,6 +80,8 @@ private: QByteArray tokensData; QString cardSourceUrl; QString cardSourceVersion; + bool cancelled = false; + bool databaseSaved = false; void migrateOracleSettings(); /** diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index 8c286fb46..ed8e04d1a 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -484,6 +484,10 @@ void LoadSetsPage::actDownloadFinishedSetsFile() { // check for a reply auto *reply = dynamic_cast(sender()); + if (wizard()->wasCancelled()) { + reply->deleteLater(); + return; + } auto errorCode = reply->error(); if (errorCode != QNetworkReply::NoError) { QMessageBox::critical(this, tr("Error"), tr("Network error: %1.").arg(reply->errorString())); @@ -895,6 +899,7 @@ bool SaveSetsPage::validatePage() return false; } + wizard()->markDatabaseSaved(); return true; } diff --git a/oracle/src/pagetemplates.cpp b/oracle/src/pagetemplates.cpp index d37818dcb..dcc8e070c 100644 --- a/oracle/src/pagetemplates.cpp +++ b/oracle/src/pagetemplates.cpp @@ -178,6 +178,10 @@ void SimpleDownloadFilePage::actDownloadFinished() { // check for a reply auto *reply = dynamic_cast(sender()); + if (wizard()->wasCancelled()) { + reply->deleteLater(); + return; + } QNetworkReply::NetworkError errorCode = reply->error(); if (errorCode != QNetworkReply::NoError) { QMessageBox::critical(this, tr("Error"), tr("Network error: %1.").arg(reply->errorString())); @@ -242,6 +246,7 @@ bool SimpleDownloadFilePage::saveToFile() // clean saved downloadData downloadData = QByteArray(); + wizard()->markDatabaseSaved(); return true; }