QObservableInterface refactored

Explicit registerArbiter/unregisterArbiter methods.

QNode: removed QReadWriteLock for the QChangeArbiter, no need to protect as
the QChangeArbiter is always set in the main thread.
QBackendNode: similar for the QBackend, all locks removed as we are always
locked when synching changes.
QObservable was removed.

Change-Id: I570afbf3e3230ac9d9613474fedd7849aba7412a
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

10 files changed, 53 insertions, 232 deletions

diff --git a/src/core/core.pri b/src/core/core.pri
index bf52180a6..36d4c8b96 100644
--- a/src/core/core.pri
+++ b/src/core/core.pri
@@ -34,7 +34,6 @@ HEADERS += \
     $$PWD/qsceneinterface.h \
     $$PWD/qbackendscenepropertychange.h \
     $$PWD/qbackendscenepropertychange_p.h \
-    $$PWD/qobservable_p.h \
     $$PWD/qobservableinterface_p.h \
     $$PWD/qobserverinterface_p.h \
     $$PWD/qchangearbiter_p.h
@@ -47,7 +46,6 @@ SOURCES += \
     $$PWD/qchangearbiter.cpp \
     $$PWD/corelogging.cpp \
     $$PWD/qobservableinterface.cpp \
-    $$PWD/qobservable.cpp \
     $$PWD/qobserverinterface.cpp \
     $$PWD/qscenechange.cpp \
     $$PWD/qscenepropertychange.cpp \
diff --git a/src/core/nodes/qbackendnode.cpp b/src/core/nodes/qbackendnode.cpp
index ecf7ea631..5fb27129b 100644
--- a/src/core/nodes/qbackendnode.cpp
+++ b/src/core/nodes/qbackendnode.cpp
@@ -50,31 +50,26 @@ QT_BEGIN_NAMESPACE
 namespace Qt3D {
 
 QBackendNodePrivate::QBackendNodePrivate(QBackendNode *qq, QBackendNode::Mode mode)
-    : QObservable()
-    , q_ptr(qq)
+    : q_ptr(qq)
     , m_mode(mode)
     , m_arbiter(Q_NULLPTR)
 {
 }
 
-void QBackendNodePrivate::registerObserver(QObserverInterface *observer)
+// Called by backend thread (renderer or other) while we are locked to sync changes
+void QBackendNodePrivate::setArbiter(QChangeArbiter *arbiter)
 {
     Q_ASSERT(m_mode == QBackendNode::ReadWrite);
-    m_arbiter = dynamic_cast<QChangeArbiter *>(observer);
-}
-
-void QBackendNodePrivate::unregisterObserver(QObserverInterface *observer)
-{
-    Q_ASSERT(m_mode == QBackendNode::ReadWrite);
-    if (dynamic_cast<QChangeArbiter *>(observer) == m_arbiter)
-        m_arbiter = Q_NULLPTR;
+    m_arbiter = arbiter;
 }
 
+// Called by backend thread/worker threads. We don't need locking
+// as setting/unsetting the arbiter cannot happen at that time
 void QBackendNodePrivate::notifyObservers(const QSceneChangePtr &e)
 {
     Q_ASSERT(m_mode == QBackendNode::ReadWrite);
     if (m_arbiter != Q_NULLPTR)
-        m_arbiter->sceneChangeEventWithLock(e);
+        m_arbiter->sceneChangeEvent(e);
 }
 
 void QBackendNodePrivate::sceneChangeEvent(const QSceneChangePtr &e)
diff --git a/src/core/nodes/qbackendnode_p.h b/src/core/nodes/qbackendnode_p.h
index 995c2c68c..a83d78081 100644
--- a/src/core/nodes/qbackendnode_p.h
+++ b/src/core/nodes/qbackendnode_p.h
@@ -43,25 +43,23 @@
 #define QT3D_QBACKENDNODE_P_H
 
 #include <QUuid>
-#include <Qt3DCore/private/qobservable_p.h>
+#include <Qt3DCore/private/qobservableinterface_p.h>
 #include <Qt3DCore/private/qobserverinterface_p.h>
 #include <Qt3DCore/qbackendnode.h>
+#include <Qt3DCore/private/qchangearbiter_p.h>
 
 QT_BEGIN_NAMESPACE
 
 namespace Qt3D {
 
-class QChangeArbiter;
-
 class QBackendNodePrivate
         : public QObserverInterface
-        , public QObservable
+        , public QObservableInterface
 {
 public:
     QBackendNodePrivate(QBackendNode *qq, QBackendNode::Mode mode);
 
-    void registerObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
-    void unregisterObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
+    void setArbiter(QChangeArbiter *arbiter) Q_DECL_OVERRIDE;
     void notifyObservers(const QSceneChangePtr &e) Q_DECL_OVERRIDE;
     void sceneChangeEvent(const QSceneChangePtr &e) Q_DECL_OVERRIDE;
 
diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 56b2c259b..670901a50 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -69,7 +69,7 @@ QNodePrivate::QNodePrivate(QNode *qq)
     q_ptr = qq;
 }
 
-// Called by QEvent::childAdded
+// Called by QEvent::childAdded (main thread)
 void QNodePrivate::addChild(QNode *childNode)
 {
 
@@ -103,7 +103,7 @@ void QNodePrivate::addChild(QNode *childNode)
     }
 }
 
-// Called by QEvent::childRemoved
+// Called by QEvent::childRemoved (main thread)
 void QNodePrivate::removeChild(QNode *childNode)
 {
     Q_ASSERT(childNode);
@@ -197,6 +197,27 @@ void QNodePrivate::registerNotifiedProperties()
     }
 }
 
