Use QProperty observation to push property updates to clients

If the property is BINDABLE but lacks a NOTIFY signal, the
client will have no way to register a callback for change
notifications. Document this behavior as such.
A future patch could synthesize signals for purely BINDABLE
properties on the client side, but this needs some more thought.

Change-Id: I5e723e294dc01890956fee179fb3ba30aecf8cc1
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

3 files changed, 138 insertions, 31 deletions

diff --git a/src/webchannel/qmetaobjectpublisher.cpp b/src/webchannel/qmetaobjectpublisher.cpp
index c97f755..b40aeab 100644
--- a/src/webchannel/qmetaobjectpublisher.cpp
+++ b/src/webchannel/qmetaobjectpublisher.cpp
@@ -183,6 +183,24 @@ const int PROPERTY_UPDATE_INTERVAL = 50;
 
 Q_DECLARE_TYPEINFO(OverloadResolutionCandidate, Q_MOVABLE_TYPE);
 
+void QWebChannelPropertyChangeNotifier::notify(
+        QPropertyObserver *self, QUntypedPropertyData *)
+{
+    auto This = static_cast<QWebChannelPropertyChangeNotifier*>(self);
+
+    // Use the indirection with Qt::AutoConnection to ensure invocation
+    // in the correct thread.
+    // Explicitly copy the parameters into the lambda so that this instance can be destroyed after posting a queued
+    // invocation. The current design doesn't allow this anyway, but I don't want bad surprises in a possible future
+    // commit.
+    QMetaObject::invokeMethod(
+                This->publisher,
+                [publisher=This->publisher, object=This->object, propertyIndex=This->propertyIndex]
+    {
+        publisher->propertyValueChanged(object, propertyIndex);
+    }, Qt::AutoConnection);
+}
+
 QMetaObjectPublisher::QMetaObjectPublisher(QWebChannel *webChannel)
     : QObject(webChannel)
     , webChannel(webChannel)
@@ -247,8 +265,8 @@ QJsonObject QMetaObjectPublisher::classInfoForObject(const QObject *object, QWeb
                 signalInfo.append(QString::fromLatin1(notifySignal));
             }
             signalInfo.append(prop.notifySignalIndex());
-        } else if (!prop.isConstant()) {
-            qWarning("Property '%s'' of object '%s' has no notify signal and is not constant, "
+        } else if (!prop.isConstant() && !prop.isBindable()) {
+            qWarning("Property '%s'' of object '%s' has no notify signal, is not bindable and is not constant, "
                      "value updates in HTML will be broken!",
                      prop.name(), object->metaObject()->className());
         }
@@ -308,8 +326,8 @@ void QMetaObjectPublisher::setClientIsIdle(bool isIdle)
     clientIsIdle = isIdle;
     if (!isIdle && timer.isActive()) {
         timer.stop();
-    } else if (isIdle && !timer.isActive()) {
-        timer.start(PROPERTY_UPDATE_INTERVAL, this);
+    } else {
+        startPropertyUpdateTimer();
     }
 }
 
@@ -330,8 +348,9 @@ QJsonObject QMetaObjectPublisher::initializeClient(QWebChannelAbstractTransport
     return objectInfos;
 }
 
