Don't emit change notifications more often than required

When a property value changes, first update all dependent bindings to their new value. Only once that is done send out all the notifications and changed signals. This way, if a property depends on multiple other properties, which all get changed, there will only be one notification; and (potentially invalid) intermediate values will not be observed. Fixes: QTBUG-89844 Change-Id: I086077934aee6dc940705f08a87bf8448708881f Reviewed-by: Lars Knoll <[email protected]> Reviewed-by: Fabian Kosmale <[email protected]> Reviewed-by: Andreas Buhr <[email protected]> Reviewed-by: Andrei Golubev <[email protected]>
fwcd · Apr 16, 2021 · bb44c18 · bb44c18
1 parent 984bc7c
commit bb44c18
Show file tree

Hide file tree

Showing 3 changed files with 99 additions and 12 deletions.
diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
@@ -98,7 +98,7 @@ void QPropertyBindingPrivate::unlinkAndDeref()
         destroyAndFreeMemory(this);
 }
 
-void QPropertyBindingPrivate::evaluate()
+void QPropertyBindingPrivate::evaluateRecursive()
 {
     if (updating) {
         error = QPropertyBindingError(QPropertyBindingError::BindingLoop);
@@ -121,23 +121,35 @@ void QPropertyBindingPrivate::evaluate()
 
     BindingEvaluationState evaluationFrame(this);
 
-    bool changed;
     auto bindingFunctor =  reinterpret_cast<std::byte *>(this) +
         QPropertyBindingPrivate::getSizeEnsuringAlignment();
     if (hasBindingWrapper) {
-        changed = staticBindingWrapper(metaType, propertyDataPtr,
+        pendingNotify = staticBindingWrapper(metaType, propertyDataPtr,
                 {vtable, bindingFunctor});
     } else {
-        changed = vtable->call(metaType, propertyDataPtr, bindingFunctor);
+        pendingNotify = vtable->call(metaType, propertyDataPtr, bindingFunctor);
     }
-    if (!changed)
+    if (!pendingNotify || !firstObserver)
         return;
 
-    // notify dependent bindings and emit changed signal
-    if (firstObserver)
+    firstObserver.noSelfDependencies(this);
+    firstObserver.evaluateBindings();
+}
+
+void QPropertyBindingPrivate::notifyRecursive()
+{
+    if (!pendingNotify)
+        return;
+    pendingNotify = false;
+    Q_ASSERT(!updating);
+    updating = true;
+    if (firstObserver) {
+        firstObserver.noSelfDependencies(this);
         firstObserver.notify(propertyDataPtr);
+    }
     if (hasStaticObserver)
         staticObserverCallback(propertyDataPtr);
+    updating = false;
 }
 
 QUntypedPropertyBinding::QUntypedPropertyBinding() = default;
@@ -248,7 +260,8 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB
             newBindingRaw->prependObserver(observer);
         newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback);
 
-        newBindingRaw->evaluate();
+        newBindingRaw->evaluateRecursive();
+        newBindingRaw->notifyRecursive();
     } else if (observer) {
         d.setObservers(observer.ptr);
     } else {
@@ -343,8 +356,10 @@ void QPropertyBindingData::registerWithCurrentlyEvaluatingBinding_helper(Binding
 void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr) const
 {
     QPropertyBindingDataPointer d{this};
-    if (QPropertyObserverPointer observer = d.firstObserver())
+    if (QPropertyObserverPointer observer = d.firstObserver()) {
+        observer.evaluateBindings();
         observer.notify(propertyDataPtr);
+    }
 }
 
 int QPropertyBindingDataPointer::observerCount() const
@@ -518,9 +533,9 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
         }
         case QPropertyObserver::ObserverNotifiesBinding:
         {
-            auto bindingToMarkDirty =  observer->binding;
+            auto bindingToNotify =  observer->binding;
             QPropertyObserverNodeProtector protector(observer);
-            bindingToMarkDirty->evaluate();
+            bindingToNotify->notifyRecursive();
             next = protector.next();
             break;
         }
