[CardInfo] Encapsulate lazy properties loading into new class (#7088)

* [CardInfo] Encapsulate lazy properties loading into new class

* check if new value was inserted

libcockatrice_card/CMakeLists.txt

@@ -5,6 +5,7 @@ set(CMAKE_AUTORCC ON)
 set(HEADERS
     libcockatrice/card/card_info.h
     libcockatrice/card/card_info_comparator.h
+    libcockatrice/card/lazy_properties_hash.h
     libcockatrice/card/database/card_database.h
     libcockatrice/card/database/card_database_loader.h
     libcockatrice/card/database/card_database_manager.h
@@ -26,6 +27,7 @@ add_library(
   ${MOC_SOURCES}
   libcockatrice/card/card_info.cpp
   libcockatrice/card/card_info_comparator.cpp
+  libcockatrice/card/lazy_properties_hash.cpp
   libcockatrice/card/database/card_database.cpp
   libcockatrice/card/database/card_database_cache.cpp
   libcockatrice/card/database/card_database_loader.cpp

libcockatrice_card/libcockatrice/card/card_info.cpp

@@ -5,10 +5,7 @@
 #include "relation/card_relation.h"
 #include "set/card_set.h"
 
-#include <QDataStream>
 #include <QDir>
-#include <QMutex>
-#include <QMutexLocker>
 #include <QRegularExpression>
 #include <QSharedPointer>
 #include <QString>
@@ -21,64 +18,33 @@ class CardInfo;
 
 using CardInfoPtr = QSharedPointer<CardInfo>;
 
-namespace
-{
-QByteArray serializeProperties(const QHash<QString, QString> &props)
-{
-    QByteArray blob;
-    QDataStream out(&blob, QIODevice::WriteOnly);
-    out.setVersion(QDataStream::Qt_6_4);
-    out << props;
-    return blob;
-}
-} // namespace
-
-void CardInfo::ensurePropertiesLoaded() const
-{
-    QMutexLocker lock(&propertiesMutex);
-    if (propertiesLoaded) {
-        return;
-    }
-    if (!propertiesBlob.isEmpty()) {
-        QDataStream in(propertiesBlob);
-        in.setVersion(QDataStream::Qt_6_4);
-        in >> propertiesCache;
-    }
-    propertiesLoaded = true;
-}
-
 const QHash<QString, QString> &CardInfo::getPropertiesHash() const
 {
-    ensurePropertiesLoaded();
-    return propertiesCache;
+    return properties.getProperties();
 }
 
 void CardInfo::setProperty(const QString &_name, const QString &_value)
 {
-    ensurePropertiesLoaded();
-    if (propertiesCache.value(_name) == _value) {
+    bool changed = properties.insert(_name, _value);
+    if (!changed) {
         return;
     }
-    propertiesCache.insert(_name, _value);
-    propertiesBlob = serializeProperties(propertiesCache);
+
     emit cardInfoChanged(smartThis);
 }
 
 CardInfo::CardInfo(const QString &_name,
                    const QString &_text,
                    bool _isToken,
-                   QHash<QString, QString> _properties,
+                   const QHash<QString, QString> &_properties,
                    const QList<CardRelation *> &_relatedCards,
                    const QList<CardRelation *> &_reverseRelatedCards,
                    SetToPrintingsMap _sets,
                    const UiAttributes _uiAttributes)
-    : name(_name), text(_text), isToken(_isToken), relatedCards(_relatedCards),
-      reverseRelatedCards(_reverseRelatedCards), setsToPrintings(std::move(_sets)), uiAttributes(_uiAttributes)
+    : name(_name), text(_text), isToken(_isToken), properties(LazyPropertiesHash(_properties)),
+      relatedCards(_relatedCards), reverseRelatedCards(_reverseRelatedCards), setsToPrintings(std::move(_sets)),
+      uiAttributes(_uiAttributes)
 {
-    propertiesCache = std::move(_properties);
-    propertiesBlob = serializeProperties(propertiesCache);
-    propertiesLoaded = true;
-
     simpleName = CardInfo::simplifyName(name);
 
     refreshCachedSets();
@@ -87,7 +53,7 @@ CardInfo::CardInfo(const QString &_name,
 CardInfo::CardInfo(const QString &_name,
                    const QString &_text,
                    bool _isToken,
-                   QByteArray _propertiesBlob,
+                   const QByteArray &_propertiesBlob,
                    const QList<CardRelation *> &_relatedCards,
                    const QList<CardRelation *> &_reverseRelatedCards,
                    SetToPrintingsMap _sets,
@@ -95,7 +61,7 @@ CardInfo::CardInfo(const QString &_name,
                    QString _simpleName,
                    QSet<QString> _altNames)
     : name(_name), simpleName(std::move(_simpleName)), text(_text), isToken(_isToken),
-      propertiesBlob(std::move(_propertiesBlob)), relatedCards(_relatedCards),
+      properties(LazyPropertiesHash(_propertiesBlob)), relatedCards(_relatedCards),
       reverseRelatedCards(_reverseRelatedCards), setsToPrintings(std::move(_sets)), uiAttributes(_uiAttributes),
       altNames(std::move(_altNames))
 {
@@ -113,14 +79,14 @@ CardInfoPtr CardInfo::newInstance(const QString &_name)
 CardInfoPtr CardInfo::newInstance(const QString &_name,
                                   const QString &_text,
                                   bool _isToken,
-                                  QHash<QString, QString> _properties,
+                                  const QHash<QString, QString> &_properties,
                                   const QList<CardRelation *> &_relatedCards,
                                   const QList<CardRelation *> &_reverseRelatedCards,
                                   SetToPrintingsMap _sets,
                                   const UiAttributes _uiAttributes)
 {
-    CardInfoPtr ptr(new CardInfo(_name, _text, _isToken, std::move(_properties), _relatedCards, _reverseRelatedCards,
-                                 _sets, _uiAttributes));
+    CardInfoPtr ptr(
+        new CardInfo(_name, _text, _isToken, _properties, _relatedCards, _reverseRelatedCards, _sets, _uiAttributes));
     ptr->setSmartPointer(ptr);
 
     for (const auto &printings : _sets) {
@@ -203,15 +169,13 @@ void CardInfo::addToSet(const CardSetPtr &_set, const PrintingInfo &_info)
 
 void CardInfo::combineLegalities(const QHash<QString, QString> &props)
 {
-    ensurePropertiesLoaded();
     QHashIterator it(props);
     while (it.hasNext()) {
         it.next();
         if (it.key().startsWith("format-")) {
-            propertiesCache.insert(it.key(), it.value());
+            properties.insert(it.key(), it.value());
         }
     }
-    propertiesBlob = serializeProperties(propertiesCache);
     emit cardInfoChanged(smartThis);
 }
 

libcockatrice_card/libcockatrice/card/card_info.h

@@ -2,6 +2,7 @@
 #define CARD_INFO_H
 
 #include "format/format_legality_rules.h"
+#include "lazy_properties_hash.h"
 #include "printing/printing_info.h"
 
 #include <QDate>
@@ -75,19 +76,8 @@ private:
     QString simpleName;    ///< Simplified name for fuzzy matching.
     QString text;          ///< Text description or rules text of the card.
     bool isToken;          ///< Whether this card is a token or not.
-    // Properties are stored as a pre-serialized blob (cheap to load) and the
-    // QHash<QString, QString> is materialized on first query, so database load avoids
-    // constructing thousands of QStrings per card.
-    mutable QByteArray propertiesBlob;               ///< Serialized properties (load form).
-    mutable QHash<QString, QString> propertiesCache; ///< Materialized properties (query form).
-    mutable bool propertiesLoaded = false;           ///< Whether propertiesCache is valid.
-    mutable QMutex propertiesMutex;                  ///< Guards lazy materialization.
 
-    /**
-     * @brief Materializes propertiesCache from propertiesBlob if not already done.
-     *        Safe to call from const getters (members are mutable).
-     */
-    void ensurePropertiesLoaded() const;
+    LazyPropertiesHash properties; ///< Key-value store of dynamic card properties.
 
     QList<CardRelation *> relatedCards;            ///< Forward references to related cards.
     QList<CardRelation *> reverseRelatedCards;     ///< Cards that refer back to this card.
@@ -114,7 +104,7 @@ public:
     explicit CardInfo(const QString &_name,
                       const QString &_text,
                       bool _isToken,
-                      QHash<QString, QString> _properties,
+                      const QHash<QString, QString> &_properties,
                       const QList<CardRelation *> &_relatedCards,
                       const QList<CardRelation *> &_reverseRelatedCards,
                       SetToPrintingsMap _sets,
@@ -144,7 +134,7 @@ public:
     explicit CardInfo(const QString &_name,
                       const QString &_text,
                       bool _isToken,
-                      QByteArray _propertiesBlob,
+                      const QByteArray &_propertiesBlob,
                       const QList<CardRelation *> &_relatedCards,
                       const QList<CardRelation *> &_reverseRelatedCards,
                       SetToPrintingsMap _sets,
@@ -161,7 +151,7 @@ public:
      */
     CardInfo(const CardInfo &other)
         : QObject(other.parent()), name(other.name), simpleName(other.simpleName), text(other.text),
-          isToken(other.isToken), propertiesBlob(other.propertiesBlob), relatedCards(other.relatedCards),
+          isToken(other.isToken), properties(other.properties), relatedCards(other.relatedCards),
           reverseRelatedCards(other.reverseRelatedCards), reverseRelatedCardsToMe(other.reverseRelatedCardsToMe),
           setsToPrintings(other.setsToPrintings), uiAttributes(other.uiAttributes), setsNames(other.setsNames),
           altNames(other.altNames)
@@ -194,7 +184,7 @@ public:
     static CardInfoPtr newInstance(const QString &_name,
                                    const QString &_text,
                                    bool _isToken,
-                                   QHash<QString, QString> _properties,
+                                   const QHash<QString, QString> &_properties,
                                    const QList<CardRelation *> &_relatedCards,
                                    const QList<CardRelation *> &_reverseRelatedCards,
                                    SetToPrintingsMap _sets,

libcockatrice_card/libcockatrice/card/database/card_database_cache.cpp

@@ -119,7 +119,7 @@ PrintingInfo readPrinting(QDataStream &in, const SetNameMap &sets)
     QByteArray propsBlob = readHashBlob(in);
     auto set = sets.value(setName);
 
-    return PrintingInfo(set, propsBlob);
+    return PrintingInfo(set, LazyPropertiesHash(propsBlob));
 }
 
 // ---- CardSet ---------------------------------------------------------------

libcockatrice_card/libcockatrice/card/database/parser/cockatrice_xml_3.cpp

@@ -248,7 +248,7 @@ void CockatriceXml3Parser::loadCardsFromXml(QXmlStreamReader &xml)
                         if (attrs.hasAttribute("rarity")) {
                             printingProps.insert("rarity", attrs.value("rarity").toString());
                         }
-                        PrintingInfo setInfo(set, printingProps);
+                        PrintingInfo setInfo(set, LazyPropertiesHash(printingProps));
                         _sets[setName].append(setInfo);
                     }
                     // related cards

libcockatrice_card/libcockatrice/card/database/parser/cockatrice_xml_4.cpp

@@ -322,7 +322,7 @@ void CockatriceXml4Parser::loadCardsFromXml(QXmlStreamReader &xml)
                             }
                             printingProps.insert(attrName, attr.value().toString());
                         }
-                        PrintingInfo printingInfo(set, printingProps);
+                        PrintingInfo printingInfo(set, LazyPropertiesHash(printingProps));
 
                         // This is very much a hack and not the right place to
                         // put this check, as it requires a reload of Cockatrice

libcockatrice_card/libcockatrice/card/lazy_properties_hash.cpp

@@ -0,0 +1,95 @@
+#include "lazy_properties_hash.h"
+
+#include <QIODevice>
+
+LazyPropertiesHash::LazyPropertiesHash() : isMaterialized(true)
+{
+}
+
+LazyPropertiesHash::LazyPropertiesHash(const QByteArray &blob) : blob(blob)
+{
+}
+
+LazyPropertiesHash::LazyPropertiesHash(const QHash<QString, QString> &properties)
+    : properties(properties), isMaterialized(true)
+{
+}
+
+LazyPropertiesHash::LazyPropertiesHash(const LazyPropertiesHash &other)
+{
+    // since we do not allow dematerialization, we only need to lock if not materialized yet
+    if (other.isMaterialized) {
+        blob = other.blob;
+        properties = other.properties;
+        isMaterialized = true;
+    } else {
+        QMutexLocker lock(&other.propertiesMutex);
+        blob = other.blob;
+        properties = other.properties;
+        isMaterialized = false;
+    }
+}
+
+LazyPropertiesHash &LazyPropertiesHash::operator=(const LazyPropertiesHash &other)
+{
+    if (this == &other) {
+        return *this;
+    }
+
+    // since we do not allow dematerialization, we only need to lock if not materialized yet
+    if (other.isMaterialized) {
+        blob = other.blob;
+        properties = other.properties;
+        isMaterialized = true;
+    } else {
+        QMutexLocker lock(&other.propertiesMutex);
+        blob = other.blob;
+        properties = other.properties;
+        isMaterialized = false;
+    }
+
+    return *this;
+}
+
+void LazyPropertiesHash::ensureMaterialized() const
+{
+    QMutexLocker lock(&propertiesMutex);
+
+    if (isMaterialized) {
+        return;
+    }
+
+    if (!blob.isEmpty()) {
+        QDataStream in(blob);
+        in.setVersion(QDataStream::Qt_6_4);
+        in >> properties;
+    }
+
+    blob.clear();
+
+    isMaterialized = true;
+}
+
+QString LazyPropertiesHash::value(const QString &key) const
+{
+    ensureMaterialized();
+    return properties.value(key);
+}
+
+bool LazyPropertiesHash::insert(const QString &key, const QString &value)
+{
+    ensureMaterialized();
+
+    if (value == properties.value(key)) {
+        return false;
+    }
+
+    properties.insert(key, value);
+    return true;
+}
+
+const QHash<QString, QString> &LazyPropertiesHash::getProperties() const
+{
+    ensureMaterialized();
+    return properties;
+}

libcockatrice_card/libcockatrice/card/lazy_properties_hash.h

@@ -0,0 +1,74 @@
+#ifndef COCKATRICE_LAZY_PROPERTIES_HASH_H
+#define COCKATRICE_LAZY_PROPERTIES_HASH_H
+
+#include <QHash>
+#include <QMutex>
+
+/**
+ * @brief A property map that can lazily deserialize blobs to avoid loading overhead.
+ *
+ * Properties are stored as a pre-serialized blob (cheap to load), and the QString is materialized on the first query,
+ * so the database load avoids constructing thousands of QString per card.
+ *
+ * Once the properties are materialized, it cannot be unmaterialized.
+ * If you want to reset the properties to an unmaterialized state, you should create a new LazyPropertiesHash.
+ */
+class LazyPropertiesHash
+{
+
+    mutable QByteArray blob;                    ///< Serialized properties (load form).
+    mutable QHash<QString, QString> properties; ///< Materialized properties (query form).
+    mutable QMutex propertiesMutex;             ///< Guards lazy materialization.
+    mutable bool isMaterialized = false;        ///< Whether propertiesCache is valid.
+
+    /**
+     * @brief Materializes properties from blob if not already done. Clears blob afterward.
+     * Safe to call from const getters (members are mutable).
+     */
+    void ensureMaterialized() const;
+
+public:
+    /**
+     * @brief Default constructor.
+     */
+    LazyPropertiesHash();
+
+    /**
+     * @brief Creates an unmaterialized LazyPropertiesHash
+     * @param blob The pre-serialized blob
+     */
+    explicit LazyPropertiesHash(const QByteArray &blob);
+
+    /**
+     * @brief Creates an already-materialized LazyPropertiesHash
+     * @param properties The properties
+     */
+    explicit LazyPropertiesHash(const QHash<QString, QString> &properties);
+
+    // Override copy constructor and copy-assignment because mutex isn't copiable
+    LazyPropertiesHash(const LazyPropertiesHash &other);
+    LazyPropertiesHash &operator=(const LazyPropertiesHash &other);
+
+    /**
+     * @brief Gets the value from the materialized properties hash
+     * @param key The key
+     * @return The value, or an empty string if the key is not present
+     */
+    QString value(const QString &key) const;
+
+    /**
+     * @brief Inserts a value into the materialized properties hash
+     * @param key The key
+     * @param value The value to insert
+     * @return True if a new value was inserted; false if the new value is the same as the existing value
+     */
+    bool insert(const QString &key, const QString &value);
+
+    /**
+     * @brief Gets a view of the materialized properties hash.
+     * @return The properties hash
+     */
+    const QHash<QString, QString> &getProperties() const;
+};
+
+#endif // COCKATRICE_LAZY_PROPERTIES_HASH_H

libcockatrice_card/libcockatrice/card/printing/printing_info.cpp

@@ -5,37 +5,14 @@
 #include <QDataStream>
 #include <QIODevice>
 
-PrintingInfo::PrintingInfo(const CardSetPtr &_set, const QHash<QString, QString> &_properties)
-    : set(_set), propertiesCache(_properties), propertiesLoaded(true)
+PrintingInfo::PrintingInfo(const CardSetPtr &_set, const LazyPropertiesHash &_properties)
+    : set(_set), properties(_properties)
 {
 }
 
-PrintingInfo::PrintingInfo(const CardSetPtr &_set, const QByteArray &_blob) : set(_set), propertiesBlob(_blob)
-{
-}
-
-void PrintingInfo::ensurePropertiesLoaded() const
-{
-    QMutexLocker lock(propertiesMutex.data());
-    if (propertiesLoaded) {
-        return;
-    }
-    propertiesCache.clear();
-    if (!propertiesBlob.isEmpty()) {
-        QDataStream in(propertiesBlob);
-        in.setVersion(QDataStream::Qt_6_4);
-        in >> propertiesCache;
-    }
-    propertiesLoaded = true;
-}
-
 void PrintingInfo::setProperty(const QString &_name, const QString &_value)
 {
-    ensurePropertiesLoaded();
-    if (propertiesCache.value(_name) == _value) {
-        return;
-    }
-    propertiesCache.insert(_name, _value);
+    properties.insert(_name, _value);
 }
 
 /**

libcockatrice_card/libcockatrice/card/printing/printing_info.h

@@ -2,12 +2,11 @@
 #define COCKATRICE_PRINTING_INFO_H
 
 #include "../set/card_set.h"
+#include "libcockatrice/card/lazy_properties_hash.h"
 
 #include <QList>
 #include <QMap>
-#include <QMutex>
 #include <QSharedPointer>
-#include <QVariant>
 
 class PrintingInfo;
 
@@ -34,15 +33,7 @@ public:
      * @param _set The set this printing belongs to (defaults to null).
      * @param _properties The printing properties (defaults to empty)
      */
-    explicit PrintingInfo(const CardSetPtr &_set = nullptr, const QHash<QString, QString> &_properties = {});
-
-    /**
-     * @brief Constructs a PrintingInfo associated with a specific set.
-     *
-     * @param _set The set this printing belongs to (defaults to null).
-     * @param _blob The serialized properties (as written by the cache writer).
-     */
-    explicit PrintingInfo(const CardSetPtr &_set, const QByteArray &_blob);
+    explicit PrintingInfo(const CardSetPtr &_set = nullptr, const LazyPropertiesHash &_properties = {});
 
     /**
      * @brief Destroys the PrintingInfo.
@@ -76,18 +67,8 @@ public:
     }
 
 private:
-    CardSetPtr set; ///< The set this variation belongs to.
-
-    // Properties are stored as a pre-serialized blob (cheap to load) and the
-    // QHash<QString, QString> is materialized on first query. This avoids constructing
-    // thousands of QStrings per card at database-load time.
-    mutable QByteArray propertiesBlob;               ///< Serialized properties (load form).
-    mutable QHash<QString, QString> propertiesCache; ///< Materialized properties (query form).
-    mutable bool propertiesLoaded = false;           ///< Whether propertiesCache is valid.
-    mutable QSharedPointer<QBasicMutex> propertiesMutex =
-        QSharedPointer<QBasicMutex>::create(); ///< Guards lazy materialization.
-
-    void ensurePropertiesLoaded() const;
+    CardSetPtr set;                ///< The set this variation belongs to.
+    LazyPropertiesHash properties; ///< Key-value store for variation-specific attributes.
 
 public:
     /**
@@ -112,8 +93,7 @@ public:
 
     [[nodiscard]] const QHash<QString, QString> &getPropertiesHash() const
     {
-        ensurePropertiesLoaded();
-        return propertiesCache;
+        return properties.getProperties();
     }
 
     /**
@@ -124,7 +104,7 @@ public:
      */
     [[nodiscard]] QString getProperty(const QString &propertyName) const
     {
-        return getPropertiesHash().value(propertyName);
+        return properties.value(propertyName);
     }
 
     /**

oracle/src/oracleimporter.cpp

@@ -316,7 +316,7 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet, const QList
             }
         }
 
-        PrintingInfo printingInfo(currentSet, printingProps);
+        PrintingInfo printingInfo(currentSet, LazyPropertiesHash(printingProps));
 
         QString numComponent;
         const QString numProperty = printingInfo.getProperty("num");