QNode: setParent create creation change only when needed

Two cases need to be handled by setParent:
1) Parent already has a backend node and receives a new child
   -> in which case we need to send the creation event to the backend
2) Parent was created in the frontend, but has no backend (delayed
   notification sending because a ctor can't call a virtual)
   -> in that case, when adding a child and setting its parent we shouldn't be
   sending the creation change. We rather let that be handled when the
   creation change for the parent is requested

Change-Id: I434c7d4e6af785c0314ac6538dc689992d90ed0c
Task-number: QTBUG-60612
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>
Reviewed-by: Oleg Evseev <ev.mipt@gmail.com>

4 files changed, 84 insertions, 4 deletions

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 42c8887ce..ce5e76a55 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -114,7 +114,7 @@ void QNodePrivate::notifyCreationChange()
     Q_Q(QNode);
     // Do nothing if we already have already sent a node creation change
     // and not a subsequent node destroyed change.
-    if (m_hasBackendNode)
+    if (m_hasBackendNode || !m_scene)
         return;
     QNodeCreatedChangeGenerator generator(q);
     const auto creationChanges = generator.creationChanges();
@@ -305,7 +305,21 @@ void QNodePrivate::_q_setParentHelper(QNode *parent)
                 visitor.traverse(q, newParentNode->d_func(), &QNodePrivate::setSceneHelper);
             }
 
-            notifyCreationChange();
+            // We want to make sure that subTreeRoot is always created before
+            // child.
+            // Given a case such as below
+            // QEntity *subTreeRoot = new QEntity(someGlobalExisitingRoot)
+            // QEntity *child = new QEntity();
+            // child->setParent(subTreeRoot)
+            // We need to take into account that subTreeRoot needs to be
+            // created in the backend before the child.
+            // Therefore we only call notifyCreationChanges if the parent
+            // hasn't been created yet as we know that when the parent will be
+            // fully created, it will also send the changes for all of its
+            // children
+
+            if (QNodePrivate::get(newParentNode)->m_hasBackendNode)
+                notifyCreationChange();
         }
 
         // If we have a valid new parent, we let him know that we are its child
diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp
index 89d396931..25c2a6dba 100644
--- a/tests/auto/core/nodes/tst_nodes.cpp
+++ b/tests/auto/core/nodes/tst_nodes.cpp
@@ -79,6 +79,8 @@ private slots:
     void appendingChildEntitiesToNode();
     void removingChildEntitiesFromNode();
 
+    void checkConstructionSetParentMix(); // QTBUG-60612
+
     void appendingComponentToEntity();
     void appendingParentlessComponentToEntity();
     void removingComponentFromEntity();
@@ -204,6 +206,11 @@ public:
         Qt3DCore::QNodePrivate::get(this)->setArbiter(arbiter);
     }
 
+    void setSimulateBackendCreated(bool created)
+    {
+        Qt3DCore::QNodePrivate::get(this)->m_hasBackendNode = created;
+    }
+
 signals:
     void customPropertyChanged();
 
@@ -232,6 +239,11 @@ public:
         Qt3DCore::QNodePrivate::get(this)->setScene(scene);
         Qt3DCore::QNodePrivate::get(this)->setArbiter(arbiter);
     }
+
+    void setSimulateBackendCreated(bool created)
+    {
+        Qt3DCore::QNodePrivate::get(this)->m_hasBackendNode = created;
+    }
 };
 
 class MyQComponent : public Qt3DCore::QComponent
@@ -389,6 +401,8 @@ void tst_Nodes::appendSingleChildNodeToNodeSceneExplicitParenting()
     QScopedPointer<MyQNode> node(new MyQNode());
     // WHEN
     node->setArbiterAndScene(&spy, &scene);
+    node->setSimulateBackendCreated(true);
+
     // THEN
     QVERIFY(Qt3DCore::QNodePrivate::get(node.data())->scene() != nullptr);
 
@@ -405,7 +419,7 @@ void tst_Nodes::appendSingleChildNodeToNodeSceneExplicitParenting()
     // THEN
     QVERIFY(child->parent() == node.data());
     QVERIFY(child->parentNode() == node.data());
-    QCOMPARE(spy.events.size(), 2);
+    QCOMPARE(spy.events.size(), 2); // Created + Child Added
     QCOMPARE(node->children().count(), 1);
     QVERIFY(Qt3DCore::QNodePrivate::get(child.data())->scene() != nullptr);
 
@@ -482,6 +496,7 @@ void tst_Nodes::appendMultipleChildNodesToNodeScene()
 
     // WHEN
     node->setArbiterAndScene(&spy, &scene);
+    node->setSimulateBackendCreated(true);
     // THEN
     QVERIFY(Qt3DCore::QNodePrivate::get(node.data())->scene() != nullptr);
 
@@ -664,6 +679,7 @@ void tst_Nodes::removingSingleChildNodeFromNode()
 
     // WHEN
     root->setArbiterAndScene(&spy, &scene);
+    root->setSimulateBackendCreated(true);
     child->setParent(root.data());
 
     // Clear any creation event
@@ -790,6 +806,47 @@ void tst_Nodes::removingChildEntitiesFromNode()
     QVERIFY(childEntity->parent() == nullptr);
 }
 
