From 3359cfcae098401b4d18e5ca4f4f09763c9cea1c Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Fri, 6 May 2022 09:57:07 +0200 Subject: 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 (cherry picked from commit d0c7c7234cb7629fbe7a04944cb5191414cc72b9) --- src/qml/qml/qqmlproperty.cpp | 2 +- src/qml/qml/qqmlvaluetype.cpp | 4 +++ src/qml/qml/qqmlvaluetype_p.h | 10 ++++++++ src/qml/qml/qqmlvmemetaobject.cpp | 6 ++--- src/qmlmodels/qqmladaptormodel.cpp | 30 +++++++++++++++++++--- src/quicktemplates2/qquickcombobox.cpp | 27 ++++++++++--------- tests/auto/qml/qqmlproperty/tst_qqmlproperty.cpp | 7 +++-- .../quickcontrols2/controls/data/tst_combobox.qml | 19 ++++++++++++++ 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()) + return entry.toMap().value(role); + + if (type == QMetaType::fromType()) + return entry.toHash().value(role); + + const QMetaType::TypeFlags typeFlags = type.flags(); + if (typeFlags & QMetaType::PointerToQObject) + return entry.value()->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(); - 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()) { + const QVariant object = model.toList().value(index); + if (object.metaType() == QMetaType::fromType()) { + 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 engine(new QQmlEngine); + QQmlComponent component(engine.data()); component.setData("import Test 1.0\nPropertyObject { wrectProperty.x: 10 }", QUrl()); QScopedPointer 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"); + } } -- cgit v1.2.3