Fix #6659: Correct logging for bottom-of-library card moves

Cause:
- This issue happens due to logic of moving the card from the top of the
  deck being reused when moving from the bottom of the deck, in a way
  that makes it impossible to check if the card came from the bottom.

Resolution:
- Updated the logging logic in the client for card moves.
- Added a gRPC parameter ('is_from_bottom') for card moves.
- Updates the server logic to reverse the order of the card move if the
'is_from_bottom' parameter is true.
- Added a test to show the expected behaviour of the fix.

NOTE: While the changes in this patch seem big, this is due to changing
the loop in the moveCard function to a helper function, in order to make
the bug fix change. The only change to the loop was to pass a
variable attribution to the moveCard function because it was redundant
to be in the loop.

cockatrice/src/game/log/message_log_widget.cpp

@@ -54,7 +54,7 @@ MessageLogWidget::getFromStr(CardZoneLogic *zone, QString cardName, int position
                     fromStr = tr(" from the top of their library");
                 }
             }
-        } else if (position >= zone->getCards().size() - 1) {
+        } else if (position == zone->getCards().size()) {
             if (cardName.isEmpty()) {
                 if (ownerChange) {
                     cardName = tr("the bottom card of %1's library").arg(zone->getPlayer()->getPlayerInfo()->getName());

cockatrice/src/game/player/player_actions.cpp

@@ -664,6 +664,7 @@ void PlayerActions::moveBottomCardsTo(const QString &targetZone, const QString &
     cmd.set_target_zone(targetZone.toStdString());
     cmd.set_x(0);
     cmd.set_y(0);
+    cmd.set_is_from_bottom(true);
 
     for (int i = maxCards - number; i < maxCards; ++i) {
         auto card = cmd.mutable_cards_to_move()->add_card();
@@ -785,6 +786,7 @@ void PlayerActions::actDrawBottomCards()
     cmd.set_target_zone(ZoneNames::HAND);
     cmd.set_x(0);
     cmd.set_y(0);
+    cmd.set_is_from_bottom(true);
 
     for (int i = maxCards - number; i < maxCards; ++i) {
         cmd.mutable_cards_to_move()->add_card()->set_card_id(i);

libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.cpp

@@ -49,6 +49,7 @@
 #include <libcockatrice/rng/rng_abstract.h>
 #include <libcockatrice/utility/trice_limits.h>
 #include <libcockatrice/utility/zone_names.h>
+#include <ranges>
 
 Server_AbstractPlayer::Server_AbstractPlayer(Server_Game *_game,
                                              int _playerId,
@@ -236,7 +237,8 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
                                                        int yCoord,
                                                        bool fixFreeSpaces,
                                                        bool undoingDraw,
-                                                       bool isReversed)
+                                                       bool isReversed,
+                                                       bool isFromBottom)
 {
     // Disallow controller change to other zones than the table.
     if (((targetzone->getType() != ServerInfo_Zone::PublicZone) || !targetzone->hasCoords()) &&
@@ -244,9 +246,12 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
         return Response::RespContextError;
     }
 
-    if (!targetzone->hasCoords() && (xCoord <= -1)) {
+    if (!targetzone->hasCoords()) {
+        yCoord = 0;
+        if (xCoord <= -1) {
             xCoord = targetzone->getCards().size();
         }
+    }
 
     std::set<MoveCardStruct> cardsToMove;
     QSet<int> cardIdsToMove;
@@ -285,7 +290,51 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
     bool revealTopStart = false;
     bool revealTopTarget = false;
 
-    for (auto cardStruct : cardsToMove) {
+    // If the cards are to be taken from the bottom of the deck, reverse the order in which they are taken.
+    if (isFromBottom) {
+        std::ranges::reverse_view reversedCardsToMove{cardsToMove};
+        for (auto card : reversedCardsToMove) {
+            processMoveCard(ges, startzone, targetzone, card, xCoord, yCoord, xIndex, revealTopStart, revealTopTarget,
+                            isReversed, undoingDraw);
+        }
+    } else {
+        for (auto card : cardsToMove) {
+            processMoveCard(ges, startzone, targetzone, card, xCoord, yCoord, xIndex, revealTopStart, revealTopTarget,
+                            isReversed, undoingDraw);
+        }
+    }
+
+    if (revealTopStart) {
+        revealTopCardIfNeeded(startzone, ges);
+    }
+    if (targetzone != startzone && revealTopTarget) {
+        revealTopCardIfNeeded(targetzone, ges);
+    }
+    if (undoingDraw) {
+        ges.setGameEventContext(Context_UndoDraw());
+    } else {
+        ges.setGameEventContext(Context_MoveCard());
+    }
+
+    if (startzone->hasCoords() && fixFreeSpaces) {
+        startzone->fixFreeSpaces(ges);
+    }
+
+    return Response::RespOk;
+}
+
+void Server_AbstractPlayer::processMoveCard(GameEventStorage &ges,
+                                            Server_CardZone *startzone,
+                                            Server_CardZone *targetzone,
+                                            MoveCardStruct cardStruct,
+                                            int xCoord,
+                                            int yCoord,
+                                            int &xIndex,
+                                            bool &revealTopStart,
+                                            bool &revealTopTarget,
+                                            bool isReversed,
+                                            bool undoingDraw)
+{
     Server_Card *card = cardStruct.card;
     int originalPosition = cardStruct.position;
 
@@ -328,8 +377,7 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
 
         if (Server_Card *stashedCard = card->takeStashedCard()) {
             stashedCard->setId(newCardId());
-                ges.enqueueGameEvent(makeCreateTokenEvent(startzone, stashedCard, card->getX(), card->getY()),
+            ges.enqueueGameEvent(makeCreateTokenEvent(startzone, stashedCard, card->getX(), card->getY()), playerId);
-                                     playerId);
             card->deleteLater();
             card = stashedCard;
         } else {
@@ -347,7 +395,6 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
         if (targetzone->hasCoords()) {
             newX = targetzone->getFreeGridColumn(newX, yCoord, card->getName(), faceDown);
         } else {
-                yCoord = 0;
             card->resetState(targetzone->getName() == ZoneNames::STACK);
         }
 
@@ -442,24 +489,6 @@ Response::ResponseCode Server_AbstractPlayer::moveCard(GameEventStorage &ges,
         // handle side effects for this card
         onCardBeingMoved(ges, cardStruct, startzone, targetzone, undoingDraw);
     }
-    }
-    if (revealTopStart) {
-        revealTopCardIfNeeded(startzone, ges);
-    }
-    if (targetzone != startzone && revealTopTarget) {
-        revealTopCardIfNeeded(targetzone, ges);
-    }
-    if (undoingDraw) {
-        ges.setGameEventContext(Context_UndoDraw());
-    } else {
-        ges.setGameEventContext(Context_MoveCard());
-    }
-
-    if (startzone->hasCoords() && fixFreeSpaces) {
-        startzone->fixFreeSpaces(ges);
-    }
-
-    return Response::RespOk;
 }
 
 void Server_AbstractPlayer::onCardBeingMoved(GameEventStorage &ges,
@@ -743,7 +772,8 @@ Server_AbstractPlayer::cmdMoveCard(const Command_MoveCard &cmd, ResponseContaine
         cardsToMove.append(&cmd.cards_to_move().card(i));
     }
 
-    return moveCard(ges, startZone, cardsToMove, targetZone, cmd.x(), cmd.y(), true, false, cmd.is_reversed());
+    return moveCard(ges, startZone, cardsToMove, targetZone, cmd.x(), cmd.y(), true, false, cmd.is_reversed(),
+                    cmd.is_from_bottom());
 }
 
 Response::ResponseCode

libcockatrice_network/libcockatrice/network/server/remote/game/server_abstract_player.h

@@ -92,7 +92,21 @@ public:
                                     int yCoord,
                                     bool fixFreeSpaces = true,
                                     bool undoingDraw = false,
-                                    bool isReversed = false);
+                                    bool isReversed = false,
+                                    bool isFromBottom = false);
+
+    void processMoveCard(GameEventStorage &ges,
+                         Server_CardZone *startzone,
+                         Server_CardZone *targetzone,
+                         MoveCardStruct cardStruct,
+                         int xCoord,
+                         int yCoord,
+                         int &xIndex,
+                         bool &revealTopStart,
+                         bool &revealTopTarget,
+                         bool isReversed,
+                         bool undoingDraw);
+
     virtual void onCardBeingMoved(GameEventStorage &ges,
                                   const MoveCardStruct &cardStruct,
                                   Server_CardZone *startzone,

libcockatrice_protocol/libcockatrice/protocol/pb/command_move_card.proto

@@ -52,4 +52,7 @@ message Command_MoveCard {
 
     // Inverts the x coordinate to apply from the end of the target zone instead of the start
     optional bool is_reversed = 8 [default = false];
+
+    // Inverts the order in which cards are moved (if false, from top to bottom)
+    optional bool is_from_bottom = 9 [default = false];
 }

tests/CMakeLists.txt

@@ -65,4 +65,5 @@ target_link_libraries(
 add_subdirectory(card_zone_algorithms)
 add_subdirectory(carddatabase)
 add_subdirectory(loading_from_clipboard)
+add_subdirectory(movecard_tests)
 add_subdirectory(oracle)

tests/movecard_tests/CMakeLists.txt

@@ -0,0 +1,16 @@
+add_executable(reverse_card_move_test reverse_card_move_test.cpp)
+
+if(NOT GTEST_FOUND)
+  add_dependencies(reverse_card_move_test gtest)
+endif()
+
+target_link_libraries(
+  reverse_card_move_test
+  PRIVATE libcockatrice_network_server_remote
+  PRIVATE libcockatrice_rng
+  PRIVATE Threads::Threads
+  PRIVATE ${GTEST_BOTH_LIBRARIES}
+  PRIVATE ${TEST_QT_MODULES}
+)
+
+add_test(NAME reverse_card_move_test COMMAND reverse_card_move_test)

tests/movecard_tests/reverse_card_move_test.cpp

@@ -0,0 +1,91 @@
+#include "game/server_abstract_player.h"
+#include "game/server_card.h"
+#include "game/server_cardzone.h"
+#include "game/server_game.h"
+#include "server_response_containers.h"
+#include "server_room.h"
+#include "server_test_helpers.h"
+
+#include <gtest/gtest.h>
+#include <libcockatrice/protocol/pb/command_move_card.pb.h>
+#include <libcockatrice/protocol/pb/serverinfo_user.pb.h>
+#include <libcockatrice/rng/rng_abstract.h>
+#include <libcockatrice/utility/zone_names.h>
+
+RNG_Abstract *rng = nullptr; // this needs to be defined due to other functions in server
+
+TEST(ReverseCardMoveTest, MoveCardFromBottomTest)
+{
+    ServerInfo_User user;
+    user.set_name("test-user");
+
+	// instatiate necessary dependencies of the library
+    FakeServer server;
+    Server_Room room(0, 0, "", "", "", "", false, "", {}, &server);
+    Server_Game game(user, 1, "", "", 2, QList<int>(), false, false, false, false, false, false, 20, false, &room);
+    Server_AbstractPlayer player(&game, 1, user, false, nullptr);
+    Server_CardZone deckZone(&player, ZoneNames::DECK, true, ServerInfo_Zone::PublicZone);
+    Server_CardZone exileZone(&player, ZoneNames::EXILE, true, ServerInfo_Zone::PublicZone);
+
+    // setup the deck with 20 useless cards
+    for (int i = 0; i < 20; i++) {
+        auto *cardUseless = new Server_Card({"Card Useless", "card-Useless"}, player.newCardId(), i, 0);
+        deckZone.insertCard(cardUseless, i, 0);
+    }
+
+    // add 4 cards to the end of it
+    auto *cardA = new Server_Card({"Card A", "card-a"}, player.newCardId(), 20, 0);
+    auto *cardB = new Server_Card({"Card B", "card-b"}, player.newCardId(), 21, 0);
+    auto *cardC = new Server_Card({"Card C", "card-c"}, player.newCardId(), 22, 0);
+    auto *cardD = new Server_Card({"Card D", "card-d"}, player.newCardId(), 23, 0);
+
+    deckZone.insertCard(cardA, 20, 0);
+    deckZone.insertCard(cardB, 21, 0);
+    deckZone.insertCard(cardC, 22, 0);
+    deckZone.insertCard(cardD, 23, 0);
+
+    // try to move them, with the expected client given order (n-3, n-2, n-1, n)
+    CardToMove moveA;
+    moveA.set_card_id(cardA->getId());
+    CardToMove moveB;
+    moveB.set_card_id(cardB->getId());
+    CardToMove moveC;
+    moveC.set_card_id(cardC->getId());
+    CardToMove moveD;
+    moveD.set_card_id(cardD->getId());
+
+    QList<const CardToMove *> cardsToMove = {&moveA, &moveB, &moveC, &moveD};
+    GameEventStorage ges;
+
+    const auto response = player.moveCard(ges, &deckZone, cardsToMove, &exileZone, 0, 0, false, false, false, true);
+
+    EXPECT_EQ(response, Response::RespOk);
+
+    int positionA;
+    int positionB;
+    int positionC;
+    int positionD;
+    // check if they are on the destination zone
+    EXPECT_EQ(exileZone.getCard(cardA->getId(), &positionA), cardA);
+    EXPECT_EQ(exileZone.getCard(cardB->getId(), &positionB), cardB);
+    EXPECT_EQ(exileZone.getCard(cardC->getId(), &positionC), cardC);
+    EXPECT_EQ(exileZone.getCard(cardD->getId(), &positionD), cardD);
+
+    // check if they are with the expected coordinates
+    EXPECT_EQ(cardA->getX(), 3);
+    EXPECT_EQ(cardB->getX(), 2);
+    EXPECT_EQ(cardC->getX(), 1);
+    EXPECT_EQ(cardD->getX(), 0);
+
+    // check if they are with the expected positions
+    EXPECT_EQ(positionA, 3);
+    EXPECT_EQ(positionB, 2);
+    EXPECT_EQ(positionC, 1);
+    EXPECT_EQ(positionD, 0);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}

tests/movecard_tests/server_test_helpers.h

@@ -0,0 +1,42 @@
+#include "server.h"
+#include "server_database_interface.h"
+
+class MockDatabaseInterface : public Server_DatabaseInterface
+{
+public:
+    AuthenticationResult checkUserPassword(Server_ProtocolHandler *,
+                                           const QString &,
+                                           const QString &,
+                                           const QString &,
+                                           QString &,
+                                           int &,
+                                           bool) override
+    {
+        return NotLoggedIn;
+    }
+    ServerInfo_User getUserData(const QString &, bool) override
+    {
+        return ServerInfo_User();
+    }
+    int getNextGameId() override
+    {
+        return 1;
+    }
+    int getNextReplayId() override
+    {
+        return 1;
+    }
+    int getActiveUserCount(QString) override
+    {
+        return 1;
+    }
+};
+
+class FakeServer : public Server
+{
+public:
+    FakeServer()
+    {
+        setDatabaseInterface(new MockDatabaseInterface());
+    }
+};