From 8ea4d49e41afccf2aea12ee2b4adb99628c12ae9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thorbj=C3=B8rn=20Lindeijer?= <bjorn@lindeijer.nl>
Date: Wed, 10 Jul 2024 12:46:50 +0200
Subject: [PATCH] Initial work on supporting list property types

* Added 'list' as option to the Add Property dialog, for adding a
  QVariantList.

* Adjusted the CustomPropertiesHelper to create sub-properties that
  display the values in the list (maybe should rather be done by the
  VariantPropertyManager).

* Added support for saving and loading list properties to TMX and JSON
  formats.
---
 src/libtiled/mapreader.cpp             | 17 ++++-
 src/libtiled/maptovariantconverter.cpp | 22 ++++++-
 src/libtiled/mapwriter.cpp             | 30 ++++++---
 src/libtiled/properties.cpp            | 21 ++-----
 src/libtiled/properties.h              |  6 +-
 src/libtiled/varianttomapconverter.cpp | 21 ++++++-
 src/tiled/addpropertydialog.cpp        |  1 +
 src/tiled/custompropertieshelper.cpp   | 87 +++++++++++++++++++++++++-
 src/tiled/custompropertieshelper.h     |  3 +
 9 files changed, 178 insertions(+), 30 deletions(-)

diff --git a/src/libtiled/mapreader.cpp b/src/libtiled/mapreader.cpp
index 820eed8b43..8a5a33afbc 100644
--- a/src/libtiled/mapreader.cpp
+++ b/src/libtiled/mapreader.cpp
@@ -137,6 +137,7 @@ class MapReaderPrivate
 
     Properties readProperties();
     void readProperty(Properties *properties, const ExportContext &context);
+    QVariant readPropertyValue(const ExportContext &context);
 
     MapReader *p;
 