+void QNodePrivate::unregisterNotifiedProperties()
+{
+    Q_Q(QNode);
+    if (!m_notifiedProperties.isEmpty())
+        return;
+
+    const int offset = QNode::staticMetaObject.propertyOffset();
+    const int count = q->metaObject()->propertyCount();
+
+    for (int index = offset; index < count; index++) {
+        const QMetaProperty property = q->metaObject()->property(index);
+        if (!property.hasNotifySignal())
+            continue;
+        const QByteArray signal = QByteArray::number(QSIGNAL_CODE)
+                + property.notifySignal().methodSignature();
+        QObject::disconnect(q, signal,
+                            q, SLOT(_q_onNodePropertyChanged()));
+    }
+    m_notifiedProperties.clear();
+}
+
 void QNodePrivate::_q_onNodePropertyChanged()
 {
     // Bail out early if we can to avoid the cost below
@@ -219,30 +240,14 @@ void QNodePrivate::_q_onNodePropertyChanged()
     }
 }
 
-// Called in the QAspectThread context
-void QNodePrivate::registerObserver(QObserverInterface *observer)
+// Called in the main thread by QScene -> following QEvent::childAdded / addChild
+void QNodePrivate::setArbiter(QChangeArbiter *arbiter)
 {
-    Q_CHECK_PTR(observer);
-
-    // For now we only care about the QChangeArbiter observing us
-    QChangeArbiter *changeArbiter = dynamic_cast<QChangeArbiter *>(observer);
-    if (changeArbiter) {
-        QWriteLocker locker(&m_observerLock);
-        m_changeArbiter = changeArbiter;
+    if (m_changeArbiter && m_changeArbiter != arbiter)
+        unregisterNotifiedProperties();
+    m_changeArbiter = arbiter;
+    if (m_changeArbiter)
         registerNotifiedProperties();
-    }
-}
-
-void QNodePrivate::unregisterObserver(QObserverInterface *observer)
-{
-    Q_CHECK_PTR(observer);
-
-    // For now we only care about the QChangeArbiter observing us
-    QChangeArbiter *changeArbiter = dynamic_cast<QChangeArbiter *>(observer);
-    if (changeArbiter == m_changeArbiter) {
-        QWriteLocker locker(&m_observerLock);
-        m_changeArbiter = Q_NULLPTR;
-    }
 }
 
 void QNode::sceneChangeEvent(const QSceneChangePtr &)
@@ -282,16 +287,8 @@ void QNodePrivate::notifyObservers(const QSceneChangePtr &change)
     if (m_blockNotifications && change->type() == NodeUpdated)
         return;
 
