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 <ulf.hermann@qt.io>

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 {

src/corelib/kernel/qproperty.h

@@ -85,7 +85,7 @@ struct Q_CORE_EXPORT QPropertyBindingSourceLocation
 template <typename Functor> class QPropertyChangeHandler;
 
 template <typename T> class QProperty;
-template <typename T, auto callbackMember> class QNotifiedProperty;
+template <typename T, auto callbackMember, auto guardCallback> class QNotifiedProperty;
 
 class QPropertyBindingErrorPrivate;
 
@@ -179,8 +179,8 @@ public:
         : QUntypedPropertyBinding(property.d.priv.binding())
     {}
 
-    template<auto notifier>
+    template<auto notifier, auto guard>
-    QPropertyBinding(const QNotifiedProperty<PropertyType, notifier> &property)
+    QPropertyBinding(const QNotifiedProperty<PropertyType, notifier, guard> &property)
         : QUntypedPropertyBinding(property.d.priv.binding())
     {}
 
@@ -382,13 +382,36 @@ namespace detail {
     struct ExtractClassFromFunctionPointer<T C::*> { using Class = C; };
 }
 
-template <typename T, auto Callback>
+template <typename T, auto Callback, auto ValueGuard=nullptr>
 class QNotifiedProperty
 {
 public:
     using value_type = T;
     using Class = typename detail::ExtractClassFromFunctionPointer<decltype(Callback)>::Class;
-    static_assert(std::is_invocable_v<decltype(Callback), Class, T> || std::is_invocable_v<decltype(Callback), Class>);
+private:
+    static bool constexpr ValueGuardModifiesArgument = std::is_invocable_r_v<bool, decltype(ValueGuard), Class, T&>;
+    static bool constexpr CallbackAcceptsOldValue = std::is_invocable_v<decltype(Callback), Class, T>;
+    static bool constexpr HasValueGuard = !std::is_same_v<decltype(ValueGuard), std::nullptr_t>;
+public:
+    static_assert(CallbackAcceptsOldValue || std::is_invocable_v<decltype(Callback), Class>);
+    static_assert(
+            std::is_invocable_r_v<bool, decltype(ValueGuard), Class, T> ||
+            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<Class *>(o)->*(ValueGuard))(*static_cast<T *>(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<typename S>
+    auto setValue(Class *owner, S &&newValue) -> std::enable_if_t<!ValueGuardModifiesArgument && std::is_same_v<S, T>, void>
     {
-        if constexpr(std::is_invocable_v<decltype(Callback), Class>) {
+        if constexpr (HasValueGuard) {
-            if (d.setValueAndReturnTrueIfChanged(std::move(newValue)))
+            if (!(owner->*ValueGuard)(newValue))
-                notify(owner);
+                return;
-        } else {
+        }
+        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<ValueGuardModifiesArgument, T, const T &> newValue)
     {
-        if constexpr(std::is_invocable_v<decltype(Callback), Class>) {
+        if constexpr (HasValueGuard) {
-            if (d.setValueAndReturnTrueIfChanged(newValue))
+            if (!(owner->*ValueGuard)(newValue))
-                notify(owner);
+                return;
-        } else {
+        }
+        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<T> setBinding(Class *owner, const QPropertyBinding<T> &newBinding)
     {
-        if constexpr(std::is_invocable_v<decltype(Callback), Class>) {
+        if constexpr (CallbackAcceptsOldValue) {
-            QPropertyBinding<T> oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o) {
+            T oldValue = value();
-                (reinterpret_cast<Class *>(o)->*Callback)();
+            QPropertyBinding<T> oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldVal) {
-            }));
+                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(oldVal));
-            notify(owner);
+            }, GuardTE));
+            notify(owner, &oldValue);
             return oldBinding;
         } else {
-            T oldValue = value();
+            QPropertyBinding<T> oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *) {
-            QPropertyBinding<T> oldBinding(d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldValue) {
+                (reinterpret_cast<Class *>(o)->*Callback)();
-                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(oldValue));
+            }, GuardTE));
-            }));
+            notify(owner);
-            notify(owner, &oldValue);
             return oldBinding;
         }
     }
@@ -475,18 +508,18 @@ public:
     QPropertyBinding<T> setBinding(Class *owner, QPropertyBinding<T> &&newBinding)
     {
         QPropertyBinding<T> b(std::move(newBinding));
-        if constexpr(std::is_invocable_v<decltype(Callback), Class>) {
+        if constexpr (CallbackAcceptsOldValue) {
-            QPropertyBinding<T> oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *) {
+            T oldValue = value();
-                (reinterpret_cast<Class *>(o)->*Callback)();
+            QPropertyBinding<T> oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *oldVal) {
-            }));
+                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(oldVal));
-            notify(owner);
+            }, GuardTE));
+            notify(owner, &oldValue);
             return oldBinding;
         } else {
-            T oldValue = value();
+            QPropertyBinding<T> oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *) {
-            QPropertyBinding<T> oldBinding(d.priv.setBinding(b, &d, owner, [](void *o, void *oldValue) {
+                (reinterpret_cast<Class *>(o)->*Callback)();
-                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(oldValue));
+            }, GuardTE));
-            }));
+            notify(owner);
-            notify(owner, &oldValue);
             return oldBinding;
         }
     }
