Fix when animating items that are already moving

The view must transition displaced/moved items that are currently
transitioning to another position; check against the current
transition-to position, not just the current item position.

Task-number: QTBUG-24522
Change-Id: Icf1c290f76ceb8c93716f1562ae0bc5a75445b78
Reviewed-by: Martin Jones <martin.jones@nokia.com>

diff --git a/src/quick/items/qquickitemviewtransition.cpp b/src/quick/items/qquickitemviewtransition.cpp
index abff768..3febc9b 100644 (file)
--- a/src/quick/items/qquickitemviewtransition.cpp
+++ b/src/quick/items/qquickitemviewtransition.cpp
@@ -383,6 +383,9 @@ bool QQuickViewItem::prepareTransition(const QRectF &viewBounds)
     if (nextTransitionType != QQuickItemViewTransitioner::NoTransition && !nextTransitionToSet)
         moveTo(item->pos());
 
+    // For move transitions (both target and displaced) and displaced transitions of other
+    // types, only run the transition if the item is actually moving to another position.
+
     switch (nextTransitionType) {
     case QQuickItemViewTransitioner::NoTransition:
     {
@@ -394,14 +397,14 @@ bool QQuickViewItem::prepareTransition(const QRectF &viewBounds)
     }
     case QQuickItemViewTransitioner::AddTransition:
     case QQuickItemViewTransitioner::RemoveTransition:
-        // For Add targets, do transition if item is moving into visible area
-        // For Remove targets, do transition if item is currently in visible area
         if (viewBounds.isNull()) {
             if (isTransitionTarget)
                 doTransition = true;
             else
-                doTransition = (nextTransitionTo != item->pos());
+                doTransition = transitionWillChangePosition();
         } else if (isTransitionTarget) {
+            // For Add targets, do transition if item is moving into visible area
+            // For Remove targets, do transition if item is currently in visible area
             doTransition = (nextTransitionType == QQuickItemViewTransitioner::AddTransition)
                     ? viewBounds.intersects(QRectF(nextTransitionTo.x(), nextTransitionTo.y(), item->width(), item->height()))
                     : viewBounds.intersects(QRectF(item->x(), item->y(), item->width(), item->height()));
@@ -410,7 +413,7 @@ bool QQuickViewItem::prepareTransition(const QRectF &viewBounds)
         } else {
             if (viewBounds.intersects(QRectF(item->x(), item->y(), item->width(), item->height()))
                     || viewBounds.intersects(QRectF(nextTransitionTo.x(), nextTransitionTo.y(), item->width(), item->height()))) {
-                doTransition = (nextTransitionTo != item->pos());
+                doTransition = transitionWillChangePosition();
             } else {
                 item->setPos(nextTransitionTo);
             }
@@ -418,7 +421,7 @@ bool QQuickViewItem::prepareTransition(const QRectF &viewBounds)
         break;
     case QQuickItemViewTransitioner::MoveTransition:
         // do transition if moving from or into visible area
-        if (nextTransitionTo != item->pos()) {
+        if (transitionWillChangePosition()) {
             doTransition = viewBounds.isNull()
                     || viewBounds.intersects(QRectF(item->x(), item->y(), item->width(), item->height()))
                     || viewBounds.intersects(QRectF(nextTransitionTo.x(), nextTransitionTo.y(), item->width(), item->height()));
@@ -472,6 +475,13 @@ void QQuickViewItem::setNextTransition(QQuickItemViewTransitioner::TransitionTyp
     isTransitionTarget = isTargetItem;
 }
 
+bool QQuickViewItem::transitionWillChangePosition() const
+{
+    if (transitionRunning() && transition->m_toPos != nextTransitionTo)
+        return true;
+    return nextTransitionTo != item->pos();
+}
+
 void QQuickViewItem::finishedTransition()
 {
     nextTransitionToSet = false;

diff --git a/src/quick/items/qquickitemviewtransition_p.h b/src/quick/items/qquickitemviewtransition_p.h
index 1ebc52c..57ea85b 100644 (file)
--- a/src/quick/items/qquickitemviewtransition_p.h
+++ b/src/quick/items/qquickitemviewtransition_p.h
@@ -152,6 +152,7 @@ private:
     friend class QQuickItemViewTransitioner;
     friend class QQuickItemViewTransitionJob;
     void setNextTransition(QQuickItemViewTransitioner::TransitionType, bool isTargetItem);
+    bool transitionWillChangePosition() const;
     void finishedTransition();
     void resetTransitionData();
 };

diff --git a/tests/auto/qtquick2/qquickgridview/data/multipleTransitions.qml b/tests/auto/qtquick2/qquickgridview/data/multipleTransitions.qml
index 45b86e2..909ec3a 100644 (file)
--- a/tests/auto/qtquick2/qquickgridview/data/multipleTransitions.qml
+++ b/tests/auto/qtquick2/qquickgridview/data/multipleTransitions.qml
@@ -10,7 +10,7 @@ Rectangle {
     // interrupting transitions will still produce the correct result)
     property int timeBetweenActions: duration / 2
 
-    property int duration: 100
+    property int duration: 300
 
     property int count: grid.count
 
@@ -46,6 +46,8 @@ Rectangle {
         property bool runningAddDisplaced: false
         property bool runningMoveTargets: false
         property bool runningMoveDisplaced: false
+        property bool runningRemoveTargets: false
+        property bool runningRemoveDisplaced: false
 
         objectName: "grid"
         width: 240
@@ -103,6 +105,30 @@ Rectangle {
                 ScriptAction { script: grid.runningMoveDisplaced = false }
             }
         }
+
+        remove: Transition {
+            id: removeTargets
+            SequentialAnimation {
+                ScriptAction { script: grid.runningRemoveTargets = true }
+                ParallelAnimation {
+                    NumberAnimation { properties: "x"; to: removeTargets_transitionTo.x; duration: root.duration }
+                    NumberAnimation { properties: "y"; to: removeTargets_transitionTo.y; duration: root.duration }
+                }
+                ScriptAction { script: grid.runningRemoveTargets = false }
+            }
+        }
+
+        removeDisplaced: Transition {
+            id: removeDisplaced
+            SequentialAnimation {
+                ScriptAction { script: grid.runningRemoveDisplaced = true }
+                ParallelAnimation {
+                    NumberAnimation { properties: "x"; from: removeDisplaced_transitionFrom.x; duration: root.duration }
+                    NumberAnimation { properties: "y"; from: removeDisplaced_transitionFrom.y; duration: root.duration }
+                }
+                ScriptAction { script: grid.runningRemoveDisplaced = false }
+            }
+        }
     }
 
     Rectangle {

diff --git a/tests/auto/qtquick2/qquickgridview/tst_qquickgridview.cpp b/tests/auto/qtquick2/qquickgridview/tst_qquickgridview.cpp
index 6d755a6..077cd2b 100644 (file)
--- a/tests/auto/qtquick2/qquickgridview/tst_qquickgridview.cpp
+++ b/tests/auto/qtquick2/qquickgridview/tst_qquickgridview.cpp
@@ -4776,12 +4776,15 @@ void tst_QQuickGridView::multipleTransitions()
     QFETCH(int, initialCount);
     QFETCH(qreal, contentY);
     QFETCH(QList<ListChange>, changes);
+    QFETCH(bool, rippleAddDisplaced);
 
     // add transitions on the left, moves on the right
     QPointF addTargets_transitionFrom(-50, -50);
     QPointF addDisplaced_transitionFrom(-50, 50);
     QPointF moveTargets_transitionFrom(50, -50);
     QPointF moveDisplaced_transitionFrom(50, 50);
+    QPointF removeTargets_transitionTo(-100, 300);
+    QPointF removeDisplaced_transitionFrom(100, 300);
 
     QmlListModel model;
     for (int i = 0; i < initialCount; i++)
@@ -4794,8 +4797,12 @@ void tst_QQuickGridView::multipleTransitions()
     ctxt->setContextProperty("addDisplaced_transitionFrom", addDisplaced_transitionFrom);
     ctxt->setContextProperty("moveTargets_transitionFrom", moveTargets_transitionFrom);
     ctxt->setContextProperty("moveDisplaced_transitionFrom", moveDisplaced_transitionFrom);
+    ctxt->setContextProperty("removeTargets_transitionTo", removeTargets_transitionTo);
+    ctxt->setContextProperty("removeDisplaced_transitionFrom", removeDisplaced_transitionFrom);
+    ctxt->setContextProperty("rippleAddDisplaced", rippleAddDisplaced);
     canvas->setSource(testFileUrl("multipleTransitions.qml"));
     canvas->show();
+    QTest::qWaitForWindowShown(canvas);
 
     QQuickGridView *gridview = findItem<QQuickGridView>(canvas->rootObject(), "grid");
     QTRY_VERIFY(gridview != 0);
@@ -4803,6 +4810,11 @@ void tst_QQuickGridView::multipleTransitions()
     QVERIFY(contentItem != 0);
     QTRY_COMPARE(QQuickItemPrivate::get(gridview)->polishScheduled, false);
 
+    if (contentY != 0) {
+        gridview->setContentY(contentY);
+        QTRY_COMPARE(QQuickItemPrivate::get(gridview)->polishScheduled, false);
+    }
+
     int timeBetweenActions = canvas->rootObject()->property("timeBetweenActions").toInt();
 
     QList<QPair<QString, QString> > targetItems;
@@ -4894,18 +4906,21 @@ void tst_QQuickGridView::multipleTransitions_data()
     QTest::addColumn<int>("initialCount");
     QTest::addColumn<qreal>("contentY");
     QTest::addColumn<QList<ListChange> >("changes");
+    QTest::addColumn<bool>("rippleAddDisplaced");
 
     // the added item and displaced items should move to final dest correctly
     QTest::newRow("add item, then move it immediately") << 10 << 0.0 << (QList<ListChange>()
-            << ListChange::insert(0, 1)
-            << ListChange::move(0, 3, 1)
-            );
+             << ListChange::insert(0, 1)
+             << ListChange::move(0, 3, 1)
+             )
+             << false;
 
     // items affected by the add should change from move to add transition
     QTest::newRow("move, then insert item before the moved item") << 20 << 0.0 << (QList<ListChange>()
             << ListChange::move(1, 10, 3)
             << ListChange::insert(0, 1)
-            );
+            )
+            << false;
 
     // items should be placed correctly if you trigger a transition then refill for that index
     QTest::newRow("add at 0, flick down, flick back to top and add at 0 again") << 20 << 0.0 << (QList<ListChange>()
@@ -4913,7 +4928,14 @@ void tst_QQuickGridView::multipleTransitions_data()
             << ListChange::setContentY(160.0)
             << ListChange::setContentY(0.0)
             << ListChange::insert(0, 1)
-            );
+            )
+            << false;
+
+    QTest::newRow("insert then remove same index, with ripple effect on add displaced") << 20 << 0.0 << (QList<ListChange>()
+            << ListChange::insert(1, 1)
+            << ListChange::remove(1, 1)
+            )
+            << true;
 }
 
 void tst_QQuickGridView::cacheBuffer()

diff --git a/tests/auto/qtquick2/qquicklistview/data/multipleTransitions.qml b/tests/auto/qtquick2/qquicklistview/data/multipleTransitions.qml
index 3e32485..8264b42 100644 (file)
--- a/tests/auto/qtquick2/qquicklistview/data/multipleTransitions.qml
+++ b/tests/auto/qtquick2/qquicklistview/data/multipleTransitions.qml
@@ -45,6 +45,8 @@ Rectangle {
         property bool runningAddDisplaced: false
         property bool runningMoveTargets: false
         property bool runningMoveDisplaced: false
+        property bool runningRemoveTargets: false
+        property bool runningRemoveDisplaced: false
 
         objectName: "list"
         focus: true
@@ -70,6 +72,7 @@ Rectangle {
             id: addDisplaced
             SequentialAnimation {
                 ScriptAction { script: list.runningAddDisplaced = true }
+                PauseAnimation { duration: rippleAddDisplaced ? addDisplaced.ViewTransition.index * root.duration/10 : 0 }
                 ParallelAnimation {
                     NumberAnimation { properties: "x"; from: addDisplaced_transitionFrom.x; duration: root.duration }
                     NumberAnimation { properties: "y"; from: addDisplaced_transitionFrom.y; duration: root.duration }
@@ -101,6 +104,30 @@ Rectangle {
                 ScriptAction { script: list.runningMoveDisplaced = false }
             }
         }
+
+        remove: Transition {
+            id: removeTargets
+            SequentialAnimation {
+                ScriptAction { script: list.runningRemoveTargets = true }
+                ParallelAnimation {
+                    NumberAnimation { properties: "x"; to: removeTargets_transitionTo.x; duration: root.duration }
+                    NumberAnimation { properties: "y"; to: removeTargets_transitionTo.y; duration: root.duration }
+                }
+                ScriptAction { script: list.runningRemoveTargets = false }
+            }
+        }
+
+        removeDisplaced: Transition {
+            id: removeDisplaced
+            SequentialAnimation {
+                ScriptAction { script: list.runningRemoveDisplaced = true }
+                ParallelAnimation {
+                    NumberAnimation { properties: "x"; from: removeDisplaced_transitionFrom.x; duration: root.duration }
+                    NumberAnimation { properties: "y"; from: removeDisplaced_transitionFrom.y; duration: root.duration }
+                }
+                ScriptAction { script: list.runningRemoveDisplaced = false }
+            }
+        }
     }
 
     Rectangle {

diff --git a/tests/auto/qtquick2/qquicklistview/tst_qquicklistview.cpp b/tests/auto/qtquick2/qquicklistview/tst_qquicklistview.cpp
index a834f1a..8f1527f 100644 (file)
--- a/tests/auto/qtquick2/qquicklistview/tst_qquicklistview.cpp
+++ b/tests/auto/qtquick2/qquicklistview/tst_qquicklistview.cpp
@@ -5787,12 +5787,14 @@ void tst_QQuickListView::multipleTransitions()
     QFETCH(int, initialCount);
     QFETCH(qreal, contentY);
     QFETCH(QList<ListChange>, changes);
+    QFETCH(bool, rippleAddDisplaced);
 
-    // add transitions on the left, moves on the right
     QPointF addTargets_transitionFrom(-50, -50);
     QPointF addDisplaced_transitionFrom(-50, 50);
     QPointF moveTargets_transitionFrom(50, -50);
     QPointF moveDisplaced_transitionFrom(50, 50);
+    QPointF removeTargets_transitionTo(-100, 300);
+    QPointF removeDisplaced_transitionFrom(100, 300);
 
     QmlListModel model;
     for (int i = 0; i < initialCount; i++)
@@ -5807,6 +5809,9 @@ void tst_QQuickListView::multipleTransitions()
     ctxt->setContextProperty("addDisplaced_transitionFrom", addDisplaced_transitionFrom);
     ctxt->setContextProperty("moveTargets_transitionFrom", moveTargets_transitionFrom);
     ctxt->setContextProperty("moveDisplaced_transitionFrom", moveDisplaced_transitionFrom);
+    ctxt->setContextProperty("removeTargets_transitionTo", removeTargets_transitionTo);
+    ctxt->setContextProperty("removeDisplaced_transitionFrom", removeDisplaced_transitionFrom);
+    ctxt->setContextProperty("rippleAddDisplaced", rippleAddDisplaced);
     canvas->setSource(testFileUrl("multipleTransitions.qml"));
     canvas->show();
     QTest::qWaitForWindowShown(canvas);
@@ -5817,6 +5822,11 @@ void tst_QQuickListView::multipleTransitions()
     QVERIFY(contentItem != 0);
     QTRY_COMPARE(QQuickItemPrivate::get(listview)->polishScheduled, false);
 
+    if (contentY != 0) {
+        listview->setContentY(contentY);
+        QTRY_COMPARE(QQuickItemPrivate::get(listview)->polishScheduled, false);
+    }
+
     int timeBetweenActions = canvas->rootObject()->property("timeBetweenActions").toInt();
 
     QList<QPair<QString, QString> > targetItems;
@@ -5897,18 +5907,21 @@ void tst_QQuickListView::multipleTransitions_data()
     QTest::addColumn<int>("initialCount");
     QTest::addColumn<qreal>("contentY");
     QTest::addColumn<QList<ListChange> >("changes");
+    QTest::addColumn<bool>("rippleAddDisplaced");
 
     // the added item and displaced items should move to final dest correctly
     QTest::newRow("add item, then move it immediately") << 10 << 0.0 << (QList<ListChange>()
             << ListChange::insert(0, 1)
             << ListChange::move(0, 3, 1)
-            );
+            )
+            << false;
 
     // items affected by the add should change from move to add transition
     QTest::newRow("move, then insert item before the moved item") << 20 << 0.0 << (QList<ListChange>()
             << ListChange::move(1, 10, 3)
             << ListChange::insert(0, 1)
-            );
+            )
+            << false;
 
     // items should be placed correctly if you trigger a transition then refill for that index
     QTest::newRow("add at 0, flick down, flick back to top and add at 0 again") << 20 << 0.0 << (QList<ListChange>()
@@ -5916,7 +5929,14 @@ void tst_QQuickListView::multipleTransitions_data()
             << ListChange::setContentY(80.0)
             << ListChange::setContentY(0.0)
             << ListChange::insert(0, 1)
-            );
+            )
+            << false;
+
+    QTest::newRow("insert then remove same index, with ripple effect on add displaced") << 20 << 0.0 << (QList<ListChange>()
+            << ListChange::insert(1, 1)
+            << ListChange::remove(1, 1)
+            )
+            << true;
 }
 
 QList<int> tst_QQuickListView::toIntList(const QVariantList &list)