Ensure node creation changes are sent before using in list properties

This completes the fix for out of order event delivery related to
creation changes. We now ensure that QNodes used as values in singular
and list properties are fully constructed on the backend before they
are referenced in properties of other nodes.

Also added a check to not recurse into sending too many changes
when adding a child node.

Written with Svenn-Arne Dragly.

Task-number: Task-number: QTBUG-65956
Change-Id: I1470e0f685c81d1277ac04ad985ec1b76f1c27c0
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

1 files changed, 83 insertions, 0 deletions

diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp
index 717bb3200..4998e07d7 100644
--- a/tests/auto/core/nodes/tst_nodes.cpp
+++ b/tests/auto/core/nodes/tst_nodes.cpp
@@ -81,6 +81,7 @@ private slots:
 
     void checkConstructionSetParentMix(); // QTBUG-60612
     void checkConstructionWithParent();
+    void checkConstructionAsListElement();
 
     void appendingComponentToEntity();
     void appendingParentlessComponentToEntity();
@@ -240,6 +241,43 @@ public slots:
         emit nodePropertyChanged(node);
     }
 
+    void addAttribute(MyQNode *attribute)
+    {
+        Qt3DCore::QNodePrivate *d = Qt3DCore::QNodePrivate::get(this);
+        if (!m_attributes.contains(attribute)) {
+            m_attributes.append(attribute);
+
+            // Ensures proper bookkeeping
+            d->registerDestructionHelper(attribute, &MyQNode::removeAttribute, m_attributes);
+
+            // We need to add it as a child of the current node if it has been declared inline
+            // Or not previously added as a child of the current node so that
+            // 1) The backend gets notified about it's creation
+            // 2) When the current node is destroyed, it gets destroyed as well
+            if (!attribute->parent())
+                attribute->setParent(this);
+
+            if (d->m_changeArbiter != nullptr) {
+                const auto change = Qt3DCore::QPropertyNodeAddedChangePtr::create(id(), attribute);
+                change->setPropertyName("attribute");
+                d->notifyObservers(change);
+            }
+        }
+    }
+
+    void removeAttribute(MyQNode *attribute)
+    {
+        Qt3DCore::QNodePrivate *d = Qt3DCore::QNodePrivate::get(this);
+        if (d->m_changeArbiter != nullptr) {
+            const auto change = Qt3DCore::QPropertyNodeRemovedChangePtr::create(id(), attribute);
+            change->setPropertyName("attribute");
+            d->notifyObservers(change);
+        }
+        m_attributes.removeOne(attribute);
+        // Remove bookkeeping connection
+        d->unregisterDestructionHelper(attribute);
+    }
+
 signals:
     void customPropertyChanged();
     void nodePropertyChanged(MyQNode *node);
@@ -247,6 +285,7 @@ signals:
 protected:
     QString m_customProperty;
     MyQNode *m_nodeProperty;
+    QVector<MyQNode *> m_attributes;
 };
 
 class MyQEntity : public Qt3DCore::QEntity
@@ -921,6 +960,50 @@ void tst_Nodes::checkConstructionWithParent()
     QCOMPARE(propertyEvent->value().value<Qt3DCore::QNodeId>(), node->id());
 }
 
+void tst_Nodes::checkConstructionAsListElement()
+{
+    // GIVEN
+    ObserverSpy spy;
+    Qt3DCore::QScene scene;
+    QScopedPointer<MyQNode> root(new MyQNode());
+
+    // WHEN
+    root->setArbiterAndScene(&spy, &scene);
+    root->setSimulateBackendCreated(true);
+
+    // THEN
+    QVERIFY(Qt3DCore::QNodePrivate::get(root.data())->scene() != nullptr);
+
+    // WHEN we create a child and then set it as a Node* property
+    auto *node = new MyQNode(root.data());
+    root->addAttribute(node);
+
+    // THEN we should get one creation change, one child added change
+    // and one property change event, in that order.
+    QCoreApplication::processEvents();
+
+    QCOMPARE(root->children().count(), 1);
+    QCOMPARE(spy.events.size(), 3); // 1 creation change, 1 child added change, 1 property change
+
+    // Ensure first event is child node's creation change
+    const auto creationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!creationEvent.isNull());
+    QCOMPARE(creationEvent->subjectId(), node->id());
+
+    const auto newChildEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!newChildEvent.isNull());
+    QCOMPARE(newChildEvent->subjectId(), root->id());
+    QCOMPARE(newChildEvent->propertyName(), "children");
+    QCOMPARE(newChildEvent->addedNodeId(), node->id());
+
+    // Ensure second and last event is property set change
+    const auto propertyEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!propertyEvent.isNull());
+    QCOMPARE(propertyEvent->subjectId(), root->id());
+    QCOMPARE(propertyEvent->propertyName(), "attribute");
+    QCOMPARE(newChildEvent->addedNodeId(), node->id());
+}
+
 void tst_Nodes::appendingParentlessComponentToEntity()
 {
     // GIVEN