From 371b2f6947779272494b34ec44445aaad0613756 Mon Sep 17 00:00:00 2001 From: Martin Jones Date: Tue, 22 Nov 2011 17:26:49 +1000 Subject: [PATCH] Fix PathView insertion/removal/move item offset Fix item positioning post model changes and add auto tests. Also fixes crash when inserting items before currentIndex, causing offset to increase beyond item count. Task-number: TBUG-22785 Change-Id: I17000ba497a190554c8b137a72b7e6551e8a0e56 Reviewed-by: Bea Lam --- src/declarative/items/qquickpathview.cpp | 43 ++-- .../qquickpathview/tst_qquickpathview.cpp | 314 ++++++++++++++++++++ 2 files changed, 334 insertions(+), 23 deletions(-) diff --git a/src/declarative/items/qquickpathview.cpp b/src/declarative/items/qquickpathview.cpp index f7e58ea..1963307 100644 --- a/src/declarative/items/qquickpathview.cpp +++ b/src/declarative/items/qquickpathview.cpp @@ -843,6 +843,10 @@ void QQuickPathView::setHighlightRangeMode(HighlightRangeMode mode) return; d->highlightRangeMode = mode; d->haveHighlightRange = d->highlightRangeMode != NoHighlightRange && d->highlightRangeStart <= d->highlightRangeEnd; + if (d->haveHighlightRange) { + d->regenerate(); + d->snapToCurrent(); + } emit highlightRangeModeChanged(); } @@ -1502,10 +1506,7 @@ void QQuickPathView::modelUpdated(const QDeclarativeChangeSet &changeSet, bool r int moveOffset; bool currentChanged = false; bool changedOffset = false; - bool removed = false; - bool inserted = false; foreach (const QDeclarativeChangeSet::Remove &r, changeSet.removes()) { - removed = true; if (moveId == -1 && d->currentIndex >= r.index + r.count) { d->currentIndex -= r.count; currentChanged = true; @@ -1525,32 +1526,36 @@ void QQuickPathView::modelUpdated(const QDeclarativeChangeSet &changeSet, bool r } if (r.index > d->currentIndex) { - if (d->offset >= r.count) { - changedOffset = true; - d->offset -= r.count; - d->offsetAdj -= r.count; - } + changedOffset = true; + d->offset -= r.count; + d->offsetAdj -= r.count; } d->modelCount -= r.count; } foreach (const QDeclarativeChangeSet::Insert &i, changeSet.inserts()) { - inserted = true; if (d->modelCount) { if (moveId == -1 && i.index <= d->currentIndex) { d->currentIndex += i.count; currentChanged = true; - } else if (d->offset != 0) { + } else { if (moveId != -1 && moveId == i.moveId) { d->currentIndex = i.index + moveOffset; currentChanged = true; } - d->offset += i.count; - d->offsetAdj += i.count; + if (i.index > d->currentIndex) { + d->offset += i.count; + d->offsetAdj += i.count; + changedOffset = true; + } } } d->modelCount += i.count; } + d->offset = qmlMod(d->offset, d->modelCount); + if (d->offset < 0) + d->offset += d->modelCount; + d->itemCache += d->items; d->items.clear(); @@ -1560,22 +1565,14 @@ void QQuickPathView::modelUpdated(const QDeclarativeChangeSet &changeSet, bool r d->offset = 0; changedOffset = true; d->tl.reset(d->moveOffset); - } else if (removed) { - d->regenerate(); + } else { if (!d->flicking && !d->moving && d->haveHighlightRange && d->highlightRangeMode == QQuickPathView::StrictlyEnforceRange) { - qreal targetOffset = qmlMod(d->modelCount - d->currentIndex, d->modelCount); - if (targetOffset != d->offset) - d->tl.set(d->moveOffset, targetOffset); + d->offset = qmlMod(d->modelCount - d->currentIndex, d->modelCount); + changedOffset = true; } - } else if (inserted) { d->firstIndex = -1; d->updateMappedRange(); d->scheduleLayout(); - if (!d->flicking && !d->moving && d->haveHighlightRange && d->highlightRangeMode == QQuickPathView::StrictlyEnforceRange) { - qreal targetOffset = qmlMod(d->modelCount - d->currentIndex, d->modelCount); - if (targetOffset != d->offset) - d->tl.set(d->moveOffset, targetOffset); - } } if (changedOffset) emit offsetChanged(); diff --git a/tests/auto/declarative/qquickpathview/tst_qquickpathview.cpp b/tests/auto/declarative/qquickpathview/tst_qquickpathview.cpp index 8b0ecc2..ee7f993 100644 --- a/tests/auto/declarative/qquickpathview/tst_qquickpathview.cpp +++ b/tests/auto/declarative/qquickpathview/tst_qquickpathview.cpp @@ -91,6 +91,12 @@ private slots: void dataModel(); void pathview2(); void pathview3(); + void insertModel_data(); + void insertModel(); + void removeModel_data(); + void removeModel(); + void moveModel_data(); + void moveModel(); void path(); void pathMoved(); void setCurrentIndex(); @@ -199,23 +205,72 @@ public: endInsertRows(); } + void insertItems(int index, const QList > &items) { + beginInsertRows(QModelIndex(), index, index+items.count()-1); + for (int i=0; i(items[i].first, items[i].second)); + endInsertRows(); + } + void removeItem(int index) { beginRemoveRows(QModelIndex(), index, index); list.removeAt(index); endRemoveRows(); } + void removeItems(int index, int count) { + emit beginRemoveRows(QModelIndex(), index, index+count-1); + while (count--) + list.removeAt(index); + emit endRemoveRows(); + } + void moveItem(int from, int to) { beginMoveRows(QModelIndex(), from, from, QModelIndex(), to); list.move(from, to); endMoveRows(); } + void moveItems(int from, int to, int count) { + beginMoveRows(QModelIndex(), from, from+count-1, QModelIndex(), to > from ? to+count : to); + move(from, to, count); + endMoveRows(); + } + void modifyItem(int idx, const QString &name, const QString &number) { list[idx] = QPair(name, number); emit dataChanged(index(idx,0), index(idx,0)); } + void move(int from, int to, int n) + { + 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) { + list.move(from, to); + } else { + QList > replaced; + int i=0; + QList >::ConstIterator it=list.begin(); it += from+n; + for (; i >::ConstIterator f=replaced.begin(); + QList >::Iterator t=list.begin(); t += from; + for (; f != replaced.end(); ++f, ++t) + *t = *f; + } + } + private: QList > list; }; @@ -330,6 +385,265 @@ void tst_QQuickPathView::pathview3() delete obj; } +void tst_QQuickPathView::insertModel_data() +{ + QTest::addColumn("mode"); + QTest::addColumn("idx"); + QTest::addColumn("count"); + QTest::addColumn("offset"); + + // We have 8 items, with currentIndex == 4 + QTest::newRow("insert after current") + << int(QQuickPathView::StrictlyEnforceRange) << 6 << 1 << 5.; + QTest::newRow("insert before current") + << int(QQuickPathView::StrictlyEnforceRange) << 2 << 1 << 4.; + QTest::newRow("insert multiple after current") + << int(QQuickPathView::StrictlyEnforceRange) << 5 << 2 << 6.; + QTest::newRow("insert multiple before current") + << int(QQuickPathView::StrictlyEnforceRange) << 1 << 2 << 4.; + QTest::newRow("insert at end") + << int(QQuickPathView::StrictlyEnforceRange) << 8 << 1 << 5.; + QTest::newRow("insert at beginning") + << int(QQuickPathView::StrictlyEnforceRange) << 0 << 1 << 4.; + QTest::newRow("insert at current") + << int(QQuickPathView::StrictlyEnforceRange) << 4 << 1 << 4.; + + QTest::newRow("no range - insert after current") + << int(QQuickPathView::NoHighlightRange) << 6 << 1 << 5.; + QTest::newRow("no range - insert before current") + << int(QQuickPathView::NoHighlightRange) << 2 << 1 << 4.; + QTest::newRow("no range - insert multiple after current") + << int(QQuickPathView::NoHighlightRange) << 5 << 2 << 6.; + QTest::newRow("no range - insert multiple before current") + << int(QQuickPathView::NoHighlightRange) << 1 << 2 << 4.; + QTest::newRow("no range - insert at end") + << int(QQuickPathView::NoHighlightRange) << 8 << 1 << 5.; + QTest::newRow("no range - insert at beginning") + << int(QQuickPathView::NoHighlightRange) << 0 << 1 << 4.; + QTest::newRow("no range - insert at current") + << int(QQuickPathView::NoHighlightRange) << 4 << 1 << 4.; +} + +void tst_QQuickPathView::insertModel() +{ + QFETCH(int, mode); + QFETCH(int, idx); + QFETCH(int, count); + QFETCH(qreal, offset); + + QQuickView *canvas = createView(); + canvas->show(); + + TestModel model; + model.addItem("Ben", "12345"); + model.addItem("Bohn", "2345"); + model.addItem("Bob", "54321"); + model.addItem("Bill", "4321"); + model.addItem("Jinny", "679"); + model.addItem("Milly", "73378"); + model.addItem("Jimmy", "3535"); + model.addItem("Barb", "9039"); + + QDeclarativeContext *ctxt = canvas->rootContext(); + ctxt->setContextProperty("testModel", &model); + + canvas->setSource(QUrl::fromLocalFile(TESTDATA("pathview0.qml"))); + qApp->processEvents(); + + QQuickPathView *pathview = findItem(canvas->rootObject(), "view"); + QVERIFY(pathview != 0); + + pathview->setHighlightRangeMode((QQuickPathView::HighlightRangeMode)mode); + + pathview->setCurrentIndex(4); + if (mode == QQuickPathView::StrictlyEnforceRange) + QTRY_COMPARE(pathview->offset(), 4.0); + else + pathview->setOffset(4); + + QList > items; + for (int i = 0; i < count; ++i) + items.append(qMakePair(QString("New"), QString::number(i))); + + model.insertItems(idx, items); + QTRY_COMPARE(pathview->offset(), offset); + + delete canvas; +} + +void tst_QQuickPathView::removeModel_data() +{ + QTest::addColumn("mode"); + QTest::addColumn("idx"); + QTest::addColumn("count"); + QTest::addColumn("offset"); + + // We have 8 items, with currentIndex == 4 + QTest::newRow("remove after current") + << int(QQuickPathView::StrictlyEnforceRange) << 6 << 1 << 3.; + QTest::newRow("remove before current") + << int(QQuickPathView::StrictlyEnforceRange) << 2 << 1 << 4.; + QTest::newRow("remove multiple after current") + << int(QQuickPathView::StrictlyEnforceRange) << 5 << 2 << 2.; + QTest::newRow("remove multiple before current") + << int(QQuickPathView::StrictlyEnforceRange) << 1 << 2 << 4.; + QTest::newRow("remove last") + << int(QQuickPathView::StrictlyEnforceRange) << 7 << 1 << 3.; + QTest::newRow("remove first") + << int(QQuickPathView::StrictlyEnforceRange) << 0 << 1 << 4.; + QTest::newRow("remove current") + << int(QQuickPathView::StrictlyEnforceRange) << 4 << 1 << 3.; + + QTest::newRow("no range - remove after current") + << int(QQuickPathView::NoHighlightRange) << 6 << 1 << 3.; + QTest::newRow("no range - remove before current") + << int(QQuickPathView::NoHighlightRange) << 2 << 1 << 4.; + QTest::newRow("no range - remove multiple after current") + << int(QQuickPathView::NoHighlightRange) << 5 << 2 << 2.; + QTest::newRow("no range - remove multiple before current") + << int(QQuickPathView::NoHighlightRange) << 1 << 2 << 4.; + QTest::newRow("no range - remove last") + << int(QQuickPathView::NoHighlightRange) << 7 << 1 << 3.; + QTest::newRow("no range - remove first") + << int(QQuickPathView::NoHighlightRange) << 0 << 1 << 4.; + QTest::newRow("no range - remove current offset") + << int(QQuickPathView::NoHighlightRange) << 4 << 1 << 4.; +} + +void tst_QQuickPathView::removeModel() +{ + QFETCH(int, mode); + QFETCH(int, idx); + QFETCH(int, count); + QFETCH(qreal, offset); + + QQuickView *canvas = createView(); + canvas->show(); + + TestModel model; + model.addItem("Ben", "12345"); + model.addItem("Bohn", "2345"); + model.addItem("Bob", "54321"); + model.addItem("Bill", "4321"); + model.addItem("Jinny", "679"); + model.addItem("Milly", "73378"); + model.addItem("Jimmy", "3535"); + model.addItem("Barb", "9039"); + + QDeclarativeContext *ctxt = canvas->rootContext(); + ctxt->setContextProperty("testModel", &model); + + canvas->setSource(QUrl::fromLocalFile(TESTDATA("pathview0.qml"))); + qApp->processEvents(); + + QQuickPathView *pathview = findItem(canvas->rootObject(), "view"); + QVERIFY(pathview != 0); + + pathview->setHighlightRangeMode((QQuickPathView::HighlightRangeMode)mode); + + pathview->setCurrentIndex(4); + if (mode == QQuickPathView::StrictlyEnforceRange) + QTRY_COMPARE(pathview->offset(), 4.0); + else + pathview->setOffset(4); + + model.removeItems(idx, count); + QTRY_COMPARE(pathview->offset(), offset); + + delete canvas; +} + + +void tst_QQuickPathView::moveModel_data() +{ + QTest::addColumn("mode"); + QTest::addColumn("from"); + QTest::addColumn("to"); + QTest::addColumn("count"); + QTest::addColumn("offset"); + + // We have 8 items, with currentIndex == 4 + QTest::newRow("move after current") + << int(QQuickPathView::StrictlyEnforceRange) << 5 << 6 << 1 << 4.; + QTest::newRow("move before current") + << int(QQuickPathView::StrictlyEnforceRange) << 2 << 3 << 1 << 4.; + QTest::newRow("move before current to after") + << int(QQuickPathView::StrictlyEnforceRange) << 2 << 6 << 1 << 5.; + QTest::newRow("move multiple after current") + << int(QQuickPathView::StrictlyEnforceRange) << 5 << 6 << 2 << 4.; + QTest::newRow("move multiple before current") + << int(QQuickPathView::StrictlyEnforceRange) << 0 << 1 << 2 << 4.; + QTest::newRow("move before current to end") + << int(QQuickPathView::StrictlyEnforceRange) << 2 << 7 << 1 << 5.; + QTest::newRow("move last to beginning") + << int(QQuickPathView::StrictlyEnforceRange) << 7 << 0 << 1 << 3.; + QTest::newRow("move current") + << int(QQuickPathView::StrictlyEnforceRange) << 4 << 6 << 1 << 2.; + + QTest::newRow("no range - move after current") + << int(QQuickPathView::NoHighlightRange) << 5 << 6 << 1 << 4.; + QTest::newRow("no range - move before current") + << int(QQuickPathView::NoHighlightRange) << 2 << 3 << 1 << 4.; + QTest::newRow("no range - move before current to after") + << int(QQuickPathView::NoHighlightRange) << 2 << 6 << 1 << 5.; + QTest::newRow("no range - move multiple after current") + << int(QQuickPathView::NoHighlightRange) << 5 << 6 << 2 << 4.; + QTest::newRow("no range - move multiple before current") + << int(QQuickPathView::NoHighlightRange) << 0 << 1 << 2 << 4.; + QTest::newRow("no range - move before current to end") + << int(QQuickPathView::NoHighlightRange) << 2 << 7 << 1 << 5.; + QTest::newRow("no range - move last to beginning") + << int(QQuickPathView::NoHighlightRange) << 7 << 0 << 1 << 3.; + QTest::newRow("no range - move current") + << int(QQuickPathView::NoHighlightRange) << 4 << 6 << 1 << 4.; + QTest::newRow("no range - move multiple incl. current") + << int(QQuickPathView::NoHighlightRange) << 0 << 1 << 5 << 4.; +} + +void tst_QQuickPathView::moveModel() +{ + QFETCH(int, mode); + QFETCH(int, from); + QFETCH(int, to); + QFETCH(int, count); + QFETCH(qreal, offset); + + QQuickView *canvas = createView(); + canvas->show(); + + TestModel model; + model.addItem("Ben", "12345"); + model.addItem("Bohn", "2345"); + model.addItem("Bob", "54321"); + model.addItem("Bill", "4321"); + model.addItem("Jinny", "679"); + model.addItem("Milly", "73378"); + model.addItem("Jimmy", "3535"); + model.addItem("Barb", "9039"); + + QDeclarativeContext *ctxt = canvas->rootContext(); + ctxt->setContextProperty("testModel", &model); + + canvas->setSource(QUrl::fromLocalFile(TESTDATA("pathview0.qml"))); + qApp->processEvents(); + + QQuickPathView *pathview = findItem(canvas->rootObject(), "view"); + QVERIFY(pathview != 0); + + pathview->setHighlightRangeMode((QQuickPathView::HighlightRangeMode)mode); + + pathview->setCurrentIndex(4); + if (mode == QQuickPathView::StrictlyEnforceRange) + QTRY_COMPARE(pathview->offset(), 4.0); + else + pathview->setOffset(4); + + model.moveItems(from, to, count); + QTRY_COMPARE(pathview->offset(), offset); + + delete canvas; +} + void tst_QQuickPathView::path() { QDeclarativeEngine engine; -- 1.7.2.5