ParentChange: Restore old position correctly

ParentChange does floating point math in doChange to calculate the
correct position. Unfortunately, while doing the conversion, we
accumulate rounding errors in the presence of rotations.

Those can lead to visual glitches, as observed in QTBUG-68176.

This patch avoids the issue by storing the old values and resetting to
them in restore.

Fixes: QTBUG-68176
Change-Id: I6ebe1ccbc601838aa664cdc723e0cd58c95e785a
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

src/quick/items/qquickstateoperations.cpp

@@ -44,6 +44,7 @@
 
 #include <QtQml/qqmlinfo.h>
 #include <QtCore/qmath.h>
+#include <memory>
 
 QT_BEGIN_NAMESPACE
 
@@ -51,15 +52,17 @@ class QQuickParentChangePrivate : public QQuickStateOperationPrivate
 {
     Q_DECLARE_PUBLIC(QQuickParentChange)
 public:
-    QQuickParentChangePrivate() : target(nullptr), parent(nullptr), origParent(nullptr), origStackBefore(nullptr),
-        rewindParent(nullptr), rewindStackBefore(nullptr) {}
-
-    QQuickItem *target;
+    QQuickItem *target = nullptr;
     QPointer<QQuickItem> parent;
-    QPointer<QQuickItem> origParent;
-    QPointer<QQuickItem> origStackBefore;
-    QQuickItem *rewindParent;
-    QQuickItem *rewindStackBefore;
+
+    struct StateSnapshot {
+        QPointer<QQuickItem> parent;
+        QPointer<QQuickItem> stackBefore;
+        qreal x = 0, y = 0, width = 0, height = 0, scale = 0, rotation = 0;
+    };
+
+    std::unique_ptr<StateSnapshot> orig;
+    std::unique_ptr<StateSnapshot> rewind;
 
     QQmlNullableValue<QQmlScriptString> xString;
     QQmlNullableValue<QQmlScriptString> yString;
@@ -68,10 +71,11 @@ public:
     QQmlNullableValue<QQmlScriptString> scaleString;
     QQmlNullableValue<QQmlScriptString> rotationString;
 
-    void doChange(QQuickItem *targetParent, QQuickItem *stackBefore = nullptr);
+    void doChange(QQuickItem *targetParent);
+    void reverseRewindHelper(const std::unique_ptr<StateSnapshot> &snapshot);
 };
 
