QQmlListReference: Calculate element metatype lazily

The calculation is expensive and only needed when inserting elements.
Realize that QQmlMetaType::listType() can never return -1 and remove the
respective early abort conditions. Furthermore, guard against nullptr
element types when inserting. Those would otherwise crash in
canConvert().

Change-Id: Ifaa9d7df1bf8ac372074d25554ec02944d3e3bff
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

5 files changed, 83 insertions, 29 deletions

diff --git a/src/qml/qml/qqmllist.cpp b/src/qml/qml/qqmllist.cpp
index 9a1f8e02d8..0aecc51e85 100644
--- a/src/qml/qml/qqmllist.cpp
+++ b/src/qml/qml/qqmllist.cpp
@@ -44,6 +44,16 @@
 
 QT_BEGIN_NAMESPACE
 
+static bool isObjectCompatible(QObject *object, QQmlListReferencePrivate *d)
+{
+    if (object) {
+        const QQmlMetaObject elementType = d->elementType();
+        if (elementType.isNull() || !QQmlMetaObject::canConvert(object, elementType))
+            return false;
+    }
+    return true;
+}
+
 QQmlListReferencePrivate::QQmlListReferencePrivate()
 : propertyType(-1), refCount(1)
 {
@@ -55,14 +65,9 @@ QQmlListReference QQmlListReferencePrivate::init(const QQmlListProperty<QObject>
 
     if (!prop.object) return rv;
 
-    QQmlEnginePrivate *p = engine?QQmlEnginePrivate::get(engine):nullptr;
-
-    int listType = QQmlMetaType::listType(propType);
-    if (listType == -1) return rv;
-
     rv.d = new QQmlListReferencePrivate;
     rv.d->object = prop.object;
-    rv.d->elementType = QQmlPropertyPrivate::rawMetaObjectForType(p, listType);
+    rv.d->setEngine(engine);
     rv.d->property = prop;
     rv.d->propertyType = propType;
 
@@ -129,7 +134,8 @@ become invalid.  That is, it is safe to hold QQmlListReference instances even af
 deleted.
 
 The \a engine is required to look up the element type, which may be a dynamically created QML type.
-If it's omitted, only pre-registered types are available.
+If it's omitted, only pre-registered types are available. The element type is needed when inserting
+values into the list and when the value meta type is explicitly retrieved.
 */
 QQmlListReference::QQmlListReference(const QVariant &variant, QQmlEngine *engine)
     : d(nullptr)
@@ -138,16 +144,9 @@ QQmlListReference::QQmlListReference(const QVariant &variant, QQmlEngine *engine
     if (!(t.flags() & QMetaType::IsQmlList))
         return;
 
-    QQmlEnginePrivate *p = engine ? QQmlEnginePrivate::get(engine) : nullptr;
-    const int listType = QQmlMetaType::listType(t.id());
-    if (listType == -1)
-        return;
-
     d = new QQmlListReferencePrivate;
     d->propertyType = t.id();
-    d->elementType = p
-            ? p->rawMetaObjectForType(listType)
-            : QQmlMetaType::qmlType(listType).baseMetaObject();
+    d->setEngine(engine);
 
     d->property.~QQmlListProperty();
     t.construct(&d->property, variant.constData());
@@ -161,8 +160,9 @@ property, an invalid QQmlListReference is created.  If \a object is destroyed af
 the reference is constructed, it will automatically become invalid.  That is, it is safe to hold
 QQmlListReference instances even after \a object is deleted.
 
-Passing \a engine is required to access some QML created list properties.  If in doubt, and an engine
-is available, pass it.
+The \a engine is required to look up the element type, which may be a dynamically created QML type.
+If it's omitted, only pre-registered types are available. The element type is needed when inserting
+values into the list and when the value meta type is explicitly retrieved.
 */
 QQmlListReference::QQmlListReference(QObject *object, const char *property, QQmlEngine *engine)
 : d(nullptr)
@@ -175,15 +175,10 @@ QQmlListReference::QQmlListReference(QObject *object, const char *property, QQml
 
     if (!data || !data->isQList()) return;
 
-    QQmlEnginePrivate *p = engine?QQmlEnginePrivate::get(engine):nullptr;
-
-    int listType = QQmlMetaType::listType(data->propType().id());
-    if (listType == -1) return;
-
     d = new QQmlListReferencePrivate;
     d->object = object;
-    d->elementType = p ? p->rawMetaObjectForType(listType) : QQmlMetaType::qmlType(listType).baseMetaObject();
     d->propertyType = data->propType().id();
+    d->setEngine(engine);
 
     void *args[] = { &d->property, nullptr };
     QMetaObject::metacall(object, QMetaObject::ReadProperty, data->coreIndex(), args);
@@ -233,12 +228,11 @@ Returns the QMetaObject for the elements stored in the list property,
 or \nullptr if the reference is invalid.
 
 The QMetaObject can be used ahead of time to determine whether a given instance can be added
-to a list.
+to a list. If you didn't pass an engine on construction this may return nullptr.
 */
 const QMetaObject *QQmlListReference::listElementType() const
 {
-    if (isValid()) return d->elementType.metaObject();
-    else return nullptr;
+    return isValid() ? d->elementType() : nullptr;
 }
 
 /*!
@@ -347,7 +341,7 @@ bool QQmlListReference::append(QObject *object) const
 {
     if (!canAppend()) return false;
 
-    if (object && !QQmlMetaObject::canConvert(object, d->elementType))
+    if (!isObjectCompatible(object, d))
         return false;
 
     d->property.append(&d->property, object);
@@ -408,7 +402,7 @@ bool QQmlListReference::replace(qsizetype index, QObject *object) const
     if (!canReplace())
         return false;
 
-    if (object && !QQmlMetaObject::canConvert(object, d->elementType))
+    if (!isObjectCompatible(object, d))
         return false;
 
     d->property.replace(&d->property, index, object);
diff --git a/src/qml/qml/qqmllist_p.h b/src/qml/qml/qqmllist_p.h
index e182ced51d..022bed977e 100644
--- a/src/qml/qml/qqmllist_p.h
+++ b/src/qml/qml/qqmllist_p.h
@@ -53,6 +53,8 @@
 
 #include "qqmllist.h"
 #include "qqmlmetaobject_p.h"
+#include "qqmlmetatype_p.h"
+#include "qqmlengine_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -64,7 +66,6 @@ public:
     static QQmlListReference init(const QQmlListProperty<QObject> &, int, QQmlEngine *);
 
     QPointer<QObject> object;
-    QQmlMetaObject elementType;
     QQmlListProperty<QObject> property;
     int propertyType;
 
@@ -75,6 +76,27 @@ public:
     static inline QQmlListReferencePrivate *get(QQmlListReference *ref) {
         return ref->d;
     }
+
+    void setEngine(QQmlEngine *engine)
+    {
+        m_elementTypeOrEngine = engine;
+    }
+
+    const QMetaObject *elementType()
+    {
+        if (m_elementTypeOrEngine.isT2()) {
+            const int listType = QQmlMetaType::listType(propertyType);
+            const QQmlEngine *engine = m_elementTypeOrEngine.asT2();
+            const QQmlEnginePrivate *p = engine ? QQmlEnginePrivate::get(engine) : nullptr;
+            m_elementTypeOrEngine = p ? p->rawMetaObjectForType(listType).metaObject()
+                                      : QQmlMetaType::qmlType(listType).baseMetaObject();
+        }
+
+        return m_elementTypeOrEngine.asT1();
+    }
+
+private:
+    QBiPointer<const QMetaObject, QQmlEngine> m_elementTypeOrEngine;
 };
 
 
diff --git a/tests/auto/qml/qqmllistreference/data/AListItem.qml b/tests/auto/qml/qqmllistreference/data/AListItem.qml
new file mode 100644
index 0000000000..016d7198f1
--- /dev/null
+++ b/tests/auto/qml/qqmllistreference/data/AListItem.qml
@@ -0,0 +1,5 @@
+import QtQml
+
+QtObject {
+    property int foo: 12
+}
diff --git a/tests/auto/qml/qqmllistreference/data/compositeListProp.qml b/tests/auto/qml/qqmllistreference/data/compositeListProp.qml
new file mode 100644
index 0000000000..99965f289b
--- /dev/null
+++ b/tests/auto/qml/qqmllistreference/data/compositeListProp.qml
@@ -0,0 +1,5 @@
+import QtQml
+
+QtObject {
+    property list<AListItem> items
+}
diff --git a/tests/auto/qml/qqmllistreference/tst_qqmllistreference.cpp b/tests/auto/qml/qqmllistreference/tst_qqmllistreference.cpp
index 92025cfb11..61565162e5 100644
--- a/tests/auto/qml/qqmllistreference/tst_qqmllistreference.cpp
+++ b/tests/auto/qml/qqmllistreference/tst_qqmllistreference.cpp
@@ -77,6 +77,7 @@ private slots:
     void engineTypes();
     void variantToList();
     void listProperty();
+    void compositeListProperty();
 };
 
 class TestType : public QObject
@@ -865,6 +866,33 @@ void tst_qqmllistreference::listProperty()
     QCOMPARE( state2->name(), QStringLiteral("MyState2") );
 }
 
+void tst_qqmllistreference::compositeListProperty()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("compositeListProp.qml"));
+    QScopedPointer<QObject> object(component.create());
+    QVERIFY(!object.isNull());
+
+    QQmlComponent item(&engine, testFileUrl("AListItem.qml"));
+    QScopedPointer<QObject> i1(item.create());
+    QScopedPointer<QObject> i2(item.create());
+    QVERIFY(!i1.isNull());
+    QVERIFY(!i2.isNull());
+
+    // Without engine
+    QQmlListReference list1(object.data(), "items");
+    QCOMPARE(list1.listElementType(), nullptr);
+
+    // Doesn't work because element type is unknown.
+    QVERIFY(!list1.append(i1.data()));
+    QVERIFY(!list1.replace(0, i2.data()));
+
+    // With engine
+    QQmlListReference list2(object.data(), "items", &engine);
+    QVERIFY(list2.listElementType() != nullptr);
+    QVERIFY(list2.append(i1.data()));
+    QVERIFY(list2.replace(0, i2.data()));
+}
 
 QTEST_MAIN(tst_qqmllistreference)