Generalize role selection mechanism from QQuickComboBox

We can have QQmlDelegateModel pick specific roles from model items. That
can easily be done for variant maps, variant hashes, objects and
gadgets. We would like to do it for anything that has a
QMetaAssociation, but as we cannot get to the original type at that
place, it's currently not possible.

The special case about variant maps with exactly one item in variant
lists is clearly insane and therefore not included in the
generalization.

This requires some cleanup in the QQmlGadgetPointerWrapper. Passing the
wrapper itself to QMetaProperty::read() has always been a rather obscure
way of reading from the gadget.

Fixes: QTBUG-102983
Change-Id: I84ecef980783e7137aa4d77070ddce47b6ead260
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
(cherry picked from commit d0c7c7234cb7629fbe7a04944cb5191414cc72b9)

8 files changed, 82 insertions, 23 deletions

diff --git a/src/qml/qml/qqmlproperty.cpp b/src/qml/qml/qqmlproperty.cpp
index 0809e44a32..ef9d6d30da 100644
--- a/src/qml/qml/qqmlproperty.cpp
+++ b/src/qml/qml/qqmlproperty.cpp
@@ -1168,7 +1168,7 @@ QVariant QQmlPropertyPrivate::readValueProperty()
 {
     auto doRead = [&](QQmlGadgetPtrWrapper *wrapper) {
         wrapper->read(object, core.coreIndex());
-        return wrapper->property(valueTypeData.coreIndex()).read(wrapper);
+        return wrapper->readOnGadget(wrapper->property(valueTypeData.coreIndex()));
     };
 
     if (isValueType()) {
diff --git a/src/qml/qml/qqmlvaluetype.cpp b/src/qml/qml/qqmlvaluetype.cpp
index 69baa03f74..e9eb623e81 100644
--- a/src/qml/qml/qqmlvaluetype.cpp
+++ b/src/qml/qml/qqmlvaluetype.cpp
@@ -51,6 +51,10 @@ QQmlValueType::QQmlValueType(QMetaType type, const QMetaObject *gadgetMetaObject
     : metaType(type)
 {
     QMetaObjectBuilder builder(gadgetMetaObject);
+
+    // This is required for calling readOnGadget() on properties from this metaObject.
+    builder.setFlags(PropertyAccessInStaticMetaCall);
+
     dynamicMetaObject = builder.toMetaObject();
 }
 
diff --git a/src/qml/qml/qqmlvaluetype_p.h b/src/qml/qml/qqmlvaluetype_p.h
index c8e2945413..aef678fba0 100644
--- a/src/qml/qml/qqmlvaluetype_p.h
+++ b/src/qml/qml/qqmlvaluetype_p.h
@@ -110,7 +110,17 @@ public:
 
     int metaTypeId() const { return valueType()->metaTypeId(); }
     int metaCall(QMetaObject::Call type, int id, void **argv);
+
     QMetaProperty property(int index) { return valueType()->metaObject()->property(index); }
+    QVariant readOnGadget(const QMetaProperty &property) const
+    {
+        return property.readOnGadget(m_gadgetPtr);
+    }
+
+    void writeOnGadget(const QMetaProperty &property, const QVariant &value)
+    {
+        property.writeOnGadget(m_gadgetPtr, value);
+    }
 
 private:
     const QQmlValueType *valueType() const;
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index a86c249e40..056ada9a8d 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -362,15 +362,15 @@ bool QQmlInterceptorMetaObject::doIntercept(QMetaObject::Call c, int id, void **
                 QVariant newValue(metaType, a[0]);
 
                 valueType->read(object, id);
-                QVariant prevComponentValue = valueProp.read(valueType);
+                QVariant prevComponentValue = valueType->readOnGadget(valueProp);
 
                 valueType->setValue(newValue);
-                QVariant newComponentValue = valueProp.read(valueType);
+                QVariant newComponentValue = valueType->readOnGadget(valueProp);
 
                 // Don't apply the interceptor if the intercepted value has not changed
                 bool updated = false;
                 if (newComponentValue != prevComponentValue) {
-                    valueProp.write(valueType, prevComponentValue);
+                    valueType->writeOnGadget(valueProp, prevComponentValue);
                     valueType->write(object, id, QQmlPropertyData::DontRemoveBinding | QQmlPropertyData::BypassInterceptor);
 
                     vi->write(newComponentValue);
diff --git a/src/qmlmodels/qqmladaptormodel.cpp b/src/qmlmodels/qqmladaptormodel.cpp
index e55845d3e2..a2c047ed32 100644
--- a/src/qmlmodels/qqmladaptormodel.cpp
+++ b/src/qmlmodels/qqmladaptormodel.cpp
@@ -672,9 +672,33 @@ public:
 
     QVariant value(const QQmlAdaptorModel &model, int index, const QString &role) const override
     {
-        return role == QLatin1String("modelData")
-                ? model.list.at(index)
-                : QVariant();
+        const QVariant entry = model.list.at(index);
+        if (role == QLatin1String("modelData"))
+            return entry;
+
+        const QMetaType type = entry.metaType();
+        if (type == QMetaType::fromType<QVariantMap>())
+            return entry.toMap().value(role);
+
+        if (type == QMetaType::fromType<QVariantHash>())
+            return entry.toHash().value(role);
+
+        const QMetaType::TypeFlags typeFlags = type.flags();
+        if (typeFlags & QMetaType::PointerToQObject)
+            return entry.value<QObject *>()->property(role.toUtf8());
+
+        const QMetaObject *metaObject = type.metaObject();
+        if (!metaObject) {
+            // NB: This acquires the lock on QQmlMetaTypeData. If we had a QQmlEngine here,
+            //     we could use QQmlGadgetPtrWrapper::instance() to avoid this.
+            if (const QQmlValueType *valueType = QQmlMetaType::valueType(type))
+                metaObject = valueType->metaObject();
+            else
+                return QVariant();
+        }
+
+        const int propertyIndex = metaObject->indexOfProperty(role.toUtf8());
+        return metaObject->property(propertyIndex).readOnGadget(entry.constData());
     }
 
     QQmlDelegateModelItem *createItem(
diff --git a/src/quicktemplates2/qquickcombobox.cpp b/src/quicktemplates2/qquickcombobox.cpp
index 32e29bf12b..1ff1291bbe 100644
--- a/src/quicktemplates2/qquickcombobox.cpp
+++ b/src/quicktemplates2/qquickcombobox.cpp
@@ -196,6 +196,7 @@ namespace {
     enum Highlighting { NoHighlight, Highlight };
 }
 
+// ### Qt7: Remove this class. Use QQmlDelegateModel instead.
 class QQuickComboBoxDelegateModel : public QQmlDelegateModel
 {
 public:
@@ -214,20 +215,22 @@ QQuickComboBoxDelegateModel::QQuickComboBoxDelegateModel(QQuickComboBox *combo)
 
 QVariant QQuickComboBoxDelegateModel::variantValue(int index, const QString &role)
 {
-    const QVariant model = combo->model();
-    if (model.userType() == QMetaType::QVariantList) {
-        QVariant object = model.toList().value(index);
-        if (object.userType() == QMetaType::QVariantMap) {
-            const QVariantMap data = object.toMap();
-            if (data.count() == 1 && role == QLatin1String("modelData"))
-                return data.first();
-            return data.value(role);
-        } else if (object.userType() == QMetaType::QObjectStar) {
-            const QObject *data = object.value<QObject *>();
-            if (data && role != QLatin1String("modelData"))
-                return data->property(role.toUtf8());
+    // ### Qt7: Get rid of this. Why do we special case lists of variant maps with
+    //          exactly one entry? There are many other ways of producing a list of
+    //          map-like things with exactly one entry. And what if some of the maps
+    //          in the list have more than one entry? You get inconsistent results.
+    if (role == QLatin1String("modelData")) {
+        const QVariant model = combo->model();
+        if (model.metaType() == QMetaType::fromType<QVariantList>()) {
+            const QVariant object = model.toList().value(index);
+            if (object.metaType() == QMetaType::fromType<QVariantMap>()) {
+                const QVariantMap data = object.toMap();
+                if (data.count() == 1)
+                    return data.first();
+            }
         }
     }
+
     return QQmlDelegateModel::variantValue(index, role);
 }
 
diff --git a/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp b/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp
index 402a68afae..5d426cff4f 100644
--- a/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp
+++ b/tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp
@@ -1943,8 +1943,8 @@ void tst_qqmlproperty::variantMapHandling()
 
 void tst_qqmlproperty::crashOnValueProperty()
 {
-    QQmlEngine *engine = new QQmlEngine;
-    QQmlComponent component(engine);
+    QScopedPointer<QQmlEngine> engine(new QQmlEngine);
+    QQmlComponent component(engine.data());
 
     component.setData("import Test 1.0\nPropertyObject { wrectProperty.x: 10 }", QUrl());
     QScopedPointer<QObject> object(component.create());
@@ -1957,8 +1957,7 @@ void tst_qqmlproperty::crashOnValueProperty()
     QCOMPARE(p.read(), QVariant(10));
 
     //don't crash once the engine is deleted
-    delete engine;
-    engine = nullptr;
+    engine.reset();
 
     QCOMPARE(p.propertyTypeName(), "int");
     QCOMPARE(p.read(), QVariant(10));
diff --git a/tests/auto/quickcontrols2/controls/data/tst_combobox.qml b/tests/auto/quickcontrols2/controls/data/tst_combobox.qml
index 7da491d499..e9575369e1 100644
--- a/tests/auto/quickcontrols2/controls/data/tst_combobox.qml
+++ b/tests/auto/quickcontrols2/controls/data/tst_combobox.qml
@@ -2336,4 +2336,23 @@ TestCase {
         compare(textField.selectedText, "")
         compare(control.currentIndex, 1)
     }
+
+    Component {
+        id: listOfGadgets
+        QtObject {
+            property var rects: [Qt.rect(1, 2, 3, 4), Qt.rect(5, 6, 7, 8)]
+        }
+    }
+
+    function test_listOfGadgetsWithRole() {
+        let model = listOfGadgets.createObject(testCase);
+        let control = createTemporaryObject(
+                comboBox, testCase, {model: model.rects, textRole: "width"});
+        verify(control);
+        compare(control.currentIndex, 0);
+        compare(control.displayText, "3");
+
+        control.currentIndex = 1;
+        compare(control.displayText, "7");
+    }
 }