Port QPropertyAnimation::targetObject to bindable properties

This is one of the more complicated ports. The target object
was represented by two variables in the past: A raw pointer and a
QPointer. The QPointer is checked in some cases to check whether
the target object still exists.
This patch introduces a targetObjectDestroyed() slot and connects
it to the destroyed(QObject*) signal of the target object.
In this slot, the animation is stopped. The checks become
obsolete thereby and it is sufficient to represent the target
Object in one raw pointer.
This raw pointer becomes a bindable property.

Fixes: QTBUG-92992
Change-Id: I7e2ddb5d8aed007400fe74bea1becf7bdfbf2563
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Sona Kurazyan <sona.kurazyan@qt.io>

src/corelib/animation/qpropertyanimation.cpp

@@ -91,7 +91,7 @@ QT_BEGIN_NAMESPACE
 
 void QPropertyAnimationPrivate::updateMetaProperty()
 {
-    if (!target || propertyName.value().isEmpty()) {
+    if (!targetObject || propertyName.value().isEmpty()) {
         propertyType = QMetaType::UnknownType;
         propertyIndex = -1;
         return;
@@ -99,19 +99,19 @@ void QPropertyAnimationPrivate::updateMetaProperty()
 
     //propertyType will be set to a valid type only if there is a Q_PROPERTY
     //otherwise it will be set to QVariant::Invalid at the end of this function
-    propertyType = targetValue->property(propertyName.value()).userType();
+    propertyType = targetObject->property(propertyName.value()).userType();
-    propertyIndex = targetValue->metaObject()->indexOfProperty(propertyName.value());
+    propertyIndex = targetObject->metaObject()->indexOfProperty(propertyName.value());
 
     if (propertyType != QMetaType::UnknownType)
         convertValues(propertyType);
     if (propertyIndex == -1) {
         //there is no Q_PROPERTY on the object
         propertyType = QMetaType::UnknownType;
-        if (!targetValue->dynamicPropertyNames().contains(propertyName))
+        if (!targetObject->dynamicPropertyNames().contains(propertyName))
             qWarning("QPropertyAnimation: you're trying to animate a non-existing property %s of "
                      "your QObject",
                      propertyName.value().constData());
-    } else if (!targetValue->metaObject()->property(propertyIndex).isWritable()) {
+    } else if (!targetObject->metaObject()->property(propertyIndex).isWritable()) {
         qWarning("QPropertyAnimation: you're trying to animate the non-writable property %s of "
                  "your QObject",
                  propertyName.value().constData());
@@ -123,10 +123,8 @@ void QPropertyAnimationPrivate::updateProperty(const QVariant &newValue)
     if (state == QAbstractAnimation::Stopped)
         return;
 
-    if (!target) {
+    if (!targetObject)
-        q_func()->stop(); //the target was destroyed we need to stop the animation
         return;
-    }
 
     if (newValue.userType() == propertyType) {
         //no conversion is needed, we directly call the QMetaObject::metacall
@@ -134,9 +132,9 @@ void QPropertyAnimationPrivate::updateProperty(const QVariant &newValue)
         int status = -1;
         int flags = 0;
         void *argv[] = { const_cast<void *>(newValue.constData()), const_cast<QVariant *>(&newValue), &status, &flags };
-        QMetaObject::metacall(targetValue, QMetaObject::WriteProperty, propertyIndex, argv);
+        QMetaObject::metacall(targetObject, QMetaObject::WriteProperty, propertyIndex, argv);
     } else {
-        targetValue->setProperty(propertyName.value().constData(), newValue);
+        targetObject->setProperty(propertyName.value().constData(), newValue);
     }
 }
 
@@ -179,22 +177,36 @@ QPropertyAnimation::~QPropertyAnimation()
  */
 QObject *QPropertyAnimation::targetObject() const
 {
-    return d_func()->target.data();
+    return d_func()->targetObject;
+}
+
+QBindable<QObject *> QPropertyAnimation::bindableTargetObject()
+{
+    return &d_func()->targetObject;
 }
 
 void QPropertyAnimation::setTargetObject(QObject *target)
 {
     Q_D(QPropertyAnimation);
-    if (d->target.data() == target)
-        return;
-
     if (d->state != QAbstractAnimation::Stopped) {
         qWarning("QPropertyAnimation::setTargetObject: you can't change the target of a running animation");
         return;
     }
 
-    d->target = d->targetValue = target;
+    d->targetObject.removeBindingUnlessInWrapper();
+    if (d->targetObject == target)
+        return;
+
+    if (d->targetObject != nullptr)
+        QObject::disconnect(d->targetObject, &QObject::destroyed, this, nullptr);
+    d->targetObject.setValueBypassingBindings(target);
+
+    if (d->targetObject != nullptr) {
+        QObject::connect(d->targetObject, &QObject::destroyed, this,
+                         [d] { d->targetObjectDestroyed(); });
+    }
     d->updateMetaProperty();
+    d->targetObject.notify();
 }
 
 /*!
@@ -265,7 +277,7 @@ void QPropertyAnimation::updateState(QAbstractAnimation::State newState,
 {
     Q_D(QPropertyAnimation);
 
-    if (!d->target && oldState == Stopped) {
+    if (!d->targetObject && oldState == Stopped) {
         qWarning("QPropertyAnimation::updateState (%s): Changing state of an animation without "
                  "target",
                  d->propertyName.value().constData());
@@ -281,9 +293,16 @@ void QPropertyAnimation::updateState(QAbstractAnimation::State newState,
         typedef QPair<QObject *, QByteArray> QPropertyAnimationPair;
         typedef QHash<QPropertyAnimationPair, QPropertyAnimation*> QPropertyAnimationHash;
         static QPropertyAnimationHash hash;
-        //here we need to use value because we need to know to which pointer
+
-        //the animation was referring in case stopped because the target was destroyed
+        // in case the targetObject gets deleted, the following happens:
-        QPropertyAnimationPair key(d->targetValue, d->propertyName);
+        // 1. targetObject's destroyed signal calls our targetObjectDestroyed.
+        // 2. targetObjectDestroyed calls stop()
+        // 3. QAbstractAnimation::stop() calls setState(Stopped)
+        // 4. setState(Stopped) calls updateState(newState, oldState)
+        // 5. we arrive here. d->targetObject is not yet set to nullptr, we can safely use it.
+        Q_ASSERT(d->targetObject);
+
+        QPropertyAnimationPair key(d->targetObject, d->propertyName);
         if (newState == Running) {
             d->updateMetaProperty();
             animToStop = hash.value(key, nullptr);
@@ -292,7 +311,7 @@ void QPropertyAnimation::updateState(QAbstractAnimation::State newState,
             // update the default start value
             if (oldState == Stopped) {
                 d->setDefaultStartEndValue(
-                        d->targetValue->property(d->propertyName.value().constData()));
+                        d->targetObject->property(d->propertyName.value().constData()));
                 //let's check if we have a start value and an end value
                 const char *what = nullptr;
                 if (!startValue().isValid() && (d->direction == Backward || !d->defaultStartEndValue.isValid())) {
@@ -308,8 +327,8 @@ void QPropertyAnimation::updateState(QAbstractAnimation::State newState,
                     qWarning("QPropertyAnimation::updateState (%s, %s, %ls): starting an animation "
                              "without %s value",
                              d->propertyName.value().constData(),
-                             d->target.data()->metaObject()->className(),
+                             d->targetObject->metaObject()->className(),
-                             qUtf16Printable(d->target.data()->objectName()), what);
+                             qUtf16Printable(d->targetObject->objectName()), what);
                 }
             }
         } else if (hash.value(key) == this) {

src/corelib/animation/qpropertyanimation.h

@@ -50,9 +50,10 @@ class QPropertyAnimationPrivate;
 class Q_CORE_EXPORT QPropertyAnimation : public QVariantAnimation
 {
     Q_OBJECT
-    Q_PROPERTY(QByteArray propertyName READ propertyName WRITE setPropertyName BINDABLE
+    Q_PROPERTY(QByteArray propertyName READ propertyName WRITE setPropertyName
-                       bindablePropertyName)
+                        BINDABLE bindablePropertyName)
-    Q_PROPERTY(QObject* targetObject READ targetObject WRITE setTargetObject)
+    Q_PROPERTY(QObject* targetObject READ targetObject WRITE setTargetObject
+                        BINDABLE bindableTargetObject)
 
 public:
     QPropertyAnimation(QObject *parent = nullptr);
@@ -61,6 +62,7 @@ public:
 
     QObject *targetObject() const;
     void setTargetObject(QObject *target);
+    QBindable<QObject *> bindableTargetObject();
 
     QByteArray propertyName() const;
     void setPropertyName(const QByteArray &propertyName);

src/corelib/animation/qpropertyanimation_p.h

@@ -64,14 +64,20 @@ class QPropertyAnimationPrivate : public QVariantAnimationPrivate
 {
    Q_DECLARE_PUBLIC(QPropertyAnimation)
 public:
-    QPropertyAnimationPrivate()
+    QPropertyAnimationPrivate() : propertyType(0), propertyIndex(-1) { }
-        : targetValue(nullptr), propertyType(0), propertyIndex(-1)
-    {
-    }
 
-    QPointer<QObject> target;
+    void setTargetObjectForwarder(QObject *target) { q_func()->setTargetObject(target); }
-    //we use targetValue to be able to unregister the target from the global hash
+    Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS(QPropertyAnimationPrivate, QObject *, targetObject,
-    QObject *targetValue;
+                                       &QPropertyAnimationPrivate::setTargetObjectForwarder,
+                                       nullptr)
+    void targetObjectDestroyed()
+    {
+        // stop() has to be called before targetObject is set to nullptr.
+        // targetObject must not be nullptr in states unequal to "Stopped".
+        q_func()->stop();
+        targetObject.setValueBypassingBindings(nullptr);
+        targetObject.notify();
+    }
 
     //for the QProperty
     int propertyType;

tests/auto/corelib/animation/qpropertyanimation/tst_qpropertyanimation.cpp

@@ -1403,11 +1403,26 @@ void tst_QPropertyAnimation::recursiveAnimations()
 
 void tst_QPropertyAnimation::bindings()
 {
-    AnimationObject o;
+    std::unique_ptr<AnimationObject> o1(new AnimationObject);
-    QPropertyAnimation a(&o, "value");
+    std::unique_ptr<AnimationObject> o2(new AnimationObject);
+    QPropertyAnimation a(o1.get(), "value");
 
-    QTestPrivate::testReadWritePropertyBasics(a, QByteArray("value"), QByteArray("realValue"),
+    QTestPrivate::testReadWritePropertyBasics<QPropertyAnimation, QByteArray>(
-                                              "propertyName");
+            a, QByteArray("realValue"), QByteArray("value"), "propertyName");
+    if (QTest::currentTestFailed()) {
+        qDebug() << "Failed property test for QPropertyAnimation::propertyName";
+        return;
+    }
+    QTestPrivate::testReadWritePropertyBasics<QPropertyAnimation, QObject *>(a, o2.get(), o1.get(),
+                                                                             "targetObject");
+    if (QTest::currentTestFailed()) {
+        qDebug() << "Failed property test for QPropertyAnimation::targetObject";
+        return;
+    }
+
+    a.setTargetObject(o1.get());
+    o1.reset();
+    QCOMPARE(a.targetObject(), nullptr);
 }
 
 QTEST_MAIN(tst_QPropertyAnimation)