Fix insertions above the visible index

The first visible item was repositioned incorrectly after an insertion
above the visible index since any insertions above the position + cache
buffer were ignored and not considered for repositioning the first item.

GridView insertion code has changed to be similar to the ListView
implementation to fix various issues when inserting above the visible
index and to remove code that crossed indexes from visibleItems with
model indexes and visible indexes.

Also adds extra insertion tests for ListView and GridView.

Change-Id: I5e129c605fdad733b61bd29850465b3b752fb63f
Reviewed-on: http://codereview.qt-project.org/6485
Reviewed-by: Bea Lam <bea.lam@nokia.com>

4 files changed, 129 insertions, 82 deletions

diff --git a/src/declarative/items/qsggridview.cpp b/src/declarative/items/qsggridview.cpp
index 099d62ee45..fd2742c2f3 100644
--- a/src/declarative/items/qsggridview.cpp
+++ b/src/declarative/items/qsggridview.cpp
@@ -173,7 +173,7 @@ public:
     virtual void repositionPackageItemAt(QSGItem *item, int index);
     virtual void resetItemPosition(FxViewItem *item, FxViewItem *toItem);
     virtual void resetFirstItemPosition();
-    virtual void moveItemBy(FxViewItem *item, const QList<FxViewItem *> &items, const QList<FxViewItem *> &movedBackwards);
+    virtual void moveItemBy(FxViewItem *item, qreal forwards, qreal backwards);
 
     virtual void createHighlight();
     virtual void updateHighlight();
@@ -181,7 +181,7 @@ public:
 
     virtual void setPosition(qreal pos);
     virtual void layoutVisibleItems();
-    bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, QList<FxViewItem *> *, QList<FxViewItem *>*, FxViewItem *);
+    bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, FxViewItem *, InsertionsResult *);
 
     virtual qreal headerSize() const;
     virtual qreal footerSize() const;
@@ -586,9 +586,10 @@ void QSGGridViewPrivate::resetFirstItemPosition()
     item->setPosition(0, 0);
 }
 
-void QSGGridViewPrivate::moveItemBy(FxViewItem *item, const QList<FxViewItem *> &forwards, const QList<FxViewItem *> &backwards)
+void QSGGridViewPrivate::moveItemBy(FxViewItem *item, qreal forwards, qreal backwards)
 {
-    int moveCount = forwards.count() - backwards.count();
+    int moveCount = (forwards / rowSize()) - (backwards / rowSize());
+
     FxGridItemSG *gridItem = static_cast<FxGridItemSG*>(item);
     gridItem->setPosition(gridItem->colPos(), gridItem->rowPos() + ((moveCount / columns) * rowSize()));
 }
@@ -1757,7 +1758,7 @@ void QSGGridView::moveCurrentIndexRight()
     }
 }
 
