Fix moving of multiple items, and moving items backwards

To fix backward movements, all backward movements are now flipped
into forward movements (as is done in ListModel implemenation).

Movement of multiple items wasn't working as views weren't removing
the right indexes in these cases.

Task-number: QTBUG-19208
Change-Id: I9866ddabc241c066bb77329a6761d500d79aaf61
Reviewed-on: http://codereview.qt.nokia.com/2262
Reviewed-by: Bea Lam <bea.lam@nokia.com>
Reviewed-by: Qt Sanity Bot <qt_sanity_bot@ovi.com>

diff --git a/src/declarative/items/qsggridview.cpp b/src/declarative/items/qsggridview.cpp
index 5531c15..214481a 100644 (file)
--- a/src/declarative/items/qsggridview.cpp
+++ b/src/declarative/items/qsggridview.cpp
@@ -1497,11 +1497,13 @@ void QSGGridView::itemsMoved(int from, int to, int count)
     }
 
     d->moveReason = QSGGridViewPrivate::Other;
-    FxGridItemSG *firstVisible = static_cast<FxGridItemSG*>(d->firstVisibleItem());
-    QHash<int,FxGridItemSG*> moved;
-    int moveByCount = 0;
 
     bool movingBackwards = from > to;
+    d->adjustMoveParameters(&from, &to, &count);
+
+    QHash<int,FxGridItemSG*> moved;
+    int moveByCount = 0;
+    FxGridItemSG *firstVisible = static_cast<FxGridItemSG*>(d->firstVisibleItem());
     int firstItemIndex = firstVisible ? firstVisible->index : -1;
 
     // if visibleItems.first() is above the content start pos, and the items
@@ -1532,19 +1534,20 @@ void QSGGridView::itemsMoved(int from, int to, int count)
         }
     }
 
