From 2ffb91ac592d69adf9458ac45074174537435918 Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Wed, 3 Feb 2021 12:02:29 +0100 Subject: [PATCH 1/3] QObjectCompatProperty: Require explicit notify For QObjectCompatProperty, which allows to do basically anything in its setter, it is actually easier to manually specify when the change should become visible. This is in line with manually writing emit calls in the old property system, and allows the preservation of class invariants. Change-Id: I585bd3f25d722ca3fd721ead85fe73dbee26c5f6 Reviewed-by: Ulf Hermann --- src/corelib/animation/qpauseanimation.cpp | 1 + src/corelib/kernel/qproperty_p.h | 5 ----- src/corelib/kernel/qtimer.cpp | 9 +++------ src/gui/image/qmovie.cpp | 1 + .../corelib/kernel/qproperty/tst_qproperty.cpp | 15 ++++++++------- 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/corelib/animation/qpauseanimation.cpp b/src/corelib/animation/qpauseanimation.cpp index 31802f094dc..c2599da6921 100644 --- a/src/corelib/animation/qpauseanimation.cpp +++ b/src/corelib/animation/qpauseanimation.cpp @@ -130,6 +130,7 @@ void QPauseAnimation::setDuration(int msecs) } Q_D(QPauseAnimation); d->duration.setValue(msecs); + d->duration.notify(); } QBindable QPauseAnimation::bindableDuration() diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index d1f4c5acba1..a355a7faea5 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -467,12 +467,7 @@ public: const bool inWrapper = inBindingWrapper(storage); if (bd && !inWrapper) bd->removeBinding(); - if constexpr (QTypeTraits::has_operator_equal_v) - if (this->val == t) - return; this->val = t; - if (!inWrapper) - notify(bd); } QPropertyBinding setBinding(const QPropertyBinding &newBinding) diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp index 49c57cb6eeb..0946e1af485 100644 --- a/src/corelib/kernel/qtimer.cpp +++ b/src/corelib/kernel/qtimer.cpp @@ -257,10 +257,9 @@ void QTimer::start() void QTimer::start(int msec) { Q_D(QTimer); - d->inter.removeBindingUnlessInWrapper(); - d->inter.setValueBypassingBindings(msec); + d->inter.setValue(msec); start(); - d->inter.markDirty(); + d->inter.notify(); } @@ -754,9 +753,7 @@ QBindable QTimer::bindableSingleShot() void QTimer::setInterval(int msec) { Q_D(QTimer); - d->inter.removeBindingUnlessInWrapper(); - - d->inter.setValueBypassingBindings(msec); + d->inter.setValue(msec); if (d->id != INV_TIMER) { // create new timer QObject::killTimer(d->id); // restart timer d->id = QObject::startTimer(msec, d->type); diff --git a/src/gui/image/qmovie.cpp b/src/gui/image/qmovie.cpp index a98a4ed154b..a293d358cf6 100644 --- a/src/gui/image/qmovie.cpp +++ b/src/gui/image/qmovie.cpp @@ -930,6 +930,7 @@ void QMovie::setSpeed(int percentSpeed) if (!d->speed && d->movieState == Running) d->nextImageTimer.start(nextFrameDelay()); d->speed.setValue(percentSpeed); + d->speed.notify(); } int QMovie::speed() const diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 6062c231892..4186607d905 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -469,12 +469,12 @@ class BindingLoopTester : public QObject BindingLoopTester() {} int eagerProp() {return eagerData.value();} - void setEagerProp(int i) { eagerData.setValue(i); } + void setEagerProp(int i) { eagerData.setValue(i); eagerData.notify(); } QBindable bindableEagerProp() {return QBindable(&eagerData);} Q_OBJECT_COMPAT_PROPERTY(BindingLoopTester, int, eagerData, &BindingLoopTester::setEagerProp) int eagerProp2() {return eagerData2.value();} - void setEagerProp2(int i) { eagerData2.setValue(i); } + void setEagerProp2(int i) { eagerData2.setValue(i); eagerData2.notify(); } QBindable bindableEagerProp2() {return QBindable(&eagerData2);} Q_OBJECT_COMPAT_PROPERTY(BindingLoopTester, int, eagerData2, &BindingLoopTester::setEagerProp2) }; @@ -536,7 +536,7 @@ public: #define GEN(N) \ int prop##N() {return propData##N.value();} \ - void setProp##N(int i) { propData##N.setValue(i); } \ + void setProp##N(int i) { if (i == propData##N) return; propData##N.setValue(i); propData##N.notify(); } \ QBindable bindableProp##N() {return QBindable(&propData##N);} \ Q_OBJECT_COMPAT_PROPERTY(ReallocTester, int, propData##N, &ReallocTester::setProp##N) GEN(1) @@ -1064,6 +1064,7 @@ public: if (i < 0) i = 0; compatData.setValue(i); + compatData.notify(); emit compatChanged(); } @@ -1390,7 +1391,7 @@ class CompatPropertyTester : public QObject CompatPropertyTester(QObject *parent = nullptr) : QObject(parent) { } int prop1() {return prop1Data.value();} - void setProp1(int i) { prop1Data.setValue(i); } + void setProp1(int i) { if (i == prop1Data) return; prop1Data.setValue(i); prop1Data.notify(); } QBindable bindableProp1() {return QBindable(&prop1Data);} Q_OBJECT_COMPAT_PROPERTY(CompatPropertyTester, int, prop1Data, &CompatPropertyTester::setProp1) @@ -1421,9 +1422,9 @@ signals: void prop3Changed(); public: - void setProp1(int val) { prop1Data.setValue(val); emit prop1Changed();} - void setProp2(int val) { prop2Data.setValue(val); emit prop2Changed();} - void setProp3(int val) { prop3Data.setValue(val); emit prop3Changed();} + void setProp1(int val) { prop1Data.setValue(val); prop1Data.notify(); emit prop1Changed();} + void setProp2(int val) { prop2Data.setValue(val); prop2Data.notify(); emit prop2Changed();} + void setProp3(int val) { prop3Data.setValue(val); prop3Data.notify(); emit prop3Changed();} int prop1() { return prop1Data; } int prop2() { return prop2Data; } From 4ceaf22bed1bc0ed2dec4628fd9d9814c0e0fd86 Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Wed, 3 Feb 2021 15:25:37 +0100 Subject: [PATCH 2/3] QObjectCompatProperty: Emit signal in notfiy There is no need to write emit and notify at the same time, as not emitting after notify does not make sense. This naturally only applies to properties with a changed signal. Change-Id: I99ff7863a509262ad9d4f7c9c5afbc66fd37001c Reviewed-by: Ulf Hermann --- src/corelib/kernel/qproperty.h | 2 +- src/corelib/kernel/qproperty_p.h | 59 +++++++++++++++---- .../kernel/qproperty/tst_qproperty.cpp | 12 ++-- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 6672f270f47..a085aac3d15 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -844,7 +844,7 @@ class Q_CORE_EXPORT QBindingStorage mutable QBindingStorageData *d = nullptr; QBindingStatus *bindingStatus = nullptr; - template + template friend class QObjectCompatProperty; public: QBindingStorage(); diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index a355a7faea5..b4d41a362b2 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -385,10 +385,11 @@ inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() con return { reinterpret_cast(ptr->d_ptr) }; } -template +template class QObjectCompatProperty : public QPropertyData { - using ThisType = QObjectCompatProperty; + using ThisType = QObjectCompatProperty; + using SignalTakesValue = std::is_invocable; Class *owner() { char *that = reinterpret_cast(this); @@ -523,6 +524,21 @@ public: bd->removeBinding(); } + void notify() + { + QBindingStorage *storage = qGetBindingStorage(owner()); + auto bd = storage->bindingData(this, false); + const bool inWrapper = inBindingWrapper(storage); + if (bd && !inWrapper) + notify(bd); + if constexpr (Signal != nullptr) { + if constexpr (SignalTakesValue::value) + (owner()->*Signal)(value()); + else + (owner()->*Signal)(); + } + } + QPropertyBinding binding() const { auto *bd = qGetBindingStorage(owner())->bindingData(this); @@ -562,24 +578,45 @@ private: } }; -#define Q_OBJECT_COMPAT_PROPERTY(Class, Type, name, setter) \ +#define Q_OBJECT_COMPAT_PROPERTY4(Class, Type, name, setter) \ static constexpr size_t _qt_property_##name##_offset() { \ - QT_WARNING_PUSH QT_WARNING_DISABLE_INVALID_OFFSETOF \ return offsetof(Class, name); \ - QT_WARNING_POP \ } \ QObjectCompatProperty name; -#define Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS(Class, Type, name, setter, value) \ - static constexpr size_t _qt_property_##name##_offset() \ - { \ - QT_WARNING_PUSH QT_WARNING_DISABLE_INVALID_OFFSETOF return offsetof(Class, name); \ - QT_WARNING_POP \ - } \ +#define Q_OBJECT_COMPAT_PROPERTY5(Class, Type, name, setter, signal) \ + static constexpr size_t _qt_property_##name##_offset() { \ + return offsetof(Class, name); \ + } \ + QObjectCompatProperty name; + +#define Q_OBJECT_COMPAT_PROPERTY(...) \ + QT_WARNING_PUSH QT_WARNING_DISABLE_INVALID_OFFSETOF \ + QT_OVERLOADED_MACRO(Q_OBJECT_COMPAT_PROPERTY, __VA_ARGS__) \ + QT_WARNING_POP + +#define Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS5(Class, Type, name, setter, value) \ + static constexpr size_t _qt_property_##name##_offset() { \ + return offsetof(Class, name); \ + } \ QObjectCompatProperty name = \ QObjectCompatProperty( \ value); +#define Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS6(Class, Type, name, setter, signal, value) \ + static constexpr size_t _qt_property_##name##_offset() { \ + return offsetof(Class, name); \ + } \ + QObjectCompatProperty name = \ + QObjectCompatProperty( \ + value); + +#define Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS(...) \ + QT_WARNING_PUSH QT_WARNING_DISABLE_INVALID_OFFSETOF \ + QT_OVERLOADED_MACRO(Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS, __VA_ARGS__) \ + QT_WARNING_POP + + namespace QtPrivate { Q_CORE_EXPORT BindingEvaluationState *suspendCurrentBindingStatus(); Q_CORE_EXPORT void restoreBindingStatus(BindingEvaluationState *status); diff --git a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp index 4186607d905..5ca5fcf9353 100644 --- a/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp +++ b/tests/auto/corelib/kernel/qproperty/tst_qproperty.cpp @@ -1422,9 +1422,9 @@ signals: void prop3Changed(); public: - void setProp1(int val) { prop1Data.setValue(val); prop1Data.notify(); emit prop1Changed();} - void setProp2(int val) { prop2Data.setValue(val); prop2Data.notify(); emit prop2Changed();} - void setProp3(int val) { prop3Data.setValue(val); prop3Data.notify(); emit prop3Changed();} + void setProp1(int val) { prop1Data.setValue(val); prop1Data.notify();} + void setProp2(int val) { prop2Data.setValue(val); prop2Data.notify();} + void setProp3(int val) { prop3Data.setValue(val); prop3Data.notify();} int prop1() { return prop1Data; } int prop2() { return prop2Data; } @@ -1435,9 +1435,9 @@ public: QBindable bindableProp3() { return QBindable(&prop3Data); } private: - Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop1Data, &FakeDependencyCreator::setProp1); - Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop2Data, &FakeDependencyCreator::setProp2); - Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop3Data, &FakeDependencyCreator::setProp3); + Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop1Data, &FakeDependencyCreator::setProp1, &FakeDependencyCreator::prop1Changed); + Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop2Data, &FakeDependencyCreator::setProp2, &FakeDependencyCreator::prop2Changed); + Q_OBJECT_COMPAT_PROPERTY(FakeDependencyCreator, int, prop3Data, &FakeDependencyCreator::setProp3, &FakeDependencyCreator::prop3Changed); }; void tst_QProperty::noFakeDependencies() From 29ef667a69378c992e31065c6f24c8fe227aaada Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 5 Feb 2021 15:02:35 +0100 Subject: [PATCH 3/3] QObjectCompatProperty: Reintroduce operator= This is a partial revert of a1a2d97e34eb7e2445877cf9e557db55a3540f9d. Reason: The new design does not notify automatically anymore, so using operator= is safe to use. Change-Id: I6cb735e40c0da72d22fcc426423eb7830901e5f4 Reviewed-by: Qt CI Bot Reviewed-by: Lars Knoll --- src/corelib/kernel/qproperty_p.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index b4d41a362b2..d0a28822939 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -471,6 +471,12 @@ public: this->val = t; } + QObjectCompatProperty &operator=(parameter_type newValue) + { + setValue(newValue); + return *this; + } + QPropertyBinding setBinding(const QPropertyBinding &newBinding) { QtPrivate::QPropertyBindingData *bd = qGetBindingStorage(owner())->bindingData(this, true);