-void QQuickParentChangePrivate::doChange(QQuickItem *targetParent, QQuickItem *stackBefore)
+void QQuickParentChangePrivate::doChange(QQuickItem *targetParent)
 {
     if (targetParent && target && target->parentItem()) {
         Q_Q(QQuickParentChange);
@@ -137,11 +141,6 @@ void QQuickParentChangePrivate::doChange(QQuickItem *targetParent, QQuickItem *s
     } else if (target) {
         target->setParentItem(targetParent);
     }
-
-    //restore the original stack position.
-    //### if stackBefore has also been reparented this won't work
-    if (target && stackBefore)
-        target->stackBefore(stackBefore);
 }
 
 /*!
@@ -305,7 +304,7 @@ bool QQuickParentChange::rotationIsSet() const
 QQuickItem *QQuickParentChange::originalParent() const
 {
     Q_D(const QQuickParentChange);
-    return d->origParent;
+    return d->orig ? d->orig->parent : nullptr;
 }
 
 /*!
@@ -473,21 +472,11 @@ void QQuickParentChange::saveOriginals()
 {
     Q_D(QQuickParentChange);
     saveCurrentValues();
-    d->origParent = d->rewindParent;
-    d->origStackBefore = d->rewindStackBefore;
+    if (!d->orig)
+        d->orig.reset(new QQuickParentChangePrivate::StateSnapshot);
+    *d->orig = *d->rewind;
 }
 
-/*void QQuickParentChange::copyOriginals(QQuickStateActionEvent *other)
-{
-    Q_D(QQuickParentChange);
-    QQuickParentChange *pc = static_cast<QQuickParentChange*>(other);
-
-    d->origParent = pc->d_func()->rewindParent;
-    d->origStackBefore = pc->d_func()->rewindStackBefore;
-
-    saveCurrentValues();
-}*/
-
 void QQuickParentChange::execute()
 {
     Q_D(QQuickParentChange);
@@ -499,10 +488,26 @@ bool QQuickParentChange::isReversable()
     return true;
 }
 
+void QQuickParentChangePrivate::reverseRewindHelper(const std::unique_ptr<QQuickParentChangePrivate::StateSnapshot> &snapshot)
+{
+    if (!target || !snapshot)
+        return;
+    target->setX(snapshot->x);
+    target->setY(snapshot->y);
+    target->setScale(snapshot->scale);
+    target->setWidth(snapshot->width);
+    target->setHeight(snapshot->height);
+    target->setRotation(snapshot->rotation);
+    target->setParentItem(snapshot->parent);
+    if (snapshot->stackBefore)
+        target->stackBefore(snapshot->stackBefore);
+}
+
+
 void QQuickParentChange::reverse()
 {
     Q_D(QQuickParentChange);
-    d->doChange(d->origParent, d->origStackBefore);
+    d->reverseRewindHelper(d->orig);
 }
 
 QQuickStateActionEvent::EventType QQuickParentChange::type() const
@@ -524,21 +529,28 @@ void QQuickParentChange::saveCurrentValues()
 {
     Q_D(QQuickParentChange);
     if (!d->target) {
-        d->rewindParent = nullptr;
-        d->rewindStackBefore = nullptr;
+        d->rewind = nullptr;
         return;
     }
 
-    d->rewindParent = d->target->parentItem();
-    d->rewindStackBefore = nullptr;
+    d->rewind.reset(new QQuickParentChangePrivate::StateSnapshot);
+    d->rewind->x = d->target->x();
+    d->rewind->y = d->target->y();
+    d->rewind->scale = d->target->scale();
+    d->rewind->width = d->target->width();
+    d->rewind->height = d->target->height();
+    d->rewind->rotation = d->target->rotation();
 
-    if (!d->rewindParent)
+    d->rewind->parent = d->target->parentItem();
+    d->rewind->stackBefore = nullptr;
+
+    if (!d->rewind->parent)
         return;
 
-    QList<QQuickItem *> children = d->rewindParent->childItems();
+    QList<QQuickItem *> children = d->rewind->parent->childItems();
     for (int ii = 0; ii < children.count() - 1; ++ii) {
         if (children.at(ii) == d->target) {
-            d->rewindStackBefore = children.at(ii + 1);
+            d->rewind->stackBefore = children.at(ii + 1);
             break;
         }
     }
@@ -547,7 +559,8 @@ void QQuickParentChange::saveCurrentValues()
 void QQuickParentChange::rewind()
 {
     Q_D(QQuickParentChange);
-    d->doChange(d->rewindParent, d->rewindStackBefore);
+    d->reverseRewindHelper(d->rewind);
+    d->rewind.reset();
 }
 
 /*!

tests/auto/quick/qquickstates/data/parentChangeCorrectReversal.qml

@@ -0,0 +1,72 @@
+import QtQuick 2.10
+import QtQuick.Layouts 1.3
+
+Item {
+    id: root
+
+    visible: true
+
+    width: 400
+    height: 200
+    property bool switchToRight: false
+    property alias stayingRectX: stayingRect.x
+
+    RowLayout {
+        id: topLayout
+
+        anchors.fill: parent
+
+        Item {
+            Layout.fillWidth: true
+            Layout.fillHeight: true
+
+            Rectangle {
+                id: leftRect
+
+                width: parent.width*(2/3)
+                height: width
+                anchors.centerIn: parent
+
+                color: "red"
+
+                Rectangle {
+                    id: stayingRect
+
+                    x: 70; y: 70
+                    width: 50; height: 50
+
+                    color: "yellow"
+                }
+            }
+        }
+
+        Item {
+            Layout.fillWidth: true
+            Layout.fillHeight: true
+
+            Rectangle {
+                id: rightRect
+
+                width: parent.height*(2/3)
+                height: width
+                anchors.centerIn: parent
+
+                color: "green"
+                rotation: 45
+            }
+        }
+    }
+
+    states: State {
+        name: "switchToRight"
+
+        ParentChange {
+            target: stayingRect
+            parent: rightRect
+            width: 70
+        }
+
+    }
+
+    state: root.switchToRight ? "switchToRight" : ""
+}

tests/auto/quick/qquickstates/tst_qquickstates.cpp

@@ -139,6 +139,7 @@ private slots:
     void revertListMemoryLeak();
     void duplicateStateName();
     void trivialWhen();
+    void parentChangeCorrectReversal();
 };
 
 void tst_qquickstates::initTestCase()
@@ -1675,6 +1676,22 @@ void tst_qquickstates::trivialWhen()
     QVERIFY(c.create());
 }
 
+void tst_qquickstates::parentChangeCorrectReversal()
+{
+    QQmlEngine engine;
+    QQmlComponent c(&engine, testFileUrl("parentChangeCorrectReversal.qml"));
+    QScopedPointer<QObject> root {c.create()};
+    QVERIFY(root);
+    QQmlProperty stayingRectX(root.get(), "stayingRectX");
+    qreal oldX = stayingRectX.read().toDouble();
+    QQmlProperty switchToRight(root.get(), "switchToRight");
+    switchToRight.write(true);
+    qreal newX = stayingRectX.read().toDouble();
+    QVERIFY(newX != oldX);
+    switchToRight.write(false);
+    QCOMPARE(oldX, stayingRectX.read().toDouble());
+}
+
 
 QTEST_MAIN(tst_qquickstates)