-    QReadLocker locker(&m_observerLock);
-    QChangeArbiter *changeArbiter = m_changeArbiter;
-    locker.unlock();
-    // There is a deadlock issue as sceneChangeEventWithLock locks the QChangeArbiter's mutex
-    // while d->m_observerLock is locked by the locker right above.
-    // In the case that a call the QChangeArbiter registerObserver which locks the QChangeArviter's mutex
-    // and calls registerObserver on the same Node with locks d->m_observerLock
-
-    if (changeArbiter != Q_NULLPTR)
-        changeArbiter->sceneChangeEventWithLock(change);
+    if (m_changeArbiter != Q_NULLPTR)
+        m_changeArbiter->sceneChangeEventWithLock(change);
 }
 
 // Inserts this tree into the main Scene tree.
diff --git a/src/core/nodes/qnode_p.h b/src/core/nodes/qnode_p.h
index 2324abe81..d44563728 100644
--- a/src/core/nodes/qnode_p.h
+++ b/src/core/nodes/qnode_p.h
@@ -43,7 +43,6 @@
 #define QT3D_QNODE_P_H
 
 #include <private/qobject_p.h>
-#include <QReadWriteLock>
 #include <Qt3DCore/qt3dcore_global.h>
 #include <Qt3DCore/qnode.h>
 #include <Qt3DCore/private/qobservableinterface_p.h>
@@ -67,8 +66,7 @@ public:
     void setScene(QSceneInterface *scene);
     QSceneInterface *scene() const;
 
-    void registerObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
-    void unregisterObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
+    void setArbiter(QChangeArbiter *arbiter) Q_DECL_OVERRIDE;
 
     void notifyPropertyChange(const QByteArray &name, const QVariant &value);
     virtual void notifyObservers(const QSceneChangePtr &change);
@@ -79,7 +77,6 @@ public:
 
     // For now this just protects access to the m_changeArbiter.
     // Later on we may decide to extend support for multiple observers.
-    QReadWriteLock m_observerLock;
     QChangeArbiter *m_changeArbiter;
     QSceneInterface *m_scene;
     mutable QUuid m_uuid;
@@ -93,6 +90,7 @@ private:
     void removeChild(QNode *childNode);
     void removeAllChildren();
     void registerNotifiedProperties();
+    void unregisterNotifiedProperties();
     void _q_onNodePropertyChanged();
 
     QHash<int, QByteArray> m_notifiedProperties;
