diff options
author | Lars Knoll <lars.knoll@qt.io> | 2021-02-04 17:20:33 +0100 |
---|---|---|
committer | Fabian Kosmale <fabian.kosmale@qt.io> | 2021-04-16 16:49:29 +0200 |
commit | bb44c18b673e2955592ee86774eb7cc25d390c17 (patch) | |
tree | 5f3bc7aca74c8c9373b407e9e7fd1dbbb534e08b | |
parent | 984bc7cc3e53e1a9003209858f89bd7fcbd5f81c (diff) |
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 <lars.knoll@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Andreas Buhr <andreas.buhr@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
-rw-r--r-- | src/corelib/kernel/qproperty.cpp | 70 | ||||
-rw-r--r-- | src/corelib/kernel/qproperty_p.h | 10 | ||||
-rw-r--r-- | tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp | 31 |
3 files changed, 99 insertions, 12 deletions
diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 1c642562a8..60f21d5b57 100644 --- 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 index 7ac02cb43d..3535d7e843 100644 --- 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 @@ private: // 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 @@ public: 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 index 12fd124b18..8cc4fe486a 100644 --- 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" |