diff options
author | Svenn-Arne Dragly <s@dragly.com> | 2019-02-21 15:54:02 +0100 |
---|---|---|
committer | Jani Heikkinen <jani.heikkinen@qt.io> | 2019-02-22 15:00:03 +0000 |
commit | e5b256effa320fb7806205f5a37a4dc1943175f0 (patch) | |
tree | 0a52d77a881588881bfd03f08414ac8c013c624c | |
parent | a48635f21fa68c5033ebe0d6832d405acfcca937 (diff) |
Fix broken creation order for nodes used as properties
Otherwise, child nodes may be constructed before their parents and their
node creation changes will arrive at the backend out-of-order.
This could result in a child node referencing a parent that we have not
yet received a creation change for.
Also add a unit test to take this case into account.
Change-Id: I26b29e63863d1686e7b9239c63297c7e6c341f4e
Task-number: QTBUG-73986
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
-rw-r--r-- | src/core/nodes/qnode.cpp | 12 | ||||
-rw-r--r-- | tests/auto/core/nodes/tst_nodes.cpp | 56 |
2 files changed, 62 insertions, 6 deletions
diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp index 83ac49471..8143ccc75 100644 --- a/src/core/nodes/qnode.cpp +++ b/src/core/nodes/qnode.cpp @@ -394,15 +394,15 @@ void QNodePrivate::propertyChanged(int propertyIndex) if (data.canConvert<QNode*>()) { QNode *node = data.value<QNode*>(); - // 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 + // Ensure the node and all ancestors have issued their node creation changes. + // 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. + // loop will still be blocked. We need to do this for all ancestors, + // since the subtree of this node otherwise can end up on the backend + // with a reference to a non-existent parent. if (node) - QNodePrivate::get(node)->_q_postConstructorInit(); + QNodePrivate::get(node)->_q_ensureBackendNodeCreated(); const QNodeId id = node ? node->id() : QNodeId(); return QVariant::fromValue(id); diff --git a/tests/auto/core/nodes/tst_nodes.cpp b/tests/auto/core/nodes/tst_nodes.cpp index 75d7a7799..accd20ceb 100644 --- a/tests/auto/core/nodes/tst_nodes.cpp +++ b/tests/auto/core/nodes/tst_nodes.cpp @@ -82,6 +82,7 @@ private slots: void checkConstructionSetParentMix(); // QTBUG-60612 void checkConstructionWithParent(); + void checkConstructionWithNonRootParent(); // QTBUG-73986 void checkConstructionAsListElement(); void checkSceneIsSetOnConstructionWithParent(); // QTBUG-69352 @@ -1119,6 +1120,61 @@ void tst_Nodes::checkConstructionWithParent() QCOMPARE(propertyEvent->value().value<Qt3DCore::QNodeId>(), node->id()); } +void tst_Nodes::checkConstructionWithNonRootParent() +{ + // GIVEN + ObserverSpy spy; + Qt3DCore::QScene scene; + QScopedPointer<MyQNode> root(new MyQNode()); + + // WHEN + root->setArbiterAndScene(&spy, &scene); + root->setSimulateBackendCreated(true); + QScopedPointer<MyQNode> parent(new MyQNode(root.data())); + + // THEN + QVERIFY(Qt3DCore::QNodePrivate::get(root.data())->scene() != nullptr); + QVERIFY(Qt3DCore::QNodePrivate::get(parent.data())->scene() != nullptr); + + // WHEN we create a child and then set it as a Node* property + auto *child = new MyQNode(parent.data()); + root->setNodeProperty(child); + + // THEN we should get + // - one creation change for parent, + // - one creation change for child, + // - one child added change for root->parent, + // - and one property change event, + // in that order. + QCoreApplication::processEvents(); + QCOMPARE(root->children().count(), 1); + QCOMPARE(parent->children().count(), 1); + + QCOMPARE(spy.events.size(), 4); // 2 creation changes, 1 child added changes, 1 property change + + // Ensure first event is parent node's creation change + const auto parentCreationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>(); + QVERIFY(!parentCreationEvent.isNull()); + QCOMPARE(parentCreationEvent->subjectId(), parent->id()); + + const auto childCreationEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QNodeCreatedChangeBase>(); + QVERIFY(!childCreationEvent.isNull()); + QCOMPARE(childCreationEvent->subjectId(), child->id()); + + const auto parentNewChildEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyNodeAddedChange>(); + QVERIFY(!parentNewChildEvent.isNull()); + QCOMPARE(parentNewChildEvent->subjectId(), root->id()); + QCOMPARE(parentNewChildEvent->propertyName(), "children"); + QCOMPARE(parentNewChildEvent->addedNodeId(), parent->id()); + + // Ensure second and last event is property set change + const auto propertyEvent = spy.events.takeFirst().change().dynamicCast<Qt3DCore::QPropertyUpdatedChange>(); + QVERIFY(!propertyEvent.isNull()); + QCOMPARE(propertyEvent->subjectId(), root->id()); + QCOMPARE(propertyEvent->propertyName(), "nodeProperty"); + QCOMPARE(propertyEvent->value().value<Qt3DCore::QNodeId>(), child->id()); +} + void tst_Nodes::checkConstructionAsListElement() { // GIVEN |