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 <fabian.kosmale@qt.io>
(cherry picked from commit 7e19885399)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/quick/items/qquickitem.cpp

@@ -3349,6 +3349,22 @@ void QQuickItemPrivate::data_clear(QQmlListProperty<QObject> *property)
     children_clear(&childrenProperty);
 }
 
+void QQuickItemPrivate::data_removeLast(QQmlListProperty<QObject> *property)
+{
+    QQuickItem *item = static_cast<QQuickItem*>(property->object);
+    QQuickItemPrivate *privateItem = QQuickItemPrivate::get(item);
+
+    QQmlListProperty<QQuickItem> childrenProperty = privateItem->children();
+    if (children_count(&childrenProperty) > 0) {
+        children_removeLast(&childrenProperty);
+        return;
+    }
+
+    QQmlListProperty<QObject> resourcesProperty = privateItem->resources();
+    if (resources_count(&resourcesProperty) > 0)
+        resources_removeLast(&resourcesProperty);
+}
+
 QObject *QQuickItemPrivate::resources_at(QQmlListProperty<QObject> *prop, qsizetype index)
 {
     QQuickItemPrivate *quickItemPrivate = QQuickItemPrivate::get(static_cast<QQuickItem *>(prop->object));
@@ -3385,6 +3401,21 @@ void QQuickItemPrivate::resources_clear(QQmlListProperty<QObject> *prop)
     }
 }
 
+void QQuickItemPrivate::resources_removeLast(QQmlListProperty<QObject> *prop)
+{
+    QQuickItem *quickItem = static_cast<QQuickItem *>(prop->object);
+    QQuickItemPrivate *quickItemPrivate = QQuickItemPrivate::get(quickItem);
+    if (quickItemPrivate->extra.isAllocated()) {//If extra is not allocated resources is empty.
+        QList<QObject *> *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<QQuickItem> *prop, qsizetype index)
 {
     QQuickItemPrivate *p = QQuickItemPrivate::get(static_cast<QQuickItem *>(prop->object));
@@ -3420,6 +3451,14 @@ void QQuickItemPrivate::children_clear(QQmlListProperty<QQuickItem> *prop)
         p->childItems.at(0)->setParentItem(nullptr);
 }
 
+void QQuickItemPrivate::children_removeLast(QQmlListProperty<QQuickItem> *prop)
+{
+    QQuickItem *that = static_cast<QQuickItem *>(prop->object);
+    QQuickItemPrivate *p = QQuickItemPrivate::get(that);
+    if (!p->childItems.isEmpty())
+        p->childItems.last()->setParentItem(nullptr);
+}
+
 qsizetype QQuickItemPrivate::visibleChildren_count(QQmlListProperty<QQuickItem> *prop)
 {
     QQuickItemPrivate *p = QQuickItemPrivate::get(static_cast<QQuickItem *>(prop->object));
@@ -3648,10 +3687,16 @@ void QQuickItemPrivate::siblingOrderChanged()
 
 QQmlListProperty<QObject> QQuickItemPrivate::data()
 {
-    return QQmlListProperty<QObject>(q_func(), nullptr, QQuickItemPrivate::data_append,
+    // Do not synthesize replace().
-                                             QQuickItemPrivate::data_count,
+    // It would be extremely expensive and wouldn't work with most methods.
-                                             QQuickItemPrivate::data_at,
+    QQmlListProperty<QObject> result;
-                                             QQuickItemPrivate::data_clear);
+    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<QObject> QQuickItemPrivate::resources()
 {
-    return QQmlListProperty<QObject>(q_func(), nullptr, QQuickItemPrivate::resources_append,
+    // Do not synthesize replace().
-                                             QQuickItemPrivate::resources_count,
+    // It would be extremely expensive and wouldn't work with most methods.
-                                             QQuickItemPrivate::resources_at,
+    QQmlListProperty<QObject> result;
-                                             QQuickItemPrivate::resources_clear);
+    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<QObject> QQuickItemPrivate::resources()
 */
 QQmlListProperty<QQuickItem> QQuickItemPrivate::children()
 {
-    return QQmlListProperty<QQuickItem>(q_func(), nullptr, QQuickItemPrivate::children_append,
+    // Do not synthesize replace().
-                                             QQuickItemPrivate::children_count,
+    // It would be extremely expensive and wouldn't work with most methods.
-                                             QQuickItemPrivate::children_at,
+    QQmlListProperty<QQuickItem> result;
-                                             QQuickItemPrivate::children_clear);
+    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;
 }
 
 /*!

src/quick/items/qquickitem_p.h

@@ -265,18 +265,21 @@ public:
     static qsizetype data_count(QQmlListProperty<QObject> *);
     static QObject *data_at(QQmlListProperty<QObject> *, qsizetype);
     static void data_clear(QQmlListProperty<QObject> *);
+    static void data_removeLast(QQmlListProperty<QObject> *);
 
     // resources property
     static QObject *resources_at(QQmlListProperty<QObject> *, qsizetype);
     static void resources_append(QQmlListProperty<QObject> *, QObject *);
     static qsizetype resources_count(QQmlListProperty<QObject> *);
     static void resources_clear(QQmlListProperty<QObject> *);
+    static void resources_removeLast(QQmlListProperty<QObject> *);
 
     // children property
     static void children_append(QQmlListProperty<QQuickItem> *, QQuickItem *);
     static qsizetype children_count(QQmlListProperty<QQuickItem> *);
     static QQuickItem *children_at(QQmlListProperty<QQuickItem> *, qsizetype);
     static void children_clear(QQmlListProperty<QQuickItem> *);
+    static void children_removeLast(QQmlListProperty<QQuickItem> *);
 
     // visibleChildren property
     static void visibleChildren_append(QQmlListProperty<QQuickItem> *prop, QQuickItem *o);

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<typename T>
+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<QObject> data
+        = item.property("data").value<QQmlListProperty<QObject>>();
+    QQmlListProperty<QObject> resources
+        = item.property("resources").value<QQmlListProperty<QObject>>();
+    QQmlListProperty<QQuickItem> children
+        = item.property("children").value<QQmlListProperty<QQuickItem>>();
+
+    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"