Fixed loading of invalid color properties

This was regression in d8b6b11 since it
used the return value of QVariant::convert to decide whether to use the
converted value or to keep the original (string) value. This, combined
with the fact that invalid colors are written out as empty strings,
broke the loading of invalid color values.

I seem to have added the check on the return value of convert to prevent
data loss when a conversion to the expected type fails. However, I'm not
entirely sure in which case this was preferable, so this change removes
the check again.

Also added an autotest, which can be adjusted or extended when we might
eventually run into the use-case for checking the "convert" return
value.

In the process, fixed the loadCircularReference test, which seems to
have been broken due to a change of behavior. And fixed the
loadAndSavePropertyTypes test, which broke due to adding "drawFill".

Closes #3793

NEWS.md

@@ -10,6 +10,7 @@
 * Scripting: Added Geometry interface with line and ellipse helpers
 * Scripting: Added WangSet.effectiveTypeForColor
 * Fixed crash when changing file property of custom class (#3783)
+* Fixed loading of invalid color properties (#3793)
 * Fixed object preview position with parallax factor on group layer (#3669)
 * Fixed hover highlight rendering with active parallax factor (#3669)
 * Fixed updating of object selection outlines when changing parallax factor (#3669)

src/libtiled/properties.cpp

@@ -352,7 +352,8 @@ QVariant ExportContext::toPropertyValue(const QVariant &value, int metaType) con
         return QVariant::fromValue(ObjectRef::fromInt(value.toInt()));
 
     QVariant convertedValue = value;
-    return convertedValue.convert(metaType) ? convertedValue : value;
+    convertedValue.convert(metaType);
+    return convertedValue;
 }
 
 void initializeMetatypes()

tests/properties/test_properties.cpp

@@ -55,6 +55,7 @@ constexpr auto propertyTypesJson = R"([
     },
     {
         "color": "#ffa0a0a4",
+        "drawFill": true,
         "id": 5,
         "members": [
             {
@@ -240,6 +241,9 @@ class test_Properties : public QObject
 private slots:
     void initTestCase();
 
+    void toPropertyValue();
+    void toPropertyValue_data();
+
     void loadAndSavePropertyTypes();
     void loadCircularReference();
     void loadEnumInNestedClass();
@@ -289,6 +293,49 @@ void test_Properties::initTestCase()
     classType.members.insert(QStringLiteral("enumFlagsInt"), flagsIntValue);
 }
 
+void test_Properties::toPropertyValue()
+{
+    QFETCH(QString, value);
+    QFETCH(QString, type);
+    QFETCH(QVariant, expected);
+
+    ExportContext context;
+    ExportValue exportValue;
+    exportValue.value = value;
+    exportValue.typeName = type;
+
+    const auto actual = context.toPropertyValue(exportValue);
+    QCOMPARE(actual, expected);
+}
+
+void test_Properties::toPropertyValue_data()
+{
+    QTest::addColumn<QString>("value");
+    QTest::addColumn<QString>("type");
+    QTest::addColumn<QVariant>("expected");
+
+    QTest::newRow("color") << QStringLiteral("#ff0000ff") << QStringLiteral("color") << QVariant::fromValue(QColor(Qt::blue));
+    QTest::newRow("invalid-color") << QString() << QStringLiteral("color") << QVariant::fromValue(QColor());
+    QTest::newRow("int") << QStringLiteral("42") << QStringLiteral("int") << QVariant::fromValue(42);
+    QTest::newRow("float") << QStringLiteral("42.0") << QStringLiteral("float") << QVariant::fromValue(42.0);
+#if QT_VERSION >= QT_VERSION_CHECK(6,0,0)
+    QTest::newRow("invalid-int") << QStringLiteral("foo") << QStringLiteral("int") << QVariant(QMetaType(QMetaType::Int));
+    QTest::newRow("invalid-float") << QStringLiteral("foo") << QStringLiteral("float") << QVariant(QMetaType(QMetaType::Double));
+#else
+    QTest::newRow("invalid-int") << QStringLiteral("foo") << QStringLiteral("int") << QVariant(QVariant::Int);
+    QTest::newRow("invalid-float") << QStringLiteral("foo") << QStringLiteral("float") << QVariant(QVariant::Double);
+#endif
+    QTest::newRow("bool-true") << QStringLiteral("true") << QStringLiteral("bool") << QVariant::fromValue(true);
+    QTest::newRow("bool-false") << QStringLiteral("false") << QStringLiteral("bool") << QVariant::fromValue(false);
+    QTest::newRow("string") << QStringLiteral("foo") << QStringLiteral("string") << QVariant::fromValue(QStringLiteral("foo"));
+#if QT_VERSION >= QT_VERSION_CHECK(6,0,0)
+    QTest::newRow("file") << QStringLiteral("/foo") << QStringLiteral("file") << QVariant::fromValue(FilePath { QUrl::fromLocalFile(QStringLiteral("/foo")) });
+#endif
+    QTest::newRow("object") << QStringLiteral("1") << QStringLiteral("object") << QVariant::fromValue(ObjectRef { 1 });
+
+    // todo: test enums and classes, and also add a test for toExportValue
+}
+
 void test_Properties::loadAndSavePropertyTypes()
 {
     QJsonParseError error;
@@ -326,11 +373,11 @@ void test_Properties::loadCircularReference()
     types.loadFromJson(doc.array(), QString());
 
     // Verify the back reference is not present
-    const auto b = types.findPropertyValueType(QStringLiteral("B"));
-    QVERIFY(b);
-    QCOMPARE(b->type, PropertyType::PT_Class);
-    const auto &membersB = static_cast<const ClassPropertyType*>(b)->members;
-    QVERIFY(!membersB.contains(QStringLiteral("a")));
+    const auto a = types.findPropertyValueType(QStringLiteral("A"));
+    QVERIFY(a);
+    QCOMPARE(a->type, PropertyType::PT_Class);
+    const auto &membersA = static_cast<const ClassPropertyType*>(a)->members;
+    QVERIFY(!membersA.contains(QStringLiteral("b")));
 }
 
 void test_Properties::loadEnumInNestedClass()