From 00c9efe541912e547c3e42161fe75a09c5abf55b Mon Sep 17 00:00:00 2001
From: Basile Clement <Elarnon@users.noreply.github.com>
Date: Mon, 6 Feb 2023 12:49:45 +0000
Subject: [PATCH] Enable buttons for current game when receiving server
 response (#4737)

* Enable buttons for current game when receiving server response

Previously, upon joining a game, we were unconditionally re-enabling the
"Join" button in the lobby, even if it was not enabled in the first
place, causing #4698. This could also lead to issues where if the user
selects a different game after joining (which they can do in case of
e.g. network connectivity issues), the "Join as spectator" button could
get incorrectly disabled.

This fixes #4698 by re-enabling the buttons based on the state of the
currently selected game at the time the response is received. This also
avoids inconsistencies if a different game has been selected in between
joining and receiving a response from the server.

* Typo: enable gameButton in enableButtons

The "create game" button was incorrectly being disabled in enableButtons
whereas (as the name indicates) it should have been enabled

* Remove misleading comment about race conditions
---
 cockatrice/src/gameselector.cpp | 55 ++++++++++++++++++++++++---------
 cockatrice/src/gameselector.h   |  3 ++
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/cockatrice/src/gameselector.cpp b/cockatrice/src/gameselector.cpp
index 2b02ea6a..a6bb5c15 100644
--- a/cockatrice/src/gameselector.cpp
+++ b/cockatrice/src/gameselector.cpp
@@ -198,9 +198,19 @@ void GameSelector::actCreate()
 
 void GameSelector::checkResponse(const Response &response)
 {
-    if (createButton)
-        createButton->setEnabled(true);
-    joinButton->setEnabled(true);
+    // NB: We re-enable buttons for the currently selected game, which may not
+    // be the same game as the one for which we are processing a response.
+    // This could lead to situations where we join a game, select a different
+    // game, join the new game, then receive the response for the original
+    // join, which would re-activate the buttons for the second game too soon.
+    // However, that is better than doing things the other ways, because then
+    // the response to the first game could lock us out of join/join as
+    // spectator for the second game!
+    //
+    // Ideally we should have a way to figure out if the current game is the
+    // same as the one we are getting a response for, but it's probably not
+    // worth the trouble.
+    enableButtons();
 
     switch (response.response_code()) {
         case Response::RespNotInRoom:
@@ -229,9 +239,6 @@ void GameSelector::checkResponse(const Response &response)
             break;
         default:;
     }
-
-    if (response.response_code() != Response::RespSpectatorsNotAllowed)
-        spectateButton->setEnabled(true);
 }
 
 void GameSelector::actJoin()
@@ -270,12 +277,39 @@ void GameSelector::actJoin()
     connect(pend, SIGNAL(finished(Response, CommandContainer, QVariant)), this, SLOT(checkResponse(Response)));
     r->sendRoomCommand(pend);
 
+    disableButtons();
+}
+
+void GameSelector::disableButtons()
+{
     if (createButton)
         createButton->setEnabled(false);
+
     joinButton->setEnabled(false);
     spectateButton->setEnabled(false);
 }
 
+void GameSelector::enableButtons()
+{
+    if (createButton)
+        createButton->setEnabled(true);
+
+    // Enable buttons for the currently selected game
+    enableButtonsForIndex(gameListView->currentIndex());
+}
+
+void GameSelector::enableButtonsForIndex(const QModelIndex &current)
+{
+    if (!current.isValid())
+        return;
+
+    const ServerInfo_Game &game = gameListModel->getGame(current.data(Qt::UserRole).toInt());
+    bool overrideRestrictions = !tabSupervisor->getAdminLocked();
+
+    spectateButton->setEnabled(game.spectators_allowed() || overrideRestrictions);
+    joinButton->setEnabled(game.player_count() < game.max_players() || overrideRestrictions);
+}
+
 void GameSelector::retranslateUi()
 {
     filterButton->setText(tr("&Filter games"));
@@ -296,14 +330,7 @@ void GameSelector::processGameInfo(const ServerInfo_Game &info)
 
 void GameSelector::actSelectedGameChanged(const QModelIndex &current, const QModelIndex & /* previous */)
 {
-    if (!current.isValid())
-        return;
-
-    const ServerInfo_Game &game = gameListModel->getGame(current.data(Qt::UserRole).toInt());
-    bool overrideRestrictions = !tabSupervisor->getAdminLocked();
-
-    spectateButton->setEnabled(game.spectators_allowed() || overrideRestrictions);
-    joinButton->setEnabled(game.player_count() < game.max_players() || overrideRestrictions);
+    enableButtonsForIndex(current);
 }
 
 void GameSelector::updateTitle()
diff --git a/cockatrice/src/gameselector.h b/cockatrice/src/gameselector.h
index 334e32b2..b37ff24d 100644
--- a/cockatrice/src/gameselector.h
+++ b/cockatrice/src/gameselector.h
@@ -49,6 +49,9 @@ private:
     GameTypeMap gameTypeMap;
 
     void updateTitle();
+    void disableButtons();
+    void enableButtons();
+    void enableButtonsForIndex(const QModelIndex &current);
 
 public:
     GameSelector(AbstractClient *_client,