QML: Fix interceptors on value types ignoring fast changes

If a property is changed and reverted in short order, any animation
attached to it may not get a chance to take effect in between. In such a
case it looks like we don't have to update the interceptor when
reverting, but we actually have to. The animation needs to be canceled,
after all.

We now have to fix the case of writing different properties of a value
type sequentially, where one has an animation attached to it, though. If
that happens, we cannot drop the animation when a _different_ property
is written later on, but we do still have to update the whole value
type. So, pass an additional argument in the relevant metacalls that
declares the property we intended to change.

Pick-to: 6.5
Fixes: QTBUG-54860
Change-Id: I3b6cad2d4707d30312cda98283928fd419c40345
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

10 files changed, 139 insertions, 77 deletions

diff --git a/src/qml/jsruntime/qv4referenceobject_p.h b/src/qml/jsruntime/qv4referenceobject_p.h
index ad6a7b5817..5378a0c362 100644
--- a/src/qml/jsruntime/qv4referenceobject_p.h
+++ b/src/qml/jsruntime/qv4referenceobject_p.h
@@ -110,6 +110,8 @@ struct ReferenceObject : public Object
     V4_NEEDS_DESTROY
 
 public:
+    static constexpr const int AllProperties = -1;
+
     template<typename HeapObject>
     static bool readReference(HeapObject *ref)
     {
@@ -131,7 +133,7 @@ public:
     }
 
     template<typename HeapObject>
-    static bool writeBack(HeapObject *ref)
+    static bool writeBack(HeapObject *ref, int internalIndex = AllProperties)
     {
         if (!ref->object() || !ref->canWriteBack())
             return false;
@@ -139,15 +141,15 @@ public:
         QV4::Scope scope(ref->internalClass->engine);
         QV4::ScopedObject object(scope, ref->object());
 
-        int flags = 0;
+        int flags = QQmlPropertyData::HasInternalIndex;
         int status = -1;
         if (ref->isVariant()) {
             QVariant variant = ref->toVariant();
-            void *a[] = { &variant, nullptr, &status, &flags };
+            void *a[] = { &variant, nullptr, &status, &flags, &internalIndex };
             return object->metacall(QMetaObject::WriteProperty, ref->property(), a);
         }
 
-        void *a[] = { ref->storagePointer(), nullptr, &status, &flags };
+        void *a[] = { ref->storagePointer(), nullptr, &status, &flags, &internalIndex };
         return object->metacall(QMetaObject::WriteProperty, ref->property(), a);
     }
 
