From bccb1a82fd796ca31b8b4777c29344bafc6d71d6 Mon Sep 17 00:00:00 2001 From: Martin Jones Date: Thu, 14 Oct 2010 14:54:33 +1000 Subject: Fix consistency of setting currentIndex in ListView and GridView. The behaviour of setting the currentIndex out of bounds, was different depending upon whether it was set -ve, beyond the end, or before component completed. The behaviour is now consistent - setting the currentIndex out of bounds is allowed and will cause the currentItem and highlightItem to become null. Task-number: QTBUG-12571 Reviewed-by: Michael Brasser --- .../graphicsitems/qdeclarativegridview.cpp | 67 ++++++++++++++-------- .../graphicsitems/qdeclarativelistview.cpp | 35 +++++++---- .../data/gridview-noCurrent.qml | 52 +++++++++++++++++ .../tst_qdeclarativegridview.cpp | 43 ++++++++++++++ .../data/listview-noCurrent.qml | 50 ++++++++++++++++ .../tst_qdeclarativelistview.cpp | 44 ++++++++++++++ 6 files changed, 257 insertions(+), 34 deletions(-) create mode 100644 tests/auto/declarative/qdeclarativegridview/data/gridview-noCurrent.qml create mode 100644 tests/auto/declarative/qdeclarativelistview/data/listview-noCurrent.qml diff --git a/src/declarative/graphicsitems/qdeclarativegridview.cpp b/src/declarative/graphicsitems/qdeclarativegridview.cpp index 6ee6b0d..f53625f 100644 --- a/src/declarative/graphicsitems/qdeclarativegridview.cpp +++ b/src/declarative/graphicsitems/qdeclarativegridview.cpp @@ -112,7 +112,7 @@ public: , bufferMode(BufferBefore | BufferAfter), snapMode(QDeclarativeGridView::NoSnap) , ownModel(false), wrap(false), autoHighlight(true) , fixCurrentVisibility(false), lazyRelease(false), layoutScheduled(false) - , deferredRelease(false), haveHighlightRange(false) {} + , deferredRelease(false), haveHighlightRange(false), currentIndexSet(false) {} void init(); void clear(); @@ -392,6 +392,7 @@ public: bool layoutScheduled : 1; bool deferredRelease : 1; bool haveHighlightRange : 1; + bool currentIndexSet : 1; }; void QDeclarativeGridViewPrivate::init() @@ -730,6 +731,8 @@ void QDeclarativeGridViewPrivate::createHighlight() QDeclarative_setParent_noEvent(item, q->contentItem()); item->setParentItem(q->contentItem()); highlight = new FxGridItem(item, q); + if (currentItem) + highlight->setPosition(currentItem->colPos(), currentItem->rowPos()); highlightXAnimator = new QSmoothedAnimation(q); highlightXAnimator->target = QDeclarativeProperty(highlight->item, QLatin1String("x")); highlightXAnimator->userDuration = highlightMoveDuration; @@ -771,8 +774,11 @@ void QDeclarativeGridViewPrivate::updateCurrent(int modelIndex) currentItem->attached->setIsCurrentItem(false); releaseItem(currentItem); currentItem = 0; - currentIndex = -1; + currentIndex = modelIndex; + emit q->currentIndexChanged(); updateHighlight(); + } else if (currentIndex != modelIndex) { + currentIndex = modelIndex; emit q->currentIndexChanged(); } return; @@ -1236,7 +1242,7 @@ void QDeclarativeGridView::setModel(const QVariant &model) d->bufferMode = QDeclarativeGridViewPrivate::BufferBefore | QDeclarativeGridViewPrivate::BufferAfter; if (isComponentComplete()) { refill(); - if (d->currentIndex >= d->model->count() || d->currentIndex < 0) { + if ((d->currentIndex >= d->model->count() || d->currentIndex < 0) && !d->currentIndexSet) { setCurrentIndex(0); } else { d->moveReason = QDeclarativeGridViewPrivate::SetIndex; @@ -1344,10 +1350,13 @@ void QDeclarativeGridView::setCurrentIndex(int index) Q_D(QDeclarativeGridView); if (d->requestedIndex >= 0) // currently creating item return; - if (isComponentComplete() && d->isValid() && index != d->currentIndex && index < d->model->count() && index >= 0) { + d->currentIndexSet = true; + if (index == d->currentIndex) + return; + if (isComponentComplete() && d->isValid()) { d->moveReason = QDeclarativeGridViewPrivate::SetIndex; d->updateCurrent(index); - } else if (index != d->currentIndex) { + } else { d->currentIndex = index; emit currentIndexChanged(); } @@ -1983,22 +1992,25 @@ void QDeclarativeGridView::keyPressEvent(QKeyEvent *event) Move the currentIndex up one item in the view. The current index will wrap if keyNavigationWraps is true and it - is currently at the end. + is currently at the end. This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeGridView::moveCurrentIndexUp() { Q_D(QDeclarativeGridView); + const int count = d->model ? d->model->count() : 0; + if (!count) + return; if (d->flow == QDeclarativeGridView::LeftToRight) { if (currentIndex() >= d->columns || d->wrap) { int index = currentIndex() - d->columns; - setCurrentIndex(index >= 0 ? index : d->model->count()-1); + setCurrentIndex((index >= 0 && index < count) ? index : count-1); } } else { if (currentIndex() > 0 || d->wrap) { int index = currentIndex() - 1; - setCurrentIndex(index >= 0 ? index : d->model->count()-1); + setCurrentIndex((index >= 0 && index < count) ? index : count-1); } } } @@ -2008,22 +2020,25 @@ void QDeclarativeGridView::moveCurrentIndexUp() Move the currentIndex down one item in the view. The current index will wrap if keyNavigationWraps is true and it - is currently at the end. + is currently at the end. This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeGridView::moveCurrentIndexDown() { Q_D(QDeclarativeGridView); + const int count = d->model ? d->model->count() : 0; + if (!count) + return; if (d->flow == QDeclarativeGridView::LeftToRight) { - if (currentIndex() < d->model->count() - d->columns || d->wrap) { + if (currentIndex() < count - d->columns || d->wrap) { int index = currentIndex()+d->columns; - setCurrentIndex(index < d->model->count() ? index : 0); + setCurrentIndex((index >= 0 && index < count) ? index : 0); } } else { - if (currentIndex() < d->model->count() - 1 || d->wrap) { + if (currentIndex() < count - 1 || d->wrap) { int index = currentIndex() + 1; - setCurrentIndex(index < d->model->count() ? index : 0); + setCurrentIndex((index >= 0 && index < count) ? index : 0); } } } @@ -2033,22 +2048,25 @@ void QDeclarativeGridView::moveCurrentIndexDown() Move the currentIndex left one item in the view. The current index will wrap if keyNavigationWraps is true and it - is currently at the end. + is currently at the end. This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeGridView::moveCurrentIndexLeft() { Q_D(QDeclarativeGridView); + const int count = d->model ? d->model->count() : 0; + if (!count) + return; if (d->flow == QDeclarativeGridView::LeftToRight) { if (currentIndex() > 0 || d->wrap) { int index = currentIndex() - 1; - setCurrentIndex(index >= 0 ? index : d->model->count()-1); + setCurrentIndex((index >= 0 && index < count) ? index : count-1); } } else { if (currentIndex() >= d->columns || d->wrap) { int index = currentIndex() - d->columns; - setCurrentIndex(index >= 0 ? index : d->model->count()-1); + setCurrentIndex((index >= 0 && index < count) ? index : count-1); } } } @@ -2058,22 +2076,25 @@ void QDeclarativeGridView::moveCurrentIndexLeft() Move the currentIndex right one item in the view. The current index will wrap if keyNavigationWraps is true and it - is currently at the end. + is currently at the end. This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeGridView::moveCurrentIndexRight() { Q_D(QDeclarativeGridView); + const int count = d->model ? d->model->count() : 0; + if (!count) + return; if (d->flow == QDeclarativeGridView::LeftToRight) { - if (currentIndex() < d->model->count() - 1 || d->wrap) { + if (currentIndex() < count - 1 || d->wrap) { int index = currentIndex() + 1; - setCurrentIndex(index < d->model->count() ? index : 0); + setCurrentIndex((index >= 0 && index < count) ? index : 0); } } else { - if (currentIndex() < d->model->count() - d->columns || d->wrap) { + if (currentIndex() < count - d->columns || d->wrap) { int index = currentIndex()+d->columns; - setCurrentIndex(index < d->model->count() ? index : 0); + setCurrentIndex((index >= 0 && index < count) ? index : 0); } } } @@ -2201,7 +2222,7 @@ void QDeclarativeGridView::componentComplete() if (d->isValid()) { refill(); d->moveReason = QDeclarativeGridViewPrivate::SetIndex; - if (d->currentIndex < 0) + if (d->currentIndex < 0 && !d->currentIndexSet) d->updateCurrent(0); else d->updateCurrent(d->currentIndex); @@ -2282,7 +2303,7 @@ void QDeclarativeGridView::itemsInserted(int modelIndex, int count) if (d->currentItem) d->currentItem->index = d->currentIndex; emit currentIndexChanged(); - } else if (d->currentIndex < 0) { + } else if (d->currentIndex < 0 && !d->currentIndexSet) { d->updateCurrent(0); } d->itemCount += count; diff --git a/src/declarative/graphicsitems/qdeclarativelistview.cpp b/src/declarative/graphicsitems/qdeclarativelistview.cpp index 6fd3b71..7dd5c75 100644 --- a/src/declarative/graphicsitems/qdeclarativelistview.cpp +++ b/src/declarative/graphicsitems/qdeclarativelistview.cpp @@ -182,7 +182,8 @@ public: , bufferMode(BufferBefore | BufferAfter) , ownModel(false), wrap(false), autoHighlight(true), haveHighlightRange(false) , correctFlick(false), inFlickCorrection(false), lazyRelease(false) - , deferredRelease(false), layoutScheduled(false), minExtentDirty(true), maxExtentDirty(true) + , deferredRelease(false), layoutScheduled(false), currentIndexSet(false) + , minExtentDirty(true), maxExtentDirty(true) {} void init(); @@ -544,6 +545,7 @@ public: bool lazyRelease : 1; bool deferredRelease : 1; bool layoutScheduled : 1; + bool currentIndexSet : 1; mutable bool minExtentDirty : 1; mutable bool maxExtentDirty : 1; }; @@ -881,6 +883,7 @@ void QDeclarativeListViewPrivate::createHighlight() } else { highlight->item->setWidth(currentItem->item->width()); } + highlight->setPosition(currentItem->itemPosition()); } QDeclarativeItemPrivate *itemPrivate = static_cast(QGraphicsItemPrivate::get(item)); itemPrivate->addItemChangeListener(this, QDeclarativeItemPrivate::Geometry); @@ -1045,8 +1048,11 @@ void QDeclarativeListViewPrivate::updateCurrent(int modelIndex) currentItem->attached->setIsCurrentItem(false); releaseItem(currentItem); currentItem = 0; - currentIndex = -1; + currentIndex = modelIndex; + emit q->currentIndexChanged(); updateHighlight(); + } else if (currentIndex != modelIndex) { + currentIndex = modelIndex; emit q->currentIndexChanged(); } return; @@ -1598,7 +1604,7 @@ void QDeclarativeListView::setModel(const QVariant &model) if (isComponentComplete()) { updateSections(); refill(); - if (d->currentIndex >= d->model->count() || d->currentIndex < 0) { + if ((d->currentIndex >= d->model->count() || d->currentIndex < 0) && !d->currentIndexSet) { setCurrentIndex(0); } else { d->moveReason = QDeclarativeListViewPrivate::SetIndex; @@ -1708,10 +1714,13 @@ void QDeclarativeListView::setCurrentIndex(int index) Q_D(QDeclarativeListView); if (d->requestedIndex >= 0) // currently creating item return; - if (isComponentComplete() && d->isValid() && index != d->currentIndex && index < d->model->count() && index >= 0) { + d->currentIndexSet = true; + if (index == d->currentIndex) + return; + if (isComponentComplete() && d->isValid()) { d->moveReason = QDeclarativeListViewPrivate::SetIndex; d->updateCurrent(index); - } else if (index != d->currentIndex) { + } else { d->currentIndex = index; emit currentIndexChanged(); } @@ -2523,16 +2532,18 @@ void QDeclarativeListView::geometryChanged(const QRectF &newGeometry, Increments the current index. The current index will wrap if keyNavigationWraps is true and it is currently at the end. + This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeListView::incrementCurrentIndex() { Q_D(QDeclarativeListView); - if (currentIndex() < d->model->count() - 1 || d->wrap) { + int count = d->model ? d->model->count() : 0; + if (count && (currentIndex() < count - 1 || d->wrap)) { d->moveReason = QDeclarativeListViewPrivate::SetIndex; int index = currentIndex()+1; - d->updateCurrent(index < d->model->count() ? index : 0); + d->updateCurrent((index >= 0 && index < count) ? index : 0); } } @@ -2541,16 +2552,18 @@ void QDeclarativeListView::incrementCurrentIndex() Decrements the current index. The current index will wrap if keyNavigationWraps is true and it is currently at the beginning. + This method has no effect if the \l count is zero. \bold Note: methods should only be called after the Component has completed. */ void QDeclarativeListView::decrementCurrentIndex() { Q_D(QDeclarativeListView); - if (currentIndex() > 0 || d->wrap) { + int count = d->model ? d->model->count() : 0; + if (count && (currentIndex() > 0 || d->wrap)) { d->moveReason = QDeclarativeListViewPrivate::SetIndex; int index = currentIndex()-1; - d->updateCurrent(index >= 0 ? index : d->model->count()-1); + d->updateCurrent((index >= 0 && index < count) ? index : count-1); } } @@ -2684,7 +2697,7 @@ void QDeclarativeListView::componentComplete() if (d->isValid()) { refill(); d->moveReason = QDeclarativeListViewPrivate::SetIndex; - if (d->currentIndex < 0) + if (d->currentIndex < 0 && !d->currentIndexSet) d->updateCurrent(0); else d->updateCurrent(d->currentIndex); @@ -2790,7 +2803,7 @@ void QDeclarativeListView::itemsInserted(int modelIndex, int count) if (d->currentItem) d->currentItem->index = d->currentIndex; emit currentIndexChanged(); - } else if (d->currentIndex < 0) { + } else if (d->currentIndex < 0 && !d->currentIndexSet) { d->updateCurrent(0); } d->itemCount += count; diff --git a/tests/auto/declarative/qdeclarativegridview/data/gridview-noCurrent.qml b/tests/auto/declarative/qdeclarativegridview/data/gridview-noCurrent.qml new file mode 100644 index 0000000..1189649 --- /dev/null +++ b/tests/auto/declarative/qdeclarativegridview/data/gridview-noCurrent.qml @@ -0,0 +1,52 @@ +import QtQuick 1.0 + +Rectangle { + property int current: grid.currentIndex + width: 240 + height: 320 + color: "#ffffff" + resources: [ + Component { + id: myDelegate + Rectangle { + id: wrapper + objectName: "wrapper" + width: 80 + height: 60 + border.color: "blue" + Text { + text: index + } + Text { + x: 40 + text: wrapper.x + ", " + wrapper.y + } + Text { + y: 20 + id: textName + objectName: "textName" + text: name + } + Text { + y: 40 + id: textNumber + objectName: "textNumber" + text: number + } + color: GridView.isCurrentItem ? "lightsteelblue" : "white" + } + } + ] + GridView { + id: grid + objectName: "grid" + focus: true + width: 240 + height: 320 + currentIndex: -1 + cellWidth: 80 + cellHeight: 60 + delegate: myDelegate + model: testModel + } +} diff --git a/tests/auto/declarative/qdeclarativegridview/tst_qdeclarativegridview.cpp b/tests/auto/declarative/qdeclarativegridview/tst_qdeclarativegridview.cpp index f7acd87..327bba2 100644 --- a/tests/auto/declarative/qdeclarativegridview/tst_qdeclarativegridview.cpp +++ b/tests/auto/declarative/qdeclarativegridview/tst_qdeclarativegridview.cpp @@ -71,6 +71,7 @@ private slots: void moved(); void changeFlow(); void currentIndex(); + void noCurrentIndex(); void defaultValues(); void properties(); void propertyChanges(); @@ -696,9 +697,51 @@ void tst_QDeclarativeGridView::currentIndex() model.insertItem(0, "Foo", "1111"); QTRY_COMPARE(canvas->rootObject()->property("current").toInt(), 29); + // check removing highlight by setting currentIndex to -1; + gridview->setCurrentIndex(-1); + + QCOMPARE(gridview->currentIndex(), -1); + QVERIFY(!gridview->highlightItem()); + QVERIFY(!gridview->currentItem()); + delete canvas; } +void tst_QDeclarativeGridView::noCurrentIndex() +{ + TestModel model; + for (int i = 0; i < 60; i++) + model.addItem("Item" + QString::number(i), QString::number(i)); + + QDeclarativeView *canvas = new QDeclarativeView(0); + canvas->setFixedSize(240,320); + + QDeclarativeContext *ctxt = canvas->rootContext(); + ctxt->setContextProperty("testModel", &model); + + QString filename(SRCDIR "/data/gridview-noCurrent.qml"); + canvas->setSource(QUrl::fromLocalFile(filename)); + + qApp->processEvents(); + + QDeclarativeGridView *gridview = findItem(canvas->rootObject(), "grid"); + QVERIFY(gridview != 0); + + QDeclarativeItem *contentItem = gridview->contentItem(); + QVERIFY(contentItem != 0); + + // current index should be -1 + QCOMPARE(gridview->currentIndex(), -1); + QVERIFY(!gridview->currentItem()); + QVERIFY(!gridview->highlightItem()); + QCOMPARE(gridview->contentY(), 0.0); + + gridview->setCurrentIndex(5); + QCOMPARE(gridview->currentIndex(), 5); + QVERIFY(gridview->currentItem()); + QVERIFY(gridview->highlightItem()); +} + void tst_QDeclarativeGridView::changeFlow() { QDeclarativeView *canvas = createView(); diff --git a/tests/auto/declarative/qdeclarativelistview/data/listview-noCurrent.qml b/tests/auto/declarative/qdeclarativelistview/data/listview-noCurrent.qml new file mode 100644 index 0000000..1997010 --- /dev/null +++ b/tests/auto/declarative/qdeclarativelistview/data/listview-noCurrent.qml @@ -0,0 +1,50 @@ +import QtQuick 1.0 + +Rectangle { + property int current: list.currentIndex + width: 240 + height: 320 + color: "#ffffff" + resources: [ + Component { + id: myDelegate + Rectangle { + id: wrapper + objectName: "wrapper" + height: 20 + width: 240 + Text { + text: index + } + Text { + x: 30 + id: textName + objectName: "textName" + text: name + } + Text { + x: 120 + id: textNumber + objectName: "textNumber" + text: number + } + Text { + x: 200 + text: wrapper.y + } + color: ListView.isCurrentItem ? "lightsteelblue" : "white" + } + } + ] + ListView { + id: list + objectName: "list" + focus: true + currentIndex: -1 + width: 240 + height: 320 + delegate: myDelegate + highlightMoveSpeed: 1000 + model: testModel + } +} diff --git a/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp b/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp index 2649c0d..080631c 100644 --- a/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp +++ b/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp @@ -85,6 +85,7 @@ private slots: void itemList(); void currentIndex(); + void noCurrentIndex(); void enforceRange(); void spacing(); void sections(); @@ -1087,9 +1088,52 @@ void tst_QDeclarativeListView::currentIndex() model.insertItem(0, "Foo", "1111"); QTRY_COMPARE(canvas->rootObject()->property("current").toInt(), 29); + // check removing highlight by setting currentIndex to -1; + listview->setCurrentIndex(-1); + + QCOMPARE(listview->currentIndex(), -1); + QVERIFY(!listview->highlightItem()); + QVERIFY(!listview->currentItem()); + delete canvas; } +void tst_QDeclarativeListView::noCurrentIndex() +{ + TestModel model; + for (int i = 0; i < 30; i++) + model.addItem("Item" + QString::number(i), QString::number(i)); + + QDeclarativeView *canvas = new QDeclarativeView(0); + canvas->setFixedSize(240,320); + + QDeclarativeContext *ctxt = canvas->rootContext(); + ctxt->setContextProperty("testModel", &model); + + QString filename(SRCDIR "/data/listview-noCurrent.qml"); + canvas->setSource(QUrl::fromLocalFile(filename)); + + qApp->processEvents(); + + QDeclarativeListView *listview = findItem(canvas->rootObject(), "list"); + QTRY_VERIFY(listview != 0); + + QDeclarativeItem *contentItem = listview->contentItem(); + QTRY_VERIFY(contentItem != 0); + + // current index should be -1 at startup + // and we should not have a currentItem or highlightItem + QCOMPARE(listview->currentIndex(), -1); + QCOMPARE(listview->contentY(), 0.0); + QVERIFY(!listview->highlightItem()); + QVERIFY(!listview->currentItem()); + + listview->setCurrentIndex(2); + QCOMPARE(listview->currentIndex(), 2); + QVERIFY(listview->highlightItem()); + QVERIFY(listview->currentItem()); +} + void tst_QDeclarativeListView::itemList() { QDeclarativeView *canvas = createView(); -- cgit v0.12