diff --git a/.ci/compile.sh b/.ci/compile.sh index f20432893..72a017f33 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -352,6 +352,62 @@ if [[ $MAKE_PACKAGE ]]; then exit 1 fi echo "Installer content is clean" + + # Fail the build if the installer is missing a Qt runtime the applications + # link against. The DLLs reach the package by globbing every *.dll out of + # the build output directories, so a module the client links statically + # (Qt6Multimedia.dll, Qt6QuickWidgets.dll) can be left out without anything + # else noticing, and the only symptom is a client that refuses to start + # after an update. required-qt-runtime.txt is written by the top level + # CMakeLists.txt from the same module list the targets are built against. + if [[ ! -f required-qt-runtime.txt ]]; then + echo "::error file=$0::required-qt-runtime.txt not found in the build dir, cannot verify the Qt runtime" + exit 1 + fi + # 7-Zip writes CRLF on Windows, so the \r has to come off before any + # whole-line comparison below. Normalize separators to /, and strip the + # install-root prefixes an NSIS listing may carry, so the entries can be + # compared as exact relative paths. -xF keeps a '.' in a DLL name from + # being a wildcard; -i because that is how the installed tree behaves - + # the uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins. + # The archive side and the manifest side have to agree on line endings. + # CMake writes required-qt-runtime.txt with the platform's line ending and + # 7-Zip writes CRLF on Windows, so a trailing \r survives on $entry and + # defeats the whole-line match - every entry then reads as missing even + # though the listing holds it verbatim. + # [$] rather than \$ so the literal dollar is a bracket expression and the + # linter does not read these as unexpanded shell variables (SC2016). + installer_paths="$("$seven_zip" l -slt "$package" | tr -d '\r' | + sed -n 's/^Path = //p' | + sed -e 's|\\|/|g' -e 's|^[$]INSTDIR/||' -e 's|^[$]OUTDIR/||' -e 's|^[$]PLUGINSDIR/||' -e 's|^\./||' -e 's|^/||')" + manifest="$(tr -d '\r' " DESTINATION ./) + endif() + endforeach() + install( DIRECTORY "${CMAKE_BINARY_DIR}/cockatrice/" DESTINATION ./ @@ -729,11 +747,15 @@ if(WIN32) PATTERN "tls/qschannelbackend.dll" ) + # The Windows install has no Resources directory: the .qm files go to + # ${COCKATRICE_WIN32_QM_INSTALL_DIR} and main.cpp looks for them at + # applicationDirPath() + "/translations". The macOS bundle above keeps + # Resources/translations, which is correct for it. install( CODE " file(WRITE \"\${CMAKE_INSTALL_PREFIX}/${qtconf_dest_dir}/qt.conf\" \"[Paths] Plugins = Plugins -Translations = Resources/translations +Translations = translations Data = Resources\") " COMPONENT Runtime diff --git a/cockatrice/src/game/game_state.cpp b/cockatrice/src/game/game_state.cpp index 854878b92..dc9ba02a3 100644 --- a/cockatrice/src/game/game_state.cpp +++ b/cockatrice/src/game/game_state.cpp @@ -29,13 +29,17 @@ void GameState::incrementGameTime() void GameState::setGameTime(int _secondsElapsed) { secondsElapsed = _secondsElapsed; + emit updateTimeElapsedLabel(formatElapsedTime(_secondsElapsed)); +} + +QString GameState::formatElapsedTime(int _secondsElapsed) +{ int seconds = _secondsElapsed; int minutes = seconds / 60; seconds -= minutes * 60; int hours = minutes / 60; minutes -= hours * 60; - emit updateTimeElapsedLabel(QString::number(hours).rightJustified(2, '0') + ":" + - QString::number(minutes).rightJustified(2, '0') + ":" + - QString::number(seconds).rightJustified(2, '0')); -} \ No newline at end of file + return QString::number(hours).rightJustified(2, '0') + ":" + QString::number(minutes).rightJustified(2, '0') + ":" + + QString::number(seconds).rightJustified(2, '0'); +} diff --git a/cockatrice/src/game/game_state.h b/cockatrice/src/game/game_state.h index 5a57d4321..94bbb853c 100644 --- a/cockatrice/src/game/game_state.h +++ b/cockatrice/src/game/game_state.h @@ -110,6 +110,11 @@ public: return hostId; } + int getSecondsElapsed() const + { + return secondsElapsed; + } + signals: void updateTimeElapsedLabel(QString newTime); void gameStarted(bool resuming); @@ -121,6 +126,9 @@ public slots: void incrementGameTime(); void setGameTime(int _secondsElapsed); +public: + static QString formatElapsedTime(int _secondsElapsed); + private: QTimer *gameTimer; int secondsElapsed; diff --git a/cockatrice/src/game/zones/view_zone_logic.cpp b/cockatrice/src/game/zones/view_zone_logic.cpp index 2ef04284a..de00581d1 100644 --- a/cockatrice/src/game/zones/view_zone_logic.cpp +++ b/cockatrice/src/game/zones/view_zone_logic.cpp @@ -154,6 +154,14 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone) reorganizeCards(); } +void ZoneViewZoneLogic::clearCards() +{ + for (auto card : cards) { + card->deleteLater(); + } + cards.clear(); +} + void ZoneViewZoneLogic::setWriteableRevealZone(bool _writeableRevealZone) { diff --git a/cockatrice/src/game/zones/view_zone_logic.h b/cockatrice/src/game/zones/view_zone_logic.h index 04ed20e45..e3080d044 100644 --- a/cockatrice/src/game/zones/view_zone_logic.h +++ b/cockatrice/src/game/zones/view_zone_logic.h @@ -41,6 +41,10 @@ public: bool prepareAddCard(int x); void removeCard(int position, bool toNewZone); void updateCardIds(CardAction action); + + /** @brief Removes all cards from the view, without touching anything else. Used in replay rewind */ + void clearCards(); + int getNumberCards() const { return numberCards; diff --git a/cockatrice/src/game_graphics/game_scene.cpp b/cockatrice/src/game_graphics/game_scene.cpp index 17af7618b..e4466f95d 100644 --- a/cockatrice/src/game_graphics/game_scene.cpp +++ b/cockatrice/src/game_graphics/game_scene.cpp @@ -735,6 +735,16 @@ void GameScene::clearViews() } } +void GameScene::emptyActiveViews() +{ + for (auto zoneView : zoneViews) { + auto logic = qobject_cast(zoneView->getZone()->getLogic()); + if (!logic->getRevealZone()) { + logic->clearCards(); + } + } +} + /** * @brief Closes the most recently added zone view. */ diff --git a/cockatrice/src/game_graphics/game_scene.h b/cockatrice/src/game_graphics/game_scene.h index 859d7a6eb..cf81eec9d 100644 --- a/cockatrice/src/game_graphics/game_scene.h +++ b/cockatrice/src/game_graphics/game_scene.h @@ -227,6 +227,9 @@ public slots: /** @brief Closes all zone views. */ void clearViews(); + /** @brief Empties the contents of all non-reveal zone views. Used for replay rewinding */ + void emptyActiveViews(); + /** @brief Closes the most recently added zone view. */ void closeMostRecentZoneView(); QTransform getViewTransform() const; diff --git a/cockatrice/src/game_graphics/log/message_log_widget.cpp b/cockatrice/src/game_graphics/log/message_log_widget.cpp index ccd903b04..06b0301bd 100644 --- a/cockatrice/src/game_graphics/log/message_log_widget.cpp +++ b/cockatrice/src/game_graphics/log/message_log_widget.cpp @@ -1,7 +1,9 @@ #include "message_log_widget.h" +#include "../../client/settings/cache_settings.h" #include "../../client/settings/card_counter_settings.h" #include "../../client/sound_engine.h" +#include "../../game/game_state.h" #include "../../game/phase.h" #include "../../game/player/player_logic.h" #include "../../interface/widgets/tabs/tab_game.h" @@ -10,6 +12,7 @@ #include #include +#include #include #include @@ -626,13 +629,12 @@ void MessageLogWidget::logSetActivePhase(int phaseNumber) soundEngine->playSound(phase.soundFileName); - appendHtml("" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") + - phase.getName() + ""); + appendHtml("" + getCurrentTime() + phase.getName() + ""); } void MessageLogWidget::logSetActivePlayer(PlayerLogic *player) { - appendHtml("
" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") + + appendHtml("
" + getCurrentTime() + QString(tr("%1's turn.")).arg(player->getPlayerInfo()->getName()) + "
"); } @@ -826,6 +828,14 @@ void MessageLogWidget::appendHtmlServerMessage(const QString &html, bool optiona ChatView::appendHtmlServerMessage(messagePrefix + html + messageSuffix, optionalIsBold, optionalFontColor); } +QString MessageLogWidget::getCurrentTime() const +{ + if (SettingsCache::instance().chat().getUseGameTime()) { + return "[" + GameState::formatElapsedTime(game->getGameState()->getSecondsElapsed()) + "] "; + } + return ChatView::getCurrentTime(); +} + void MessageLogWidget::connectToPlayerEventHandler(PlayerEventHandler *playerEventHandler) { connect(playerEventHandler, &PlayerEventHandler::logSay, this, &MessageLogWidget::logSay); diff --git a/cockatrice/src/game_graphics/log/message_log_widget.h b/cockatrice/src/game_graphics/log/message_log_widget.h index a145d358d..375c6df4d 100644 --- a/cockatrice/src/game_graphics/log/message_log_widget.h +++ b/cockatrice/src/game_graphics/log/message_log_widget.h @@ -104,6 +104,9 @@ public slots: void appendHtmlServerMessage(const QString &html, bool optionalIsBold = false, QString optionalFontColor = QString()) override; + +private: + [[nodiscard]] QString getCurrentTime() const override; }; #endif diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp index c84a46528..bac270580 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_update.cpp @@ -265,8 +265,16 @@ void DlgUpdate::downloadSuccessful(const QUrl &filepath) // NSIS needs the /D= argument to be an UNQUOTED string, even if it contains spaces. Qt likes to quote arguments if // they contain spaces, so we use the windows exclusive QProcess::setNativeArguments in the only case where this is // relevant, which preserves the argument unquoted. + // + // /PID= tells the installer which process to wait for before it replaces anything. We launch the installer first + // and only quit afterwards, so it starts while this process is still alive and still has every Qt runtime DLL + // mapped, and a mapped DLL cannot be deleted. Without the handle the installer can only poll for an image name and + // guess, and a wrong guess is what leaves the update writing new executables next to runtime files of the previous + // version, which fails on the next start with "The procedure entry point ... could not be located in the dynamic + // link library + // ...Qt6QuickWidgets.dll". /D= has to stay the last parameter, which is why it is last here too. #ifdef Q_OS_WIN - process.setNativeArguments(QString("/R /D=%1").arg(appDir)); + process.setNativeArguments(QString("/R /PID=%1 /D=%2").arg(QCoreApplication::applicationPid()).arg(appDir)); #else // Linux/macOS: normal argument passing (not relevant since they update differently.) process.setArguments({"/R", QString("/D=%1").arg(appDir)}); diff --git a/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp b/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp index 0a287c9ce..ae9b2deb4 100644 --- a/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp +++ b/cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp @@ -167,7 +167,7 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold, QString htmlText = "" + - QDateTime::currentDateTime().toString("[hh:mm:ss] ") + html + ""; + getCurrentTime() + html + "
"; if (optionalIsBold) { htmlText = "" + htmlText + ""; @@ -179,6 +179,11 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold, } } +QString ChatView::getCurrentTime() const +{ + return QDateTime::currentDateTime().toString("[hh:mm:ss] "); +} + void ChatView::appendCardTag(QTextCursor &cursor, const QString &cardName) { QTextCharFormat oldFormat = cursor.charFormat(); @@ -290,7 +295,7 @@ void ChatView::appendMessage(QString message, timeFormat.setForeground(serverMessageColor); timeFormat.setFontWeight(QFont::Bold); cursor.setCharFormat(timeFormat); - cursor.insertText(QDateTime::currentDateTime().toString("[hh:mm:ss] ")); + cursor.insertText(getCurrentTime()); } // nickname diff --git a/cockatrice/src/interface/widgets/server/chat_view/chat_view.h b/cockatrice/src/interface/widgets/server/chat_view/chat_view.h index 671b35163..06e3a0ed8 100644 --- a/cockatrice/src/interface/widgets/server/chat_view/chat_view.h +++ b/cockatrice/src/interface/widgets/server/chat_view/chat_view.h @@ -107,9 +107,8 @@ public: ChatView(TabSupervisor *_tabSupervisor, AbstractGame *_game, bool _showTimestamps, QWidget *parent = nullptr); void retranslateUi(); void appendHtml(const QString &html); - void virtual appendHtmlServerMessage(const QString &html, - bool optionalIsBold = false, - QString optionalFontColor = QString()); + virtual void + appendHtmlServerMessage(const QString &html, bool optionalIsBold = false, QString optionalFontColor = QString()); void appendMessage(QString message, RoomMessageTypeFlags messageType = {}, const ServerInfo_User &userInfo = {}, @@ -119,6 +118,7 @@ public: QString getRecentChatLog(int maxMessages = 50) const; protected: + [[nodiscard]] virtual QString getCurrentTime() const; void enterEvent(QEnterEvent *event) override; void leaveEvent(QEvent *event) override; void mouseMoveEvent(QMouseEvent *event) override; diff --git a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp b/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp index 8be76eea0..967de2f2c 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp @@ -634,9 +634,10 @@ void UserInfoPopup::onGamesContextMenu(const QPoint &pos) "QMenu::item:selected{background:%4;}") .arg(colorStr(theme.bg), colorStr(theme.text), colorStr(theme.border), colorStr(theme.buttonHover))); - const bool canJoin = !game.started() && game.player_count() < game.max_players(); + // Never grey the join action out: GameSelector::joinGame() asks whether to + // fall back to spectating when the game is full or already running, so a + // disabled entry would just read as a dead button. QAction *join = menu.addAction(tr("Join game")); - join->setEnabled(canJoin); QAction *spec = nullptr; if (game.spectators_allowed()) { diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp index d9e1e1e7e..3b8fc08b7 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp @@ -430,9 +430,6 @@ UserListWidget::UserListWidget(TabSupervisor *_tabSupervisor, hidePopup(true); requestVisibleItemResources(); }); - - // Forward join requests from popup upward - connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this, &UserListWidget::joinGameRequested); } else { // Dialog mode: keyboard selection drives the Invite button. connect(userTree, &QTreeWidget::currentItemChanged, this, [this](QTreeWidgetItem *current, QTreeWidgetItem *) { @@ -692,6 +689,17 @@ void UserListWidget::connectPopupSignals() [this](const QString &n) { userContextMenu->execAdjustJudge(n, true); }); connect(userInfoPopup, &UserInfoPopup::demoteFromJudgeRequested, this, [this](const QString &n) { userContextMenu->execAdjustJudge(n, false); }); + + // Joining is a tab-level action, not a per-user one: the popup's games list + // spans every room the user is in, so the target room may not be open yet. + // Hand it to the supervisor, which owns the intent that joins the room if + // needed. The popup closes here because that intent switches tabs, which + // would otherwise strand it on a list that is no longer on screen. + connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this, + [this](const int gameId, const int roomId, const bool asSpectator) { + hidePopup(true); + tabSupervisor->joinGameFromUserCard(gameId, roomId, asSpectator); + }); } bool UserListWidget::eventFilter(QObject *obj, QEvent *event) diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.h b/cockatrice/src/interface/widgets/server/user/user_list_widget.h index 412271160..9802387b2 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.h +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.h @@ -172,7 +172,6 @@ signals: void removeBuddy(const QString &userName); void addIgnore(const QString &userName); void removeIgnore(const QString &userName); - void joinGameRequested(int gameId, int roomId, bool asSpectator); void sectionExpanded(Section section, bool expanded); /** Dialog mode: the user activated (Enter/double-click) the given row. */ void userActivated(const QString &userName); diff --git a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp index a3b89f8c4..68e3f2ac7 100644 --- a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp @@ -64,6 +64,10 @@ MessagesSettingsPage::MessagesSettingsPage() connect(&ignoreAllPrivateMessagesCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(), &ChatSettings::setIgnoreAllPrivateMessages); + useGameTimeCheckBox.setChecked(SettingsCache::instance().chat().getUseGameTime()); + connect(&useGameTimeCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(), + &ChatSettings::setUseGameTime); + customAlertString = new QLineEdit(); customAlertString->setText(SettingsCache::instance().chat().getHighlightWords()); connect(customAlertString, &QLineEdit::textChanged, &SettingsCache::instance().chat(), @@ -82,6 +86,7 @@ MessagesSettingsPage::MessagesSettingsPage() chatGrid->addWidget(&mentionPopups, 6, 0); chatGrid->addWidget(&roomHistory, 7, 0); chatGrid->addWidget(&ignoreAllPrivateMessagesCheckBox, 8, 0); + chatGrid->addWidget(&useGameTimeCheckBox, 9, 0); chatGroupBox = new QGroupBox; chatGroupBox->setLayout(chatGrid); @@ -263,6 +268,7 @@ void MessagesSettingsPage::retranslateUi() mentionPopups.setText(tr("Enable desktop notification for mentions")); roomHistory.setText(tr("Enable room message history on join")); ignoreAllPrivateMessagesCheckBox.setText(tr("Ignore all private messages")); + useGameTimeCheckBox.setText(tr("Use game time instead of local time in game logs")); hexLabel.setText(tr("(Color is hexadecimal)")); hexHighlightLabel.setText(tr("(Color is hexadecimal)")); customAlertStringLabel.setText(tr("Separate words with a space, alphanumeric characters only")); diff --git a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h index 436ebbad9..4d491136f 100644 --- a/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/messages_settings_page.h @@ -41,6 +41,7 @@ private: QCheckBox mentionPopups; QCheckBox roomHistory; QCheckBox ignoreAllPrivateMessagesCheckBox; + QCheckBox useGameTimeCheckBox; QGroupBox *chatGroupBox; QGroupBox *highlightGroupBox; QGroupBox *messageGroupBox; diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.cpp b/cockatrice/src/interface/widgets/tabs/tab_game.cpp index 035ab1004..23976340b 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_game.cpp @@ -259,7 +259,7 @@ void TabGame::linkCardToChat(const QString &cardName) sayEdit->setFocus(); } -void TabGame::resetChatAndPhase() +void TabGame::resetForRewind() { // reset chat log messageLog->clearChat(); @@ -267,6 +267,9 @@ void TabGame::resetChatAndPhase() // reset phase markers game->getGameState()->setCurrentPhase(-1); + // reset view zones + static_cast(gameView->scene())->emptyActiveViews(); + // reset spectator state so the replay can rebuild it from the start game->getPlayerManager()->clearSpectators(); playerListWidget->clearSpectators(); @@ -1270,7 +1273,7 @@ void TabGame::createReplayDock(GameReplay *replay) replayDock->setWidget(replayWidget); replayDock->setFloating(false); - connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetChatAndPhase); + connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetForRewind); connect(replayWidget, &ReplayWidget::eventReplayed, game->getGameEventHandler(), [this](const auto &event, auto options) { game->getGameEventHandler()->processGameEventContainer(event, nullptr, options); diff --git a/cockatrice/src/interface/widgets/tabs/tab_game.h b/cockatrice/src/interface/widgets/tabs/tab_game.h index a05b49a9f..29a2f40a3 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_game.h +++ b/cockatrice/src/interface/widgets/tabs/tab_game.h @@ -204,7 +204,7 @@ public: public slots: void viewCardInfo(const CardRef &cardRef = {}) const; - void resetChatAndPhase(); + void resetForRewind(); void updateTimeElapsedLabel(QString newTime); void addPlayerToAutoCompleteList(QString playerName); void removePlayerFromAutoCompleteList(QString playerName); diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp index c9bda7703..0dc246eaa 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp @@ -1100,10 +1100,27 @@ void TabSupervisor::replayLeft(TabGame *tab) } void TabSupervisor::joinReportGame(const int gameId, const int roomId) +{ + joinGameByIntent(gameId, roomId, true); +} + +void TabSupervisor::joinGameFromUserCard(const int gameId, const int roomId, const bool asSpectator) +{ + joinGameByIntent(gameId, roomId, asSpectator); +} + +/** + * Runs the join/spectate intent for @p gameId in @p roomId. + * + * The intent owns the whole flow: if the room tab is not open yet it joins the + * room first, then retries the game, so callers do not have to care whether the + * room happens to be on screen. + */ +void TabSupervisor::joinGameByIntent(const int gameId, const int roomId, const bool asSpectator) { auto *remoteClient = qobject_cast(client); if (!remoteClient) { - actShowPopup(tr("Report joins are only available on a remote server.")); + actShowPopup(tr("Game joins are only available on a remote server.")); return; } @@ -1112,7 +1129,7 @@ void TabSupervisor::joinReportGame(const int gameId, const int roomId) ctx->roomContext.serverContext.port = QString::number(remoteClient->peerPort()); ctx->roomContext.roomId = roomId; ctx->gameId = gameId; - ctx->asSpectator = true; + ctx->asSpectator = asSpectator; auto *joinGameIntent = new IntentJoinServerGame(this, remoteClient, std::move(ctx)); joinGameIntent->setParent(this); diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.h b/cockatrice/src/interface/widgets/tabs/tab_supervisor.h index 066c84a77..1a4f7f749 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.h +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.h @@ -128,6 +128,8 @@ private: static QString sanitizeTabName(QString dirty); static QString sanitizeHtml(QString dirty); void resetTabsMenu(); + /** Shared join/spectate driver behind the report tab and the profile hover card. */ + void joinGameByIntent(int gameId, int roomId, bool asSpectator); public: explicit TabSupervisor(AbstractClient *_client, QMenu *tabsMenu, QWidget *parent = nullptr); @@ -198,6 +200,8 @@ public slots: TabEdhRec *addEdhrecTab(const CardInfoPtr &cardToQuery, bool isCommander = false); void openReplay(GameReplay *replay); void joinReportGame(int gameId, int roomId); + /** Joins or spectates a game listed on a user's profile hover card. */ + void joinGameFromUserCard(int gameId, int roomId, bool asSpectator); void openTabModeration(const QString &userName = {}); void switchToFirstAvailableNetworkTab(); void maximizeMainWindow(); diff --git a/cockatrice/src/single_instance_manager.cpp b/cockatrice/src/single_instance_manager.cpp index eff0e6586..8d363ff34 100644 --- a/cockatrice/src/single_instance_manager.cpp +++ b/cockatrice/src/single_instance_manager.cpp @@ -1,6 +1,7 @@ #include "single_instance_manager.h" #include +#include namespace { @@ -155,19 +156,44 @@ void SingleInstanceManager::handleNewConnection() socket->write(ACK_MESSAGE); socket->flush(); - // Drop the payload from the buffer before handling it: the handlers - // run synchronously and can spin a nested event loop (e.g. a modal - // dialog) that re-reads this socket, which would re-parse and re-emit - // the same files. + // Drop the payload from the buffer before handing it off, so a + // re-entrant event loop cannot re-parse and re-emit the same files. buffer->clear(); *expectedSize = 0; + // Take the guard while the socket is still alive: QPointer has to be + // built from a live pointer, and by the time the handlers below return + // the socket may already be gone. + QPointer guardedSocket(socket); + + // Hand the payload over synchronously, exactly as before, so the + // "Join game?" confirmation still appears. A handler can spin a nested + // event loop (that modal box is one), and the sending instance exits as + // soon as it has the ACK above, so while that loop runs the peer closes + // and this socket is disconnected. emit filesReceived(files); - socket->disconnectFromServer(); + // The handlers run synchronously, so returning from the emit means the + // readyRead frame has unwound and it is safe to touch the socket again. + // This teardown must therefore NOT be queued: a queued call would be + // picked up by the nested loop above while the frame is still on the + // stack, which is precisely the use-after-free this guards against. + // The guard covers the one remaining way the socket can die under us -- + // the primary being torn down (e.g. quit) while the dialog was open -- + // since QLocalServer owns and destroys its pending connections. + if (guardedSocket) { + guardedSocket->disconnectFromServer(); + guardedSocket->deleteLater(); + } return; } }); - connect(socket, &QLocalSocket::disconnected, socket, &QLocalSocket::deleteLater); + // Deliberately no `disconnected -> deleteLater()` here. The handlers above run + // synchronously and may spin a nested event loop, during which the sender exits + // and this socket is disconnected. A deleteLater() triggered from there is not + // deferred to the outer loop: Qt frees the object as soon as that nested loop + // unwinds, which is still inside this readyRead emission, so Qt's own signal + // dispatch is left holding a freed socket. The socket is instead closed and + // deleted once the handlers have returned, on the code path above. } diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h b/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h index cdf2da5eb..04dad9243 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h @@ -21,6 +21,7 @@ public: [[nodiscard]] virtual bool getShowMentionPopup() const = 0; [[nodiscard]] virtual bool getRoomHistory() const = 0; [[nodiscard]] virtual bool getIgnoreAllPrivateMessages() const = 0; + [[nodiscard]] virtual bool getUseGameTime() const = 0; [[nodiscard]] virtual QString getHighlightWords() const = 0; }; diff --git a/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp b/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp index 974173371..0ee29b4f5 100644 --- a/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/chat_settings.cpp @@ -70,6 +70,11 @@ bool ChatSettings::getIgnoreAllPrivateMessages() const return getValue("ignoreAllPrivateMessages", QString(), QString(), false).toBool(); } +bool ChatSettings::getUseGameTime() const +{ + return getValue("useGameTime", QString(), QString(), false).toBool(); +} + QString ChatSettings::getHighlightWords() const { return getValue("highlightWords").toString(); @@ -141,6 +146,11 @@ void ChatSettings::setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages) setValue(_ignoreAllPrivateMessages, "ignoreAllPrivateMessages"); } +void ChatSettings::setUseGameTime(bool _useGameTime) +{ + setValue(_useGameTime, "useGameTime"); +} + void ChatSettings::setHighlightWords(const QString &_highlightWords) { setValue(_highlightWords, "highlightWords"); diff --git a/libcockatrice_settings/libcockatrice/settings/chat_settings.h b/libcockatrice_settings/libcockatrice/settings/chat_settings.h index 9566671e2..d928b23b7 100644 --- a/libcockatrice_settings/libcockatrice/settings/chat_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/chat_settings.h @@ -24,6 +24,7 @@ public: [[nodiscard]] bool getShowMentionPopup() const override; [[nodiscard]] bool getRoomHistory() const override; [[nodiscard]] bool getIgnoreAllPrivateMessages() const override; + [[nodiscard]] bool getUseGameTime() const override; [[nodiscard]] QString getHighlightWords() const override; void setChatMention(bool _chatMention); @@ -39,6 +40,7 @@ public: void setShowMentionPopups(bool _showMentionPopups); void setRoomHistory(bool _roomHistory); void setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages); + void setUseGameTime(bool _useGameTime); void setHighlightWords(const QString &_highlightWords); signals: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d80995d26..1f9cdb9a3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -55,6 +55,17 @@ add_executable( ) target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) +# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt. +if(WITH_CLIENT) + add_executable( + single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp + single_instance_manager_test.cpp + ) + target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + + add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test) +endif() + find_package(GTest) if(NOT GTEST_FOUND) @@ -98,6 +109,7 @@ if(NOT GTEST_FOUND) add_dependencies(latency_tracker_test gtest) add_dependencies(metrics_registry_test gtest) add_dependencies(loader_local_matching_test gtest) + add_dependencies(single_instance_manager_test gtest) endif() include_directories(${GTEST_INCLUDE_DIRS}) @@ -153,6 +165,10 @@ target_link_libraries( loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) +if(WITH_CLIENT) + target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) +endif() + add_subdirectory(card_zone_algorithms) add_subdirectory(carddatabase) add_subdirectory(deck_list_model) diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index 56555eae6..379f6cd84 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -306,6 +306,12 @@ TEST_F(SettingsDefaultsTest, Chat_IgnoreAllPrivateMessages_Default) ASSERT_EQ(s.getIgnoreAllPrivateMessages(), false); } +TEST_F(SettingsDefaultsTest, Chat_UseGameTime_Default) +{ + ChatSettings s(settingsPath, nullptr); + ASSERT_EQ(s.getUseGameTime(), false); +} + // --- PersonalSettings --- TEST_F(SettingsDefaultsTest, Personal_Lang_Default) diff --git a/tests/single_instance_manager_test.cpp b/tests/single_instance_manager_test.cpp new file mode 100644 index 000000000..49193850d --- /dev/null +++ b/tests/single_instance_manager_test.cpp @@ -0,0 +1,134 @@ +#include "single_instance_manager.h" + +#include +#include +#include +#include +#include +#include +#include + +namespace +{ +constexpr int HANDSHAKE_TIMEOUT_MS = 5000; +constexpr int MODAL_DIALOG_MS = 50; + +const QStringList PAYLOAD{QStringLiteral("cockatrice://joingame?hostname=127.0.0.1&port=4747&roomid=0&gameid=1")}; + +/// Serializes a payload the way SingleInstanceManager::forwardToPrimary() does. +QByteArray encodePayload(const QStringList &files) +{ + QByteArray payload; + QDataStream out(&payload, QIODevice::WriteOnly); + out << files; + + QByteArray message; + QDataStream msgStream(&message, QIODevice::WriteOnly); + msgStream << static_cast(payload.size()); + message.append(payload); + return message; +} +} // namespace + +// Regression test for the use-after-free in SingleInstanceManager::handleNewConnection(). +// +// The forwarded payload is handed to the handlers from inside the forwarding +// socket's readyRead emission, which is what lets the "Join game?" confirmation +// appear at all. A handler can spin a nested event loop (a modal QMessageBox), +// and the sending instance exits as soon as it has the acknowledgment, so the +// peer closes *while that loop runs*. deleteLater() called from inside such a +// nested loop does not wait for the outer loop: Qt frees the object as soon as +// the nested loop unwinds, still inside the readyRead emission, so Qt's own +// signal dispatch and the handler lambda both end up holding a freed socket. +// Nothing may therefore delete the socket from the disconnected signal, and the +// teardown is queued until that frame has unwound. +TEST(SingleInstanceManagerTest, HandlersRunSynchronouslyAndSocketIsNotTouchedAfterTheyReturn) +{ + // Scope the hand-off socket to this test process so a Cockatrice the + // developer happens to be running cannot claim it. + qputenv("USER", QByteArrayLiteral("cockatrice-test-") + QByteArray::number(QCoreApplication::applicationPid())); + + SingleInstanceManager primary; + ASSERT_TRUE(primary.tryRun(QStringList())) << "could not become the primary instance"; + + auto *server = primary.findChild(); + ASSERT_NE(nullptr, server) << "the primary instance is not listening"; + + QLocalSocket sender; + QStringList handled; + QByteArray acknowledgment; + int handlerCalls = 0; + bool socketConnectedWhenHandled = false; + bool socketReleased = false; + + // Waited on after the handlers return: the deferred teardown deletes the + // forwarding socket once the readyRead frame has unwound. + QEventLoop teardownLoop; + + QEventLoop loop; + QObject::connect(&primary, &SingleInstanceManager::filesReceived, &primary, [&](const QStringList &files) { + ++handlerCalls; + + // The payload must reach the handlers while the forwarding socket is + // still connected: deferring the emit would suppress the modal "Join + // game?" confirmation this hand-off exists to show. + auto *connection = server->findChild(); + socketConnectedWhenHandled = connection != nullptr && connection->state() == QLocalSocket::ConnectedState; + + // Observe the socket's destruction rather than polling for it. `destroyed` + // is emitted just before the object goes away, so this stays correct + // however the teardown ends up deleting it, and `&primary` keeps the + // observer alive even once the socket itself is gone. + if (connection != nullptr) { + QObject::connect(connection, &QObject::destroyed, &primary, [&] { + socketReleased = true; + teardownLoop.quit(); + }); + } + + // The primary acknowledges the payload before handling it, so that a + // sender waiting on the acknowledgment never mistakes a busy primary + // for a dead one. + sender.waitForReadyRead(HANDSHAKE_TIMEOUT_MS); + acknowledgment = sender.readAll(); + + // Stand in for the modal "Join game?" confirmation: the sending instance + // goes away while this nested loop is running, which is what used to + // destroy the socket underneath this readyRead frame. + sender.abort(); + + QEventLoop modalDialog; + QTimer::singleShot(MODAL_DIALOG_MS, &modalDialog, &QEventLoop::quit); + modalDialog.exec(); + + handled = files; + loop.quit(); + }); + + sender.connectToServer(server->serverName()); + ASSERT_TRUE(sender.waitForConnected(HANDSHAKE_TIMEOUT_MS)); + sender.write(encodePayload(PAYLOAD)); + sender.flush(); + + QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &loop, &QEventLoop::quit); + loop.exec(); + + EXPECT_EQ(1, handlerCalls) << "the payload must be handed to the handlers exactly once"; + EXPECT_EQ(PAYLOAD, handled); + EXPECT_TRUE(socketConnectedWhenHandled) << "the payload was not handled from inside the readyRead emission"; + EXPECT_FALSE(acknowledgment.isEmpty()) << "the primary never acknowledged the payload"; + + // The deferred teardown must not be skipped, and must not delete the socket + // from inside the nested loop above: either would show up here as a crash or + // as a socket that outlives the hand-off and leaks a connection per payload. + QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &teardownLoop, &QEventLoop::quit); + teardownLoop.exec(); + EXPECT_TRUE(socketReleased) << "the forwarding socket was never released after the hand-off"; +} + +int main(int argc, char **argv) +{ + QCoreApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}