+void tst_Nodes::checkConstructionSetParentMix()
+{
+    // 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
+    Qt3DCore::QEntity *subTreeRoot = new Qt3DCore::QEntity(root.data());
+
+    for (int i = 0; i < 100; ++i) {
+        Qt3DCore::QEntity *child = new Qt3DCore::QEntity();
+        child->setParent(subTreeRoot);
+    }
+
+    // THEN
+    QCoreApplication::processEvents();
+    QCOMPARE(root->children().count(), 1);
+    QCOMPARE(subTreeRoot->children().count(), 100);
+    QCOMPARE(spy.events.size(), 102); // 1 subTreeRoot creation change, 100 child creation, 1 child added (subTree to root)
+
+    // Ensure first event is subTreeRoot
+    const Qt3DCore::QNodeCreatedChangeBasePtr firstEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>();
+    QVERIFY(!firstEvent.isNull());
+    QCOMPARE(firstEvent->subjectId(), subTreeRoot->id());
+    QCOMPARE(firstEvent->parentId(), root->id());
+
+    const Qt3DCore::QPropertyNodeAddedChangePtr lastEvent = spy.events.takeLast().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>();
+    QVERIFY(!lastEvent.isNull());
+    QCOMPARE(lastEvent->subjectId(), root->id());
+    QCOMPARE(lastEvent->propertyName(), "children");
+    QCOMPARE(lastEvent->addedNodeId(), subTreeRoot->id());
+}
+
 void tst_Nodes::appendingParentlessComponentToEntity()
 {
     // GIVEN
@@ -798,6 +855,8 @@ void tst_Nodes::appendingParentlessComponentToEntity()
     {
         QScopedPointer<MyQEntity> entity(new MyQEntity());
         entity->setArbiterAndScene(&entitySpy);
+        entity->setSimulateBackendCreated(true);
+
         MyQComponent *comp = new MyQComponent();
         comp->setArbiter(&componentSpy);
 
@@ -816,7 +875,7 @@ void tst_Nodes::appendingParentlessComponentToEntity()
         QVERIFY(comp->parentNode() == entity.data());
         QCOMPARE(entitySpy.events.size(), 1);
         QVERIFY(entitySpy.events.first().wasLocked());
-        QCOMPARE(componentSpy.events.size(), 2);        // first event is parent being set
+        QCOMPARE(componentSpy.events.size(), 1);
 
         // Note: since QEntity has no scene in this test case, we only have the
         // ComponentAdded event In theory we should also get the NodeCreated event
diff --git a/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp b/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
index 061de06fa..faccb6d34 100644
--- a/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
+++ b/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
@@ -442,6 +442,7 @@ void tst_QChangeArbiter::registerSceneObserver()
     tst_Node *root = new tst_Node();
     Qt3DCore::QNode *child = new tst_Node();
     Qt3DCore::QNodePrivate::get(root)->setScene(scene.data());
+    Qt3DCore::QNodePrivate::get(root)->m_hasBackendNode = true;
     scene->addObservable(root);
 
     QList<tst_SimpleObserver *> observers;
@@ -573,6 +574,7 @@ void tst_QChangeArbiter::unregisterSceneObservers()
     tst_Node *root = new tst_Node();
     Qt3DCore::QNode *child = new tst_Node();
     Qt3DCore::QNodePrivate::get(root)->setScene(scene.data());
+    Qt3DCore::QNodePrivate::get(root)->m_hasBackendNode = true;
     scene->addObservable(root);
 
     QList<tst_SimpleObserver *> observers;
@@ -796,6 +798,7 @@ void tst_QChangeArbiter::distributePropertyChanges()
     // WHEN
     PropertyTestNode *root = new PropertyTestNode();
     Qt3DCore::QNodePrivate::get(root)->setScene(scene.data());
+    Qt3DCore::QNodePrivate::get(root)->m_hasBackendNode = true;
     scene->addObservable(root);
 
     tst_SimpleObserver *rootObserver = new tst_SimpleObserver();
@@ -890,6 +893,7 @@ void tst_QChangeArbiter::distributeBackendChanges()
     // WHEN
     tst_Node *root = new tst_Node();
     Qt3DCore::QNodePrivate::get(root)->setScene(scene.data());
+    Qt3DCore::QNodePrivate::get(root)->m_hasBackendNode = true;
     scene->addObservable(root);
 
     tst_BackendObserverObservable *backenObserverObservable = new tst_BackendObserverObservable();
diff --git a/tests/auto/core/qscene/tst_qscene.cpp b/tests/auto/core/qscene/tst_qscene.cpp
index f4b04362d..78f17e8fa 100644
--- a/tests/auto/core/qscene/tst_qscene.cpp
+++ b/tests/auto/core/qscene/tst_qscene.cpp
@@ -297,6 +297,8 @@ void tst_QScene::deleteChildNode()
     Qt3DCore::QNode *root2 = new tst_Node();
     Qt3DCore::QNodePrivate::get(root1)->setScene(scene);
     Qt3DCore::QNodePrivate::get(root2)->setScene(scene);
+    Qt3DCore::QNodePrivate::get(root1)->m_hasBackendNode = true;
+    Qt3DCore::QNodePrivate::get(root2)->m_hasBackendNode = true;
 
     // WHEN
     scene->addObservable(root1);
@@ -358,6 +360,7 @@ void tst_QScene::removeChildNode()
 
     Qt3DCore::QNode *root = new tst_Node;
     Qt3DCore::QNodePrivate::get(root)->setScene(scene);
+    Qt3DCore::QNodePrivate::get(root)->m_hasBackendNode = true;
     scene->addObservable(root);
 
     // WHEN