@@ -495,17 +528,17 @@ public:
     {
         if (newBinding.valueMetaType().id() != qMetaTypeId<T>())
             return false;
-        if constexpr(std::is_invocable_v<decltype(Callback), Class>) {
+        if constexpr (CallbackAcceptsOldValue) {
+            T oldValue = value();
+            d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldVal) {
+                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(oldVal));
+            }, GuardTE);
+            notify(owner, &oldValue);
+        } else {
             d.priv.setBinding(newBinding, &d, owner, [](void *o, void *) {
                 (reinterpret_cast<Class *>(o)->*Callback)();
-            });
+            }, GuardTE);
             notify(owner);
-        } else {
-            T oldValue = value();
-            d.priv.setBinding(newBinding, &d, owner, [](void *o, void *oldValue) {
-                (reinterpret_cast<Class *>(o)->*Callback)(*reinterpret_cast<T *>(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<decltype(Callback), Class>)
+        if constexpr (std::is_invocable_v<decltype(Callback), Class>) {
+            Q_UNUSED(oldValue);
             (owner->*Callback)();
-        else
+        } else {
             (owner->*Callback)(*oldValue);
+        }
     }
 
     Q_DISABLE_COPY_MOVE(QNotifiedProperty)
@@ -581,8 +616,8 @@ public:
     void setSource(const QProperty<PropertyType> &property)
     { setSource(property.d.priv); }
 
-    template <typename PropertyType, auto notifier>
+    template <typename PropertyType, auto notifier, auto guard>
-    void setSource(const QNotifiedProperty<PropertyType, notifier> &property)
+    void setSource(const QNotifiedProperty<PropertyType, notifier, guard> &property)
     { setSource(property.d.priv); }
 
 protected:
@@ -642,8 +677,8 @@ public:
         setSource(property);
     }
 
-    template <typename PropertyType, typename Class, void(Class::*Callback)()>
+    template <typename PropertyType, auto Callback, auto Guard>
-    QPropertyChangeHandler(const QNotifiedProperty<PropertyType, Callback> &property, Functor handler)
+    QPropertyChangeHandler(const QNotifiedProperty<PropertyType, Callback, Guard> &property, Functor handler)
         : QPropertyObserver([](QPropertyObserver *self, void *) {
               auto This = static_cast<QPropertyChangeHandler<Functor>*>(self);
               This->m_handler();
@@ -675,9 +710,9 @@ QPropertyChangeHandler<Functor> QProperty<T>::subscribe(Functor f)
     return onValueChanged(f);
 }
 
-template <typename T, auto Callback>
+template <typename T, auto Callback, auto ValueGuard>
 template<typename Functor>
-QPropertyChangeHandler<Functor> QNotifiedProperty<T, Callback>::onValueChanged(Functor f)
+QPropertyChangeHandler<Functor> QNotifiedProperty<T, Callback, ValueGuard>::onValueChanged(Functor f)
 {
 #if defined(__cpp_lib_is_invocable) && (__cpp_lib_is_invocable >= 201703L)
     static_assert(std::is_invocable_v<Functor>, "Functor callback must be callable without any parameters");
@@ -685,9 +720,9 @@ QPropertyChangeHandler<Functor> QNotifiedProperty<T, Callback>::onValueChanged(F
     return QPropertyChangeHandler<Functor>(*this, f);
 }
 
-template <typename T, auto Callback>
+template <typename T, auto Callback, auto ValueGuard>
 template<typename Functor>
-QPropertyChangeHandler<Functor> QNotifiedProperty<T, Callback>::subscribe(Functor f)
+QPropertyChangeHandler<Functor> QNotifiedProperty<T, Callback, ValueGuard>::subscribe(Functor f)
 {
 #if defined(__cpp_lib_is_invocable) && (__cpp_lib_is_invocable >= 201703L)
     static_assert(std::is_invocable_v<Functor>, "Functor callback must be callable without any parameters");

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());

src/corelib/kernel/qpropertybinding_p.h

@@ -81,6 +81,7 @@ private:
         struct {
             void *staticObserver;
             void (*staticObserverCallback)(void*, void*);
+            bool (*staticGuardCallback)(void*, void*);
         };
     };
     QScopedPointer<std::vector<QPropertyObserver>> 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();

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();

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<int> 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<int>(0, newValue, 100); return true; }
+
+    QNotifiedProperty<int, &This::callback, &This::trivialGuard> property;
+    QNotifiedProperty<int, &This::callback2, &This::reject42> property2;
+    QNotifiedProperty<int, &This::callback3, &This::bound> property3;
+};
+
+void tst_QProperty::notifiedPropertyWithGuard()
+{
+    {
+        // property with guard that returns always true is the same as using no guard
+        ClassWithNotifiedPropertyWithGuard instance;
+
+        std::array<QProperty<int>, 5> otherProperties = {
+            QProperty<int>([&]() { return instance.property + 1; }),
+            QProperty<int>([&]() { return instance.property + 2; }),
+            QProperty<int>([&]() { return instance.property + 3; }),
+            QProperty<int>([&]() { return instance.property + 4; }),
+            QProperty<int>([&]() { 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<int> injectedValue(100);
+        instance.property.setBinding(&instance, [&injectedValue]() { return injectedValue.value(); });
+        auto subscriber = [&] { ++subscribedCount; };
+        std::array<QPropertyChangeHandler<decltype (subscriber)>, 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<int> 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"