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>

4 files changed, 112 insertions, 2 deletions

diff --git a/src/core/changes/qpropertynodeaddedchange.cpp b/src/core/changes/qpropertynodeaddedchange.cpp
index 390a170b6..9f8e50872 100644
--- a/src/core/changes/qpropertynodeaddedchange.cpp
+++ b/src/core/changes/qpropertynodeaddedchange.cpp
@@ -76,6 +76,16 @@ QPropertyNodeAddedChange::QPropertyNodeAddedChange(QNodeId subjectId, QNode *nod
 {
     Q_D(QPropertyNodeAddedChange);
     d->m_addedNodeIdTypePair = QNodeIdTypePair(node->id(), QNodePrivate::findStaticMetaObject(node->metaObject()));
+
+    // Ensure the node has issued a node creation change. We can end
+    // up here if a newly created node with a parent is immediately set
+    // as a property on another node. In this case the deferred call to
+    // _q_postConstructorInit() will not have happened yet as the event
+    // loop will still be blocked. So force it here and we catch this
+    // eventuality in the _q_postConstructorInit() function so that we
+    // do not repeat the creation and new child scene change events.
+    if (node)
+        QNodePrivate::get(node)->_q_postConstructorInit();
 }
 
 /*! \internal */
diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 09bd597e6..738ce6144 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -74,6 +74,7 @@ QNodePrivate::QNodePrivate()
     , m_blockNotifications(false)
     , m_hasBackendNode(false)
     , m_enabled(true)
+    , m_notifiedParent(false)
     , m_defaultPropertyTrackMode(QNode::TrackFinalValues)
     , m_propertyChangesSetup(false)
     , m_signals(this)
@@ -210,13 +211,19 @@ void QNodePrivate::_q_addChild(QNode *childNode)
     Q_ASSERT(childNode);
     Q_ASSERT_X(childNode->parent() == q_func(), Q_FUNC_INFO,  "not a child of this node");
 
+    // Have we already notified the parent about its new child? If so, bail out
+    // early so that we do not send more than one new child event to the backend
+    QNodePrivate *childD = QNodePrivate::get(childNode);
+    if (childD->m_notifiedParent == true)
+        return;
+
     // Store our id as the parentId in the child so that even if the child gets
     // removed from the scene as part of the destruction of the parent, when the
     // parent's children are deleted in the QObject dtor, we still have access to
     // the parentId. If we didn't store this, we wouldn't have access at that time
     // because the parent would then only be a QObject, the QNode part would have
     // been destroyed already.
-    QNodePrivate::get(childNode)->m_parentId = m_id;
+    childD->m_parentId = m_id;
 
     if (!m_scene)
         return;
@@ -224,6 +231,11 @@ void QNodePrivate::_q_addChild(QNode *childNode)
     // We need to send a QPropertyNodeAddedChange to the backend
     // to notify the backend that we have a new child
     if (m_changeArbiter != nullptr) {
+        // Flag that we have notified the parent. We do this immediately before
+        // creating the change because that recurses back into this function and
+        // we need to catch that to avoid sending more than one new child event
+        // to the backend.
+        childD->m_notifiedParent = true;
         const auto change = QPropertyNodeAddedChangePtr::create(m_id, childNode);
         change->setPropertyName("children");
         notifyObservers(change);
@@ -299,6 +311,9 @@ void QNodePrivate::_q_setParentHelper(QNode *parent)
             notifyDestructionChangesAndRemoveFromScene();
     }
 
+    // Flag that we need to notify any new parent
+    m_notifiedParent = false;
+
     // Basically QObject::setParent but for QObjectPrivate
     QObjectPrivate::setParent_helper(parent);
     QNode *newParentNode = q->parentNode();
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index ad9d2376e..87a0226f1 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -100,6 +100,7 @@ public:
     bool m_blockNotifications;
     bool m_hasBackendNode;
     bool m_enabled;
+    bool m_notifiedParent;
     QNode::PropertyTrackingMode m_defaultPropertyTrackMode;
     QHash<QString, QNode::PropertyTrackingMode> m_trackedPropertiesOverrides;
 
@@ -137,10 +138,11 @@ public:
 
     static const QMetaObject *findStaticMetaObject(const QMetaObject *metaObject);
 
+    void _q_postConstructorInit();
+
 private:
     void notifyCreationChange();
     void notifyDestructionChangesAndRemoveFromScene();
-    void _q_postConstructorInit();
     void _q_addChild(QNode *childNode);
     void _q_removeChild(QNode *childNode);
     void _q_setParentHelper(QNode *parent);
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