QNode: Defer QScene registration until after object construcion

We used to call m_scene->addObservable() from the ctor of QNode.
This would lead to the QScene calling QNode::setArbiter(), which in
turn calls QNodePrivate::registerNotifiedProperties(). This method
relies on the meta-class of 'this' in order to connect the
property change listener to all property change signals. So, since
the QNode instance is not yet initialized properly, no connections
are made at all and no property update notifications will work.

This patch fixes this issue by deferring the call to after object
construction.

Change-Id: I45ef46c416f88a84ca55c9b9833312d0fd433d0e
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>

4 files changed, 138 insertions, 8 deletions

diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 8dbcb295a..e4bb5d4a3 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -97,10 +97,8 @@ void QNodePrivate::init(QNode *parent)
     m_scene = parentPrivate->m_scene;
     Q_Q(QNode);
     if (m_scene) {
-        m_scene->addObservable(q); // Sets the m_changeArbiter to that of the scene
-
-        // Scehdule the backend notification
-        QMetaObject::invokeMethod(q, "_q_notifyCreationAndChildChanges", Qt::QueuedConnection);
+        // schedule the backend notification and scene registering -> set observers through scene
+        QMetaObject::invokeMethod(q, "_q_postConstructorInit", Qt::QueuedConnection);
     }
 }
 
@@ -166,8 +164,10 @@ void QNodePrivate::notifyDestructionChangesAndRemoveFromScene()
  * parent backend node of its new child. This is called in a deferred manner
  * by the QNodePrivate::init() method to notify the backend of newly created
  * nodes with a parent that is already part of the scene.
+ *
+ * Also notify the scene of this node, so it may set it's change arbiter.
  */
-void QNodePrivate::_q_notifyCreationAndChildChanges()
+void QNodePrivate::_q_postConstructorInit()
 {
     Q_Q(QNode);
 
@@ -176,6 +176,9 @@ void QNodePrivate::_q_notifyCreationAndChildChanges()
     if (!parentNode)
         return;
 
+    if (m_scene)
+        m_scene->addObservable(q); // Sets the m_changeArbiter to that of the scene
+
     // Let the backend know we have been added to the scene
     notifyCreationChange();
 
@@ -187,7 +190,7 @@ void QNodePrivate::_q_notifyCreationAndChildChanges()
 /*!
  * \internal
  *
- * Called by _q_setParentHelper() or _q_notifyCreationAndChildChanges()
+ * Called by _q_setParentHelper() or _q_postConstructorInit()
  * on the main thread.
  */
 void QNodePrivate::_q_addChild(QNode *childNode)
diff --git a/src/core/nodes/qnode.h b/src/core/nodes/qnode.h
index 697fcc903..cadcc7d5c 100644
--- a/src/core/nodes/qnode.h
+++ b/src/core/nodes/qnode.h
@@ -105,7 +105,7 @@ private:
     // when dealing with QNode objects
     void setParent(QObject *) Q_DECL_EQ_DELETE;
 
-    Q_PRIVATE_SLOT(d_func(), void _q_notifyCreationAndChildChanges())
+    Q_PRIVATE_SLOT(d_func(), void _q_postConstructorInit())
     Q_PRIVATE_SLOT(d_func(), void _q_addChild(Qt3DCore::QNode *))
     Q_PRIVATE_SLOT(d_func(), void _q_removeChild(Qt3DCore::QNode *))
     Q_PRIVATE_SLOT(d_func(), void _q_setParentHelper(Qt3DCore::QNode *))
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index dec114084..a90011128 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -135,7 +135,7 @@ public:
 private:
     void notifyCreationChange();
     void notifyDestructionChangesAndRemoveFromScene();
-    void _q_notifyCreationAndChildChanges();
+    void _q_postConstructorInit();
     void _q_addChild(QNode *childNode);
     void _q_removeChild(QNode *childNode);
     void _q_setParentHelper(QNode *parent);
diff --git a/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp b/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
index 42566123d..d98cd5e91 100644
--- a/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
+++ b/tests/auto/core/qchangearbiter/tst_qchangearbiter.cpp
@@ -59,6 +59,7 @@ private slots:
     void unregisterObservers();
     void unregisterSceneObservers();
     void distributeFrontendChanges();
+    void distributePropertyChanges();
     void distributeBackendChanges();
 };
 
@@ -138,6 +139,44 @@ private:
     QList<Qt3DCore::QSceneChangePtr> m_lastChanges;
 };
 
+// used to test property change notifications
+class PropertyTestNode : public Qt3DCore::QNode
+{
+    Q_OBJECT
+    Q_PROPERTY(int prop1 READ prop1 WRITE setProp1 NOTIFY prop1Changed)
+    Q_PROPERTY(float prop2 READ prop2 WRITE setProp2 NOTIFY prop2Changed)
+
+public:
+    explicit PropertyTestNode(Qt3DCore::QNode *parent = 0) : Qt3DCore::QNode(parent)
+    {}
+
+    int prop1() const { return m_prop1; }
+    void setProp1(int v)
+    {
+        if (m_prop1 != v) {
+            m_prop1 = v;
+            Q_EMIT prop1Changed(v);
+        }
+    }
+
+    float prop2() const { return m_prop2; }
+    void setProp2(float v)
+    {
+        if (m_prop2 != v) {
+            m_prop2 = v;
+            Q_EMIT prop2Changed(v);
+        }
+    }
+
+Q_SIGNALS:
+    void prop1Changed(int v);
+    void prop2Changed(float v);
+
+private:
+    int m_prop1 = 0;
+    float m_prop2 = 0.0f;
+};
+
 class tst_SimpleObserver : public Qt3DCore::QObserverInterface
 {
 public:
@@ -713,6 +752,94 @@ void tst_QChangeArbiter::distributeFrontendChanges()
     Qt3DCore::QChangeArbiter::destroyThreadLocalChangeQueue(arbiter.data());
 }
 