-void QMetaObjectPublisher::initializePropertyUpdates(const QObject *const object, const QJsonObject &objectInfo)
+void QMetaObjectPublisher::initializePropertyUpdates(QObject *const object, const QJsonObject &objectInfo)
 {
+    auto *metaObject = object->metaObject();
     auto *signalHandler = signalHandlerFor(object);
     for (const auto propertyInfoVar : objectInfo[KEY_PROPERTIES].toArray()) {
         const QJsonArray &propertyInfo = propertyInfoVar.toArray();
@@ -341,22 +360,32 @@ void QMetaObjectPublisher::initializePropertyUpdates(const QObject *const object
         }
         const int propertyIndex = propertyInfo.at(0).toInt();
         const QJsonArray &signalData = propertyInfo.at(2).toArray();
+        const QMetaProperty metaProp = metaObject->property(propertyIndex);
 
-        if (signalData.isEmpty()) {
-            // Property without NOTIFY signal
-            continue;
-        }
+        if (!signalData.isEmpty()) {
+            // Property with a NOTIFY signal
+            const int signalIndex = signalData.at(1).toInt();
 
-        const int signalIndex = signalData.at(1).toInt();
+            QSet<int> &connectedProperties = signalToPropertyMap[object][signalIndex];
 
-        QSet<int> &connectedProperties = signalToPropertyMap[object][signalIndex];
+            // Only connect for a property update once
+            if (connectedProperties.isEmpty()) {
+                signalHandler->connectTo(object, signalIndex);
+            }
 
-        // Only connect for a property update once
-        if (connectedProperties.isEmpty()) {
-            signalHandler->connectTo(object, signalIndex);
+            connectedProperties.insert(propertyIndex);
+        } else if (metaProp.isBindable()) {
+            const auto [begin, end] = propertyObservers.equal_range(object);
+            const auto it = std::find_if(begin, end, [&](auto &n) {
+                return n.second.propertyIndex == propertyIndex;
+            });
+            // Only connect for a property update once
+            if (it == end) {
+                auto it = propertyObservers.emplace(
+                            object, QWebChannelPropertyChangeNotifier{this, object, propertyIndex});
+                metaProp.bindable(object).observe(&it->second);
+            }
         }
-
-        connectedProperties.insert(propertyIndex);
     }
 
     // also always connect to destroyed signal
@@ -383,16 +412,21 @@ void QMetaObjectPublisher::sendPendingPropertyUpdates()
         QJsonObject properties;
         // maps signal index to list of arguments of the last emit
         QJsonObject sigs;
-        const SignalToArgumentsMap::const_iterator sigEnd = it.value().constEnd();
-        for (SignalToArgumentsMap::const_iterator sigIt = it.value().constBegin(); sigIt != sigEnd; ++sigIt) {
-            // TODO: can we get rid of the int <-> string conversions here?
-            for (const int propertyIndex : objectsSignalToPropertyMap.value(sigIt.key())) {
-                const QMetaProperty &property = metaObject->property(propertyIndex);
-                Q_ASSERT(property.isValid());
-                properties[QString::number(propertyIndex)] = wrapResult(property.read(object), nullptr, objectId);
-            }
+
+        const auto indexes = it.value().propertyIndices(objectsSignalToPropertyMap);
+
+        // TODO: can we get rid of the int <-> string conversions here?
+        for (const int propertyIndex : indexes) {
+            const QMetaProperty &property = metaObject->property(propertyIndex);
+            Q_ASSERT(property.isValid());
+            properties[QString::number(propertyIndex)] = wrapResult(property.read(object), nullptr, objectId);
+        }
+
+        const auto sigMap = it.value().signalMap;
+        for (auto sigIt = sigMap.constBegin(); sigIt != sigMap.constEnd(); ++sigIt) {
             sigs[QString::number(sigIt.key())] = QJsonArray::fromVariantList(sigIt.value());
         }
+
         QJsonObject obj;
         obj[KEY_OBJECT] = objectId;
         obj[KEY_SIGNALS] = sigs;
@@ -572,10 +606,22 @@ void QMetaObjectPublisher::signalEmitted(const QObject *object, const int signal
             objectDestroyed(object);
         }
     } else {
-        pendingPropertyUpdates[object][signalIndex] = arguments;
-        if (clientIsIdle && !blockUpdates && !timer.isActive()) {
-            timer.start(PROPERTY_UPDATE_INTERVAL, this);
-        }
+        auto &propertyUpdate = pendingPropertyUpdates[object];
+        propertyUpdate.signalMap[signalIndex] = arguments;
+        startPropertyUpdateTimer();
+    }
+}
+
+void QMetaObjectPublisher::propertyValueChanged(const QObject *object, const int propertyIndex)
+{
+    pendingPropertyUpdates[object].plainProperties.insert(propertyIndex);
+    startPropertyUpdateTimer();
+}
+
+void QMetaObjectPublisher::startPropertyUpdateTimer()
+{
+    if (clientIsIdle && !blockUpdates && !timer.isActive()) {
+        timer.start(PROPERTY_UPDATE_INTERVAL, this);
     }
 }
 
@@ -595,6 +641,7 @@ void QMetaObjectPublisher::objectDestroyed(const QObject *object)
         signalToPropertyMap.remove(object);
     }
     pendingPropertyUpdates.remove(object);
+    propertyObservers.erase(object);
 }
 
 QObject *QMetaObjectPublisher::unwrapObject(const QString &objectId) const
diff --git a/src/webchannel/qmetaobjectpublisher_p.h b/src/webchannel/qmetaobjectpublisher_p.h
index e87e4f0..f54f569 100644
--- a/src/webchannel/qmetaobjectpublisher_p.h
+++ b/src/webchannel/qmetaobjectpublisher_p.h
@@ -86,8 +86,24 @@ enum MessageType {
     TYPES_LAST_VALUE = 10
 };
 
+class QMetaObjectPublisher;
 class QWebChannel;
 class QWebChannelAbstractTransport;