@@ -534,6 +549,39 @@ void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr)
     }
 }
 
+#ifndef QT_NO_DEBUG
+void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *binding)
+{
+    auto observer = const_cast<QPropertyObserver*>(ptr);
+    // See also comment in notify()
+    while (observer) {
+        if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding)
+            Q_ASSERT(observer->binding != binding);
+
+        observer = observer->next.data();
+    }
+
+}
+#endif
+
+void QPropertyObserverPointer::evaluateBindings()
+{
+    auto observer = const_cast<QPropertyObserver*>(ptr);
+    // See also comment in notify()
+    while (observer) {
+        QPropertyObserver *next = observer->next.data();
+
+        if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) {
+            auto bindingToEvaluate =  observer->binding;
+            QPropertyObserverNodeProtector protector(observer);
+            bindingToEvaluate->evaluateRecursive();
+            next = protector.next();
+        }
+
+        observer = next;
+    }
+}
+
 void QPropertyObserverPointer::observeProperty(QPropertyBindingDataPointer property)
 {
     if (ptr->prev)

diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
@@ -109,6 +109,12 @@ struct QPropertyObserverPointer
     void setAliasedProperty(QUntypedPropertyData *propertyPtr);
 
     void notify(QUntypedPropertyData *propertyDataPtr);
+#ifndef QT_NO_DEBUG
+    void noSelfDependencies(QPropertyBindingPrivate *binding);
+#else
+    void noSelfDependencies(QPropertyBindingPrivate *) {}
+#endif
+    void evaluateBindings();
     void observeProperty(QPropertyBindingDataPointer property);
 
     explicit operator bool() const { return ptr != nullptr; }
@@ -175,6 +181,7 @@ class Q_CORE_EXPORT QPropertyBindingPrivate : public QtPrivate::RefCounted
     // used to detect binding loops for lazy evaluated properties
     bool updating = false;
     bool hasStaticObserver = false;
+    bool pendingNotify = false;
     bool hasBindingWrapper:1;
     // used to detect binding loops for eagerly evaluated properties
     bool isQQmlPropertyBinding:1;
@@ -306,7 +313,8 @@ class Q_CORE_EXPORT QPropertyBindingPrivate : public QtPrivate::RefCounted
 
     void unlinkAndDeref();
 
-    void evaluate();
+    void evaluateRecursive();
+    void notifyRecursive();
 
     static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding)
     { return static_cast<QPropertyBindingPrivate *>(binding.d.data()); }

diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -95,6 +95,7 @@ private slots:
     void noFakeDependencies();
 
     void bindablePropertyWithInitialization();
+    void noDoubleNotification();
 };
 
 void tst_QProperty::functorBinding()
@@ -1606,6 +1607,36 @@ void tst_QProperty::bindablePropertyWithInitialization()
     QCOMPARE(tester.prop3().anotherValue, 20);
 }
 
+void tst_QProperty::noDoubleNotification()
+{
+    /* dependency graph for this test
+       x --> y means y depends on x
+      a-->b-->d
+      \       ^
+       \->c--/
+    */
+    QProperty<int> a(0);
+    QProperty<int> b;
+    b.setBinding([&](){ return a.value(); });
+    QProperty<int> c;
+    c.setBinding([&](){ return a.value(); });
+    QProperty<int> d;
+    d.setBinding([&](){ return b.value() + c.value(); });
+    int nNotifications = 0;
+    int expected = 0;
+    auto connection = d.subscribe([&](){
+        ++nNotifications;
+        QCOMPARE(d.value(), expected);
+    });
+    QCOMPARE(nNotifications, 1);
+    expected = 2;
+    a = 1;
+    QCOMPARE(nNotifications, 2);
+    expected = 4;
+    a = 2;
+    QCOMPARE(nNotifications, 3);
+}
+
 QTEST_MAIN(tst_QProperty);
 
 #include "tst_qproperty.moc"