From: Bea Lam Date: Wed, 27 Jul 2011 08:19:01 +0000 (+1000) Subject: Fix positioning of items after move X-Git-Url: http://git.silmor.de/gitweb/?a=commitdiff_plain;h=ae4bc85da09983c7c2ea36a283b6c123223b3287;p=konrad%2Fqtdeclarative.git 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 Reviewed-by: Martin Jones --- diff --git a/src/declarative/items/qsggridview.cpp b/src/declarative/items/qsggridview.cpp index 1aa7292..8559379 100644 --- a/src/declarative/items/qsggridview.cpp +++ b/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(d->firstVisibleItem()); QHash 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::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(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(d->visibleItems.first()); + first->setPosition(first->colPos(), first->rowPos() + ((moveByCount / d->columns) * d->rowSize())); + } + d->layout(); } diff --git a/src/declarative/items/qsglistview.cpp b/src/declarative/items/qsglistview.cpp index 0d6a494..abfb4fe 100644 --- a/src/declarative/items/qsglistview.cpp +++ b/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 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::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,7 +1919,8 @@ void QSGListView::itemsMoved(int from, int to, int count) } // Ensure we don't cause an ugly list scroll. - static_cast(d->visibleItems.first())->setPosition(d->visibleItems.first()->position() + moveBy); + if (d->visibleItems.count()) + static_cast(d->visibleItems.first())->setPosition(d->visibleItems.first()->position() + moveBy); d->updateSections(); d->layout(); diff --git a/tests/auto/declarative/qsggridview/tst_qsggridview.cpp b/tests/auto/declarative/qsggridview/tst_qsggridview.cpp index 76b3032..d2115e1 100644 --- a/tests/auto/declarative/qsggridview/tst_qsggridview.cpp +++ b/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 findItems(QSGItem *parent, const QString &objectName); void dumpTree(QSGItem *parent, int depth = 0); }; + +template +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 (; ibegin(); it += from; + for (; ibegin(); 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(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); - - QSGText *name = findItem(contentItem, "textName", 1); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(1)); - QSGText *number = findItem(contentItem, "textNumber", 1); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(1)); + QSGItem *currentItem = gridview->currentItem(); + QTRY_VERIFY(currentItem != 0); - name = findItem(contentItem, "textName", 8); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(8)); - number = findItem(contentItem, "textNumber", 8); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(8)); + gridview->setContentY(contentY); + model.moveItems(from, to, count); - // Confirm items positioned correctly + // Confirm items positioned correctly and indexes correct + int firstVisibleIndex = qCeil(contentY / 60.0) * 3; int itemCount = findItems(contentItem, "wrapper").count(); - for (int i = 0; i < model.count() && i < itemCount; ++i) { - QSGItem *item = findItem(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); - } - gridview->setContentY(120); - - // move outside visible area - model.moveItem(1, 25); - - // Confirm items positioned correctly and indexes correct - itemCount = findItems(contentItem, "wrapper").count()-1; - for (int i = 6; i < model.count()-6 && i < itemCount+6; ++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(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(contentItem, "textName", i); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(i)); - number = findItem(contentItem, "textNumber", i); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(i)); - } + QVERIFY2(item, QTest::toString(QString("Item %1 not found").arg(i))); - // move from outside visible into visible - model.moveItem(28, 8); - - // Confirm items positioned correctly and indexes correct - for (int i = 6; i < model.count()-6 && i < itemCount+6; ++i) { - QSGItem *item = findItem(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); + QTRY_VERIFY(item->y() == (i/3)*60 + itemsOffsetAfterMove); + name = findItem(contentItem, "textName", i); - QTRY_VERIFY(name != 0); + QVERIFY(name != 0); QTRY_COMPARE(name->text(), model.name(i)); number = findItem(contentItem, "textNumber", i); - QTRY_VERIFY(number != 0); + QVERIFY(number != 0); QTRY_COMPARE(number->text(), model.number(i)); + + // current index should have been updated + if (item == currentItem) + QTRY_COMPARE(gridview->currentIndex(), i); } delete canvas; } +void tst_QSGGridView::moved_data() +{ + QTest::addColumn("contentY"); + QTest::addColumn("from"); + QTest::addColumn("to"); + QTest::addColumn("count"); + QTest::addColumn("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" + diff --git a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp b/tests/auto/declarative/qsglistview/tst_qsglistview.cpp index 19bc0f0..30daeb8 100644 --- a/tests/auto/declarative/qsglistview/tst_qsglistview.cpp +++ b/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 QList 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 +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 (; ibegin(); it += from; + for (; ibegin(); 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(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(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 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(contentItem, "textName", 1); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(1)); - QSGText *number = findItem(contentItem, "textNumber", 1); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(1)); - - name = findItem(contentItem, "textName", 4); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(4)); - number = findItem(contentItem, "textNumber", 4); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(4)); + listview->setContentY(contentY); + model.moveItems(from, to, count); + qApp->processEvents(); - // Confirm items positioned correctly + // Confirm items positioned correctly and indexes correct + int firstVisibleIndex = qCeil(contentY / 20.0); int itemCount = findItems(contentItem, "wrapper").count(); - for (int i = 0; i < model.count() && i < itemCount; ++i) { - QSGItem *item = findItem(contentItem, "wrapper", i); - if (!item) qWarning() << "Item" << i << "not found"; - QTRY_VERIFY(item); - 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) { + for (int i = firstVisibleIndex; i < model.count() && i < itemCount; ++i) { + if (i >= firstVisibleIndex + 16) // index has moved out of view + continue; QSGItem *item = findItem(contentItem, "wrapper", i); - if (!item) qWarning() << "Item" << i << "not found"; - QTRY_VERIFY(item); - QTRY_COMPARE(item->y(), i*20.0 + 20); + QVERIFY2(item, QTest::toString(QString("Item %1 not found").arg(i))); + QTRY_COMPARE(item->y(), i*20.0 + itemsOffsetAfterMove); name = findItem(contentItem, "textName", i); - QTRY_VERIFY(name != 0); + QVERIFY(name != 0); QTRY_COMPARE(name->text(), model.name(i)); number = findItem(contentItem, "textNumber", i); - QTRY_VERIFY(number != 0); + QVERIFY(number != 0); QTRY_COMPARE(number->text(), model.number(i)); - } - - // move from outside visible into visible - model.moveItem(20, 4); - // Confirm items positioned correctly and indexes correct - for (int i = 3; i < model.count() && i < itemCount; ++i) { - QSGItem *item = findItem(contentItem, "wrapper", i); - if (!item) qWarning() << "Item" << i << "not found"; - QTRY_VERIFY(item); - QTRY_COMPARE(item->y(), i*20.0 + 20); - name = findItem(contentItem, "textName", i); - QTRY_VERIFY(name != 0); - QTRY_COMPARE(name->text(), model.name(i)); - number = findItem(contentItem, "textNumber", i); - QTRY_VERIFY(number != 0); - QTRY_COMPARE(number->text(), model.number(i)); + // current index should have been updated + if (item == currentItem) + QTRY_COMPARE(listview->currentIndex(), i); } delete canvas; delete testObject; } +void tst_QSGListView::moved_data() +{ + QTest::addColumn("contentY"); + QTest::addColumn("from"); + QTest::addColumn("to"); + QTest::addColumn("count"); + QTest::addColumn("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(); } +void tst_QSGListView::qListModelInterface_moved_data() +{ + moved_data(); +} + void tst_QSGListView::qAbstractItemModel_moved() { moved(); } +void tst_QSGListView::qAbstractItemModel_moved_data() +{ + moved_data(); +} + void tst_QSGListView::qListModelInterface_clear() { clear(); @@ -2937,3 +3021,4 @@ void tst_QSGListView::dumpTree(QSGItem *parent, int depth) QTEST_MAIN(tst_QSGListView) #include "tst_qsglistview.moc" +