@@ -1448,6 +1449,13 @@ void MapReaderPrivate::readProperty(Properties *properties, const ExportContext
     const QXmlStreamAttributes atts = xml.attributes();
     QString propertyName = atts.value(QLatin1String("name")).toString();
 
+    properties->insert(propertyName, readPropertyValue(context));
+}
+
+QVariant MapReaderPrivate::readPropertyValue(const ExportContext &context)
+{
+    const QXmlStreamAttributes atts = xml.attributes();
+
     ExportValue exportValue;
     exportValue.typeName = atts.value(QLatin1String("type")).toString();
     exportValue.propertyTypeName = atts.value(QLatin1String("propertytype")).toString();
@@ -1455,6 +1463,8 @@ void MapReaderPrivate::readProperty(Properties *properties, const ExportContext
     const QString propertyValue = atts.value(QLatin1String("value")).toString();
     exportValue.value = propertyValue;
 
+    QVariantList values;
+
     while (xml.readNext() != QXmlStreamReader::Invalid) {
         if (xml.isEndElement()) {
             break;
@@ -1464,12 +1474,17 @@ void MapReaderPrivate::readProperty(Properties *properties, const ExportContext
         } else if (xml.isStartElement()) {
             if (xml.name() == QLatin1String("properties"))
                 exportValue.value = readProperties();
+            else if (xml.name() == QLatin1String("value"))
+                values.append(readPropertyValue(context));
             else
                 readUnknownElement();
         }
     }
 
-    properties->insert(propertyName, context.toPropertyValue(exportValue));
+    if (exportValue.typeName == QLatin1String("list"))
+        exportValue.value = values;
+
+    return context.toPropertyValue(exportValue);
 }
 
 
diff --git a/src/libtiled/maptovariantconverter.cpp b/src/libtiled/maptovariantconverter.cpp
index 93e7fb882f..c3b8943644 100644
--- a/src/libtiled/maptovariantconverter.cpp
+++ b/src/libtiled/maptovariantconverter.cpp
@@ -819,12 +819,30 @@ void MapToVariantConverter::addProperties(QVariantMap &variantMap,
 
             QVariantMap propertyVariantMap;
             propertyVariantMap[QStringLiteral("name")] = it.key();
-            propertyVariantMap[QStringLiteral("value")] = exportValue.value;
             propertyVariantMap[QStringLiteral("type")] = exportValue.typeName;
-
             if (!exportValue.propertyTypeName.isEmpty())
                 propertyVariantMap[QStringLiteral("propertytype")] = exportValue.propertyTypeName;
 
+            if (exportValue.typeName == QLatin1String("list")) {
+                const QVariantList values = it.value().toList();
+                QVariantList valuesVariantList;
+
+                // todo: this doesn't support lists of lists
+                for (const QVariant &value : values) {
+                    QVariantMap valueVariantMap;
+                    const auto exportValue = context.toExportValue(value);
+                    valueVariantMap[QStringLiteral("value")] = exportValue.value;
+                    valueVariantMap[QStringLiteral("type")] = exportValue.typeName;
+                    if (!exportValue.propertyTypeName.isEmpty())
+                        valueVariantMap[QStringLiteral("propertytype")] = exportValue.propertyTypeName;
+                    valuesVariantList << valueVariantMap;
+                }
+
+                propertyVariantMap[QStringLiteral("value")] = valuesVariantList;
+            } else {
+                propertyVariantMap[QStringLiteral("value")] = exportValue.value;
+            }
+
             propertiesVariantList << propertyVariantMap;
         }
 
diff --git a/src/libtiled/mapwriter.cpp b/src/libtiled/mapwriter.cpp
index d6c4f3265d..ce806b13cf 100644
--- a/src/libtiled/mapwriter.cpp
+++ b/src/libtiled/mapwriter.cpp
@@ -905,24 +905,40 @@ void MapWriterPrivate::writeProperties(QXmlStreamWriter &w,
         if (!exportValue.propertyTypeName.isEmpty())
             w.writeAttribute(QStringLiteral("propertytype"), exportValue.propertyTypeName);
 
-        // For class property values, write out the original value, so that the
-        // propertytype attribute can also be written for their members where
-        // applicable.
-        if (exportValue.value.userType() == QMetaType::QVariantMap) {
+        switch (exportValue.value.userType()) {
+        case QMetaType::QVariantList: {
+            const auto values = exportValue.value.toList();
+            for (const QVariant &v : values) {
+                const auto exportValue = context.toExportValue(v);
+                w.writeStartElement(QStringLiteral("value"));
+                if (exportValue.typeName != QLatin1String("string"))
+                    w.writeAttribute(QStringLiteral("type"), exportValue.typeName);
+                if (!exportValue.propertyTypeName.isEmpty())
+                    w.writeAttribute(QStringLiteral("propertytype"), exportValue.propertyTypeName);
+                w.writeCharacters(exportValue.value.toString());
+                w.writeEndElement();
+            }
+            break;
+        }
+        case QMetaType::QVariantMap:
+            // Write out the original value, so that the propertytype attribute
+            // can also be written for their members where applicable.
             writeProperties(w, it.value().value<PropertyValue>().value.toMap());
-        } else {
+            break;
+        default:
             const QString value = exportValue.value.toString();
 
             if (value.contains(QLatin1Char('\n')))
                 w.writeCharacters(value);
             else
                 w.writeAttribute(QStringLiteral("value"), value);
+            break;
         }
 
-        w.writeEndElement();
+        w.writeEndElement(); // </property>
     }
 
-    w.writeEndElement();
+    w.writeEndElement(); // </properties>
 }
 
 void MapWriterPrivate::writeImage(QXmlStreamWriter &w,
diff --git a/src/libtiled/properties.cpp b/src/libtiled/properties.cpp
index cd05bfacca..3a4975c318 100644
--- a/src/libtiled/properties.cpp
+++ b/src/libtiled/properties.cpp
@@ -196,21 +196,6 @@ void aggregateProperties(AggregatedProperties &aggregated, const Properties &pro
     }
 }
 
-int propertyValueId()
-{
-    return qMetaTypeId<PropertyValue>();
-}
-
-int filePathTypeId()
-{
-    return qMetaTypeId<FilePath>();
-}
-
-int objectRefTypeId()
-{
-    return qMetaTypeId<ObjectRef>();
-}
-
 QString typeToName(int type)
 {
     // We can't handle the PropertyValue purely by its type ID, since we need to
@@ -226,6 +211,8 @@ QString typeToName(int type)
         return QStringLiteral("color");
     case QMetaType::QVariantMap:
         return QStringLiteral("class");
+    case QMetaType::QVariantList:
+        return QStringLiteral("list");
 
     default:
         if (type == filePathTypeId())
@@ -309,6 +296,7 @@ ExportValue ExportContext::toExportValue(const QVariant &value) const
     } else if (metaType == objectRefTypeId()) {
         exportValue.value = ObjectRef::toInt(value.value<ObjectRef>());
     } else {
+        // Other values, including lists, do not need special handling here
         exportValue.value = value;
     }
 
@@ -343,6 +331,9 @@ QVariant ExportContext::toPropertyValue(const QVariant &value, int metaType) con
     if (metaType == QMetaType::QVariantMap || metaType == propertyValueId())
         return value;   // should be covered by property type
 
+    if (metaType == QMetaType::QVariantList)
+        return value;   // list elements should be converted individually
+
     if (metaType == filePathTypeId()) {
         const QUrl url = toUrl(value.toString(), mPath);
         return QVariant::fromValue(FilePath { url });
diff --git a/src/libtiled/properties.h b/src/libtiled/properties.h
index 2f999f32c9..09f5c48a28 100644
--- a/src/libtiled/properties.h
+++ b/src/libtiled/properties.h
@@ -185,9 +185,9 @@ TILEDSHARED_EXPORT void mergeProperties(Properties &target, const Properties &so
 TILEDSHARED_EXPORT QJsonArray propertiesToJson(const Properties &properties, const ExportContext &context = ExportContext());
 TILEDSHARED_EXPORT Properties propertiesFromJson(const QJsonArray &json, const ExportContext &context = ExportContext());
 
-TILEDSHARED_EXPORT int propertyValueId();
-TILEDSHARED_EXPORT int filePathTypeId();
-TILEDSHARED_EXPORT int objectRefTypeId();
+constexpr int propertyValueId() { return qMetaTypeId<PropertyValue>(); }
+constexpr int filePathTypeId() { return qMetaTypeId<FilePath>(); }
+constexpr int objectRefTypeId() { return qMetaTypeId<ObjectRef>(); }
 
 TILEDSHARED_EXPORT QString typeToName(int type);
 TILEDSHARED_EXPORT QString typeName(const QVariant &value);
diff --git a/src/libtiled/varianttomapconverter.cpp b/src/libtiled/varianttomapconverter.cpp
index 2c2b9bf3b5..f81ccd9b2c 100644
--- a/src/libtiled/varianttomapconverter.cpp
+++ b/src/libtiled/varianttomapconverter.cpp
@@ -190,7 +190,26 @@ Properties VariantToMapConverter::toProperties(const QVariant &propertiesVariant
         exportValue.typeName = propertyVariantMap[QStringLiteral("type")].toString();
         exportValue.propertyTypeName = propertyVariantMap[QStringLiteral("propertytype")].toString();
 
-        properties[propertyName] = context.toPropertyValue(exportValue);
+        auto &value = properties[propertyName];
+
+        if (exportValue.typeName == QLatin1String("list")) {
+            const QVariantList values = exportValue.value.toList();
+            QVariantList convertedList;
+            convertedList.reserve(values.size());
+            for (const QVariant &value : values) {
+                const QVariantMap valueVariantMap = value.toMap();
+                ExportValue itemValue;
+                itemValue.value = valueVariantMap[QStringLiteral("value")];
+                itemValue.typeName = valueVariantMap[QStringLiteral("type")].toString();
+                itemValue.propertyTypeName = valueVariantMap[QStringLiteral("propertytype")].toString();
+
+                // todo: this doesn't support lists of lists
+                convertedList.append(context.toPropertyValue(itemValue));
+            }
+            value = convertedList;
+        } else {
+            value = context.toPropertyValue(exportValue);
+        }
     }
 
     return properties;
diff --git a/src/tiled/addpropertydialog.cpp b/src/tiled/addpropertydialog.cpp
index 230ba545ad..1e00ac3621 100644
--- a/src/tiled/addpropertydialog.cpp
+++ b/src/tiled/addpropertydialog.cpp
@@ -67,6 +67,7 @@ void AddPropertyDialog::initialize(const Tiled::ClassPropertyType *parentClassTy
     mUi->typeBox->addItem(plain, typeToName(QMetaType::Int),       0);
     mUi->typeBox->addItem(plain, typeToName(objectRefTypeId()),    QVariant::fromValue(ObjectRef()));
     mUi->typeBox->addItem(plain, typeToName(QMetaType::QString),   QString());
+    mUi->typeBox->addItem(plain, typeToName(QMetaType::QVariantList),  QVariantList());
 
     for (const auto propertyType : Object::propertyTypes()) {
         // Avoid suggesting the creation of circular dependencies between types
diff --git a/src/tiled/custompropertieshelper.cpp b/src/tiled/custompropertieshelper.cpp
index a64dd7ed3a..6aa24f03fe 100644
--- a/src/tiled/custompropertieshelper.cpp
+++ b/src/tiled/custompropertieshelper.cpp
@@ -31,6 +31,13 @@
 
 namespace Tiled {
 
+/**
+ * Constructs a custom properties helper that manages properties in the given
+ * \a propertyBrowser.
+ *
+ * Uses its own VariantPropertyManager to create the properties and
+ * instantiates a VariantEditorFactory which creates the editor widgets.
+ */
 CustomPropertiesHelper::CustomPropertiesHelper(QtAbstractPropertyBrowser *propertyBrowser,
                                                QObject *parent)
     : QObject(parent)
@@ -56,6 +63,14 @@ CustomPropertiesHelper::~CustomPropertiesHelper()
     mPropertyBrowser->unsetFactoryForManager(mPropertyManager);
 }
 
+/**
+ * Creates a top-level property with the given \a name and \a value.
+ *
+ * The property is not added to the property browser. It should be added either
+ * directly or to a suitable group property.
+ *
+ * The name of the property needs to be unique.
+ */
 QtVariantProperty *CustomPropertiesHelper::createProperty(const QString &name,
                                                           const QVariant &value)
 {
@@ -71,6 +86,17 @@ QtVariantProperty *CustomPropertiesHelper::createProperty(const QString &name,
     return property;
 }
 
+/**
+ * Implementation of property creation. Used by createProperty for creating
+ * top-level properties and by setPropertyAttributes for creating nested
+ * properties.
+ *
+ * The \a value is only used for determining the type of the property, it is
+ * not set on the property.
+ *
+ * This function works recursively, creating nested properties for class
+ * properties.
+ */
 QtVariantProperty *CustomPropertiesHelper::createPropertyInternal(const QString &name,
                                                                   const QVariant &value)
 {
@@ -99,6 +125,14 @@ QtVariantProperty *CustomPropertiesHelper::createPropertyInternal(const QString
             }
             }
         }
+    } else if (type == QMetaType::QVariantList) {
+        // In case of list values, we need an expandable property with the list
+        // values as subproperties (though creation of such properties will
+        // only be done once the value is set)
+
+        // todo: lists probably need their own type here, such that a widget
+        // can be created that allows for adding elements
+        type = VariantPropertyManager::unstyledGroupTypeId();
     }
 
     if (type == objectRefTypeId())
@@ -127,6 +161,11 @@ QtVariantProperty *CustomPropertiesHelper::createPropertyInternal(const QString
     return property;
 }
 
+/**
+ * Deletes the given top-level property.
+ *
+ * Should only be used for properties created with createProperty.
+ */
 void CustomPropertiesHelper::deleteProperty(QtProperty *property)
 {
     Q_ASSERT(hasProperty(property));
@@ -135,6 +174,13 @@ void CustomPropertiesHelper::deleteProperty(QtProperty *property)
     deletePropertyInternal(property);
 }
 
+/**
+ * Implementation of property deletion. Used by deleteProperty for deleting
+ * top-level properties and by deleteSubProperties for deleting nested
+ * properties.
+ *
+ * This function works recursively, also deleting all nested properties.
+ */
 void CustomPropertiesHelper::deletePropertyInternal(QtProperty *property)
 {
     Q_ASSERT(mPropertyTypeIds.contains(property));
@@ -143,6 +189,12 @@ void CustomPropertiesHelper::deletePropertyInternal(QtProperty *property)
     delete property;
 }
 
+/**
+ * Deletes all sub-properties of the given \a property.
+ *
+ * Used when a property is being deleted or before refreshing the nested
+ * properties that represent class members.
+ */
 void CustomPropertiesHelper::deleteSubProperties(QtProperty *property)
 {
     const auto subProperties = property->subProperties();
@@ -154,6 +206,9 @@ void CustomPropertiesHelper::deleteSubProperties(QtProperty *property)
     }
 }
 
+/**
+ * Removes all properties.
+ */
 void CustomPropertiesHelper::clear()
 {
     QHashIterator<QtProperty *, int> it(mPropertyTypeIds);
@@ -171,7 +226,10 @@ QVariant CustomPropertiesHelper::toDisplayValue(QVariant value) const
         value = value.value<PropertyValue>().value;
 
     if (value.userType() == objectRefTypeId())
-        value = QVariant::fromValue(DisplayObjectRef { value.value<ObjectRef>(), mMapDocument });
+        value = QVariant::fromValue(DisplayObjectRef {
+                                        value.value<ObjectRef>(),
+                                        mMapDocument
+                                    });
 
     return value;
 }
@@ -226,6 +284,24 @@ void CustomPropertiesHelper::onValueChanged(QtProperty *property, const QVariant
             static_cast<QtVariantProperty*>(subProperty)->setValue(toDisplayValue(value));
         }
     }
+
+    if (value.userType() == QMetaType::QVariantList) {
+        QScopedValueRollback<bool> updating(mUpdating, true);
+
+        // Delete any existing sub-properties
+        deleteSubProperties(property);
+
+        // Create a sub-property for each list value
+        const auto values = value.toList();
+        for (int i = 0; i < values.size(); ++i) {
+            const auto &value = values.at(i);
+
+            auto subProperty = createPropertyInternal(QString::number(i), value);
+            subProperty->setValue(toDisplayValue(value));
+            property->addSubProperty(subProperty);
+            mPropertyParents.insert(subProperty, property);
+        }
+    }
 }
 
 void CustomPropertiesHelper::resetProperty(QtProperty *property)
@@ -278,6 +354,15 @@ void CustomPropertiesHelper::propertyTypesChanged()
     }
 }
 
+/**
+ * When the given \a propertyType is an EnumPropertyType, sets the appropriate
+ * attributes on the given \a property.
+ *
+ * Also creates sub-properties for members when the given \a propertyType is a
+ * ClassPropertyType.
+ *
+ * Called after property creation, as well as when the property types changed.
+ */
 void CustomPropertiesHelper::setPropertyAttributes(QtVariantProperty *property,
                                                    const PropertyType &propertyType)
 {
diff --git a/src/tiled/custompropertieshelper.h b/src/tiled/custompropertieshelper.h
index 1006b4a071..8492ef82bb 100644
--- a/src/tiled/custompropertieshelper.h
+++ b/src/tiled/custompropertieshelper.h
@@ -34,6 +34,9 @@ class MapDocument;
 class PropertyType;
 class VariantEditorFactory;
 
+/**
+ * Helper class for managing custom properties in a QtAbstractPropertyBrowser.
+ */
 class CustomPropertiesHelper : public QObject
 {
     Q_OBJECT