From 11562ade7a06522bc7c93d28393bdb052e909d2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Basile=20Cl=C3=A9ment?= Date: Mon, 17 Mar 2025 17:48:11 +0100 Subject: [PATCH] Avoid double `close` in `TabSupervisor` In #5447, the `TabSupervisor` logic was changed to go through `closeRequest` instead of `deleteLater`. Unfortunately, this introduced a bug where we close every (most) tabs twice due to `closeRequest` calling `close()`, which is a high-level widget function that triggers a whole lot of events, including `QHideEvent`s, which manifests through #5697. #5735 was a tentative fix for the issue, but it mis-diagnosed the problem and does not actually fix it, as shown by #5740. This patch makes `closeRequest` call `deleteLater` instead, which is a low-level function that doesn't trigger all those events. To make sure slots happening on tab close are still processed, use the `QObject::destroyed` signal instead of the custom `Tab::closed` event. This also fixes what I believe was the missing piece in #5447: `TabSupervisor`'s destructor calling `TabSupervisor::stop`, which in turn sends out a bunch of signals which is never a good idea while in the middle of destruction. Instead, `TabSupervisor`'s destructor now simply deletes its (non-`QObject`) children. I suspect that with `TabSupervisor`'s destructor no longer calling `TabSupervisor::stop` the `closeRequest` API can be simplified again; this is left for later as this PR is focused on fixing the crashes. Supersedes #5735 and #5740. Fixes #5697 (again). --- .../client/tabs/abstract_tab_deck_editor.cpp | 4 +- cockatrice/src/client/tabs/tab.cpp | 13 +----- cockatrice/src/client/tabs/tab.h | 7 ---- cockatrice/src/client/tabs/tab_game.cpp | 2 +- cockatrice/src/client/tabs/tab_message.cpp | 2 +- cockatrice/src/client/tabs/tab_room.cpp | 2 +- cockatrice/src/client/tabs/tab_supervisor.cpp | 40 +++++++++---------- 7 files changed, 25 insertions(+), 45 deletions(-) diff --git a/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp b/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp index 92e63230b..3df317cf7 100644 --- a/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp +++ b/cockatrice/src/client/tabs/abstract_tab_deck_editor.cpp @@ -554,5 +554,5 @@ void AbstractTabDeckEditor::closeRequest(bool forced) } emit deckEditorClosing(this); - close(); -} \ No newline at end of file + deleteLater(); +} diff --git a/cockatrice/src/client/tabs/tab.cpp b/cockatrice/src/client/tabs/tab.cpp index d31b86bc9..5c27f9dfd 100644 --- a/cockatrice/src/client/tabs/tab.cpp +++ b/cockatrice/src/client/tabs/tab.cpp @@ -44,16 +44,7 @@ void Tab::deleteCardInfoPopup(const QString &cardName) } } -/** - * Overrides the closeEvent in order to emit a close signal - */ -void Tab::closeEvent(QCloseEvent *event) -{ - emit closed(); - event->accept(); -} - void Tab::closeRequest(bool /*forced*/) { - close(); -} \ No newline at end of file + deleteLater(); +} diff --git a/cockatrice/src/client/tabs/tab.h b/cockatrice/src/client/tabs/tab.h index f82f9a7b2..cec40c373 100644 --- a/cockatrice/src/client/tabs/tab.h +++ b/cockatrice/src/client/tabs/tab.h @@ -13,12 +13,6 @@ class Tab : public QMainWindow signals: void userEvent(bool globalEvent = true); void tabTextChanged(Tab *tab, const QString &newTabText); - /** - * Emitted when the tab is closed (because Qt doesn't provide a built-in close signal) - * This signal is emitted from this class's overridden Tab::closeEvent method. - * Make sure any subclasses that override closeEvent still emit this signal from there. - */ - void closed(); protected: TabSupervisor *tabSupervisor; @@ -29,7 +23,6 @@ protected: protected slots: void showCardInfoPopup(const QPoint &pos, const QString &cardName, const QString &providerId); void deleteCardInfoPopup(const QString &cardName); - void closeEvent(QCloseEvent *event) override; private: QString currentCardName, currentProviderId; diff --git a/cockatrice/src/client/tabs/tab_game.cpp b/cockatrice/src/client/tabs/tab_game.cpp index 1b2dc0327..940c9e717 100644 --- a/cockatrice/src/client/tabs/tab_game.cpp +++ b/cockatrice/src/client/tabs/tab_game.cpp @@ -405,7 +405,7 @@ void TabGame::closeRequest(bool forced) emit gameClosing(this); - close(); + deleteLater(); } void TabGame::replayNextEvent(Player::EventProcessingOptions options) diff --git a/cockatrice/src/client/tabs/tab_message.cpp b/cockatrice/src/client/tabs/tab_message.cpp index 66e2e3582..775310740 100644 --- a/cockatrice/src/client/tabs/tab_message.cpp +++ b/cockatrice/src/client/tabs/tab_message.cpp @@ -89,7 +89,7 @@ QString TabMessage::getTabText() const void TabMessage::closeRequest(bool /*forced*/) { emit talkClosing(this); - close(); + deleteLater(); } void TabMessage::sendMessage() diff --git a/cockatrice/src/client/tabs/tab_room.cpp b/cockatrice/src/client/tabs/tab_room.cpp index 02243cd19..80e181e6c 100644 --- a/cockatrice/src/client/tabs/tab_room.cpp +++ b/cockatrice/src/client/tabs/tab_room.cpp @@ -175,7 +175,7 @@ void TabRoom::closeRequest(bool /*forced*/) { sendRoomCommand(prepareRoomCommand(Command_LeaveRoom())); emit roomClosing(this); - close(); + deleteLater(); } void TabRoom::tabActivated() diff --git a/cockatrice/src/client/tabs/tab_supervisor.cpp b/cockatrice/src/client/tabs/tab_supervisor.cpp index 8ba5d791a..9df3fa661 100644 --- a/cockatrice/src/client/tabs/tab_supervisor.cpp +++ b/cockatrice/src/client/tabs/tab_supervisor.cpp @@ -169,7 +169,17 @@ TabSupervisor::TabSupervisor(AbstractClient *_client, QMenu *tabsMenu, QWidget * TabSupervisor::~TabSupervisor() { - stop(); + // Note: this used to call stop(), but stop() is doing a bunch of stuff + // including emitting some signals and we don't want to do that in a + // destructor. + + for (auto &localClient : localClients) { + localClient->deleteLater(); + } + localClients.clear(); + + delete userInfo; + userInfo = nullptr; } void TabSupervisor::retranslateUi() @@ -251,20 +261,6 @@ void TabSupervisor::closeEvent(QCloseEvent *event) event->ignore(); } } - - // Close the game tabs in order to make sure they store their layout. - QSet gameTabsToRemove; - for (auto it = gameTabs.begin(), end = gameTabs.end(); it != end; ++it) { - if (it.value()->close()) { - gameTabsToRemove.insert(it.key()); - } else { - event->ignore(); - } - } - - for (auto tabId : gameTabsToRemove) { - gameTabs.remove(tabId); - } } AbstractClient *TabSupervisor::getClient() const @@ -495,7 +491,7 @@ void TabSupervisor::openTabVisualDeckStorage() { tabVisualDeckStorage = new TabDeckStorageVisual(this); myAddTab(tabVisualDeckStorage, aTabVisualDeckStorage); - connect(tabVisualDeckStorage, &Tab::closed, this, [this] { + connect(tabVisualDeckStorage, &QObject::destroyed, this, [this] { tabVisualDeckStorage = nullptr; aTabVisualDeckStorage->setChecked(false); }); @@ -518,7 +514,7 @@ void TabSupervisor::openTabServer() tabServer = new TabServer(this, client); connect(tabServer, &TabServer::roomJoined, this, &TabSupervisor::addRoomTab); myAddTab(tabServer, aTabServer); - connect(tabServer, &Tab::closed, this, [this] { + connect(tabServer, &QObject::destroyed, this, [this] { tabServer = nullptr; aTabServer->setChecked(false); }); @@ -543,7 +539,7 @@ void TabSupervisor::openTabAccount() connect(tabAccount, &TabAccount::userJoined, this, &TabSupervisor::processUserJoined); connect(tabAccount, &TabAccount::userLeft, this, &TabSupervisor::processUserLeft); myAddTab(tabAccount, aTabAccount); - connect(tabAccount, &Tab::closed, this, [this] { + connect(tabAccount, &QObject::destroyed, this, [this] { tabAccount = nullptr; aTabAccount->setChecked(false); }); @@ -566,7 +562,7 @@ void TabSupervisor::openTabDeckStorage() tabDeckStorage = new TabDeckStorage(this, client, userInfo); connect(tabDeckStorage, &TabDeckStorage::openDeckEditor, this, &TabSupervisor::addDeckEditorTab); myAddTab(tabDeckStorage, aTabDeckStorage); - connect(tabDeckStorage, &Tab::closed, this, [this] { + connect(tabDeckStorage, &QObject::destroyed, this, [this] { tabDeckStorage = nullptr; aTabDeckStorage->setChecked(false); }); @@ -589,7 +585,7 @@ void TabSupervisor::openTabReplays() tabReplays = new TabReplays(this, client, userInfo); connect(tabReplays, &TabReplays::openReplay, this, &TabSupervisor::openReplay); myAddTab(tabReplays, aTabReplays); - connect(tabReplays, &Tab::closed, this, [this] { + connect(tabReplays, &QObject::destroyed, this, [this] { tabReplays = nullptr; aTabReplays->setChecked(false); }); @@ -612,7 +608,7 @@ void TabSupervisor::openTabAdmin() tabAdmin = new TabAdmin(this, client, (userInfo->user_level() & ServerInfo_User::IsAdmin)); connect(tabAdmin, &TabAdmin::adminLockChanged, this, &TabSupervisor::adminLockChanged); myAddTab(tabAdmin, aTabAdmin); - connect(tabAdmin, &Tab::closed, this, [this] { + connect(tabAdmin, &QObject::destroyed, this, [this] { tabAdmin = nullptr; aTabAdmin->setChecked(false); }); @@ -634,7 +630,7 @@ void TabSupervisor::openTabLog() { tabLog = new TabLog(this, client); myAddTab(tabLog, aTabLog); - connect(tabLog, &Tab::closed, this, [this] { + connect(tabLog, &QObject::destroyed, this, [this] { tabLog = nullptr; aTabAdmin->setChecked(false); });