ListView: Sanitize visibleItems list after model insertions

In QQuickListViewPrivate::applyInsertionChange(), we update the
visibleItems list by first shifting the currently visible items
and then we add as many items as the model was added and at the
right position. We do this in such a way that we won't create
items that will not be visible right away (and may be deleted
right after by removeNonVisibleItems()). However, this may leave
gaps in the item index sequence, and QQuickListView doesn't always
recover gracefully from it.

The purpose of this patch is to make sure those gaps are cleared
right after inserting the new items. Since the insertions can happen
in two different places (either before or after the first visible
item) we need to update the visibleItems list accordingly. The way
we sanitize visibleItems is by removing those items that lie beyond
a possible index gap. If insertion happens before the first visible
item, we'll remove all those items before the insertion point. If
the insertion happens after the first visible item, we'll remove the
items after the insertion point.

Besides that, the logic for inserting before the visible position was
wrong. As items are inserted bottom-up in that case, the insertion
would start by just accounting for the item's size until the condition

    pos > from && insertionIdx < visibleIndex

would become false only because 'pos' would be small enough. After
that, the next loop run would start adding items before the 'from'
position, which is wrong. Our fix is to move the condition outside
the loop if the insertion index will be before the visible index
and just account for the items' size in that case. Otherwise, the
insertion happens as usual until pos < from.

Change-Id: I35767cf6e9737bea1fe7677e580245fc7172710c
Task-number: QTBUG-48870
Reviewed-by: Shawn Rutledge <shawn.rutledge@theqtcompany.com>
Reviewed-by: Robin Burchell <robin.burchell@viroteck.net>

2 files changed, 14 insertions, 6 deletions

diff --git a/tests/auto/quick/qquicklistview/BLACKLIST b/tests/auto/quick/qquicklistview/BLACKLIST
index 02adeacf05..269696ce8c 100644
--- a/tests/auto/quick/qquicklistview/BLACKLIST
+++ b/tests/auto/quick/qquicklistview/BLACKLIST
@@ -2,5 +2,3 @@
 *
 [enforceRange_withoutHighlight]
 osx
-[QTBUG_48870_fastModelUpdates]
-*
diff --git a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
index ce2337ccb2..b3a6edb66a 100644
--- a/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
+++ b/tests/auto/quick/qquicklistview/tst_qquicklistview.cpp
@@ -592,8 +592,7 @@ void tst_QQuickListView::inserted(const QUrl &source)
 
     QQuickItem *item = findItem<QQuickItem>(contentItem, "wrapper", 0);
     QVERIFY(item);
-    QCOMPARE(item->y(), 0.);
-    QTRY_COMPARE(listview->contentY(), qreal(0));
+    QTRY_COMPARE(item->y() - listview->contentY(), 0.);
 
     delete window;
     delete testObject;
@@ -644,7 +643,8 @@ void tst_QQuickListView::inserted_more(QQuickItemView::VerticalLayoutDirection v
 
     QTRY_COMPARE(listview->property("count").toInt(), model.count());
 
-
+    // FIXME This is NOT checking anything about visibleItems.first()
+#if 0
     // check visibleItems.first() is in correct position
     QQuickItem *item0 = findItem<QQuickItem>(contentItem, "wrapper", 0);
     QVERIFY(item0);
@@ -652,6 +652,7 @@ void tst_QQuickListView::inserted_more(QQuickItemView::VerticalLayoutDirection v
         QCOMPARE(item0->y(), -item0->height() - itemsOffsetAfterMove);
     else
         QCOMPARE(item0->y(), itemsOffsetAfterMove);
+#endif
 
     QList<QQuickItem*> items = findItems<QQuickItem>(contentItem, "wrapper");
     int firstVisibleIndex = -1;
@@ -667,12 +668,21 @@ void tst_QQuickListView::inserted_more(QQuickItemView::VerticalLayoutDirection v
     // Confirm items positioned correctly and indexes correct
     QQuickText *name;
     QQuickText *number;
+    const qreal visibleFromPos = listview->contentY() - listview->displayMarginBeginning() - listview->cacheBuffer();
+    const qreal visibleToPos = listview->contentY() + listview->height() + listview->displayMarginEnd() + listview->cacheBuffer();
     for (int i = firstVisibleIndex; i < model.count() && i < items.count(); ++i) {
         QQuickItem *item = findItem<QQuickItem>(contentItem, "wrapper", i);
         QVERIFY2(item, QTest::toString(QString("Item %1 not found").arg(i)));
         qreal pos = i*20.0 + itemsOffsetAfterMove;
         if (verticalLayoutDirection == QQuickItemView::BottomToTop)
-            pos = -item0->height() - pos;
+            pos = -item->height() - pos;
+        // Items outside the visible area (including cache buffer) should be skipped
+        if (pos > visibleToPos || pos < visibleFromPos) {
+            QTRY_VERIFY2(QQuickItemPrivate::get(item)->culled || item->y() < visibleFromPos || item->y() > visibleToPos,
+                     QTest::toString(QString("index %5, y %1, from %2, to %3, expected pos %4, culled %6").
+                                     arg(item->y()).arg(visibleFromPos).arg(visibleToPos).arg(pos).arg(i).arg(bool(QQuickItemPrivate::get(item)->culled))));
+            continue;
+        }
         QTRY_COMPARE(item->y(), pos);
         name = findItem<QQuickText>(contentItem, "textName", i);
         QVERIFY(name != 0);