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 <[email protected]>

1 files changed, 51 insertions, 38 deletions

diff --git a/src/quick/items/qquickstateoperations.cpp b/src/quick/items/qquickstateoperations.cpp
index 31d1c91649..07767d377d 100644
--- a/src/quick/items/qquickstateoperations.cpp
+++ b/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();
+
+    d->rewind->parent = d->target->parentItem();
+    d->rewind->stackBefore = nullptr;
 
-    if (!d->rewindParent)
+    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();
 }
 
 /*!