QListView: don't scroll if selected items are removed

For SingleSelection, removing the selected item will select the nearest
item and, if autoScroll is enabled, ensures that the newly selected
item is visible in the viewport. This may result in scrolling.

For Multi- or ExtendedSelection, this should not happen, as having no
selection is perfectly fine in those modes.
However, QListView still tried to scroll to the current item in response
to the currentIndexChanged signal. Since the currentIndex is at this
point already hidden, the rectangle for it became invalid, and the
attempt to scroll resulted in a one-pixel up-movement of the viewport
(since the invalid rectangle has width == height == -1).

Fix this by not scrolling if the rect for the index is invalid. Note that
the index is still valid at this point, so we can't shortcut the call
stack earlier. Add test that exercises the different combinations of
ViewMode and SelectionMode, and demonstrates the one-pixel
movement without the fix.

Fixes: QTBUG-94788
Change-Id: I1f36973eadb46e8c9b8b8068bc76ee09e9f490dd
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
(cherry picked from commit 26bebd2037eb69f7c939c899e3238a3e0f0a2376)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

2 files changed, 71 insertions, 0 deletions

diff --git a/src/widgets/itemviews/qlistview.cpp b/src/widgets/itemviews/qlistview.cpp
index 10e9fbb878..9b3221f84e 100644
--- a/src/widgets/itemviews/qlistview.cpp
+++ b/src/widgets/itemviews/qlistview.cpp
@@ -572,6 +572,8 @@ void QListView::scrollTo(const QModelIndex &index, ScrollHint hint)
         return;
 
     const QRect rect = visualRect(index);
+    if (!rect.isValid())
+        return;
     if (hint == EnsureVisible && d->viewport->rect().contains(rect)) {
         d->viewport->update(rect);
         return;
diff --git a/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp b/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
index 38c34aab76..7bd14729d8 100644
--- a/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
+++ b/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
@@ -173,6 +173,8 @@ private slots:
     void internalDragDropMove();
     void spacingWithWordWrap_data();
     void spacingWithWordWrap();
+    void scrollOnRemove_data();
+    void scrollOnRemove();
 };
 
 // Testing get/set functions
@@ -2850,5 +2852,72 @@ void tst_QListView::spacingWithWordWrap()
     }
 }
 
+void tst_QListView::scrollOnRemove_data()
+{
+    QTest::addColumn<QListView::ViewMode>("viewMode");
+    QTest::addColumn<QAbstractItemView::SelectionMode>("selectionMode");
+
+    const QMetaObject &mo = QListView::staticMetaObject;
+    const auto viewModeEnum = mo.enumerator(mo.indexOfEnumerator("ViewMode"));
+    const auto selectionModeEnum = mo.enumerator(mo.indexOfEnumerator("SelectionMode"));
+    for (auto viewMode : { QListView::ListMode, QListView::IconMode }) {
+        const char *viewModeName = viewModeEnum.valueToKey(viewMode);
+        for (int index = 0; index < selectionModeEnum.keyCount(); ++index) {
+            const auto selectionMode = QAbstractItemView::SelectionMode(selectionModeEnum.value(index));
+            const char *selectionModeName = selectionModeEnum.valueToKey(selectionMode);
+            QTest::addRow("%s, %s", viewModeName, selectionModeName) << viewMode << selectionMode;
+        }
+    }
+}
+
+void tst_QListView::scrollOnRemove()
+{
+    QFETCH(QListView::ViewMode, viewMode);
+    QFETCH(QAbstractItemView::SelectionMode, selectionMode);
+
+    QPixmap pixmap;
+    if (viewMode == QListView::IconMode) {
+        pixmap = QPixmap(25, 25);
+        pixmap.fill(Qt::red);
+    }
+
+    QStandardItemModel model;
+    for (int i = 0; i < 50; ++i) {
+        QStandardItem *item = new QStandardItem(QString::number(i));
+        item->setIcon(pixmap);
+        model.appendRow(item);
+    }
+
+    QWidget widget;
+    QListView view(&widget);
+    view.setFixedSize(100, 100);
+    view.setAutoScroll(true);
+    if (viewMode == QListView::IconMode)
+        view.setWrapping(true);
+    view.setModel(&model);
+    view.setSelectionMode(selectionMode);
+    view.setViewMode(viewMode);
+
+    widget.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&widget));
+
+    QCOMPARE(view.verticalScrollBar()->value(), 0);
+    const QModelIndex item25 = model.index(25, 0);
+    view.scrollTo(item25);
+    QTRY_VERIFY(view.verticalScrollBar()->value() > 0); // layout and scrolling are delayed
+    const int item25Position = view.verticalScrollBar()->value();
+    // selecting a fully visible item shouldn't scroll
+    view.selectionModel()->setCurrentIndex(item25, QItemSelectionModel::SelectCurrent);
+    QTRY_COMPARE(view.verticalScrollBar()->value(), item25Position);
+
+    // removing the selected item might scroll if another item is selected
+    model.removeRow(25);
+
+    // if nothing is selected now, then the view should not have scrolled
+    if (!view.selectionModel()->selectedIndexes().count())
+        QTRY_COMPARE(view.verticalScrollBar()->value(), item25Position);
+}
+
+
 QTEST_MAIN(tst_QListView)
 #include "tst_qlistview.moc"