+
+struct QWebChannelPropertyChangeNotifier : QPropertyObserver
+{
+    QWebChannelPropertyChangeNotifier(QMetaObjectPublisher *publisher, const QObject *object, int propertyIndex)
+        : QPropertyObserver(&QWebChannelPropertyChangeNotifier::notify),
+          publisher(publisher), object(object), propertyIndex(propertyIndex)
+    {
+    }
+
+    QMetaObjectPublisher *publisher = nullptr;
+    const QObject *object = nullptr;
+    int propertyIndex = 0;
+    static void notify(QPropertyObserver *, QUntypedPropertyData *);
+};
+
 class Q_WEBCHANNEL_EXPORT QMetaObjectPublisher : public QObject
 {
     Q_OBJECT
@@ -136,7 +152,7 @@ public:
      * When receiving a notify signal, it will store the information in pendingPropertyUpdates which
      * gets send via a Qt.propertyUpdate message to the server when the grouping timer timeouts.
      */
-    void initializePropertyUpdates(const QObject *const object, const QJsonObject &objectInfo);
+    void initializePropertyUpdates(QObject *const object, const QJsonObject &objectInfo);
 
     /**
      * Send the clients the new property values since the last time this function was invoked.
@@ -186,6 +202,17 @@ public:
     void signalEmitted(const QObject *object, const int signalIndex, const QVariantList &arguments);
 
     /**
+     * Callback for bindable property value changes which forwards the change to the webchannel clients.
+     */
+    void propertyValueChanged(const QObject *object, const int propertyIndex);
+
+    /**
+     * Called after a property has been updated. Starts the update timer if
+     * the client is idle and updates are not blocked.
+     */
+    void startPropertyUpdateTimer();
+
+    /**
      * Callback for registered or wrapped objects which erases all data related to @p object.
      *
      * @sa signalEmitted
@@ -319,10 +346,32 @@ private:
     typedef QHash<int, QSet<int> > SignalToPropertyNameMap;
     QHash<const QObject *, SignalToPropertyNameMap> signalToPropertyMap;
 
+    // Keeps property observers alive for as long as we track an object
+    std::unordered_multimap<const QObject*, QWebChannelPropertyChangeNotifier> propertyObservers;
+
     // Objects that changed their properties and are waiting for idle client.
-    // map of object name to map of signal index to arguments
     typedef QHash<int, QVariantList> SignalToArgumentsMap;
-    typedef QHash<const QObject *, SignalToArgumentsMap> PendingPropertyUpdates;
+
+    // A set of plain property index (for bindable properties) and a map of
+    // signal index to arguments (for property updates from a notify signal).
+    // NOTIFY signals and their arguments are first collected and then mapped to
+    // the corresponding property in sendPendingPropertyUpdates()
+    struct PropertyUpdate
+    {
+    public:
+        SignalToArgumentsMap signalMap;
+        QSet<int> plainProperties;
+
+        /**
+         * Given a SignalToPropertyNameMap, returns the set of all property
+         * indices of properties that were changed in this PropertyUpdate.
+         */
+        QSet<int> propertyIndices(const SignalToPropertyNameMap &map) const;
+    };
+
+    // map of object to either a property index for plain bindable properties
+    // or a to map of signal index to arguments
+    typedef QHash<const QObject *, PropertyUpdate> PendingPropertyUpdates;
     PendingPropertyUpdates pendingPropertyUpdates;
 
     // Aggregate property updates since we get multiple Qt.idle message when we have multiple
@@ -331,6 +380,14 @@ private:
     QBasicTimer timer;
 };
 
+inline QSet<int> QMetaObjectPublisher::PropertyUpdate::propertyIndices(const SignalToPropertyNameMap &map) const {
+    auto indexes = plainProperties;
+    for (auto it = signalMap.cbegin(); it != signalMap.cend(); ++it) {
+        indexes += map.value(it.key());
+    }
+    return indexes;
+}
+
 QT_END_NAMESPACE
 
 #endif // QMETAOBJECTPUBLISHER_P_H
diff --git a/src/webchannel/qwebchannel.cpp b/src/webchannel/qwebchannel.cpp
index 050d04e..2d7cc2d 100644
--- a/src/webchannel/qwebchannel.cpp
+++ b/src/webchannel/qwebchannel.cpp
@@ -175,6 +175,9 @@ QHash<QString, QObject *> QWebChannel::registeredObjects() const
     The properties, signals and public methods of the \a object are published to the remote clients.
     There, an object with the identifier \a id is then constructed.
 
+    \note A property that is \c BINDABLE but does not have a \c NOTIFY signal will have working property
+          updates on the client side, but no mechanism to register a callback for the change notifications.
+
     \note A current limitation is that objects must be registered before any client is initialized.
 
     \sa QWebChannel::registerObjects(), QWebChannel::deregisterObject(), QWebChannel::registeredObjects()