diff --git a/src/core/qobservable.cpp b/src/core/qobservable.cpp
deleted file mode 100644
index 9a68f73a3..000000000
--- a/src/core/qobservable.cpp
+++ /dev/null
@@ -1,82 +0,0 @@
-/****************************************************************************
-**
-** Copyright (C) 2014 Klaralvdalens Datakonsult AB (KDAB).
-** Contact: http://www.qt-project.org/legal
-**
-** This file is part of the Qt3D module of the Qt Toolkit.
-**
-** $QT_BEGIN_LICENSE:LGPL$
-** Commercial License Usage
-** Licensees holding valid commercial Qt licenses may use this file in
-** accordance with the commercial license agreement provided with the
-** Software or, alternatively, in accordance with the terms contained in
-** a written agreement between you and Digia.  For licensing terms and
-** conditions see http://qt.digia.com/licensing.  For further information
-** use the contact form at http://qt.digia.com/contact-us.
-**
-** GNU Lesser General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU Lesser
-** General Public License version 2.1 as published by the Free Software
-** Foundation and appearing in the file LICENSE.LGPL included in the
-** packaging of this file.  Please review the following information to
-** ensure the GNU Lesser General Public License version 2.1 requirements
-** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
-**
-** In addition, as a special exception, Digia gives you certain additional
-** rights.  These rights are described in the Digia Qt LGPL Exception
-** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
-**
-** GNU General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU
-** General Public License version 3.0 as published by the Free Software
-** Foundation and appearing in the file LICENSE.GPL included in the
-** packaging of this file.  Please review the following information to
-** ensure the GNU General Public License version 3.0 requirements will be
-** met: http://www.gnu.org/copyleft/gpl.html.
-**
-**
-** $QT_END_LICENSE$
-**
-****************************************************************************/
-
-#include "qobservable_p.h"
-#include "qobserverinterface_p.h"
-
-QT_BEGIN_NAMESPACE
-
-namespace Qt3D {
-
-QObservable::QObservable()
-    : m_lock(QReadWriteLock::NonRecursive)
-{
-}
-
-void QObservable::registerObserver(QObserverInterface *observer)
-{
-    QWriteLocker locker(&m_lock);
-    if (!m_observers.contains(observer))
-        m_observers.append(observer);
-}
-
-void QObservable::unregisterObserver(QObserverInterface *observer)
-{
-    QWriteLocker locker(&m_lock);
-    m_observers.removeOne(observer);
-}
-
-// This calls sceneChangeEvent on the QChangeArbiter
-void QObservable::notifyObservers(const QSceneChangePtr &e)
-{
-    QReadLocker locker(&m_lock);
-    Q_FOREACH (QObserverInterface *observer, m_observers)
-        observer->sceneChangeEvent(e);
-}
-
-const QList<QObserverInterface *> &QObservable::observers() const
-{
-    return m_observers;
-}
-
-} // namespace Qt3D
-
-QT_END_NAMESPACE
diff --git a/src/core/qobservable_p.h b/src/core/qobservable_p.h
deleted file mode 100644
index 6ad431962..000000000
--- a/src/core/qobservable_p.h
+++ /dev/null
@@ -1,74 +0,0 @@
-/****************************************************************************
-**
-** Copyright (C) 2014 Klaralvdalens Datakonsult AB (KDAB).
-** Contact: http://www.qt-project.org/legal
-**
-** This file is part of the Qt3D module of the Qt Toolkit.
-**
-** $QT_BEGIN_LICENSE:LGPL$
-** Commercial License Usage
-** Licensees holding valid commercial Qt licenses may use this file in
-** accordance with the commercial license agreement provided with the
-** Software or, alternatively, in accordance with the terms contained in
-** a written agreement between you and Digia.  For licensing terms and
-** conditions see http://qt.digia.com/licensing.  For further information
-** use the contact form at http://qt.digia.com/contact-us.
-**
-** GNU Lesser General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU Lesser
-** General Public License version 2.1 as published by the Free Software
-** Foundation and appearing in the file LICENSE.LGPL included in the
-** packaging of this file.  Please review the following information to
-** ensure the GNU Lesser General Public License version 2.1 requirements
-** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
-**
-** In addition, as a special exception, Digia gives you certain additional
-** rights.  These rights are described in the Digia Qt LGPL Exception
-** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
-**
-** GNU General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU
-** General Public License version 3.0 as published by the Free Software
-** Foundation and appearing in the file LICENSE.GPL included in the
-** packaging of this file.  Please review the following information to
-** ensure the GNU General Public License version 3.0 requirements will be
-** met: http://www.gnu.org/copyleft/gpl.html.
-**
-**
-** $QT_END_LICENSE$
-**
-****************************************************************************/
-
-#ifndef QT3D_QOBSERVABLE_P_H
-#define QT3D_QOBSERVABLE_P_H
-
-#include <Qt3DCore/private/qobservableinterface_p.h>
-#include <QReadWriteLock>
-
-QT_BEGIN_NAMESPACE
-
-namespace Qt3D {
-
-class QObservable : public QObservableInterface
-{
-public:
-    QObservable();
-
-    // In most cases, only the QChangeArbiter should be able to call these
-    // Might be worth making them private and having a friend class
-    void registerObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
-    void unregisterObserver(QObserverInterface *observer) Q_DECL_OVERRIDE;
-
-protected:
-    void notifyObservers(const QSceneChangePtr &e) Q_DECL_OVERRIDE;
-    const QList<QObserverInterface *> &observers() const;
-
-    QList<QObserverInterface *> m_observers;
-    QReadWriteLock m_lock;
-};
-
-} // namespace Qt3D
-
-QT_END_NAMESPACE
-
-#endif // QT3D_QOBSERVABLE_P_H
diff --git a/src/core/qobservableinterface_p.h b/src/core/qobservableinterface_p.h
index f35764c0f..4f11d2875 100644
--- a/src/core/qobservableinterface_p.h
+++ b/src/core/qobservableinterface_p.h
@@ -49,14 +49,14 @@ QT_BEGIN_NAMESPACE
 namespace Qt3D {
 
 class QObserverInterface;
+class QChangeArbiter;
 
 class QT3DCORESHARED_EXPORT QObservableInterface
 {
 public:
     virtual ~QObservableInterface();
 
-    virtual void registerObserver(QObserverInterface *observer) = 0;
-    virtual void unregisterObserver(QObserverInterface *observer) = 0;
+    virtual void setArbiter(QChangeArbiter *arbiter) = 0;
 
 protected:
     virtual void notifyObservers(const QSceneChangePtr &e) = 0;
diff --git a/src/core/qscene.cpp b/src/core/qscene.cpp
index 13268b962..7b4c92d30 100644
--- a/src/core/qscene.cpp
+++ b/src/core/qscene.cpp
@@ -85,7 +85,7 @@ void QScene::addObservable(QObservableInterface *observable, const QUuid &uuid)
     d->m_observablesLookupTable.insert(uuid, observable);
     d->m_observableToUuid.insert(observable, uuid);
     if (d->m_arbiter != Q_NULLPTR)
-        observable->registerObserver(d->m_arbiter);
+        observable->setArbiter(d->m_arbiter);
 }
 
 // Called by main thread only
@@ -96,7 +96,7 @@ void QScene::addObservable(QNode *observable)
         QWriteLocker lock(&d->m_lock);
         d->m_nodeLookupTable.insert(observable->uuid(), observable);
         if (d->m_arbiter != Q_NULLPTR)
-            observable->d_func()->registerObserver(d->m_arbiter);
+            observable->d_func()->setArbiter(d->m_arbiter);
     }
 }
 
