Fix positioning of items after move

If a move happened after a contentY change but before a refill, the
position of the first visible item wasn't calculated correctly.

If the first item from visibleItems was above the content start
position and a move operation caused the items below it to move
away, this first item was not being moved to the correct position
(i.e. above the next available visible item) causing the following
visible items to be positioned incorrectly on the next refill.

ListView supported this previously but only adjusted positioning for
items before the content position, instead of for all moved items.

Fixed for both ListView and GridView and added more move-related tests.

Task-number: QTBUG-20528
Change-Id: I2ba1a2f5e49f790e694c6e1486f649f10d09c256
Reviewed-on: http://codereview.qt.nokia.com/2261
Reviewed-by: Qt Sanity Bot <qt_sanity_bot@ovi.com>
Reviewed-by: Martin Jones <martin.jones@nokia.com>

src/declarative/items/qsggridview.cpp

@@ -1500,10 +1500,18 @@ 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;
     int firstItemIndex = firstVisible ? firstVisible->index : -1;
 
+    // if visibleItems.first() is above the content start pos, and the items
+    // beneath it are moved, ensure this first item is later repositioned correctly
+    // (to above the next visible item) so that subsequent layout() is correct
+    bool repositionFirstItem = firstVisible
+            && d->visibleItems.first()->position() < firstVisible->position()
+            && from > d->visibleItems.first()->index;
+
     QList<FxViewItem*>::Iterator it = d->visibleItems.begin();
     while (it != d->visibleItems.end()) {
         FxViewItem *item = *it;
@@ -1511,6 +1519,8 @@ void QSGGridView::itemsMoved(int from, int to, int count)
             // take the items that are moving
             item->index += (to-from);
             moved.insert(item->index, static_cast<FxGridItemSG*>(item));
+            if (repositionFirstItem)
+                moveByCount++;
             it = d->visibleItems.erase(it);
         } else {
             if (item->index > from && item->index != -1) {
@@ -1594,6 +1604,12 @@ void QSGGridView::itemsMoved(int from, int to, int count)
         d->releaseItem(item);
     }
 
+    // Ensure we don't cause an ugly list scroll.
+    if (d->visibleItems.count() && moveByCount > 0) {
+        FxGridItemSG *first = static_cast<FxGridItemSG*>(d->visibleItems.first());
+        first->setPosition(first->colPos(), first->rowPos() + ((moveByCount / d->columns) * d->rowSize()));
+    }
+
     d->layout();
 }
 

src/declarative/items/qsglistview.cpp

@@ -1815,13 +1815,19 @@ void QSGListView::itemsMoved(int from, int to, int count)
 
     d->moveReason = QSGListViewPrivate::Other;
     FxViewItem *firstVisible = d->firstVisibleItem();
-    qreal firstItemPos = firstVisible->position();
     QHash<int,FxViewItem*> moved;
     int moveBy = 0;
 
     bool movingBackwards = from > to;
     int firstItemIndex = firstVisible ? firstVisible->index : -1;
 
+    // if visibleItems.first() is above the content start pos, and the items
+    // beneath it are moved, ensure this first item is later repositioned correctly
+    // (to above the next visible item) so that subsequent layout() is correct
+    bool repositionFirstItem = firstVisible
+            && d->visibleItems.first()->position() < firstVisible->position()
+            && from > d->visibleItems.first()->index;
+
     QList<FxViewItem*>::Iterator it = d->visibleItems.begin();
     while (it != d->visibleItems.end()) {
         FxViewItem *item = *it;
@@ -1829,7 +1835,7 @@ void QSGListView::itemsMoved(int from, int to, int count)
             // take the items that are moving
             item->index += (to-from);
             moved.insert(item->index, item);
-            if (item->position() < firstItemPos)
+            if (repositionFirstItem)
                 moveBy += item->size();
             it = d->visibleItems.erase(it);
         } else {
@@ -1913,6 +1919,7 @@ void QSGListView::itemsMoved(int from, int to, int count)
     }
 
     // Ensure we don't cause an ugly list scroll.
+    if (d->visibleItems.count())
         static_cast<FxListItemSG*>(d->visibleItems.first())->setPosition(d->visibleItems.first()->position() + moveBy);
 
     d->updateSections();

tests/auto/declarative/qsggridview/tst_qsggridview.cpp

@@ -77,6 +77,7 @@ private slots:
     void removed();
     void clear();
     void moved();
+    void moved_data();
     void swapWithFirstItem();
     void changeFlow();
     void currentIndex();