+void tst_QChangeArbiter::distributePropertyChanges()
+{
+    // GIVEN
+    QScopedPointer<Qt3DCore::QChangeArbiter> arbiter(new Qt3DCore::QChangeArbiter());
+    QScopedPointer<Qt3DCore::QScene> scene(new Qt3DCore::QScene());
+    QScopedPointer<Qt3DCore::QAbstractPostman> postman(new tst_PostManObserver);
+    arbiter->setPostman(postman.data());
+    arbiter->setScene(scene.data());
+    postman->setScene(scene.data());
+    scene->setArbiter(arbiter.data());
+    // Replaces initialize as we have no JobManager in this case
+    Qt3DCore::QChangeArbiter::createThreadLocalChangeQueue(arbiter.data());
+
+    // Test change notifications made to the root node:
+
+    // WHEN
+    PropertyTestNode *root = new PropertyTestNode();
+    Qt3DCore::QNodePrivate::get(root)->setScene(scene.data());
+    scene->addObservable(root);
+
+    tst_SimpleObserver *rootObserver = new tst_SimpleObserver();
+    arbiter->registerObserver(rootObserver, root->id());
+    arbiter->syncChanges();
+
+    // THEN
+    QVERIFY(rootObserver->lastChange().isNull());
+
+    // WHEN
+    root->setProp1(root->prop1() + 1);
+    arbiter->syncChanges();
+
+    // THEN
+    QVERIFY(!rootObserver->lastChange().isNull());
+    QCOMPARE(rootObserver->lastChange()->type(), Qt3DCore::PropertyUpdated);
+    Qt3DCore::QPropertyUpdatedChangePtr propChange = qSharedPointerDynamicCast<Qt3DCore::QPropertyUpdatedChange>(rootObserver->lastChange());
+    QCOMPARE(root->id(), propChange->subjectId());
+    QCOMPARE(QString(propChange->propertyName()), QString("prop1"));
+
+    // WHEN
+    root->setProp2(root->prop2() + 1.f);
+    arbiter->syncChanges();
+
+    // THEN
+    QVERIFY(!rootObserver->lastChange().isNull());
+    QCOMPARE(rootObserver->lastChange()->type(), Qt3DCore::PropertyUpdated);
+    propChange = qSharedPointerDynamicCast<Qt3DCore::QPropertyUpdatedChange>(rootObserver->lastChange());
+    QCOMPARE(root->id(), propChange->subjectId());
+    QCOMPARE(QString(propChange->propertyName()), QString("prop2"));
+
+    // Test change notifications made to an entity that was added to the scene
+    // via QNode::setParent()
+
+    // WHEN
+    PropertyTestNode *setParentChild = new PropertyTestNode();
+    setParentChild->setParent(root);
+    tst_SimpleObserver *setParentChildObserver = new tst_SimpleObserver();
+    arbiter->registerObserver(setParentChildObserver, setParentChild->id());
+    setParentChild->setProp2(setParentChild->prop2() + 1.f);
+    arbiter->syncChanges();
+
+    // THEN
+    QVERIFY(!setParentChildObserver->lastChange().isNull());
+    QCOMPARE(setParentChildObserver->lastChange()->type(), Qt3DCore::PropertyUpdated);
+    propChange = qSharedPointerDynamicCast<Qt3DCore::QPropertyUpdatedChange>(setParentChildObserver->lastChange());
+    QCOMPARE(setParentChild->id(), propChange->subjectId());
+    QCOMPARE(QString(propChange->propertyName()), QString("prop2"));
+
+    // Test change notifications made to an entity that was added to the scene
+    // via the QNode() constructor parent parameter
+
+    // WHEN
+    PropertyTestNode *directChild = new PropertyTestNode(root);
+    QCoreApplication::processEvents();  // make sure the post-ctor initialization is executed for the node
+    tst_SimpleObserver *directChildObserver = new tst_SimpleObserver();
+    arbiter->registerObserver(directChildObserver, directChild->id());
+    directChild->setProp1(directChild->prop1() + 1);
+    arbiter->syncChanges();
+
+    // THEN
+    QVERIFY(!directChildObserver->lastChange().isNull());
+    QCOMPARE(directChildObserver->lastChange()->type(), Qt3DCore::PropertyUpdated);
+    propChange = qSharedPointerDynamicCast<Qt3DCore::QPropertyUpdatedChange>(directChildObserver->lastChange());
+    QCOMPARE(directChild->id(), propChange->subjectId());
+    QCOMPARE(QString(propChange->propertyName()), QString("prop1"));
+
+    Qt3DCore::QChangeArbiter::destroyThreadLocalChangeQueue(arbiter.data());
+}
+
 void tst_QChangeArbiter::distributeBackendChanges()
 {