-bool QSGGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Insert &change, QList<FxViewItem *> *movedBackwards, QList<FxViewItem *> *addedItems, FxViewItem *firstVisible)
+bool QSGGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Insert &change, FxViewItem *firstVisible, InsertionsResult *insertResult)
 {
     Q_Q(QSGGridView);
 
@@ -1772,7 +1773,7 @@ bool QSGGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
             --i;
         if (visibleItems.at(i)->index + 1 == modelIndex) {
             // Special case of appending an item to the model.
-            index = visibleIndex + visibleItems.count();
+            index = visibleItems.count();
         } else {
             if (modelIndex <= visibleIndex) {
                 // Insert before visible items
@@ -1787,19 +1788,10 @@ bool QSGGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
         }
     }
 
-    int insertCount = count;
-    if (index < visibleIndex && visibleItems.count()) {
-        insertCount -= visibleIndex - index;
-        index = visibleIndex;
-        modelIndex = visibleIndex;
-    }
-
     qreal tempPos = isRightToLeftTopToBottom() ? -position()-size()+q->width()+1 : position();
-    int to = buffer+tempPos+size()-1;
     int colPos = 0;
     int rowPos = 0;
     if (visibleItems.count()) {
-        index -= visibleIndex;
         if (index < visibleItems.count()) {
             FxGridItemSG *gridItem = static_cast<FxGridItemSG*>(visibleItems.at(index));
             colPos = gridItem->colPos();
@@ -1823,30 +1815,69 @@ bool QSGGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
             item->index += count;
     }
 
-    int i = 0;
-    bool prevAddedCount = addedItems->count();
-    while (i < insertCount && rowPos <= to + rowSize()*(columns - (colPos/colSize()))/qreal(columns)) {
-        FxViewItem *item = 0;
-        if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
-            if (item->index > modelIndex + i)
-                movedBackwards->append(item);
-            item->index = modelIndex + i;
+    int prevAddedCount = insertResult->addedItems.count();
+    if (firstVisible && rowPos < firstVisible->position()) {
+        // Insert items before the visible item.
+        int insertionIdx = index;
+        int i = count - 1;
+        int from = tempPos - buffer;
+
+        while (i >= 0) {
+            if (rowPos > from) {
+                insertResult->sizeAddedBeforeVisible += rowSize();
+            } else {
+                FxViewItem *item = 0;
+                if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
+                    if (item->index > modelIndex + i)
+                        insertResult->movedBackwards.append(item);
+                    item->index = modelIndex + i;
+                }
+                if (!item)
+                    item = createItem(modelIndex + i);
+
+                visibleItems.insert(insertionIdx, item);
+                if (!change.isMove()) {
+                    insertResult->addedItems.append(item);
+                    insertResult->sizeAddedBeforeVisible += rowSize();
+                }
+            }
+            colPos -= colSize();
+            if (colPos < 0) {
+                colPos = colSize() * (columns - 1);
+                rowPos -= rowSize();
+            }
+            index++;
+            i--;
         }
-        if (!item)
-            item = createItem(modelIndex + i);
-        visibleItems.insert(index, item);
-        if (!change.isMove())
-            addedItems->append(item);
-        colPos += colSize();
-        if (colPos > colSize() * (columns-1)) {
-            colPos = 0;
-            rowPos += rowSize();
+    } else {
+        int i = 0;
+        int to = buffer+tempPos+size()-1;
+        while (i < count && rowPos <= to + rowSize()*(columns - (colPos/colSize()))/qreal(columns)) {
+            FxViewItem *item = 0;
+            if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
+                if (item->index > modelIndex + i)
+                    insertResult->movedBackwards.append(item);
+                item->index = modelIndex + i;
+            }
+            if (!item)
+                item = createItem(modelIndex + i);
+
+            visibleItems.insert(index, item);
+            if (!change.isMove())
+                insertResult->addedItems.append(item);
+            colPos += colSize();
+            if (colPos > colSize() * (columns-1)) {
+                colPos = 0;
+                rowPos += rowSize();
+            }
+            ++index;
+            ++i;
         }
-        ++index;
-        ++i;
     }
 
-    return addedItems->count() > prevAddedCount;
+    updateVisibleIndex();
+
+    return insertResult->addedItems.count() > prevAddedCount;
 }
 
 /*!
diff --git a/src/declarative/items/qsgitemview.cpp b/src/declarative/items/qsgitemview.cpp
index 9415f9eda9..075024db0f 100644
--- a/src/declarative/items/qsgitemview.cpp
+++ b/src/declarative/items/qsgitemview.cpp
@@ -1437,7 +1437,7 @@ bool QSGItemViewPrivate::applyModelChanges()
     FxViewItem *firstVisible = firstVisibleItem();
     FxViewItem *origVisibleItemsFirst = visibleItems.count() ? visibleItems.first() : 0;
     int firstItemIndex = firstVisible ? firstVisible->index : -1;
-    QList<FxViewItem *> removedBeforeFirstVisible;
+    qreal removedBeforeFirstVisibleBy = 0;
 
     const QVector<QDeclarativeChangeSet::Remove> &removals = currentChanges.pendingChanges.removes();
     for (int i=0; i<removals.count(); i++) {
@@ -1468,7 +1468,7 @@ bool QSGItemViewPrivate::applyModelChanges()
                     ++it;
                 } else {
                     if (firstVisible && item->position() < firstVisible->position() && item != visibleItems.first())
-                        removedBeforeFirstVisible.append(item);
+                        removedBeforeFirstVisibleBy += item->size();
                     if (removals[i].isMove()) {
                         currentChanges.removedItems.insert(removals[i].moveKey(item->index), item);
                     } else {
@@ -1487,38 +1487,38 @@ bool QSGItemViewPrivate::applyModelChanges()
 
     const QVector<QDeclarativeChangeSet::Insert> &insertions = currentChanges.pendingChanges.inserts();
     bool addedVisible = false;
-    QList<FxViewItem *> addedItems;
-    QList<FxViewItem *> movedBackwards;
+    InsertionsResult insertResult;
+    bool allInsertionsBeforeVisible = true;
 
     for (int i=0; i<insertions.count(); i++) {
         bool wasEmpty = visibleItems.isEmpty();
-        if (applyInsertionChange(insertions[i], &movedBackwards, &addedItems, firstVisible))
+        if (applyInsertionChange(insertions[i], firstVisible, &insertResult))
             addedVisible = true;
+        if (insertions[i].index >= visibleIndex)
+            allInsertionsBeforeVisible = false;
         if (wasEmpty && !visibleItems.isEmpty())
             resetFirstItemPosition();
         itemCount += insertions[i].count;
-        updateVisibleIndex();
     }
-    for (int i=0; i<addedItems.count(); ++i)
-        addedItems.at(i)->attached->emitAdd();
+    for (int i=0; i<insertResult.addedItems.count(); ++i)
+        insertResult.addedItems.at(i)->attached->emitAdd();
 
     // if the first visible item has moved, ensure another one takes its place
     // so that we avoid shifting all content forwards
-    // (don't use items from removedBeforeFirstVisible - if an item is removed from
-    // before the first visible, the first visible should not move upwards)
+    // (if an item is removed from before the first visible, the first visible should not move upwards)
     if (firstVisible && firstItemIndex >= 0) {
         bool found = false;
-        for (int i=0; i<movedBackwards.count(); i++) {
-            if (movedBackwards[i]->index == firstItemIndex) {
+        for (int i=0; i<insertResult.movedBackwards.count(); i++) {
+            if (insertResult.movedBackwards[i]->index == firstItemIndex) {
                 // an item has moved backwards up to the first visible's position
-                resetItemPosition(movedBackwards[i], firstVisible);
-                movedBackwards.removeAt(i);
+                resetItemPosition(insertResult.movedBackwards[i], firstVisible);
+                insertResult.movedBackwards.removeAt(i);
                 found = true;
                 break;
             }
         }
-        if (!found) {
-            // first visible item has moved forward, another item takes its place
+        if (!found && !allInsertionsBeforeVisible) {
+            // first visible item has moved forward, another visible item takes its place
             FxViewItem *item = visibleItem(firstItemIndex);
             if (item)
                 resetItemPosition(item, firstVisible);
@@ -1529,9 +1529,12 @@ bool QSGItemViewPrivate::applyModelChanges()
     if (firstVisible && visibleItems.count() && visibleItems.first() != firstVisible) {
         // ensure first item is placed at correct postion if moving backward
         // since it will be used to position all subsequent items
-        if (movedBackwards.count() && origVisibleItemsFirst)
+        if (insertResult.movedBackwards.count() && origVisibleItemsFirst)
             resetItemPosition(visibleItems.first(), origVisibleItemsFirst);
-        moveItemBy(visibleItems.first(), removedBeforeFirstVisible, movedBackwards);
+        qreal moveBackwardsBy = insertResult.sizeAddedBeforeVisible;
+        for (int i=0; i<insertResult.movedBackwards.count(); i++)
+            moveBackwardsBy += insertResult.movedBackwards[i]->size();
+        moveItemBy(visibleItems.first(), removedBeforeFirstVisibleBy, moveBackwardsBy);
     }
 
     // Whatever removed/moved items remain are no longer visible items.
diff --git a/src/declarative/items/qsgitemview_p_p.h b/src/declarative/items/qsgitemview_p_p.h
index 2d39dcec29..5eaa84c6b2 100644
--- a/src/declarative/items/qsgitemview_p_p.h
+++ b/src/declarative/items/qsgitemview_p_p.h
@@ -101,6 +101,14 @@ class QSGItemViewPrivate : public QSGFlickablePrivate
 public:
     QSGItemViewPrivate();
 
+    struct InsertionsResult {
+        QList<FxViewItem *> addedItems;
+        QList<FxViewItem *> movedBackwards;
+        qreal sizeAddedBeforeVisible;
+
+        InsertionsResult() : sizeAddedBeforeVisible(0) {}
+    };
+
     enum BufferMode { NoBuffer = 0x00, BufferBefore = 0x01, BufferAfter = 0x02 };
     enum MovementReason { Other, SetIndex, Mouse };
 
@@ -227,11 +235,11 @@ protected:
     virtual void repositionPackageItemAt(QSGItem *item, int index) = 0;
     virtual void resetItemPosition(FxViewItem *item, FxViewItem *toItem) = 0;
     virtual void resetFirstItemPosition() = 0;
-    virtual void moveItemBy(FxViewItem *item, const QList<FxViewItem *> &, const QList<FxViewItem *> &) = 0;
+    virtual void moveItemBy(FxViewItem *item, qreal forwards, qreal backwards) = 0;
 
     virtual void layoutVisibleItems() = 0;
     virtual void changedVisibleIndex(int newIndex) = 0;
-    virtual bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, QList<FxViewItem *> *, QList<FxViewItem *> *, FxViewItem *) = 0;
+    virtual bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, FxViewItem *, InsertionsResult *) = 0;
 
     virtual void initializeViewItem(FxViewItem *) {}
     virtual void initializeCurrentItem() {}
diff --git a/src/declarative/items/qsglistview.cpp b/src/declarative/items/qsglistview.cpp
index beeeda45e9..75472c2a79 100644
--- a/src/declarative/items/qsglistview.cpp
+++ b/src/declarative/items/qsglistview.cpp
@@ -95,7 +95,7 @@ public:
     virtual void repositionPackageItemAt(QSGItem *item, int index);
     virtual void resetItemPosition(FxViewItem *item, FxViewItem *toItem);
     virtual void resetFirstItemPosition();
-    virtual void moveItemBy(FxViewItem *item, const QList<FxViewItem *> &items, const QList<FxViewItem *> &movedBackwards);
+    virtual void moveItemBy(FxViewItem *item, qreal forwards, qreal backwards);
 
     virtual void createHighlight();
     virtual void updateHighlight();
@@ -103,7 +103,7 @@ public:
 
     virtual void setPosition(qreal pos);
     virtual void layoutVisibleItems();
-    bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, QList<FxViewItem *> *, QList<FxViewItem *> *, FxViewItem *firstVisible);
+    bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, FxViewItem *firstVisible, InsertionsResult *);
 
     virtual void updateSections();
     QSGItem *getSectionItem(const QString &section);
@@ -727,14 +727,10 @@ void QSGListViewPrivate::resetFirstItemPosition()
     item->setPosition(0);
 }
 
-void QSGListViewPrivate::moveItemBy(FxViewItem *item, const QList<FxViewItem *> &forwards, const QList<FxViewItem *> &backwards)
+void QSGListViewPrivate::moveItemBy(FxViewItem *item, qreal forwards, qreal backwards)
 {
-    qreal pos = 0;
-    for (int i=0; i<forwards.count(); i++)
-        pos += forwards[i]->size();
-    for (int i=0; i<backwards.count(); i++)
-        pos -= backwards[i]->size();
-    static_cast<FxListItemSG*>(item)->setPosition(item->position() + pos);
+    qreal diff = forwards - backwards;
+    static_cast<FxListItemSG*>(item)->setPosition(item->position() + diff);
 }
 
 void QSGListViewPrivate::createHighlight()
@@ -2347,7 +2343,7 @@ void QSGListView::updateSections()
     }
 }
 
-bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Insert &change, QList<FxViewItem *> *movedBackwards, QList<FxViewItem *> *addedItems, FxViewItem *firstVisible)
+bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Insert &change, FxViewItem *firstVisible, InsertionsResult *insertResult)
 {
     Q_Q(QSGListView);
 
@@ -2390,27 +2386,34 @@ bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
                                                 : visibleItems.last()->endPosition()+spacing;
     }
 
-    int prevAddedCount = addedItems->count();
+    int prevAddedCount = insertResult->addedItems.count();
     if (firstVisible && pos < firstVisible->position()) {
         // Insert items before the visible item.
         int insertionIdx = index;
         int i = 0;
         int from = tempPos - buffer;
 
-        for (i = count-1; i >= 0 && pos > from; --i) {
-            FxViewItem *item = 0;
-            if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
-                if (item->index > modelIndex + i)
-                    movedBackwards->append(item);
-                item->index = modelIndex + i;
-            }
-            if (!item)
-                item = createItem(modelIndex + i);
+        for (i = count-1; i >= 0; --i) {
+            if (pos > from) {
+                insertResult->sizeAddedBeforeVisible += averageSize;
+                pos -= averageSize;
+            } else {
+                FxViewItem *item = 0;
+                if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
+                    if (item->index > modelIndex + i)
+                        insertResult->movedBackwards.append(item);
+                    item->index = modelIndex + i;
+                }
+                if (!item)
+                    item = createItem(modelIndex + i);
 
-            visibleItems.insert(insertionIdx, item);
-            if (!change.isMove())
-                addedItems->append(item);
-            pos -= item->size() + spacing;
+                visibleItems.insert(insertionIdx, item);
+                if (!change.isMove()) {
+                    insertResult->addedItems.append(item);
+                    insertResult->sizeAddedBeforeVisible += item->size();
+                }
+                pos -= item->size() + spacing;
+            }
             index++;
         }
     } else {
@@ -2420,7 +2423,7 @@ bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
             FxViewItem *item = 0;
             if (change.isMove() && (item = currentChanges.removedItems.take(change.moveKey(modelIndex + i)))) {
                 if (item->index > modelIndex + i)
-                    movedBackwards->append(item);
+                    insertResult->movedBackwards.append(item);
                 item->index = modelIndex + i;
             }
             if (!item)
@@ -2428,7 +2431,7 @@ bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
 
             visibleItems.insert(index, item);
             if (!change.isMove())
-                addedItems->append(item);
+                insertResult->addedItems.append(item);
             pos += item->size() + spacing;
             ++index;
         }
@@ -2440,7 +2443,9 @@ bool QSGListViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::Inser
             item->index += count;
     }
 
-    return addedItems->count() > prevAddedCount;
+    updateVisibleIndex();
+
+    return insertResult->addedItems.count() > prevAddedCount;
 }