From 965e47f1758079aaf53bfd7a4e0577a249114cb9 Mon Sep 17 00:00:00 2001 From: Martin Jones Date: Wed, 20 Oct 2010 13:16:50 +1000 Subject: ListView item insertion didn't handle delayed item removal correctly. The delayed removal items weren't handled correctly by mapRangeFromModel() function. Use mapFromModel() instead which gives us what we actually want and remove unused mapRangeFromModel(). Task-number: QTBUG-14471 Reviewed-by: Michael Brasser --- .../graphicsitems/qdeclarativelistview.cpp | 34 +++------------------- .../tst_qdeclarativelistview.cpp | 9 ++++++ 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/src/declarative/graphicsitems/qdeclarativelistview.cpp b/src/declarative/graphicsitems/qdeclarativelistview.cpp index 7dd5c75..83965f5 100644 --- a/src/declarative/graphicsitems/qdeclarativelistview.cpp +++ b/src/declarative/graphicsitems/qdeclarativelistview.cpp @@ -394,44 +394,19 @@ public: } // map a model index to visibleItems index. - // These may differ if removed items are still present in the visible list, - // e.g. doing a removal animation int mapFromModel(int modelIndex) const { if (modelIndex < visibleIndex || modelIndex >= visibleIndex + visibleItems.count()) return -1; for (int i = 0; i < visibleItems.count(); ++i) { FxListItem *listItem = visibleItems.at(i); if (listItem->index == modelIndex) - return i + visibleIndex; + return i; if (listItem->index > modelIndex) return -1; } return -1; // Not in visibleList } - bool mapRangeFromModel(int &index, int &count) const { - if (index + count < visibleIndex) - return false; - - int lastIndex = -1; - for (int i = visibleItems.count()-1; i >= 0; --i) { - FxListItem *listItem = visibleItems.at(i); - if (listItem->index != -1) { - lastIndex = listItem->index; - break; - } - } - - if (index > lastIndex) - return false; - - int last = qMin(index + count - 1, lastIndex); - index = qMax(index, visibleIndex); - count = last - index + 1; - - return true; - } - void updateViewport() { Q_Q(QDeclarativeListView); if (orient == QDeclarativeListView::Vertical) { @@ -2811,15 +2786,15 @@ void QDeclarativeListView::itemsInserted(int modelIndex, int count) return; } - int overlapCount = count; - if (!d->mapRangeFromModel(modelIndex, overlapCount)) { + int index = d->mapFromModel(modelIndex); + if (index < 0) { int i = d->visibleItems.count() - 1; while (i > 0 && d->visibleItems.at(i)->index == -1) --i; if (d->visibleItems.at(i)->index + 1 == modelIndex && d->visibleItems.at(i)->endPosition() < d->buffer+d->position()+d->size()-1) { // Special case of appending an item to the model. - modelIndex = d->visibleIndex + d->visibleItems.count(); + index = d->visibleItems.count(); } else { if (modelIndex < d->visibleIndex) { // Insert before visible items @@ -2846,7 +2821,6 @@ void QDeclarativeListView::itemsInserted(int modelIndex, int count) // At least some of the added items will be visible - int index = modelIndex - d->visibleIndex; // index can be the next item past the end of the visible items list (i.e. appended) int pos = index < d->visibleItems.count() ? d->visibleItems.at(index)->position() : d->visibleItems.at(index-1)->endPosition()+d->spacing+1; diff --git a/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp b/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp index 080631c..79fef7a 100644 --- a/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp +++ b/tests/auto/declarative/qdeclarativelistview/tst_qdeclarativelistview.cpp @@ -673,6 +673,15 @@ void tst_QDeclarativeListView::removed(bool animated) QTRY_COMPARE(item->y(),80+i*20.0); } + model.removeItems(1, 17); +// QTest::qWait(300); + + model.removeItems(2, 1); + model.addItem("New", "1"); + + QTRY_VERIFY(name = findItem(contentItem, "textName", model.count()-1)); + QCOMPARE(name->text(), QString("New")); + delete canvas; } -- cgit v0.12 From e778e0b09d79d311ece2950e499180eccae26b8e Mon Sep 17 00:00:00 2001 From: Martin Jones Date: Wed, 20 Oct 2010 16:23:48 +1000 Subject: Ensure we don't refill the view before all model changes are complete. Calling setPosition(0) in itemsRemoved caused the view to be refilled, which is a bad thing if the model has changed but not all updates have been handled. Fixed in both ListView and GridView. Task-number: QTBUG-14548 Reviewed-by: Michael Brasser --- .../graphicsitems/qdeclarativegridview.cpp | 23 +++++++++++--------- .../graphicsitems/qdeclarativelistview.cpp | 25 ++++++++++++---------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/declarative/graphicsitems/qdeclarativegridview.cpp b/src/declarative/graphicsitems/qdeclarativegridview.cpp index f53625f..bbc03f3 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), currentIndexSet(false) {} + , deferredRelease(false), haveHighlightRange(false), currentIndexCleared(false) {} void init(); void clear(); @@ -392,7 +392,7 @@ public: bool layoutScheduled : 1; bool deferredRelease : 1; bool haveHighlightRange : 1; - bool currentIndexSet : 1; + bool currentIndexCleared : 1; }; void QDeclarativeGridViewPrivate::init() @@ -1242,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) && !d->currentIndexSet) { + if ((d->currentIndex >= d->model->count() || d->currentIndex < 0) && !d->currentIndexCleared) { setCurrentIndex(0); } else { d->moveReason = QDeclarativeGridViewPrivate::SetIndex; @@ -1330,7 +1330,8 @@ void QDeclarativeGridView::setDelegate(QDeclarativeComponent *delegate) \qmlproperty Item GridView::currentItem The \c currentIndex property holds the index of the current item, and - \c currentItem holds the current item. + \c currentItem holds the current item. Setting the currentIndex to -1 + will clear the highlight and set currentItem to null. If highlightFollowsCurrentItem is \c true, setting either of these properties will smoothly scroll the GridView so that the current @@ -1350,7 +1351,7 @@ void QDeclarativeGridView::setCurrentIndex(int index) Q_D(QDeclarativeGridView); if (d->requestedIndex >= 0) // currently creating item return; - d->currentIndexSet = true; + d->currentIndexCleared = (index == -1); if (index == d->currentIndex) return; if (isComponentComplete() && d->isValid()) { @@ -1831,6 +1832,8 @@ void QDeclarativeGridView::viewportMoved() { Q_D(QDeclarativeGridView); QDeclarativeFlickable::viewportMoved(); + if (!d->itemCount) + return; d->lazyRelease = true; if (d->flickingHorizontally || d->flickingVertically) { if (yflick()) { @@ -2222,7 +2225,7 @@ void QDeclarativeGridView::componentComplete() if (d->isValid()) { refill(); d->moveReason = QDeclarativeGridViewPrivate::SetIndex; - if (d->currentIndex < 0 && !d->currentIndexSet) + if (d->currentIndex < 0 && !d->currentIndexCleared) d->updateCurrent(0); else d->updateCurrent(d->currentIndex); @@ -2297,13 +2300,13 @@ void QDeclarativeGridView::itemsInserted(int modelIndex, int count) return; if (!d->visibleItems.count() || d->model->count() <= 1) { d->scheduleLayout(); - if (d->currentIndex >= modelIndex) { + if (d->itemCount && d->currentIndex >= modelIndex) { // adjust current item index d->currentIndex += count; if (d->currentItem) d->currentItem->index = d->currentIndex; emit currentIndexChanged(); - } else if (d->currentIndex < 0 && !d->currentIndexSet) { + } else if (!d->currentIndex || (d->currentIndex < 0 && !d->currentIndexCleared)) { d->updateCurrent(0); } d->itemCount += count; @@ -2411,7 +2414,7 @@ void QDeclarativeGridView::itemsInserted(int modelIndex, int count) } } - if (d->currentIndex >= modelIndex) { + if (d->itemCount && d->currentIndex >= modelIndex) { // adjust current item index d->currentIndex += count; if (d->currentItem) { @@ -2498,8 +2501,8 @@ void QDeclarativeGridView::itemsRemoved(int modelIndex, int count) if (removedVisible && d->visibleItems.isEmpty()) { d->timeline.clear(); - d->setPosition(0); if (d->itemCount == 0) { + d->setPosition(0); d->updateHeader(); d->updateFooter(); update(); diff --git a/src/declarative/graphicsitems/qdeclarativelistview.cpp b/src/declarative/graphicsitems/qdeclarativelistview.cpp index 83965f5..e29f285 100644 --- a/src/declarative/graphicsitems/qdeclarativelistview.cpp +++ b/src/declarative/graphicsitems/qdeclarativelistview.cpp @@ -182,7 +182,7 @@ public: , bufferMode(BufferBefore | BufferAfter) , ownModel(false), wrap(false), autoHighlight(true), haveHighlightRange(false) , correctFlick(false), inFlickCorrection(false), lazyRelease(false) - , deferredRelease(false), layoutScheduled(false), currentIndexSet(false) + , deferredRelease(false), layoutScheduled(false), currentIndexCleared(false) , minExtentDirty(true), maxExtentDirty(true) {} @@ -520,7 +520,7 @@ public: bool lazyRelease : 1; bool deferredRelease : 1; bool layoutScheduled : 1; - bool currentIndexSet : 1; + bool currentIndexCleared : 1; mutable bool minExtentDirty : 1; mutable bool maxExtentDirty : 1; }; @@ -1579,7 +1579,7 @@ void QDeclarativeListView::setModel(const QVariant &model) if (isComponentComplete()) { updateSections(); refill(); - if ((d->currentIndex >= d->model->count() || d->currentIndex < 0) && !d->currentIndexSet) { + if ((d->currentIndex >= d->model->count() || d->currentIndex < 0) && !d->currentIndexCleared) { setCurrentIndex(0); } else { d->moveReason = QDeclarativeListViewPrivate::SetIndex; @@ -1669,7 +1669,8 @@ void QDeclarativeListView::setDelegate(QDeclarativeComponent *delegate) \qmlproperty Item ListView::currentItem The \c currentIndex property holds the index of the current item, and - \c currentItem holds the current item. + \c currentItem holds the current item. Setting the currentIndex to -1 + will clear the highlight and set currentItem to null. If highlightFollowsCurrentItem is \c true, setting either of these properties will smoothly scroll the ListView so that the current @@ -1689,7 +1690,7 @@ void QDeclarativeListView::setCurrentIndex(int index) Q_D(QDeclarativeListView); if (d->requestedIndex >= 0) // currently creating item return; - d->currentIndexSet = true; + d->currentIndexCleared = (index == -1); if (index == d->currentIndex) return; if (isComponentComplete() && d->isValid()) { @@ -2304,6 +2305,8 @@ void QDeclarativeListView::viewportMoved() { Q_D(QDeclarativeListView); QDeclarativeFlickable::viewportMoved(); + if (!d->itemCount) + return; d->lazyRelease = true; refill(); if (d->flickingHorizontally || d->flickingVertically || d->movingHorizontally || d->movingVertically) @@ -2518,7 +2521,7 @@ void QDeclarativeListView::incrementCurrentIndex() if (count && (currentIndex() < count - 1 || d->wrap)) { d->moveReason = QDeclarativeListViewPrivate::SetIndex; int index = currentIndex()+1; - d->updateCurrent((index >= 0 && index < count) ? index : 0); + setCurrentIndex((index >= 0 && index < count) ? index : 0); } } @@ -2538,7 +2541,7 @@ void QDeclarativeListView::decrementCurrentIndex() if (count && (currentIndex() > 0 || d->wrap)) { d->moveReason = QDeclarativeListViewPrivate::SetIndex; int index = currentIndex()-1; - d->updateCurrent((index >= 0 && index < count) ? index : count-1); + setCurrentIndex((index >= 0 && index < count) ? index : count-1); } } @@ -2672,7 +2675,7 @@ void QDeclarativeListView::componentComplete() if (d->isValid()) { refill(); d->moveReason = QDeclarativeListViewPrivate::SetIndex; - if (d->currentIndex < 0 && !d->currentIndexSet) + if (d->currentIndex < 0 && !d->currentIndexCleared) d->updateCurrent(0); else d->updateCurrent(d->currentIndex); @@ -2772,13 +2775,13 @@ void QDeclarativeListView::itemsInserted(int modelIndex, int count) d->moveReason = QDeclarativeListViewPrivate::Other; if (!d->visibleItems.count() || d->model->count() <= 1) { d->scheduleLayout(); - if (d->currentIndex >= modelIndex) { + if (d->itemCount && d->currentIndex >= modelIndex) { // adjust current item index d->currentIndex += count; if (d->currentItem) d->currentItem->index = d->currentIndex; emit currentIndexChanged(); - } else if (d->currentIndex < 0 && !d->currentIndexSet) { + } else if (!d->currentIndex || (d->currentIndex < 0 && !d->currentIndexCleared)) { d->updateCurrent(0); } d->itemCount += count; @@ -2884,7 +2887,7 @@ void QDeclarativeListView::itemsInserted(int modelIndex, int count) } diff = pos - initialPos; } - if (d->currentIndex >= modelIndex) { + if (d->itemCount && d->currentIndex >= modelIndex) { // adjust current item index d->currentIndex += count; if (d->currentItem) { -- cgit v0.12