reduce unnecessary CardItem creation in ViewZone addCard process (#5513)

cockatrice/src/game/zones/card_zone.cpp

@@ -141,7 +141,7 @@ void CardZone::addCard(CardItem *card, const bool reorganize, const int x, const
     }
 
     for (auto *view : views) {
-        if (view->getIsReversed() || (x <= view->getCards().size()) || (view->getNumberCards() == -1)) {
+        if (view->prepareAddCard(x)) {
             view->addCard(new CardItem(player, nullptr, card->getName(), card->getProviderId(), card->getId()),
                           reorganize, x, y);
         }

cockatrice/src/game/zones/view_zone.cpp

@@ -282,6 +282,46 @@ void ZoneViewZone::setPileView(int _pileView)
     reorganizeCards();
 }
 
+/**
+ * Checks if inserting a card at the given position requires an actual new card to be created and added to the view.
+ * Also does any cardId updates that would be required if a card is inserted in that position.
+ *
+ * Note that this method can end up modifying the cardIds despite returning false.
+ * (for example, if the card is inserted into a hidden portion of the deck while the view is reversed)
+ *
+ * Make sure to call this method once before calling addCard(), so that you skip creating a new CardItem and calling
+ * addCard() if it's not required.
+ *
+ * @param x The position to insert the card at.
+ * @return Whether to proceed with calling addCard.
+ */
+bool ZoneViewZone::prepareAddCard(int x)
+{
+    bool doInsert = false;
+    if (!isReversed) {
+        if (x <= cards.size() || cards.size() == -1) {
+            doInsert = true;
+        }
+    } else {
+        // map x (which is in origZone indexes) to this viewZone's cardList index
+        int firstId = cards.isEmpty() ? origZone->getCards().size() : cards.front()->getId();
+        int insertionIndex = x - firstId;
+        if (insertionIndex >= 0) {
+            // card was put into a portion of the deck that's in the view
+            doInsert = true;
+        } else {
+            // card was put into a portion of the deck that's not in the view; update ids but don't insert card
+            updateCardIds(ADD_CARD);
+        }
+    }
+
+    return doInsert;
+}
+
+/**
+ * Make sure prepareAddCard() was called before calling addCard().
+ * This method assumes we already checked that the card is being inserted into the visible portion
+ */
 void ZoneViewZone::addCardImpl(CardItem *card, int x, int /*y*/)
 {
     if (!isReversed) {
@@ -295,15 +335,8 @@ void ZoneViewZone::addCardImpl(CardItem *card, int x, int /*y*/)
         // map x (which is in origZone indexes) to this viewZone's cardList index
         int firstId = cards.isEmpty() ? origZone->getCards().size() : cards.front()->getId();
         int insertionIndex = x - firstId;
-        if (insertionIndex >= 0) {
+        // qMin to prevent out-of-bounds error when bottoming a card that is already in the view
-            // card was put into a portion of the deck that's in the view
+        cards.insert(qMin(insertionIndex, cards.size()), card);
-            // qMin to prevent out-of-bounds error when bottoming a card that is already in the view
-            cards.insert(qMin(insertionIndex, cards.size()), card);
-        } else {
-            // card was put into a portion of the deck that's not in the view
-            updateCardIds(ADD_CARD);
-            return;
-        }
     }
 
     card->setParentItem(this);

cockatrice/src/game/zones/view_zone.h

@@ -70,6 +70,7 @@ public:
     void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) override;
     void reorganizeCards() override;
     void initializeCards(const QList<const ServerInfo_Card *> &cardList = QList<const ServerInfo_Card *>());
+    bool prepareAddCard(int x);
     void removeCard(int position);
     int getNumberCards() const
     {