@@ -107,8 +107,7 @@ void QScene::removeObservable(QObservableInterface *observable, const QUuid &uui
     QWriteLocker lock(&d->m_lock);
     d->m_observablesLookupTable.remove(uuid, observable);
     d->m_observableToUuid.remove(observable);
-    if (d->m_arbiter != Q_NULLPTR)
-        observable->unregisterObserver(d->m_arbiter);
+    observable->setArbiter(Q_NULLPTR);
 }
 
 // Called by main thread
@@ -120,14 +119,12 @@ void QScene::removeObservable(QNode *observable)
         QUuid nodeUuid = observable->uuid();
         QObservableList observables = d->m_observablesLookupTable.values(nodeUuid);
         Q_FOREACH (QObservableInterface *o, observables) {
-            if (d->m_arbiter != Q_NULLPTR)
-                o->unregisterObserver(d->m_arbiter);
+            o->setArbiter(Q_NULLPTR);
             d->m_observableToUuid.remove(o);
         }
         d->m_observablesLookupTable.remove(nodeUuid);
         d->m_nodeLookupTable.remove(nodeUuid);
-        if (d->m_arbiter != Q_NULLPTR)
-            observable->d_func()->unregisterObserver(d->m_arbiter);
+        observable->d_func()->setArbiter(Q_NULLPTR);
     }
 }
 
diff --git a/tests/auto/core/qscene/tst_qscene.cpp b/tests/auto/core/qscene/tst_qscene.cpp
index 40480d3f9..b8c11bcb9 100644
--- a/tests/auto/core/qscene/tst_qscene.cpp
+++ b/tests/auto/core/qscene/tst_qscene.cpp
@@ -70,15 +70,9 @@ private slots:
 class tst_Observable : public Qt3D::QObservableInterface
 {
 public:
-    void registerObserver(Qt3D::QObserverInterface *observer)
+    void setArbiter(Qt3D::QChangeArbiter *observer)
     {
-        m_arbiter = dynamic_cast<Qt3D::QChangeArbiter *>(observer);
-        QVERIFY(m_arbiter != Q_NULLPTR);
-    }
-
-    void unregisterObserver(Qt3D::QObserverInterface *observer)
-    {
-        QVERIFY(m_arbiter == observer);
+        m_arbiter = observer;
     }
 
 protected: