From a31dde22dbb37643dcabab12b2ac4e2f1d79f958 Mon Sep 17 00:00:00 2001
From: Fabian Kosmale <fabian.kosmale@qt.io>
Date: Fri, 23 Oct 2020 15:40:38 +0200
Subject: [PATCH] QProperty: Fix notification logic for eager properties

This ensurse that we do not do dobule notifications in setValue.
Moerover we avoid needless notifications in markDirtyAndNotifyObservers
when the value did not change. Lastly, if the value did actually change,
we pass that information along to notify, so that we do not evaluate the
eager property twice.
Fixes a test-case which errorneously relied on the old behavior, and
adds a new test which verifies that the fix works.

Change-Id: I8ec6fa2fe8611565dfc603ceab3ba5f92999b26c
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
---
 src/corelib/kernel/qproperty.cpp              |  7 +++--
 src/corelib/kernel/qproperty_p.h              |  6 ++--
 .../kernel/qproperty/tst_qproperty.cpp        | 29 ++++++++++++++++++-
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp
index 103c3d8482..147103aed9 100644
--- a/src/corelib/kernel/qproperty.cpp
+++ b/src/corelib/kernel/qproperty.cpp
@@ -110,12 +110,15 @@ void QPropertyBindingPrivate::markDirtyAndNotifyObservers()
 
     eagerlyUpdating = true;
     QScopeGuard guard([&](){eagerlyUpdating = false;});
+    bool knownIfChanged = false;
     if (requiresEagerEvaluation()) {
         // these are compat properties that we will need to evaluate eagerly
-        evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr);
+        if (!evaluateIfDirtyAndReturnTrueIfValueChanged(propertyDataPtr))
+            return;
+        knownIfChanged = true;
     }
     if (firstObserver)
-        firstObserver.notify(this, propertyDataPtr);
+        firstObserver.notify(this, propertyDataPtr, knownIfChanged);
     if (hasStaticObserver)
         staticObserverCallback(propertyDataPtr);
 }
diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h
index 99a145c65a..9a0daaee3a 100644
--- a/src/corelib/kernel/qproperty_p.h
+++ b/src/corelib/kernel/qproperty_p.h
@@ -422,13 +422,15 @@ public:
         QBindingStorage *storage = qGetBindingStorage(owner());
         auto *bd = storage->bindingData(this);
         // make sure we don't remove the binding if called from the bindingWrapper
-        if (bd && !inBindingWrapper(storage))
+        const bool inWrapper = inBindingWrapper(storage);
+        if (bd && !inWrapper)
             bd->removeBinding();
         if constexpr (QTypeTraits::has_operator_equal_v<T>)
             if (this->val == t)
                 return;
         this->val = t;
-        notify(bd);
+        if (!inWrapper)
+            notify(bd);
     }
 
     QObjectCompatProperty &operator=(parameter_type newValue)
diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
index 472ba6031f..31807bf1a7 100644
--- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
+++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp
@@ -86,6 +86,7 @@ private slots:
     void aliasOnMetaProperty();
 
     void modifyObserverListWhileIterating();
+    void compatPropertyNoDobuleNotification();
 };
 
 void tst_QProperty::functorBinding()
@@ -552,11 +553,12 @@ void tst_QProperty::realloc()
         QCOMPARE(tester.prop1(), 12);
 
         tester.bindableProp1().setBinding([&](){return tester.prop5();});
+        QCOMPARE(modificationCount, 2);
         tester.bindableProp2().setBinding([&](){return tester.prop5();});
         tester.bindableProp3().setBinding([&](){return tester.prop5();});
         tester.bindableProp4().setBinding([&](){return tester.prop5();});
         tester.bindableProp5().setBinding([&]() -> int{return 42;});
-        QCOMPARE(modificationCount, 2);
+        QCOMPARE(modificationCount, 3);
     }
 };
 
@@ -1326,6 +1328,31 @@ void tst_QProperty::modifyObserverListWhileIterating()
     }
 }
 
+class CompatPropertyTester : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(int prop1 READ prop1 WRITE setProp1 BINDABLE bindableProp1)
+    public:
+    CompatPropertyTester(QObject *parent = nullptr) : QObject(parent) { }
+
+    int prop1() {return prop1Data.value();}
+    void setProp1(int i) { prop1Data = i; }
+    QBindable<int> bindableProp1() {return QBindable<int>(&prop1Data);}
+    Q_OBJECT_COMPAT_PROPERTY(CompatPropertyTester, int, prop1Data, &CompatPropertyTester::setProp1)
+
+};
+
+void tst_QProperty::compatPropertyNoDobuleNotification()
+{
+    CompatPropertyTester tester;
+    int counter = 0;
+    QProperty<int> iprop {1};
+    tester.bindableProp1().setBinding([&]() -> int {return iprop;});
+    auto observer = tester.bindableProp1().onValueChanged([&](){++counter;});
+    iprop.setValue(2);
+    QCOMPARE(counter, 1);
+}
+
 QTEST_MAIN(tst_QProperty);
 
 #include "tst_qproperty.moc"