GridView should re-layout if add/remove before visible index

Unlike ListView, GridView must redo its layout if an item is added or
removed from before the visible items, since it affects the column/row
layout.

Task-number: QTBUG-21588
Change-Id: Id333bc653033751c45d127973e94fae4580c55b0
Reviewed-by: Bea Lam <bea.lam@nokia.com>

4 files changed, 107 insertions, 8 deletions

diff --git a/src/declarative/items/qquickgridview.cpp b/src/declarative/items/qquickgridview.cpp
index 2793032a48..a4133143ea 100644
--- a/src/declarative/items/qquickgridview.cpp
+++ b/src/declarative/items/qquickgridview.cpp
@@ -182,6 +182,7 @@ public:
     virtual void setPosition(qreal pos);
     virtual void layoutVisibleItems();
     bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, FxViewItem *, InsertionsResult *);
+    virtual bool needsRefillForAddedOrRemovedIndex(int index) const;
 
     virtual qreal headerSize() const;
     virtual qreal footerSize() const;
@@ -1853,6 +1854,13 @@ bool QQuickGridViewPrivate::applyInsertionChange(const QDeclarativeChangeSet::In
     return insertResult->addedItems.count() > prevAddedCount;
 }
 
+bool QQuickGridViewPrivate::needsRefillForAddedOrRemovedIndex(int modelIndex) const
+{
+    // If we add or remove items before visible items, a layout may be
+    // required to ensure item 0 is in the first column.
+    return modelIndex < visibleIndex;
+}
+
 /*!
     \qmlmethod QtQuick2::GridView::positionViewAtIndex(int index, PositionMode mode)
 
diff --git a/src/declarative/items/qquickitemview.cpp b/src/declarative/items/qquickitemview.cpp
index 923db4747a..79787d777c 100644
--- a/src/declarative/items/qquickitemview.cpp
+++ b/src/declarative/items/qquickitemview.cpp
@@ -1413,7 +1413,7 @@ bool QQuickItemViewPrivate::applyModelChanges()
     moveReason = QQuickItemViewPrivate::Other;
 
     int prevCount = itemCount;
-    bool removedVisible = false;
+    bool visibleAffected = false;
     bool viewportChanged = !currentChanges.pendingChanges.removes().isEmpty()
             || !currentChanges.pendingChanges.inserts().isEmpty();
 
@@ -1432,8 +1432,8 @@ bool QQuickItemViewPrivate::applyModelChanges()
             FxViewItem *item = *it;
             if (item->index == -1 || item->index < removals[i].index) {
                 // already removed, or before removed items
-                if (item->index < removals[i].index && !removedVisible)
-                    removedVisible = true;
+                if (!visibleAffected && item->index < removals[i].index)
+                    visibleAffected = true;
                 ++it;
             } else if (item->index >= removals[i].index + removals[i].count) {
                 // after removed items
@@ -1441,7 +1441,7 @@ bool QQuickItemViewPrivate::applyModelChanges()
                 ++it;
             } else {
                 // removed item
-                removedVisible = true;
+                visibleAffected = true;
                 if (!removals[i].isMove())
                     item->attached->emitRemove();
 
@@ -1463,20 +1463,22 @@ bool QQuickItemViewPrivate::applyModelChanges()
                 }
             }
         }
-
+        if (!visibleAffected && needsRefillForAddedOrRemovedIndex(removals[i].index))
+            visibleAffected = true;
     }
     if (!removals.isEmpty())
         updateVisibleIndex();
 
     const QVector<QDeclarativeChangeSet::Insert> &insertions = currentChanges.pendingChanges.inserts();
-    bool addedVisible = false;
     InsertionsResult insertResult;
     bool allInsertionsBeforeVisible = true;
 
     for (int i=0; i<insertions.count(); i++) {
         bool wasEmpty = visibleItems.isEmpty();
         if (applyInsertionChange(insertions[i], firstVisible, &insertResult))
-            addedVisible = true;
+            visibleAffected = true;
+        if (!visibleAffected && needsRefillForAddedOrRemovedIndex(insertions[i].index))
+            visibleAffected = true;
         if (insertions[i].index >= visibleIndex)
             allInsertionsBeforeVisible = false;
         if (wasEmpty && !visibleItems.isEmpty())
@@ -1542,7 +1544,8 @@ bool QQuickItemViewPrivate::applyModelChanges()
     if (prevCount != itemCount)
         emit q->countChanged();
 
-    bool visibleAffected = removedVisible || addedVisible || !currentChanges.pendingChanges.changes().isEmpty();
+    if (!visibleAffected)
+        visibleAffected = !currentChanges.pendingChanges.changes().isEmpty();
     if (!visibleAffected && viewportChanged)
         updateViewport();
 
diff --git a/src/declarative/items/qquickitemview_p_p.h b/src/declarative/items/qquickitemview_p_p.h
index 398de84c25..ca4c0ceba9 100644
--- a/src/declarative/items/qquickitemview_p_p.h
+++ b/src/declarative/items/qquickitemview_p_p.h
@@ -240,6 +240,7 @@ protected:
     virtual void layoutVisibleItems() = 0;
     virtual void changedVisibleIndex(int newIndex) = 0;
     virtual bool applyInsertionChange(const QDeclarativeChangeSet::Insert &, FxViewItem *, InsertionsResult *) = 0;
+    virtual bool needsRefillForAddedOrRemovedIndex(int) const { return false; }
 
     virtual void initializeViewItem(FxViewItem *) {}
     virtual void initializeCurrentItem() {}
diff --git a/tests/auto/declarative/qquickgridview/tst_qquickgridview.cpp b/tests/auto/declarative/qquickgridview/tst_qquickgridview.cpp
index 9492605359..a190d878ad 100644
--- a/tests/auto/declarative/qquickgridview/tst_qquickgridview.cpp
+++ b/tests/auto/declarative/qquickgridview/tst_qquickgridview.cpp
@@ -72,6 +72,8 @@ private slots:
     void inserted_more();
     void inserted_more_data();
     void removed();
+    void addOrRemoveBeforeVisible();
+    void addOrRemoveBeforeVisible_data();
     void clear();
     void moved();
     void moved_data();
@@ -740,6 +742,91 @@ void tst_QQuickGridView::removed()
     delete canvas;
 }
 
+void tst_QQuickGridView::addOrRemoveBeforeVisible()
+{
+    // QTBUG-21588: ensure re-layout is done on grid after adding or removing
+    // items from before the visible area
+
+    QFETCH(bool, doAdd);
+    QFETCH(qreal, newTopContentY);
+
+    QQuickView *canvas = createView();
+    canvas->show();
+
+    TestModel model;
+    for (int i = 0; i < 30; i++)
+        model.addItem("Item" + QString::number(i), "");
+
+    QDeclarativeContext *ctxt = canvas->rootContext();
+    ctxt->setContextProperty("testModel", &model);
+    ctxt->setContextProperty("testRightToLeft", QVariant(false));
+    ctxt->setContextProperty("testTopToBottom", QVariant(false));
+    canvas->setSource(QUrl::fromLocalFile(TESTDATA("gridview1.qml")));
+
+    QQuickGridView *gridview = findItem<QQuickGridView>(canvas->rootObject(), "grid");
+    QTRY_VERIFY(gridview != 0);
+    QQuickItem *contentItem = gridview->contentItem();
+    QTRY_VERIFY(contentItem != 0);
+
+    QQuickText *name = findItem<QQuickText>(contentItem, "textName", 0);
+    QTRY_COMPARE(name->text(), QString("Item0"));
+
+    gridview->setCurrentIndex(0);
+    qApp->processEvents();
+
+    // scroll down until item 0 is no longer drawn
+    // (bug not triggered if we just move using content y, since that doesn't
+    // refill and change the visible items)
+    gridview->setCurrentIndex(24);
+    qApp->processEvents();
+
+    QTRY_COMPARE(gridview->currentIndex(), 24);
+    QTRY_COMPARE(gridview->contentY(), 220.0);
+
+    QTest::qWait(100);  // wait for refill to complete
+    QTRY_VERIFY(!findItem<QQuickItem>(contentItem, "wrapper", 0));  // 0 shouldn't be visible
+
+    if (doAdd) {
+        model.insertItem(0, "New Item", "New Item number");
+        QTRY_COMPARE(gridview->count(), 31);
+    } else {
+        model.removeItem(0);
+        QTRY_COMPARE(gridview->count(), 29);
+    }
+
+    // scroll back up and item 0 should be gone
+    gridview->setCurrentIndex(0);
+    qApp->processEvents();
+    QTRY_COMPARE(gridview->currentIndex(), 0);
+    QTRY_COMPARE(gridview->contentY(), newTopContentY);
+
+    name = findItem<QQuickText>(contentItem, "textName", 0);
+    if (doAdd)
+        QCOMPARE(name->text(), QString("New Item"));
+    else
+        QCOMPARE(name->text(), QString("Item1"));
+
+    // Confirm items positioned correctly
+    int itemCount = findItems<QQuickItem>(contentItem, "wrapper").count();
+    for (int i = 0; i < model.count() && i < itemCount; ++i) {
+        QTRY_VERIFY(findItem<QQuickItem>(contentItem, "wrapper", i));
+        QQuickItem *item = findItem<QQuickItem>(contentItem, "wrapper", i);
+        QTRY_VERIFY(item->x() == (i%3)*80);
+        QTRY_VERIFY(item->y() == (i/3)*60 + newTopContentY);
+    }
+
+    delete canvas;
+}
+
+void tst_QQuickGridView::addOrRemoveBeforeVisible_data()
+{
+    QTest::addColumn<bool>("doAdd");
+    QTest::addColumn<qreal>("newTopContentY");
+
+    QTest::newRow("add") << true << -60.0;
+    QTest::newRow("remove") << false << 0.0;
+}
+
 void tst_QQuickGridView::clear()
 {
     QQuickView *canvas = createView();