From e18a060c034beec6c7f4a22044255d9fb55d091a Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 19 Jun 2020 13:58:53 +0200 Subject: [PATCH] QNotifiedProperty: Add guard callback A guard callback is a predicate which takes the new value set by setValue or computed as the result of a binding expression. If it returns false, the value is discarded and the old value is kept. Note that due to lazyness, when setting a binding, we still notify everyone as the binding is only evaluated on demand, and the guard can thus only run when someone actually queries the value. Note further that a guard is allowed to modify the value that is passed to it (e.g. to clamp it to a certain range). Task-number: QTBUG-85032 Change-Id: I3551e4357fe5780fb75da80bf8be208ec152dc2a Reviewed-by: Ulf Hermann --- src/corelib/kernel/qproperty.cpp | 5 +- src/corelib/kernel/qproperty.h | 143 +++++++++++------- src/corelib/kernel/qpropertybinding.cpp | 10 +- src/corelib/kernel/qpropertybinding_p.h | 4 +- src/corelib/kernel/qpropertyprivate.h | 3 +- .../kernel/qproperty/tst_qproperty.cpp | 131 ++++++++++++++++ 6 files changed, 236 insertions(+), 60 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index 7ee035030f4..b362319f224 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -95,7 +95,8 @@ QPropertyBase::~QPropertyBase() QUntypedPropertyBinding QPropertyBase::setBinding(const QUntypedPropertyBinding &binding, void *propertyDataPtr, void *staticObserver, - void (*staticObserverCallback)(void*, void*)) + void (*staticObserverCallback)(void*, void*), + bool (*guardCallback)(void *, void*)) { QPropertyBindingPrivatePtr oldBinding; QPropertyBindingPrivatePtr newBinding = binding.d; @@ -122,7 +123,7 @@ QUntypedPropertyBinding QPropertyBase::setBinding(const QUntypedPropertyBinding newBinding->setProperty(propertyDataPtr); if (observer) newBinding->prependObserver(observer); - newBinding->setStaticObserver(staticObserver, staticObserverCallback); + newBinding->setStaticObserver(staticObserver, staticObserverCallback, guardCallback); } else if (observer) { d.setObservers(observer.ptr); } else { diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index b1b4dbeae10..5ba4d2eda7c 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -85,7 +85,7 @@ struct Q_CORE_EXPORT QPropertyBindingSourceLocation template class QPropertyChangeHandler; template class QProperty; -template class QNotifiedProperty; +template class QNotifiedProperty; class QPropertyBindingErrorPrivate; @@ -179,8 +179,8 @@ public: : QUntypedPropertyBinding(property.d.priv.binding()) {} - template - QPropertyBinding(const QNotifiedProperty &property) + template + QPropertyBinding(const QNotifiedProperty &property) : QUntypedPropertyBinding(property.d.priv.binding()) {} @@ -382,13 +382,36 @@ namespace detail { struct ExtractClassFromFunctionPointer { using Class = C; }; } -template +template class QNotifiedProperty { public: using value_type = T; using Class = typename detail::ExtractClassFromFunctionPointer::Class; - static_assert(std::is_invocable_v || std::is_invocable_v); +private: + static bool constexpr ValueGuardModifiesArgument = std::is_invocable_r_v; + static bool constexpr CallbackAcceptsOldValue = std::is_invocable_v; + static bool constexpr HasValueGuard = !std::is_same_v; +public: + static_assert(CallbackAcceptsOldValue || std::is_invocable_v); + static_assert( + std::is_invocable_r_v || + ValueGuardModifiesArgument || + !HasValueGuard, + "Guard has wrong signature"); +private: + // type erased guard functions, casts its arguments to the correct types + static constexpr bool (*GuardTEHelper)(void *, void*) = [](void *o, void *newValue){ + if constexpr (HasValueGuard) { // Guard->* is invalid if Guard == nullptr + return (reinterpret_cast(o)->*(ValueGuard))(*static_cast(newValue)); + } else { + Q_UNUSED(o); // some compilers complain about unused variables + Q_UNUSED(newValue); + return true; + } + }; + static constexpr bool(*GuardTE)(void *, void*) = HasValueGuard ? GuardTEHelper : nullptr; +public: QNotifiedProperty() = default; @@ -428,46 +451,56 @@ public: return value(); } - void setValue(Class *owner, T &&newValue) + template + auto setValue(Class *owner, S &&newValue) -> std::enable_if_t, void> { - if constexpr(std::is_invocable_v) { - if (d.setValueAndReturnTrueIfChanged(std::move(newValue))) - notify(owner); - } else { + if constexpr (HasValueGuard) { + if (!(owner->*ValueGuard)(newValue)) + return; + } + if constexpr (CallbackAcceptsOldValue) { T oldValue = value(); // TODO: kind of pointless if there was no change if (d.setValueAndReturnTrueIfChanged(std::move(newValue))) notify(owner, &oldValue); + } else { + if (d.setValueAndReturnTrueIfChanged(std::move(newValue))) + notify(owner); } d.priv.removeBinding(); } - void setValue(Class *owner, const T &newValue) + void setValue(Class *owner, std::conditional_t newValue) { - if constexpr(std::is_invocable_v) { - if (d.setValueAndReturnTrueIfChanged(newValue)) - notify(owner); - } else { + if constexpr (HasValueGuard) { + if (!(owner->*ValueGuard)(newValue)) + return; + } + if constexpr (CallbackAcceptsOldValue) { + // When newValue is T, we move it, if it's const T& it stays const T& and won't get moved T oldValue = value(); - if (d.setValueAndReturnTrueIfChanged(newValue)) + if (d.setValueAndReturnTrueIfChanged(std::move(newValue))) notify(owner, &oldValue); + } else { + if (d.setValueAndReturnTrueIfChanged(std::move(newValue))) + notify(owner); } d.priv.removeBinding(); } QPropertyBinding setBinding(Class *owner, const QPropertyBinding &newBinding) { - if constexpr(std::is_invocable_v) { - QPropertyBinding oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o) { - (reinterpret_cast(o)->*Callback)(); - })); - notify(owner); + if constexpr (CallbackAcceptsOldValue) { + T oldValue = value(); + QPropertyBinding oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldVal) { + (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldVal)); + }, GuardTE)); + notify(owner, &oldValue); return oldBinding; } else { - T oldValue = value(); - QPropertyBinding oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldValue) { - (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldValue)); - })); - notify(owner, &oldValue); + QPropertyBinding oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *) { + (reinterpret_cast(o)->*Callback)(); + }, GuardTE)); + notify(owner); return oldBinding; } } @@ -475,18 +508,18 @@ public: QPropertyBinding setBinding(Class *owner, QPropertyBinding &&newBinding) { QPropertyBinding b(std::move(newBinding)); - if constexpr(std::is_invocable_v) { - QPropertyBinding oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *) { - (reinterpret_cast(o)->*Callback)(); - })); - notify(owner); + if constexpr (CallbackAcceptsOldValue) { + T oldValue = value(); + QPropertyBinding oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *oldVal) { + (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldVal)); + }, GuardTE)); + notify(owner, &oldValue); return oldBinding; } else { - T oldValue = value(); - QPropertyBinding oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *oldValue) { - (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldValue)); - })); - notify(owner, &oldValue); + QPropertyBinding oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *) { + (reinterpret_cast(o)->*Callback)(); + }, GuardTE)); + notify(owner); return oldBinding; } } @@ -495,17 +528,17 @@ public: { if (newBinding.valueMetaType().id() != qMetaTypeId()) return false; - if constexpr(std::is_invocable_v) { + if constexpr (CallbackAcceptsOldValue) { + T oldValue = value(); + d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldVal) { + (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldVal)); + }, GuardTE); + notify(owner, &oldValue); + } else { d.priv.setBinding(newBinding, &d, owner, [](void *o, void *) { (reinterpret_cast(o)->*Callback)(); - }); + }, GuardTE); notify(owner); - } else { - T oldValue = value(); - d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldValue) { - (reinterpret_cast(o)->*Callback)(*reinterpret_cast(oldValue)); - }); - notify(owner, &oldValue); } return true; } @@ -544,10 +577,12 @@ private: void notify(Class *owner, T *oldValue=nullptr) { d.priv.notifyObservers(&d); - if constexpr(std::is_invocable_v) + if constexpr (std::is_invocable_v) { + Q_UNUSED(oldValue); (owner->*Callback)(); - else + } else { (owner->*Callback)(*oldValue); + } } Q_DISABLE_COPY_MOVE(QNotifiedProperty) @@ -581,8 +616,8 @@ public: void setSource(const QProperty &property) { setSource(property.d.priv); } - template - void setSource(const QNotifiedProperty &property) + template + void setSource(const QNotifiedProperty &property) { setSource(property.d.priv); } protected: @@ -642,8 +677,8 @@ public: setSource(property); } - template - QPropertyChangeHandler(const QNotifiedProperty &property, Functor handler) + template + QPropertyChangeHandler(const QNotifiedProperty &property, Functor handler) : QPropertyObserver([](QPropertyObserver *self, void *) { auto This = static_cast*>(self); This->m_handler(); @@ -675,9 +710,9 @@ QPropertyChangeHandler QProperty::subscribe(Functor f) return onValueChanged(f); } -template +template template -QPropertyChangeHandler QNotifiedProperty::onValueChanged(Functor f) +QPropertyChangeHandler QNotifiedProperty::onValueChanged(Functor f) { #if defined(__cpp_lib_is_invocable) && (__cpp_lib_is_invocable >= 201703L) static_assert(std::is_invocable_v, "Functor callback must be callable without any parameters"); @@ -685,9 +720,9 @@ QPropertyChangeHandler QNotifiedProperty::onValueChanged(F return QPropertyChangeHandler(*this, f); } -template +template template -QPropertyChangeHandler QNotifiedProperty::subscribe(Functor f) +QPropertyChangeHandler QNotifiedProperty::subscribe(Functor f) { #if defined(__cpp_lib_is_invocable) && (__cpp_lib_is_invocable >= 201703L) static_assert(std::is_invocable_v, "Functor callback must be callable without any parameters"); diff --git a/src/corelib/kernel/qpropertybinding.cpp b/src/corelib/kernel/qpropertybinding.cpp index 3a0f54b44af..8602ef957f0 100644 --- a/src/corelib/kernel/qpropertybinding.cpp +++ b/src/corelib/kernel/qpropertybinding.cpp @@ -111,7 +111,10 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged() bool newValue = false; evalError = evaluationFunction(metaType, &newValue); if (evalError.type() == QPropertyBindingError::NoError) { - if (propertyPtr->extraBit() != newValue) { + bool updateAllowed = true; + if (hasStaticObserver && staticGuardCallback) + updateAllowed = staticGuardCallback(staticObserver, &newValue); + if (updateAllowed && propertyPtr->extraBit() != newValue) { propertyPtr->setExtraBit(newValue); changed = true; } @@ -121,7 +124,10 @@ bool QPropertyBindingPrivate::evaluateIfDirtyAndReturnTrueIfValueChanged() evalError = evaluationFunction(metaType, resultVariant.data()); if (evalError.type() == QPropertyBindingError::NoError) { int compareResult = 0; - if (!QMetaType::compare(propertyDataPtr, resultVariant.constData(), metaType.id(), &compareResult) || compareResult != 0) { + bool updateAllowed = true; + if (hasStaticObserver && staticGuardCallback) + updateAllowed = staticGuardCallback(staticObserver, resultVariant.data()); + if (updateAllowed && (!QMetaType::compare(propertyDataPtr, resultVariant.constData(), metaType.id(), &compareResult) || compareResult != 0)) { changed = true; metaType.destruct(propertyDataPtr); metaType.construct(propertyDataPtr, resultVariant.constData()); diff --git a/src/corelib/kernel/qpropertybinding_p.h b/src/corelib/kernel/qpropertybinding_p.h index 99a6b22b4b8..151f5543d55 100644 --- a/src/corelib/kernel/qpropertybinding_p.h +++ b/src/corelib/kernel/qpropertybinding_p.h @@ -81,6 +81,7 @@ private: struct { void *staticObserver; void (*staticObserverCallback)(void*, void*); + bool (*staticGuardCallback)(void*, void*); }; }; QScopedPointer> heapObservers; @@ -107,7 +108,7 @@ public: void setDirty(bool d) { dirty = d; } void setProperty(void *propertyPtr) { propertyDataPtr = propertyPtr; } - void setStaticObserver(void *observer, void (*callback)(void*, void*)) + void setStaticObserver(void *observer, void (*callback)(void*, void*), bool (*guardCallback)(void *, void*)) { if (observer) { if (!hasStaticObserver) { @@ -123,6 +124,7 @@ public: hasStaticObserver = true; staticObserver = observer; staticObserverCallback = callback; + staticGuardCallback = guardCallback; } else if (hasStaticObserver) { hasStaticObserver = false; new (&inlineDependencyObservers) ObserverArray(); diff --git a/src/corelib/kernel/qpropertyprivate.h b/src/corelib/kernel/qpropertyprivate.h index 43318f25c0a..145f2c66b9a 100644 --- a/src/corelib/kernel/qpropertyprivate.h +++ b/src/corelib/kernel/qpropertyprivate.h @@ -84,7 +84,8 @@ public: QUntypedPropertyBinding setBinding(const QUntypedPropertyBinding &newBinding, void *propertyDataPtr, void *staticObserver = nullptr, - void (*staticObserverCallback)(void *, void *) = nullptr); + void (*staticObserverCallback)(void *, void *) = nullptr, + bool (*guardCallback)(void *, void *) = nullptr); QPropertyBindingPrivate *binding(); void evaluateIfDirty(); diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 7200bf176ec..53d361ca267 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -73,6 +73,7 @@ private slots: void propertyAlias(); void notifiedProperty(); void notifiedPropertyWithOldValueCallback(); + void notifiedPropertyWithGuard(); }; void tst_QProperty::functorBinding() @@ -876,6 +877,136 @@ void tst_QProperty::notifiedPropertyWithOldValueCallback() QCOMPARE(instance.property.value(), 4); } +struct ClassWithNotifiedPropertyWithGuard +{ + using This = ClassWithNotifiedPropertyWithGuard; + QVector recordedValues; + + void callback() { recordedValues << property.value(); } + void callback2() { recordedValues << property2.value(); } + void callback3() {} + bool trivialGuard(int ) {return true;} + bool reject42(int newValue) {return newValue != 42;} + bool bound(int &newValue) { newValue = qBound(0, newValue, 100); return true; } + + QNotifiedProperty property; + QNotifiedProperty property2; + QNotifiedProperty property3; +}; + +void tst_QProperty::notifiedPropertyWithGuard() +{ + { + // property with guard that returns always true is the same as using no guard + ClassWithNotifiedPropertyWithGuard instance; + + std::array, 5> otherProperties = { + QProperty([&]() { return instance.property + 1; }), + QProperty([&]() { return instance.property + 2; }), + QProperty([&]() { return instance.property + 3; }), + QProperty([&]() { return instance.property + 4; }), + QProperty([&]() { return instance.property + 5; }), + }; + + auto check = [&] { + const int val = instance.property.value(); + for (int i = 0; i < int(otherProperties.size()); ++i) + QCOMPARE(otherProperties[i].value(), val + i + 1); + }; + + QVERIFY(instance.recordedValues.isEmpty()); + check(); + + instance.property.setValue(&instance, 42); + QCOMPARE(instance.recordedValues.count(), 1); + QCOMPARE(instance.recordedValues.at(0), 42); + instance.recordedValues.clear(); + check(); + + instance.property.setValue(&instance, 42); + QVERIFY(instance.recordedValues.isEmpty()); + check(); + + int subscribedCount = 0; + QProperty injectedValue(100); + instance.property.setBinding(&instance, [&injectedValue]() { return injectedValue.value(); }); + auto subscriber = [&] { ++subscribedCount; }; + std::array, 10> subscribers = { + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber), + instance.property.subscribe(subscriber) + }; + + QCOMPARE(subscribedCount, 10); + subscribedCount = 0; + + QCOMPARE(instance.property.value(), 100); + QCOMPARE(instance.recordedValues.count(), 1); + QCOMPARE(instance.recordedValues.at(0), 100); + instance.recordedValues.clear(); + check(); + QCOMPARE(subscribedCount, 0); + + injectedValue = 200; + QCOMPARE(instance.property.value(), 200); + QCOMPARE(instance.recordedValues.count(), 1); + QCOMPARE(instance.recordedValues.at(0), 200); + instance.recordedValues.clear(); + check(); + QCOMPARE(subscribedCount, 10); + subscribedCount = 0; + + injectedValue = 400; + QCOMPARE(instance.property.value(), 400); + QCOMPARE(instance.recordedValues.count(), 1); + QCOMPARE(instance.recordedValues.at(0), 400); + instance.recordedValues.clear(); + check(); + QCOMPARE(subscribedCount, 10); + } + + { + // Values can be rejected + ClassWithNotifiedPropertyWithGuard instance2; + + instance2.property2.setValue(&instance2, 1); + instance2.property2.setBinding(&instance2, [](){return 42;}); + instance2.property2.setValue(&instance2, 2); + instance2.property2.setBinding(&instance2, [](){return 3;}); + instance2.property2.setValue(&instance2, 42); + // Note that we get 1 twice + // This is an unfortunate result of the lazyness used for bindings + // When we call setBinding, the binding does not get evaluated, but we + // call the callback in notify; the callback will in our case query the + // properties' value. At that point we then evaluate the binding, and + // notice that the value is in fact disallowed. Thus we return the old + // value. + QVector expected {1, 1, 2, 3}; + QCOMPARE(instance2.recordedValues, expected); + } + + { + // guard can modify incoming values + ClassWithNotifiedPropertyWithGuard instance3; + instance3.property3.setValue(&instance3, 5); + int i1 = 5; + QCOMPARE(instance3.property3.value(), i1); + instance3.property3.setBinding(&instance3, [](){return 255;}); + QCOMPARE(instance3.property3.value(), 100); + const int i2 = -1; + instance3.property3.setValue(&instance3, i2); + QCOMPARE(instance3.property3.value(), 0); + } +} + + QTEST_MAIN(tst_QProperty); #include "tst_qproperty.moc"