From 9100ed3f8872cf59d77698fabb57e0452eceaee1 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Tue, 2 May 2023 16:06:25 +0200 Subject: [PATCH] QQuickItem: Do not synthesize replace() for data/resources/children Those properties are not actually sequential containers. They have some internal logic that refuses certain operations and changes the semantics of others. We should not run things like splice() on them. We can natively implement removeLast(), though. Fixes: QTBUG-112949 Change-Id: Ic9fa84f98a68428df9e958ba7fc72b0987e8601f Reviewed-by: Fabian Kosmale (cherry picked from commit 7e1988539983531ecce589d76479f5bbe9bdb9b6) Reviewed-by: Qt Cherry-pick Bot --- src/quick/items/qquickitem.cpp | 82 ++++++++-- src/quick/items/qquickitem_p.h | 3 + .../auto/quick/qquickitem/tst_qquickitem.cpp | 150 ++++++++++++++++++ 3 files changed, 222 insertions(+), 13 deletions(-) diff --git a/src/quick/items/qquickitem.cpp b/src/quick/items/qquickitem.cpp index ab7efccb3c..5a92ef22c9 100644 --- a/src/quick/items/qquickitem.cpp +++ b/src/quick/items/qquickitem.cpp @@ -3349,6 +3349,22 @@ void QQuickItemPrivate::data_clear(QQmlListProperty *property) children_clear(&childrenProperty); } +void QQuickItemPrivate::data_removeLast(QQmlListProperty *property) +{ + QQuickItem *item = static_cast(property->object); + QQuickItemPrivate *privateItem = QQuickItemPrivate::get(item); + + QQmlListProperty childrenProperty = privateItem->children(); + if (children_count(&childrenProperty) > 0) { + children_removeLast(&childrenProperty); + return; + } + + QQmlListProperty resourcesProperty = privateItem->resources(); + if (resources_count(&resourcesProperty) > 0) + resources_removeLast(&resourcesProperty); +} + QObject *QQuickItemPrivate::resources_at(QQmlListProperty *prop, qsizetype index) { QQuickItemPrivate *quickItemPrivate = QQuickItemPrivate::get(static_cast(prop->object)); @@ -3385,6 +3401,21 @@ void QQuickItemPrivate::resources_clear(QQmlListProperty *prop) } } +void QQuickItemPrivate::resources_removeLast(QQmlListProperty *prop) +{ + QQuickItem *quickItem = static_cast(prop->object); + QQuickItemPrivate *quickItemPrivate = QQuickItemPrivate::get(quickItem); + if (quickItemPrivate->extra.isAllocated()) {//If extra is not allocated resources is empty. + QList *resources = &quickItemPrivate->extra->resourcesList; + if (resources->isEmpty()) + return; + + qmlobject_disconnect(resources->last(), QObject, SIGNAL(destroyed(QObject*)), + quickItem, QQuickItem, SLOT(_q_resourceObjectDeleted(QObject*))); + resources->removeLast(); + } +} + QQuickItem *QQuickItemPrivate::children_at(QQmlListProperty *prop, qsizetype index) { QQuickItemPrivate *p = QQuickItemPrivate::get(static_cast(prop->object)); @@ -3420,6 +3451,14 @@ void QQuickItemPrivate::children_clear(QQmlListProperty *prop) p->childItems.at(0)->setParentItem(nullptr); } +void QQuickItemPrivate::children_removeLast(QQmlListProperty *prop) +{ + QQuickItem *that = static_cast(prop->object); + QQuickItemPrivate *p = QQuickItemPrivate::get(that); + if (!p->childItems.isEmpty()) + p->childItems.last()->setParentItem(nullptr); +} + qsizetype QQuickItemPrivate::visibleChildren_count(QQmlListProperty *prop) { QQuickItemPrivate *p = QQuickItemPrivate::get(static_cast(prop->object)); @@ -3648,10 +3687,16 @@ void QQuickItemPrivate::siblingOrderChanged() QQmlListProperty QQuickItemPrivate::data() { - return QQmlListProperty(q_func(), nullptr, QQuickItemPrivate::data_append, - QQuickItemPrivate::data_count, - QQuickItemPrivate::data_at, - QQuickItemPrivate::data_clear); + // Do not synthesize replace(). + // It would be extremely expensive and wouldn't work with most methods. + QQmlListProperty result; + result.object = q_func(); + result.append = QQuickItemPrivate::data_append; + result.count = QQuickItemPrivate::data_count; + result.at = QQuickItemPrivate::data_at; + result.clear = QQuickItemPrivate::data_clear; + result.removeLast = QQuickItemPrivate::data_removeLast; + return result; } /*! @@ -4991,10 +5036,16 @@ void QQuickItemPrivate::dumpItemTree(int indent) const QQmlListProperty QQuickItemPrivate::resources() { - return QQmlListProperty(q_func(), nullptr, QQuickItemPrivate::resources_append, - QQuickItemPrivate::resources_count, - QQuickItemPrivate::resources_at, - QQuickItemPrivate::resources_clear); + // Do not synthesize replace(). + // It would be extremely expensive and wouldn't work with most methods. + QQmlListProperty result; + result.object = q_func(); + result.append = QQuickItemPrivate::resources_append; + result.count = QQuickItemPrivate::resources_count; + result.at = QQuickItemPrivate::resources_at; + result.clear = QQuickItemPrivate::resources_clear; + result.removeLast = QQuickItemPrivate::resources_removeLast; + return result; } /*! @@ -5016,11 +5067,16 @@ QQmlListProperty QQuickItemPrivate::resources() */ QQmlListProperty QQuickItemPrivate::children() { - return QQmlListProperty(q_func(), nullptr, QQuickItemPrivate::children_append, - QQuickItemPrivate::children_count, - QQuickItemPrivate::children_at, - QQuickItemPrivate::children_clear); - + // Do not synthesize replace(). + // It would be extremely expensive and wouldn't work with most methods. + QQmlListProperty result; + result.object = q_func(); + result.append = QQuickItemPrivate::children_append; + result.count = QQuickItemPrivate::children_count; + result.at = QQuickItemPrivate::children_at; + result.clear = QQuickItemPrivate::children_clear; + result.removeLast = QQuickItemPrivate::children_removeLast; + return result; } /*! diff --git a/src/quick/items/qquickitem_p.h b/src/quick/items/qquickitem_p.h index 3d8a1d2a7d..927e527e56 100644 --- a/src/quick/items/qquickitem_p.h +++ b/src/quick/items/qquickitem_p.h @@ -265,18 +265,21 @@ public: static qsizetype data_count(QQmlListProperty *); static QObject *data_at(QQmlListProperty *, qsizetype); static void data_clear(QQmlListProperty *); + static void data_removeLast(QQmlListProperty *); // resources property static QObject *resources_at(QQmlListProperty *, qsizetype); static void resources_append(QQmlListProperty *, QObject *); static qsizetype resources_count(QQmlListProperty *); static void resources_clear(QQmlListProperty *); + static void resources_removeLast(QQmlListProperty *); // children property static void children_append(QQmlListProperty *, QQuickItem *); static qsizetype children_count(QQmlListProperty *); static QQuickItem *children_at(QQmlListProperty *, qsizetype); static void children_clear(QQmlListProperty *); + static void children_removeLast(QQmlListProperty *); // visibleChildren property static void visibleChildren_append(QQmlListProperty *prop, QQuickItem *o); diff --git a/tests/auto/quick/qquickitem/tst_qquickitem.cpp b/tests/auto/quick/qquickitem/tst_qquickitem.cpp index 23616156d6..061dea3ad7 100644 --- a/tests/auto/quick/qquickitem/tst_qquickitem.cpp +++ b/tests/auto/quick/qquickitem/tst_qquickitem.cpp @@ -228,6 +228,7 @@ private slots: void polishLoopDetection(); void objectCastInDestructor(); + void listsAreNotLists(); private: @@ -2462,6 +2463,155 @@ void tst_qquickitem::objectCastInDestructor() QVERIFY(QTest::qWaitFor([&destroyed]{ return destroyed; })); } +template +void verifyListProperty(const T &data) +{ + QVERIFY(data.object); + QVERIFY(data.append); + QVERIFY(data.count); + QVERIFY(data.at); + QVERIFY(data.clear); + QVERIFY(data.removeLast); + + // We must not synthesize the replace and removeLast methods on those properties. + // They would be even more broken than the explicitly defined methods. + QVERIFY(!data.replace); +} + +void tst_qquickitem::listsAreNotLists() +{ + QQuickItem item; + QQuickItem child; + QObject resource; + + QQmlListProperty data + = item.property("data").value>(); + QQmlListProperty resources + = item.property("resources").value>(); + QQmlListProperty children + = item.property("children").value>(); + + verifyListProperty(data); + verifyListProperty(resources); + verifyListProperty(children); + + + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + children.append(&children, &child); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 1); + children.removeLast(&children); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + data.append(&data, &child); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 1); + data.removeLast(&data); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + children.append(&children, &child); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 1); + data.removeLast(&data); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + data.append(&data, &child); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 1); + children.removeLast(&children); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + + + resources.append(&resources, &resource); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + resources.removeLast(&resources); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + data.append(&data, &resource); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + data.removeLast(&data); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + resources.append(&resources, &resource); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + data.removeLast(&data); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + data.append(&data, &resource); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + resources.removeLast(&resources); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + + + children.append(&children, &child); + resources.append(&resources, &resource); + QCOMPARE(data.count(&data), 2); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 1); + children.removeLast(&children); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + resources.removeLast(&resources); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + + + children.append(&children, &child); + resources.append(&resources, &resource); + QCOMPARE(data.count(&data), 2); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 1); + resources.removeLast(&resources); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 1); + children.removeLast(&children); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); + + + data.append(&data, &child); + data.append(&data, &resource); + QCOMPARE(data.count(&data), 2); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 1); + data.removeLast(&data); + QCOMPARE(data.count(&data), 1); + QCOMPARE(resources.count(&resources), 1); + QCOMPARE(children.count(&children), 0); + data.removeLast(&data); + QCOMPARE(data.count(&data), 0); + QCOMPARE(resources.count(&resources), 0); + QCOMPARE(children.count(&children), 0); +} + QTEST_MAIN(tst_qquickitem) #include "tst_qquickitem.moc"