diff --git a/src/qml/qml/qqmlproperty.cpp b/src/qml/qml/qqmlproperty.cpp
index cfdcc9ea60..18e2c417ff 100644
--- a/src/qml/qml/qqmlproperty.cpp
+++ b/src/qml/qml/qqmlproperty.cpp
@@ -1271,29 +1271,31 @@ static void removeValuePropertyBinding(
 template<typename Op>
 bool changePropertyAndWriteBack(
             QObject *object, int coreIndex, QQmlGadgetPtrWrapper *wrapper,
-            QQmlPropertyData::WriteFlags flags, Op op)
+            QQmlPropertyData::WriteFlags flags, int internalIndex, Op op)
 {
     wrapper->read(object, coreIndex);
     const bool rv = op(wrapper);
-    wrapper->write(object, coreIndex, flags);
+    wrapper->write(object, coreIndex, flags, internalIndex);
     return rv;
 }
 
 template<typename Op>
 bool changeThroughGadgetPtrWrapper(
-            QObject *object, const QQmlPropertyData &core,
-            const QQmlRefPointer<QQmlContextData> &context, QQmlPropertyData::WriteFlags flags,
-            Op op)
+        QObject *object, const QQmlPropertyData &core,
+        const QQmlRefPointer<QQmlContextData> &context, QQmlPropertyData::WriteFlags flags,
+        int internalIndex, Op op)
 {
     if (QQmlGadgetPtrWrapper *wrapper = context
             ? QQmlGadgetPtrWrapper::instance(context->engine(), core.propType())
             : nullptr) {
-        return changePropertyAndWriteBack(object, core.coreIndex(), wrapper, flags, op);
+        return changePropertyAndWriteBack(
+                    object, core.coreIndex(), wrapper, flags, internalIndex, op);
     }
 
     if (QQmlValueType *valueType = QQmlMetaType::valueType(core.propType())) {
         QQmlGadgetPtrWrapper wrapper(valueType, nullptr);
-        return changePropertyAndWriteBack(object, core.coreIndex(), &wrapper, flags, op);
+        return changePropertyAndWriteBack(
+                    object, core.coreIndex(), &wrapper, flags, internalIndex, op);
     }
 
     return false;
@@ -1309,8 +1311,9 @@ bool QQmlPropertyPrivate::writeValueProperty(
     if (!valueTypeData.isValid())
         return write(object, core, value, context, flags);
 
-    return changeThroughGadgetPtrWrapper(object, core, context, flags,
-                                         [&](QQmlGadgetPtrWrapper *wrapper) {
+    return changeThroughGadgetPtrWrapper(
+                object, core, context, flags | QQmlPropertyData::HasInternalIndex,
+                valueTypeData.coreIndex(), [&](QQmlGadgetPtrWrapper *wrapper) {
         return write(wrapper, valueTypeData, value, context, flags);
     });
 }
@@ -1324,8 +1327,9 @@ bool QQmlPropertyPrivate::resetValueProperty(
     if (!valueTypeData.isValid())
         return reset(object, core, flags);
 
-    return changeThroughGadgetPtrWrapper(object, core, context, flags,
-                                         [&](QQmlGadgetPtrWrapper *wrapper) {
+    return changeThroughGadgetPtrWrapper(
+                object, core, context, flags | QQmlPropertyData::HasInternalIndex,
+                valueTypeData.coreIndex(), [&](QQmlGadgetPtrWrapper *wrapper) {
         return reset(wrapper, valueTypeData, flags);
     });
 }
diff --git a/src/qml/qml/qqmlpropertydata_p.h b/src/qml/qml/qqmlpropertydata_p.h
index b51b0ba619..19a045107d 100644
--- a/src/qml/qml/qqmlpropertydata_p.h
+++ b/src/qml/qml/qqmlpropertydata_p.h
@@ -28,7 +28,8 @@ public:
     enum WriteFlag {
         BypassInterceptor = 0x01,
         DontRemoveBinding = 0x02,
-        RemoveBindingOnAliasWrite = 0x04
+        RemoveBindingOnAliasWrite = 0x04,
+        HasInternalIndex = 0x8,
     };
     Q_DECLARE_FLAGS(WriteFlags, WriteFlag)
 
diff --git a/src/qml/qml/qqmlvaluetype.cpp b/src/qml/qml/qqmlvaluetype.cpp
index f234cb1312..428ce9b697 100644
--- a/src/qml/qml/qqmlvaluetype.cpp
+++ b/src/qml/qml/qqmlvaluetype.cpp
@@ -43,11 +43,12 @@ void QQmlGadgetPtrWrapper::read(QObject *obj, int idx)
     QMetaObject::metacall(obj, QMetaObject::ReadProperty, idx, a);
 }
 
-void QQmlGadgetPtrWrapper::write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags) const
+void QQmlGadgetPtrWrapper::write(
+        QObject *obj, int idx, QQmlPropertyData::WriteFlags flags, int internalIndex) const
 {
     Q_ASSERT(m_gadgetPtr);
     int status = -1;
-    void *a[] = { m_gadgetPtr, nullptr, &status, &flags };
+    void *a[] = { m_gadgetPtr, nullptr, &status, &flags, &internalIndex };
     QMetaObject::metacall(obj, QMetaObject::WriteProperty, idx, a);
 }
 
diff --git a/src/qml/qml/qqmlvaluetype_p.h b/src/qml/qml/qqmlvaluetype_p.h
index 779ba01f12..bc7a210cdf 100644
--- a/src/qml/qml/qqmlvaluetype_p.h
+++ b/src/qml/qml/qqmlvaluetype_p.h
@@ -19,6 +19,7 @@
 
 #include <private/qqmlnullablevalue_p.h>
 #include <private/qmetatype_p.h>
+#include <private/qv4referenceobject_p.h>
 
 #include <QtCore/qobject.h>
 #include <QtCore/qrect.h>
@@ -69,7 +70,8 @@ public:
     ~QQmlGadgetPtrWrapper();
 
     void read(QObject *obj, int idx);
-    void write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags) const;
+    void write(QObject *obj, int idx, QQmlPropertyData::WriteFlags flags,
+               int internalIndex = QV4::ReferenceObject::AllProperties) const;
     QVariant value() const;
     void setValue(const QVariant &value);
 
diff --git a/src/qml/qml/qqmlvaluetypewrapper.cpp b/src/qml/qml/qqmlvaluetypewrapper.cpp
index 5ad5747e7f..4ef757b8e1 100644
--- a/src/qml/qml/qqmlvaluetypewrapper.cpp
+++ b/src/qml/qml/qqmlvaluetypewrapper.cpp
@@ -118,9 +118,9 @@ bool Heap::QQmlValueTypeWrapper::readReference()
     return enforcesLocation() || QV4::ReferenceObject::readReference(this);
 }
 
-bool Heap::QQmlValueTypeWrapper::writeBack()
+bool Heap::QQmlValueTypeWrapper::writeBack(int propertyIndex)
 {
-    return isAttachedToProperty() && QV4::ReferenceObject::writeBack(this);
+    return isAttachedToProperty() && QV4::ReferenceObject::writeBack(this, propertyIndex);
 }
 
 ReturnedValue QQmlValueTypeWrapper::create(
@@ -178,9 +178,12 @@ int QQmlValueTypeWrapper::virtualMetacall(
     switch (call) {
     case QMetaObject::ReadProperty:
         break;
-    case QMetaObject::InvokeMetaMethod:
     case QMetaObject::WriteProperty:
     case QMetaObject::ResetProperty:
+        if (wrapper->d()->object())
+            wrapper->d()->writeBack(index);
+        break;
+    case QMetaObject::InvokeMetaMethod:
     case QMetaObject::CustomCall:
         if (wrapper->d()->object())
             wrapper->d()->writeBack();
@@ -840,7 +843,7 @@ bool QQmlValueTypeWrapper::virtualPut(Managed *m, PropertyKey id, const Value &v
     property.writeOnGadget(gadget, v);
 
     if (heapObject)
-        r->d()->writeBack();
+        r->d()->writeBack(pd.coreIndex());
 
     return true;
 }
diff --git a/src/qml/qml/qqmlvaluetypewrapper_p.h b/src/qml/qml/qqmlvaluetypewrapper_p.h
index cb8bfdf106..0c4cbb7dd3 100644
--- a/src/qml/qml/qqmlvaluetypewrapper_p.h
+++ b/src/qml/qml/qqmlvaluetypewrapper_p.h
@@ -73,7 +73,7 @@ DECLARE_HEAP_OBJECT(QQmlValueTypeWrapper, ReferenceObject) {
     bool setVariant(const QVariant &variant);
 
     bool readReference();
-    bool writeBack();
+    bool writeBack(int propertyIndex = QV4::ReferenceObject::AllProperties);
 
 private:
     void setMetaObject(const QMetaObject *metaObject) { m_metaObject = metaObject; }
diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp
index 05b9328d99..c863fa6647 100644
--- a/src/qml/qml/qqmlvmemetaobject.cpp
+++ b/src/qml/qml/qqmlvmemetaobject.cpp
@@ -275,63 +275,72 @@ bool QQmlInterceptorMetaObject::doIntercept(QMetaObject::Call c, int id, void **
 
         if (metaType.isValid()) {
             if (valueIndex != -1 && c == QMetaObject::WriteProperty) {
-                // TODO: handle intercepting bindable properties for value types?
-                QQmlGadgetPtrWrapper *valueType = QQmlGadgetPtrWrapper::instance(
-                            data->context->engine(), metaType);
-                Q_ASSERT(valueType);
-
-                //
-                // Consider the following case:
-                //  color c = { 0.1, 0.2, 0.3 }
-                //  interceptor exists on c.r
-                //  write { 0.2, 0.4, 0.6 }
-                //
-                // The interceptor may choose not to update the r component at this
-                // point (for example, a behavior that creates an animation). But we
-                // need to ensure that the g and b components are updated correctly.
-                //
-                // So we need to perform a full write where the value type is:
-                //    r = old value, g = new value, b = new value
-                //
-                // And then call the interceptor which may or may not write the
-                // new value to the r component.
-                //
-                // This will ensure that the other components don't contain stale data
-                // and any relevant signals are emitted.
-                //
-                // To achieve this:
-                //   (1) Store the new value type as a whole (needed due to
-                //       aliasing between a[0] and static storage in value type).
-                //   (2) Read the entire existing value type from object -> valueType temp.
-                //   (3) Read the previous value of the component being changed
-                //       from the valueType temp.
-                //   (4) Write the entire new value type into the temp.
-                //   (5) Overwrite the component being changed with the old value.
-                //   (6) Perform a full write to the value type (which may emit signals etc).
-                //   (7) Issue the interceptor call with the new component value.
-                //
-
-                QMetaProperty valueProp = valueType->property(valueIndex);
-                QVariant newValue(metaType, a[0]);
-
-                valueType->read(object, id);
-                QVariant prevComponentValue = valueType->readOnGadget(valueProp);
-
-                valueType->setValue(newValue);
-                QVariant newComponentValue = valueType->readOnGadget(valueProp);
-
-                // Don't apply the interceptor if the intercepted value has not changed
-                bool updated = false;
-                if (newComponentValue != prevComponentValue) {
+
+                // If we didn't intend to change the property this interceptor cares about,
+                // then don't bother intercepting it. There may be an animation running on
+                // the property. We shouldn't disturb it.
+                const int changedProperty
+                        = (*static_cast<int *>(a[3]) & QQmlPropertyData::HasInternalIndex)
+                            ? *static_cast<int *>(a[4])
+                            : QV4::ReferenceObject::AllProperties;
+                if (changedProperty == QV4::ReferenceObject::AllProperties
+                        || changedProperty == valueIndex) {
+                    // TODO: handle intercepting bindable properties for value types?
+                    QQmlGadgetPtrWrapper *valueType = QQmlGadgetPtrWrapper::instance(
+                                data->context->engine(), metaType);
+                    Q_ASSERT(valueType);
+
+                    //
+                    // Consider the following case:
+                    //  color c = { 0.1, 0.2, 0.3 }
+                    //  interceptor exists on c.r
+                    //  write { 0.2, 0.4, 0.6 }
+                    //
+                    // The interceptor may choose not to update the r component at this
+                    // point (for example, a behavior that creates an animation). But we
+                    // need to ensure that the g and b components are updated correctly.
+                    //
+                    // So we need to perform a full write where the value type is:
+                    //    r = old value, g = new value, b = new value
+                    //
+                    // And then call the interceptor which may or may not write the
+                    // new value to the r component.
+                    //
+                    // This will ensure that the other components don't contain stale data
+                    // and any relevant signals are emitted.
+                    //
+                    // To achieve this:
+                    //   (1) Store the new value type as a whole (needed due to
+                    //       aliasing between a[0] and static storage in value type).
+                    //   (2) Read the entire existing value type from object -> valueType temp.
+                    //   (3) Read the previous value of the component being changed
+                    //       from the valueType temp.
+                    //   (4) Write the entire new value type into the temp.
+                    //   (5) Overwrite the component being changed with the old value.
+                    //   (6) Perform a full write to the value type (which may emit signals etc).
+                    //   (7) Issue the interceptor call with the new component value.
+                    //
+
+                    QMetaProperty valueProp = valueType->property(valueIndex);
+                    QVariant newValue(metaType, a[0]);
+
+                    valueType->read(object, id);
+                    QVariant prevComponentValue = valueType->readOnGadget(valueProp);
+
+                    valueType->setValue(newValue);
+                    QVariant newComponentValue = valueType->readOnGadget(valueProp);
+
+                    // If the intercepted value seemingly has not changed, we still need to
+                    // invoke the interceptor. There may be a pending animation that will
+                    // change the value soon. Such an animation needs to be canceled if the
+                    // current value is explicitly set.
+                    // So, we cannot return here if prevComponentValue == newComponentValue.
                     valueType->writeOnGadget(valueProp, prevComponentValue);
                     valueType->write(object, id, QQmlPropertyData::DontRemoveBinding | QQmlPropertyData::BypassInterceptor);
 
                     vi->write(newComponentValue);
-                    updated = true;
-                }
-
-                if (updated)
                     return true;
+                }
             } else if (c == QMetaObject::WriteProperty) {
                 vi->write(QVariant(metaType, a[0]));
                 return true;
@@ -993,7 +1002,8 @@ int QQmlVMEMetaObject::metaCall(QObject *o, QMetaObject::Call c, int _id, void *
                         int rv = QMetaObject::metacall(valueType, c, valueTypePropertyIndex, a);
 
                         if (c == QMetaObject::WriteProperty)
-                            valueType->write(target, coreIndex, {});
+                            valueType->write(target, coreIndex, QQmlPropertyData::HasInternalIndex,
+                                             valueTypePropertyIndex);
 
                         return rv;
                     } else {
diff --git a/tests/auto/qml/qqmlmetatype/data/animationOnValueType.qml b/tests/auto/qml/qqmlmetatype/data/animationOnValueType.qml
new file mode 100644
index 0000000000..196715dd74
--- /dev/null
+++ b/tests/auto/qml/qqmlmetatype/data/animationOnValueType.qml
@@ -0,0 +1,26 @@
+import QtQuick
+
+Text {
+    property bool large: false
+    property bool check: false
+    font.pointSize: large ? 24 : 12
+    font.letterSpacing: check ? 24 : 12
+
+    Behavior on font.pointSize {
+        SmoothedAnimation { duration: 100; }
+    }
+
+    Behavior on font.letterSpacing {
+        SmoothedAnimation { duration: 100; }
+    }
+
+    Component.onCompleted: {
+        large = true;
+        large = false;
+        check = true;
+    }
+
+    property real pointSize: font.pointSize
+    property real letterSpacing: font.letterSpacing
+}
+
diff --git a/tests/auto/qml/qqmlmetatype/tst_qqmlmetatype.cpp b/tests/auto/qml/qqmlmetatype/tst_qqmlmetatype.cpp
index 6f4221539d..c1bac33d87 100644
--- a/tests/auto/qml/qqmlmetatype/tst_qqmlmetatype.cpp
+++ b/tests/auto/qml/qqmlmetatype/tst_qqmlmetatype.cpp
@@ -49,6 +49,8 @@ private slots:
 
     void enumsInRecursiveImport_data();
     void enumsInRecursiveImport();
+
+    void revertValueTypeAnimation();
 };
 
 class TestType : public QObject
@@ -711,6 +713,17 @@ void tst_qqmlmetatype::enumsInRecursiveImport()
     QTRY_COMPARE(obj->property("color").toString(), QString("green"));
 }
 
+void tst_qqmlmetatype::revertValueTypeAnimation()
+{
+    QQmlEngine engine;
+    QQmlComponent c(&engine, testFileUrl("animationOnValueType.qml"));
+    QVERIFY2(c.isReady(), qPrintable(c.errorString()));
+
+    QScopedPointer<QObject> o(c.create());
+    QTRY_COMPARE(o->property("letterSpacing").toDouble(), 24.0);
+    QCOMPARE(o->property("pointSize").toDouble(), 12.0);
+}
+
 QTEST_MAIN(tst_qqmlmetatype)
 
 #include "tst_qqmlmetatype.moc"