-    int remaining = count;
+    int movedCount = 0;
     int endIndex = d->visibleIndex;
     it = d->visibleItems.begin();
     while (it != d->visibleItems.end()) {
         FxViewItem *item = *it;
-        if (remaining && item->index >= to && item->index < to + count) {
+        if (movedCount < count && item->index >= to && item->index < to + count) {
             // place items in the target position, reusing any existing items
-            FxGridItemSG *movedItem = moved.take(item->index);
+            int targetIndex = item->index + movedCount;
+            FxGridItemSG *movedItem = moved.take(targetIndex);
             if (!movedItem)
-                movedItem = static_cast<FxGridItemSG*>(d->createItem(item->index));
+                movedItem = static_cast<FxGridItemSG*>(d->createItem(targetIndex));
             it = d->visibleItems.insert(it, movedItem);
             ++it;
-            --remaining;
+            ++movedCount;
         } else {
             if (item->index != -1) {
                 if (item->index >= to) {

diff --git a/src/declarative/items/qsgitemview.cpp b/src/declarative/items/qsgitemview.cpp
index 18015ed..bcb4a8a 100644 (file)
--- a/src/declarative/items/qsgitemview.cpp
+++ b/src/declarative/items/qsgitemview.cpp
@@ -1089,6 +1089,18 @@ int QSGItemViewPrivate::mapFromModel(int modelIndex) const
     return -1; // Not in visibleList
 }
 
+void QSGItemViewPrivate::adjustMoveParameters(int *from, int *to, int *count) const
+{
+    if (*from > *to) {
+        // Only move forwards - flip if backwards moving
+        int tfrom = *from;
+        int tto = *to;
+        *from = tto;
+        *to = tto + *count;
+        *count = tfrom - tto;
+    }
+}
+
 void QSGItemViewPrivate::init()
 {
     Q_Q(QSGItemView);

diff --git a/src/declarative/items/qsgitemview_p_p.h b/src/declarative/items/qsgitemview_p_p.h
index bc2e45f..3113a8b 100644 (file)
--- a/src/declarative/items/qsgitemview_p_p.h
+++ b/src/declarative/items/qsgitemview_p_p.h
@@ -92,6 +92,7 @@ public:
     FxViewItem *visibleItem(int modelIndex) const;
     FxViewItem *firstVisibleItem() const;
     int mapFromModel(int modelIndex) const;
+    void adjustMoveParameters(int *from, int *to, int *count) const;
 
     virtual void init();
     virtual void updateCurrent(int modelIndex);

diff --git a/src/declarative/items/qsglistview.cpp b/src/declarative/items/qsglistview.cpp
index 7fbfe3c..37b027c 100644 (file)
--- a/src/declarative/items/qsglistview.cpp
+++ b/src/declarative/items/qsglistview.cpp
@@ -1813,11 +1813,13 @@ void QSGListView::itemsMoved(int from, int to, int count)
     }
 
     d->moveReason = QSGListViewPrivate::Other;
-    FxViewItem *firstVisible = d->firstVisibleItem();
-    QHash<int,FxViewItem*> moved;
-    int moveBy = 0;
 
     bool movingBackwards = from > to;
+    d->adjustMoveParameters(&from, &to, &count);
+
+    QHash<int,FxViewItem*> moved;
+    int moveBy = 0;
+    FxViewItem *firstVisible = d->firstVisibleItem();
     int firstItemIndex = firstVisible ? firstVisible->index : -1;
 
     // if visibleItems.first() is above the content start pos, and the items
@@ -1845,21 +1847,20 @@ void QSGListView::itemsMoved(int from, int to, int count)
         }
     }
 
-    int remaining = count;
+    int movedCount = 0;
     int endIndex = d->visibleIndex;
     it = d->visibleItems.begin();
     while (it != d->visibleItems.end()) {
         FxViewItem *item = *it;
-        if (remaining && item->index >= to && item->index < to + count) {
+        if (movedCount < count && item->index >= to && item->index < to + count) {
             // place items in the target position, reusing any existing items
-            FxViewItem *movedItem = moved.take(item->index);
+            int targetIndex = item->index + movedCount;
+            FxViewItem *movedItem = moved.take(targetIndex);
             if (!movedItem)
-                movedItem = d->createItem(item->index);
-            if (item->index <= firstVisible->index)
-                moveBy -= movedItem->size();
+                movedItem = d->createItem(targetIndex);
             it = d->visibleItems.insert(it, movedItem);
             ++it;
-            --remaining;
+            ++movedCount;
         } else {
             if (item->index != -1) {
                 if (item->index >= to) {

diff --git a/tests/auto/declarative/qsggridview/tst_qsggridview.cpp b/tests/auto/declarative/qsggridview/tst_qsggridview.cpp
index d2115e1..9a401f6 100644 (file)
--- a/tests/auto/declarative/qsggridview/tst_qsggridview.cpp
+++ b/tests/auto/declarative/qsggridview/tst_qsggridview.cpp
@@ -710,6 +710,68 @@ void tst_QSGGridView::moved_data()
             << 0.0
             << 29 << 14 << 1
             << 0.0;
+
+    QTest::newRow("move 1 backwards, from visible -> non-visible")
+            << 120.0     // show 6-23
+            << 7 << 1 << 1
+            << 60.0 * 2;   // this results in a forward movement that removes 6 items (2 rows)
+
+    QTest::newRow("move 1 backwards, from visible -> non-visible (move first item)")
+            << 120.0     // show 6-23
+            << 7 << 0 << 1
+            << 60.0 * 2;     // first visible item moved, so all items shift 2 rows down with it
+
+
+    QTest::newRow("move multiple forwards, within visible items")
+            << 0.0
+            << 0 << 5 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple forwards, from non-visible -> visible")
+            << 120.0     // show 6-23
+            << 1 << 6 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple forwards, from non-visible -> visible (move first item)")
+            << 120.0     // show 6-23
+            << 0 << 6 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple forwards, from visible -> non-visible")
+            << 0.0
+            << 1 << 16 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple forwards, from visible -> non-visible (move first item)")
+            << 0.0
+            << 0 << 16 << 3
+            << 0.0;
+
+
+    QTest::newRow("move multiple backwards, within visible items")
+            << 0.0
+            << 4 << 1 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from non-visible -> visible")
+            << 0.0
+            << 20 << 4 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from non-visible -> visible (move last item)")
+            << 0.0
+            << 27 << 10 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from visible -> non-visible")
+            << 120.0     // show 6-23
+            << 16 << 1 << 3
+            << 60.0 * 5;   // this results in a forward movement that removes 15 items (5 rows)
+
+    QTest::newRow("move multiple backwards, from visible -> non-visible (move first item)")
+            << 120.0     // show 6-23
+            << 16 << 0 << 3
+            << 60.0 * 5;    // this results in a forward movement that removes 16 items (5 rows)
 }
 
 void tst_QSGGridView::swapWithFirstItem()

diff --git a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp b/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
index e2e4b04..c7a68e5 100644 (file)
--- a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
+++ b/tests/auto/declarative/qsglistview/tst_qsglistview.cpp
@@ -960,6 +960,68 @@ void tst_QSGListView::moved_data()
             << 0.0
             << 29 << 15 << 1
             << 0.0;
+
+    QTest::newRow("move 1 backwards, from visible -> non-visible")
+            << 80.0     // show 4-19
+            << 16 << 1 << 1
+            << 20.0 * 15;   // this results in a forward movement that removes 15 items
+
+    QTest::newRow("move 1 backwards, from visible -> non-visible (move first item)")
+            << 80.0     // show 4-19
+            << 16 << 0 << 1
+            << 20.0 * 16;   // everything should move to after item 16
+
+
+    QTest::newRow("move multiple forwards, within visible items")
+            << 0.0
+            << 0 << 5 << 3
+            << 20.0 * 3;
+
+    QTest::newRow("move multiple forwards, from non-visible -> visible")
+            << 80.0     // show 4-19
+            << 1 << 5 << 3
+            << 20.0 * 3;    // moving 3 from above the content y should adjust y positions accordingly
+
+    QTest::newRow("move multiple forwards, from non-visible -> visible (move first item)")
+            << 80.0     // show 4-19
+            << 0 << 5 << 3
+            << 20.0 * 3;        // moving 3 from above the content y should adjust y positions accordingly
+
+    QTest::newRow("move multiple forwards, from visible -> non-visible")
+            << 0.0
+            << 1 << 16 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple forwards, from visible -> non-visible (move first item)")
+            << 0.0
+            << 0 << 16 << 3
+            << 20.0 * 3;
+
+
+    QTest::newRow("move multiple backwards, within visible items")
+            << 0.0
+            << 4 << 1 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from non-visible -> visible")
+            << 0.0
+            << 20 << 4 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from non-visible -> visible (move last item)")
+            << 0.0
+            << 27 << 10 << 3
+            << 0.0;
+
+    QTest::newRow("move multiple backwards, from visible -> non-visible")
+            << 80.0     // show 4-19
+            << 16 << 1 << 3
+            << 20.0 * 15;   // this results in a forward movement that removes 15 items
+
+    QTest::newRow("move multiple backwards, from visible -> non-visible (move first item)")
+            << 80.0     // show 4-19
+            << 16 << 0 << 3
+            << 20.0 * 16;
 }
 
 void tst_QSGListView::swapWithFirstItem()