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

2 files changed, 68 insertions, 34 deletions

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<QDeclarativeItemPrivate*>(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;