@@ -115,6 +116,29 @@ private:
     QList<T*> findItems(QSGItem *parent, const QString &objectName);
     void dumpTree(QSGItem *parent, int depth = 0);
 };
+
+template<typename T>
+void tst_qsggridview_move(int from, int to, int n, T *items)
+{
+    if (n == 1) {
+        items->move(from, to);
+    } else {
+        T replaced;
+        int i=0;
+        typename T::ConstIterator it=items->begin(); it += from+n;
+        for (; i<to-from; ++i,++it)
+            replaced.append(*it);
+        i=0;
+        it=items->begin(); it += from;
+        for (; i<n; ++i,++it)
+            replaced.append(*it);
+        typename T::ConstIterator f=replaced.begin();
+        typename T::Iterator t=items->begin(); t += from;
+        for (; f != replaced.end(); ++f, ++t)
+            *t = *f;
+    }
+}
+
 void tst_QSGGridView::initTestCase()
 {
     QSGView canvas;
@@ -191,6 +215,12 @@ public:
         emit endMoveRows();
     }
 
+    void moveItems(int from, int to, int count) {
+        emit beginMoveRows(QModelIndex(), from, from+count-1, QModelIndex(), to > from ? to+count : to);
+        tst_qsggridview_move(from, to, count, &list);
+        emit endMoveRows();
+    }
+
     void modifyItem(int idx, const QString &name, const QString &number) {
         list[idx] = QPair<QString,QString>(name, number);
         emit dataChanged(index(idx,0), index(idx,0));
@@ -566,7 +596,16 @@ void tst_QSGGridView::clear()
 
 void tst_QSGGridView::moved()
 {
+    QFETCH(qreal, contentY);
+    QFETCH(int, from);
+    QFETCH(int, to);
+    QFETCH(int, count);
+    QFETCH(qreal, itemsOffsetAfterMove);
+
+    QSGText *name;
+    QSGText *number;
     QSGView *canvas = createView();
+    canvas->show();
 
     TestModel model;
     for (int i = 0; i < 30; i++)
@@ -586,74 +625,93 @@ void tst_QSGGridView::moved()
     QSGItem *contentItem = gridview->contentItem();
     QTRY_VERIFY(contentItem != 0);
 
-    model.moveItem(1, 8);
+    QSGItem *currentItem = gridview->currentItem();
+    QTRY_VERIFY(currentItem != 0);
 
-    QSGText *name = findItem<QSGText>(contentItem, "textName", 1);
+    gridview->setContentY(contentY);
-    QTRY_VERIFY(name != 0);
+    model.moveItems(from, to, count);
-    QTRY_COMPARE(name->text(), model.name(1));
-    QSGText *number = findItem<QSGText>(contentItem, "textNumber", 1);
-    QTRY_VERIFY(number != 0);
-    QTRY_COMPARE(number->text(), model.number(1));
 
-    name = findItem<QSGText>(contentItem, "textName", 8);
+    // Confirm items positioned correctly and indexes correct
-    QTRY_VERIFY(name != 0);
+    int firstVisibleIndex = qCeil(contentY / 60.0) * 3;
-    QTRY_COMPARE(name->text(), model.name(8));
-    number = findItem<QSGText>(contentItem, "textNumber", 8);
-    QTRY_VERIFY(number != 0);
-    QTRY_COMPARE(number->text(), model.number(8));
-
-    // Confirm items positioned correctly
     int itemCount = findItems<QSGItem>(contentItem, "wrapper").count();
-    for (int i = 0; i < model.count() && i < itemCount; ++i) {
+
+    for (int i = firstVisibleIndex; i < model.count() && i < itemCount; ++i) {
+        if (i >= firstVisibleIndex + 18)    // index has moved out of view
+            continue;
         QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
+        QVERIFY2(item, QTest::toString(QString("Item %1 not found").arg(i)));
-        QTRY_VERIFY(item);
+
         QTRY_VERIFY(item->x() == (i%3)*80);
-        QTRY_VERIFY(item->y() == (i/3)*60);
+        QTRY_VERIFY(item->y() == (i/3)*60 + itemsOffsetAfterMove);
-    }
 
-    gridview->setContentY(120);
-
-    // move outside visible area
-    model.moveItem(1, 25);
-
-    // Confirm items positioned correctly and indexes correct
-    itemCount = findItems<QSGItem>(contentItem, "wrapper").count()-1;
-    for (int i = 6; i < model.count()-6 && i < itemCount+6; ++i) {
-        QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
-        QTRY_VERIFY(item);
-        QTRY_COMPARE(item->x(), qreal((i%3)*80));
-        QTRY_COMPARE(item->y(), qreal((i/3)*60));
         name = findItem<QSGText>(contentItem, "textName", i);
-        QTRY_VERIFY(name != 0);
+        QVERIFY(name != 0);
         QTRY_COMPARE(name->text(), model.name(i));
         number = findItem<QSGText>(contentItem, "textNumber", i);
-        QTRY_VERIFY(number != 0);
+        QVERIFY(number != 0);
         QTRY_COMPARE(number->text(), model.number(i));
-    }
 
-    // move from outside visible into visible
+        // current index should have been updated
-    model.moveItem(28, 8);
+        if (item == currentItem)
-
+            QTRY_COMPARE(gridview->currentIndex(), i);
-    // Confirm items positioned correctly and indexes correct
-    for (int i = 6; i < model.count()-6 && i < itemCount+6; ++i) {
-        QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
-        QTRY_VERIFY(item);
-        QTRY_VERIFY(item->x() == (i%3)*80);
-        QTRY_VERIFY(item->y() == (i/3)*60);
-        name = findItem<QSGText>(contentItem, "textName", i);
-        QTRY_VERIFY(name != 0);
-        QTRY_COMPARE(name->text(), model.name(i));
-        number = findItem<QSGText>(contentItem, "textNumber", i);
-        QTRY_VERIFY(number != 0);
-        QTRY_COMPARE(number->text(), model.number(i));
     }
 
     delete canvas;
 }
 
+void tst_QSGGridView::moved_data()
+{
+    QTest::addColumn<qreal>("contentY");
+    QTest::addColumn<int>("from");
+    QTest::addColumn<int>("to");
+    QTest::addColumn<int>("count");
+    QTest::addColumn<qreal>("itemsOffsetAfterMove");
+
+    // model starts with 30 items, each 80x60, in area 240x320
+    // 18 items should be visible at a time
+
+    QTest::newRow("move 1 forwards, within visible items")
+            << 0.0
+            << 1 << 8 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from non-visible -> visible")
+            << 120.0     // show 6-23
+            << 1 << 23 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from non-visible -> visible (move first item)")
+            << 120.0     // // show 6-23
+            << 0 << 6 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from visible -> non-visible")
+            << 0.0
+            << 1 << 20 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from visible -> non-visible (move first item)")
+            << 0.0
+            << 0 << 20 << 1
+            << 0.0;
+
+
+    QTest::newRow("move 1 backwards, within visible items")
+            << 0.0
+            << 10 << 5 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 backwards, from non-visible -> visible")
+            << 0.0
+            << 28 << 8 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 backwards, from non-visible -> visible (move last item)")
+            << 0.0
+            << 29 << 14 << 1
+            << 0.0;
+}
+
 void tst_QSGGridView::swapWithFirstItem()
 {
     // QTBUG_9697
@@ -2395,3 +2453,4 @@ void tst_QSGGridView::dumpTree(QSGItem *parent, int depth)
 QTEST_MAIN(tst_QSGGridView)
 
 #include "tst_qsggridview.moc"
+

tests/auto/declarative/qsglistview/tst_qsglistview.cpp

@@ -86,7 +86,9 @@ private slots:
     void qAbstractItemModel_removed();
 
     void qListModelInterface_moved();
+    void qListModelInterface_moved_data();
     void qAbstractItemModel_moved();
+    void qAbstractItemModel_moved_data();
 
     void qListModelInterface_clear();
     void qAbstractItemModel_clear();
@@ -141,6 +143,8 @@ private:
     template<typename T>
     QList<T*> findItems(QSGItem *parent, const QString &objectName);
     void dumpTree(QSGItem *parent, int depth = 0);
+
+    void moved_data();
 };
 
 void tst_QSGListView::initTestCase()
@@ -193,6 +197,36 @@ public:
     int mCacheBuffer;
 };
 
+template<typename T>
+void tst_qsglistview_move(int from, int to, int n, T *items)
+{
+    if (from > to) {
+        // Only move forwards - flip if backwards moving
+        int tfrom = from;
+        int tto = to;
+        from = tto;
+        to = tto+n;
+        n = tfrom-tto;
+    }
+    if (n == 1) {
+        items->move(from, to);
+    } else {
+        T replaced;
+        int i=0;
+        typename T::ConstIterator it=items->begin(); it += from+n;
+        for (; i<to-from; ++i,++it)
+            replaced.append(*it);
+        i=0;
+        it=items->begin(); it += from;
+        for (; i<n; ++i,++it)
+            replaced.append(*it);
+        typename T::ConstIterator f=replaced.begin();
+        typename T::Iterator t=items->begin(); t += from;
+        for (; f != replaced.end(); ++f, ++t)
+            *t = *f;
+    }
+}
+
 class TestModel : public QListModelInterface
 {
     Q_OBJECT
@@ -275,6 +309,11 @@ public:
         emit itemsMoved(from, to, 1);
     }
 
+    void moveItems(int from, int to, int count) {
+        tst_qsglistview_move(from, to, count, &list);
+        emit itemsMoved(from, to, count);
+    }
+
     void modifyItem(int index, const QString &name, const QString &number) {
         list[index] = QPair<QString,QString>(name, number);
         emit itemsChanged(index, 1, roles());
@@ -356,6 +395,12 @@ public:
         emit endMoveRows();
     }
 
+    void moveItems(int from, int to, int count) {
+        emit beginMoveRows(QModelIndex(), from, from+count-1, QModelIndex(), to > from ? to+count : to);
+        tst_qsglistview_move(from, to, count, &list);
+        emit endMoveRows();
+    }
+
     void modifyItem(int idx, const QString &name, const QString &number) {
         list[idx] = QPair<QString,QString>(name, number);
         emit dataChanged(index(idx,0), index(idx,0));
@@ -592,7 +637,6 @@ void tst_QSGListView::removed(bool animated)
     ctxt->setContextProperty("testModel", &model);
 
     TestObject *testObject = new TestObject;
-    testObject->setAnimate(animated);
     ctxt->setContextProperty("testObject", testObject);
 
     canvas->setSource(QUrl::fromLocalFile(SRCDIR "/data/listviewtest.qml"));
@@ -790,11 +834,19 @@ void tst_QSGListView::clear()
     delete testObject;
 }
 
-
 template <class T>
 void tst_QSGListView::moved()
 {
+    QFETCH(qreal, contentY);
+    QFETCH(int, from);
+    QFETCH(int, to);
+    QFETCH(int, count);
+    QFETCH(qreal, itemsOffsetAfterMove);
+
+    QSGText *name;
+    QSGText *number;
     QSGView *canvas = createView();
+    canvas->show();
 
     T model;
     for (int i = 0; i < 30; i++)
@@ -815,71 +867,93 @@ void tst_QSGListView::moved()
     QSGItem *contentItem = listview->contentItem();
     QTRY_VERIFY(contentItem != 0);
 
-    model.moveItem(1, 4);
+    QSGItem *currentItem = listview->currentItem();
+    QTRY_VERIFY(currentItem != 0);
 
-    QSGText *name = findItem<QSGText>(contentItem, "textName", 1);
+    listview->setContentY(contentY);
-    QTRY_VERIFY(name != 0);
+    model.moveItems(from, to, count);
-    QTRY_COMPARE(name->text(), model.name(1));
+    qApp->processEvents();
-    QSGText *number = findItem<QSGText>(contentItem, "textNumber", 1);
-    QTRY_VERIFY(number != 0);
-    QTRY_COMPARE(number->text(), model.number(1));
 
-    name = findItem<QSGText>(contentItem, "textName", 4);
+    // Confirm items positioned correctly and indexes correct
-    QTRY_VERIFY(name != 0);
+    int firstVisibleIndex = qCeil(contentY / 20.0);
-    QTRY_COMPARE(name->text(), model.name(4));
-    number = findItem<QSGText>(contentItem, "textNumber", 4);
-    QTRY_VERIFY(number != 0);
-    QTRY_COMPARE(number->text(), model.number(4));
-
-    // Confirm items positioned correctly
     int itemCount = findItems<QSGItem>(contentItem, "wrapper").count();
-    for (int i = 0; i < model.count() && i < itemCount; ++i) {
+
+    for (int i = firstVisibleIndex; i < model.count() && i < itemCount; ++i) {
+        if (i >= firstVisibleIndex + 16)    // index has moved out of view
+            continue;
         QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
+        QVERIFY2(item, QTest::toString(QString("Item %1 not found").arg(i)));
-        QTRY_VERIFY(item);
+        QTRY_COMPARE(item->y(), i*20.0 + itemsOffsetAfterMove);
-        QTRY_VERIFY(item->y() == i*20);
-    }
-
-    listview->setContentY(80);
-
-    // move outside visible area
-    model.moveItem(1, 18);
-
-    // Confirm items positioned correctly and indexes correct
-    for (int i = 3; i < model.count() && i < itemCount; ++i) {
-        QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
-        QTRY_VERIFY(item);
-        QTRY_COMPARE(item->y(), i*20.0 + 20);
         name = findItem<QSGText>(contentItem, "textName", i);
-        QTRY_VERIFY(name != 0);
+        QVERIFY(name != 0);
         QTRY_COMPARE(name->text(), model.name(i));
         number = findItem<QSGText>(contentItem, "textNumber", i);
-        QTRY_VERIFY(number != 0);
+        QVERIFY(number != 0);
         QTRY_COMPARE(number->text(), model.number(i));
-    }
 
-    // move from outside visible into visible
+        // current index should have been updated
-    model.moveItem(20, 4);
+        if (item == currentItem)
-
+            QTRY_COMPARE(listview->currentIndex(), i);
-    // Confirm items positioned correctly and indexes correct
-    for (int i = 3; i < model.count() && i < itemCount; ++i) {
-        QSGItem *item = findItem<QSGItem>(contentItem, "wrapper", i);
-        if (!item) qWarning() << "Item" << i << "not found";
-        QTRY_VERIFY(item);
-        QTRY_COMPARE(item->y(), i*20.0 + 20);
-        name = findItem<QSGText>(contentItem, "textName", i);
-        QTRY_VERIFY(name != 0);
-        QTRY_COMPARE(name->text(), model.name(i));
-        number = findItem<QSGText>(contentItem, "textNumber", i);
-        QTRY_VERIFY(number != 0);
-        QTRY_COMPARE(number->text(), model.number(i));
     }
 
     delete canvas;
     delete testObject;
 }
 
+void tst_QSGListView::moved_data()
+{
+    QTest::addColumn<qreal>("contentY");
+    QTest::addColumn<int>("from");
+    QTest::addColumn<int>("to");
+    QTest::addColumn<int>("count");
+    QTest::addColumn<qreal>("itemsOffsetAfterMove");
+
+    // model starts with 30 items, each 20px high, in area 320px high
+    // 16 items should be visible at a time
+    // itemsOffsetAfterMove should be > 0 whenever items above the visible pos have moved
+
+    QTest::newRow("move 1 forwards, within visible items")
+            << 0.0
+            << 1 << 4 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from non-visible -> visible")
+            << 80.0     // show 4-19
+            << 1 << 18 << 1
+            << 20.0;
+
+    QTest::newRow("move 1 forwards, from non-visible -> visible (move first item)")
+            << 80.0     // show 4-19
+            << 0 << 4 << 1
+            << 20.0;
+
+    QTest::newRow("move 1 forwards, from visible -> non-visible")
+            << 0.0
+            << 1 << 16 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 forwards, from visible -> non-visible (move first item)")
+            << 0.0
+            << 0 << 16 << 1
+            << 20.0;
+
+
+    QTest::newRow("move 1 backwards, within visible items")
+            << 0.0
+            << 4 << 1 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 backwards, from non-visible -> visible")
+            << 0.0
+            << 20 << 4 << 1
+            << 0.0;
+
+    QTest::newRow("move 1 backwards, from non-visible -> visible (move last item)")
+            << 0.0
+            << 29 << 15 << 1
+            << 0.0;
+}
+
 void tst_QSGListView::swapWithFirstItem()
 {
     QSGView *canvas = createView();
@@ -903,7 +977,7 @@ void tst_QSGListView::swapWithFirstItem()
 
     // ensure content position is stable
     listview->setContentY(0);
-    model.moveItem(10, 0);
+    model.moveItem(1, 0);
     QTRY_VERIFY(listview->contentY() == 0);
 
     delete testObject;
@@ -2848,11 +2922,21 @@ void tst_QSGListView::qListModelInterface_moved()
     moved<TestModel>();
 }
 
+void tst_QSGListView::qListModelInterface_moved_data()
+{
+    moved_data();
+}
+
 void tst_QSGListView::qAbstractItemModel_moved()
 {
     moved<TestModel2>();
 }
 
+void tst_QSGListView::qAbstractItemModel_moved_data()
+{
+    moved_data();
+}
+
 void tst_QSGListView::qListModelInterface_clear()
 {
     clear<TestModel>();
@@ -2937,3 +3021,4 @@ void tst_QSGListView::dumpTree(QSGItem *parent, int depth)
 QTEST_MAIN(tst_QSGListView)
 
 #include "tst_qsglistview.moc"
+