diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 21abdda84ea..d0c7a29cdaf 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -154,11 +154,13 @@ struct QPropertyDelayedNotifications Change notifications are sent later with notify (following the logic of separating binding updates and notifications used in non-deferred updates). */ - void evaluateBindings(qsizetype index, QBindingStatus *status) { + [[nodiscard]] PendingBindingObserverList evaluateBindings( + qsizetype index, QBindingStatus *status) { + PendingBindingObserverList bindingObservers; auto *delayed = delayedProperties + index; auto *bindingData = delayed->originalBindingData; if (!bindingData) - return; + return bindingObservers; bindingData->d_ptr = delayed->d_ptr; Q_ASSERT(!(bindingData->d_ptr & QPropertyBindingData::DelayedNotificationBit)); @@ -170,7 +172,8 @@ struct QPropertyDelayedNotifications QPropertyBindingDataPointer bindingDataPointer{bindingData}; QPropertyObserverPointer observer = bindingDataPointer.firstObserver(); if (observer) - observer.evaluateBindings(status); + observer.evaluateBindings(bindingObservers, status); + return bindingObservers; } /*! @@ -252,8 +255,11 @@ void Qt::endPropertyUpdateGroup() // update all delayed properties auto start = data; while (data) { - for (qsizetype i = 0; i < data->used; ++i) - data->evaluateBindings(i, status); + for (qsizetype i = 0; i < data->used; ++i) { + PendingBindingObserverList bindingObservers = data->evaluateBindings(i, status); + Q_UNUSED(bindingObservers); + // ### TODO: Use bindingObservers for notify + } data = data->next; } // notify all delayed properties @@ -289,11 +295,11 @@ void QPropertyBindingPrivate::unlinkAndDeref() destroyAndFreeMemory(this); } -void QPropertyBindingPrivate::evaluateRecursive(QBindingStatus *status) +void QPropertyBindingPrivate::evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status) { if (!status) status = &bindingStatus; - return evaluateRecursive_inline(status); + return evaluateRecursive_inline(bindingObservers, status); } void QPropertyBindingPrivate::notifyRecursive() @@ -312,6 +318,31 @@ void QPropertyBindingPrivate::notifyRecursive() updating = false; } +void QPropertyBindingPrivate::notifyNonRecursive(const PendingBindingObserverList &bindingObservers) +{ + notifyNonRecursive(); + for (auto &&bindingObserver: bindingObservers) { + bindingObserver.binding()->notifyNonRecursive(); + } +} + +QPropertyBindingPrivate::NotificationState QPropertyBindingPrivate::notifyNonRecursive() +{ + if (!pendingNotify) + return Delayed; + pendingNotify = false; + Q_ASSERT(!updating); + updating = true; + if (firstObserver) { + firstObserver.noSelfDependencies(this); + firstObserver.notifyOnlyChangeHandler(propertyDataPtr); + } + if (hasStaticObserver) + staticObserverCallback(propertyDataPtr); + updating = false; + return Sent; +} + /*! Constructs a null QUntypedPropertyBinding. @@ -479,8 +510,9 @@ QUntypedPropertyBinding QPropertyBindingData::setBinding(const QUntypedPropertyB newBindingRaw->prependObserver(observer); newBindingRaw->setStaticObserver(staticObserverCallback, guardCallback); - newBindingRaw->evaluateRecursive(); - newBindingRaw->notifyRecursive(); + PendingBindingObserverList bindingObservers; + newBindingRaw->evaluateRecursive(bindingObservers); + newBindingRaw->notifyNonRecursive(bindingObservers); } else if (observer) { d.setObservers(observer.ptr); } else { @@ -588,6 +620,7 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr return; QPropertyBindingDataPointer d{this}; + PendingBindingObserverList bindingObservers; if (QPropertyObserverPointer observer = d.firstObserver()) { auto status = storage ? storage->bindingStatus : nullptr; QPropertyDelayedNotifications *delay; @@ -602,14 +635,16 @@ void QPropertyBindingData::notifyObservers(QUntypedPropertyData *propertyDataPtr delay->addProperty(this, propertyDataPtr); return; } - observer.evaluateBindings(status); + observer.evaluateBindings(bindingObservers, status); } else { return; } // evaluateBindings() can trash the observers. We need to re-fetch here. if (QPropertyObserverPointer observer = d.firstObserver()) - observer.notify(propertyDataPtr); + observer.notifyOnlyChangeHandler(propertyDataPtr); + for (auto &&bindingObserver: bindingObservers) + bindingObserver.binding()->notifyNonRecursive(); } int QPropertyBindingDataPointer::observerCount() const @@ -735,90 +770,18 @@ void QPropertyObserverPointer::setBindingToNotify_unsafe(QPropertyBindingPrivate } /*! + \class QPropertyObserverNodeProtector \internal QPropertyObserverNodeProtector is a RAII wrapper which takes care of the internal switching logic for QPropertyObserverPointer::notify (described ibidem) */ -struct [[nodiscard]] QPropertyObserverNodeProtector { - QPropertyObserverBase m_placeHolder; - QPropertyObserverNodeProtector(QPropertyObserver *observer) - { - // insert m_placeholder after observer into the linked list - QPropertyObserver *next = observer->next.data(); - m_placeHolder.next = next; - observer->next = static_cast(&m_placeHolder); - if (next) - next->prev = &m_placeHolder.next; - m_placeHolder.prev = &observer->next; - m_placeHolder.next.setTag(QPropertyObserver::ObserverIsPlaceholder); - } - QPropertyObserver *next() const { return m_placeHolder.next.data(); } - - ~QPropertyObserverNodeProtector() { - QPropertyObserverPointer d{static_cast(&m_placeHolder)}; - d.unlink_fast(); - } -}; - -/*! \internal +/*! + \fn QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) + \internal \a propertyDataPtr is a pointer to the observed property's property data */ -void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) -{ - auto observer = const_cast(ptr); - /* - * The basic idea of the loop is as follows: We iterate over all observers in the linked list, - * and execute the functionality corresponding to their tag. - * However, complication arise due to the fact that the triggered operations might modify the list, - * which includes deletion and move of the current and next nodes. - * Therefore, we take a few safety precautions: - * 1. Before executing any action which might modify the list, we insert a placeholder node after the current node. - * As that one is stack allocated and owned by us, we can rest assured that it is - * still there after the action has executed, and placeHolder->next points to the actual next node in the list. - * Note that taking next at the beginning of the loop does not work, as the executed action might either move - * or delete that node. - * 2. After the triggered action has finished, we can use the next pointer in the placeholder node as a safe way to - * retrieve the next node. - * 3. Some care needs to be taken to avoid infinite recursion with change handlers, so we add an extra test there, that - * checks whether we're already have the same change handler in our call stack. This can be done by checking whether - * the node after the current one is a placeholder node. - */ - while (observer) { - QPropertyObserver *next = observer->next.data(); - switch (QPropertyObserver::ObserverTag(observer->next.tag())) { - case QPropertyObserver::ObserverNotifiesChangeHandler: - { - auto handlerToCall = observer->changeHandler; - // prevent recursion - if (next && next->next.tag() == QPropertyObserver::ObserverIsPlaceholder) { - observer = next->next.data(); - continue; - } - // handlerToCall might modify the list - QPropertyObserverNodeProtector protector(observer); - handlerToCall(observer, propertyDataPtr); - next = protector.next(); - break; - } - case QPropertyObserver::ObserverNotifiesBinding: - { - auto bindingToNotify = observer->binding; - QPropertyObserverNodeProtector protector(observer); - bindingToNotify->notifyRecursive(); - next = protector.next(); - break; - } - case QPropertyObserver::ObserverIsPlaceholder: - // recursion is already properly handled somewhere else - break; - case QPropertyObserver::ObserverIsAlias: - break; - default: Q_UNREACHABLE(); - } - observer = next; - } -} + #ifndef QT_NO_DEBUG void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *binding) @@ -838,7 +801,7 @@ void QPropertyObserverPointer::noSelfDependencies(QPropertyBindingPrivate *bindi } #endif -void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status) +void QPropertyObserverPointer::evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status) { Q_ASSERT(status); auto observer = const_cast(ptr); @@ -847,9 +810,10 @@ void QPropertyObserverPointer::evaluateBindings(QBindingStatus *status) QPropertyObserver *next = observer->next.data(); if (QPropertyObserver::ObserverTag(observer->next.tag()) == QPropertyObserver::ObserverNotifiesBinding) { + bindingObservers.push_back(observer); auto bindingToEvaluate = observer->binding; QPropertyObserverNodeProtector protector(observer); - bindingToEvaluate->evaluateRecursive_inline(status); + bindingToEvaluate->evaluateRecursive_inline(bindingObservers, status); next = protector.next(); } diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index b850fa88326..aea8f73cabd 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -57,6 +57,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -65,6 +66,34 @@ namespace QtPrivate { struct QBindingStatusAccessToken {}; } + +/*! + \internal + Similar to \c QPropertyBindingPrivatePtr, but stores a + \c QPropertyObserver * linking to the QPropertyBindingPrivate* + instead of the QPropertyBindingPrivate* itself + */ +struct QBindingObserverPtr +{ +private: + QPropertyObserver *d = nullptr; +public: + QBindingObserverPtr() = default; + Q_DISABLE_COPY(QBindingObserverPtr); + void swap(QBindingObserverPtr &other) noexcept + { qt_ptr_swap(d, other.d); } + QBindingObserverPtr(QBindingObserverPtr &&other) : d(std::exchange(other.d, nullptr)) {} + QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_MOVE_AND_SWAP(QBindingObserverPtr); + + + inline QBindingObserverPtr(QPropertyObserver *observer); + inline ~QBindingObserverPtr(); + inline QPropertyBindingPrivate *binding() const; + inline QPropertyObserver *operator ->(); +}; + +using PendingBindingObserverList = QVarLengthArray; + // Keep all classes related to QProperty in one compilation unit. Performance of this code is crucial and // we need to allow the compiler to inline where it makes sense. @@ -98,6 +127,25 @@ struct Q_AUTOTEST_EXPORT QPropertyBindingDataPointer } }; +struct [[nodiscard]] QPropertyObserverNodeProtector { + QPropertyObserverBase m_placeHolder; + QPropertyObserverNodeProtector(QPropertyObserver *observer) + { + // insert m_placeholder after observer into the linked list + QPropertyObserver *next = observer->next.data(); + m_placeHolder.next = next; + observer->next = static_cast(&m_placeHolder); + if (next) + next->prev = &m_placeHolder.next; + m_placeHolder.prev = &observer->next; + m_placeHolder.next.setTag(QPropertyObserver::ObserverIsPlaceholder); + } + + QPropertyObserver *next() const { return m_placeHolder.next.data(); } + + ~QPropertyObserverNodeProtector(); +}; + // This is a helper "namespace" struct QPropertyObserverPointer { @@ -110,18 +158,28 @@ struct QPropertyObserverPointer void setBindingToNotify_unsafe(QPropertyBindingPrivate *binding); void setChangeHandler(QPropertyObserver::ChangeHandler changeHandler); + enum class Notify {Everything, OnlyChangeHandlers}; + + template void notify(QUntypedPropertyData *propertyDataPtr); + void notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr); #ifndef QT_NO_DEBUG void noSelfDependencies(QPropertyBindingPrivate *binding); #else void noSelfDependencies(QPropertyBindingPrivate *) {} #endif - void evaluateBindings(QBindingStatus *status); + void evaluateBindings(PendingBindingObserverList &bindingObservers, QBindingStatus *status); void observeProperty(QPropertyBindingDataPointer property); explicit operator bool() const { return ptr != nullptr; } QPropertyObserverPointer nextObserver() const { return {ptr->next.data()}; } + + QPropertyBindingPrivate *binding() const + { + Q_ASSERT(ptr->next.tag() == QPropertyObserver::ObserverNotifiesBinding); + return ptr->binding; + }; }; class QPropertyBindingErrorPrivate : public QSharedData @@ -329,10 +387,21 @@ public: void unlinkAndDeref(); - void evaluateRecursive(QBindingStatus *status = nullptr); - void Q_ALWAYS_INLINE evaluateRecursive_inline(QBindingStatus *status); + void evaluateRecursive(PendingBindingObserverList &bindingObservers, QBindingStatus *status = nullptr); + + // ### TODO: remove as soon as declarative no longer needs this overload + void evaluateRecursive() + { + PendingBindingObserverList bindingObservers; + evaluateRecursive(bindingObservers); + } + + void Q_ALWAYS_INLINE evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status); void notifyRecursive(); + void notifyNonRecursive(const PendingBindingObserverList &bindingObservers); + enum NotificationState : bool { Delayed, Sent }; + NotificationState notifyNonRecursive(); static QPropertyBindingPrivate *get(const QUntypedPropertyBinding &binding) { return static_cast(binding.d.data()); } @@ -700,7 +769,7 @@ struct QUntypedBindablePrivate } }; -inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *status) +inline void QPropertyBindingPrivate::evaluateRecursive_inline(PendingBindingObserverList &bindingObservers, QBindingStatus *status) { if (updating) { error = QPropertyBindingError(QPropertyBindingError::BindingLoop); @@ -739,9 +808,88 @@ inline void QPropertyBindingPrivate::evaluateRecursive_inline(QBindingStatus *st return; firstObserver.noSelfDependencies(this); - firstObserver.evaluateBindings(status); + firstObserver.evaluateBindings(bindingObservers, status); } +template +void QPropertyObserverPointer::notify(QUntypedPropertyData *propertyDataPtr) +{ + auto observer = const_cast(ptr); + /* + * The basic idea of the loop is as follows: We iterate over all observers in the linked list, + * and execute the functionality corresponding to their tag. + * However, complication arise due to the fact that the triggered operations might modify the list, + * which includes deletion and move of the current and next nodes. + * Therefore, we take a few safety precautions: + * 1. Before executing any action which might modify the list, we insert a placeholder node after the current node. + * As that one is stack allocated and owned by us, we can rest assured that it is + * still there after the action has executed, and placeHolder->next points to the actual next node in the list. + * Note that taking next at the beginning of the loop does not work, as the executed action might either move + * or delete that node. + * 2. After the triggered action has finished, we can use the next pointer in the placeholder node as a safe way to + * retrieve the next node. + * 3. Some care needs to be taken to avoid infinite recursion with change handlers, so we add an extra test there, that + * checks whether we're already have the same change handler in our call stack. This can be done by checking whether + * the node after the current one is a placeholder node. + */ + while (observer) { + QPropertyObserver *next = observer->next.data(); + switch (QPropertyObserver::ObserverTag(observer->next.tag())) { + case QPropertyObserver::ObserverNotifiesChangeHandler: + { + auto handlerToCall = observer->changeHandler; + // prevent recursion + if (next && next->next.tag() == QPropertyObserver::ObserverIsPlaceholder) { + observer = next->next.data(); + continue; + } + // handlerToCall might modify the list + QPropertyObserverNodeProtector protector(observer); + handlerToCall(observer, propertyDataPtr); + next = protector.next(); + break; + } + case QPropertyObserver::ObserverNotifiesBinding: + { + if constexpr (notifyPolicy == Notify::Everything) { + auto bindingToNotify = observer->binding; + QPropertyObserverNodeProtector protector(observer); + bindingToNotify->notifyRecursive(); + next = protector.next(); + } + break; + } + case QPropertyObserver::ObserverIsPlaceholder: + // recursion is already properly handled somewhere else + break; + case QPropertyObserver::ObserverIsAlias: + break; + default: Q_UNREACHABLE(); + } + observer = next; + } +} + +inline void QPropertyObserverPointer::notifyOnlyChangeHandler(QUntypedPropertyData *propertyDataPtr) +{ + notify(propertyDataPtr); +} + +inline QPropertyObserverNodeProtector::~QPropertyObserverNodeProtector() +{ + QPropertyObserverPointer d{static_cast(&m_placeHolder)}; + d.unlink_fast(); +} + +QBindingObserverPtr::QBindingObserverPtr(QPropertyObserver *observer) : d(observer) +{ + Q_ASSERT(d); + QPropertyObserverPointer{d}.binding()->addRef(); +} +QBindingObserverPtr::~QBindingObserverPtr() { if (d) QPropertyObserverPointer{d}.binding()->deref(); } +QPropertyBindingPrivate *QBindingObserverPtr::binding() const { return QPropertyObserverPointer{d}.binding(); } +QPropertyObserver *QBindingObserverPtr::operator->() { return d; } + QT_END_NAMESPACE #endif // QPROPERTY_P_H diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index 6a9316e2d74..c098cea9d83 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -54,6 +54,7 @@ #include #include #include +#include #include @@ -64,6 +65,9 @@ class QBindingStorage; template class QObjectCompatProperty; +struct QBindingObserverPtr; +using PendingBindingObserverList = QVarLengthArray; + namespace QtPrivate { // QPropertyBindingPrivatePtr operates on a RefCountingMixin solely so that we can inline // the constructor and copy constructor diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index b8041238450..53a1b74df1d 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -121,6 +121,8 @@ private slots: void qpropertyAlias(); void scheduleNotify(); + + void notifyAfterAllDepsGone(); }; void tst_QProperty::functorBinding() @@ -2029,6 +2031,28 @@ void tst_QProperty::scheduleNotify() QCOMPARE(p.value(), 0); } +void tst_QProperty::notifyAfterAllDepsGone() +{ + bool b = true; + QProperty iprop; + QProperty jprop(42); + iprop.setBinding([&](){ + if (b) + return jprop.value(); + return 13; + }); + int changeCounter = 0; + auto keepAlive = iprop.onValueChanged([&](){ changeCounter++; }); + QCOMPARE(iprop.value(), 42); + jprop = 44; + QCOMPARE(iprop.value(), 44); + QCOMPARE(changeCounter, 1); + b = false; + jprop = 43; + QCOMPARE(iprop.value(), 13); + QCOMPARE(changeCounter, 2); +} + QTEST_MAIN(tst_QProperty); #undef QT_SOURCE_LOCATION_NAMESPACE