Fix QListView assert when the last row is moved in IconMode

After the last row is moved, 0 will be returned when obtaining
row and column data. At this time, QListView::doitemslayout will
not call d->doitemslayout, so the QBspTree data structure will
not be cleaned up, leaving a stale tree structure behind. This
will trigger an assert during paintEvent handling if QListView is
set to IconMode

In QListView::ListMode the test for a valid model index doesn't
use an assert.

Call QListViewPrivate::clear explicitly if the column count is 0
so that the QBspTree and other data structures are cleared.

Add a test case that simulates this scenario by implementing a
model that returns a 0 column count for an index after the model
structure was changed through a move of rows.

Done-with: Volker Hilsheimer
Fixes: QTBUG-95463
Change-Id: I36419be5459b8ced930c619f538482ea1db4ad03
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit ca69e5aeef2fef540e687475ac00a4f332fdc5f3)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

src/widgets/itemviews/qlistview.cpp

@@ -1575,12 +1575,14 @@ void QListView::doItemsLayout()
     setState(ExpandingState);
     if (d->model->columnCount(d->root) > 0) { // no columns means no contents
         d->resetBatchStartRow();
-        if (layoutMode() == SinglePass)
+        if (layoutMode() == SinglePass) {
             d->doItemsLayout(d->model->rowCount(d->root)); // layout everything
-        else if (!d->batchLayoutTimer.isActive()) {
+        } else if (!d->batchLayoutTimer.isActive()) {
             if (!d->doItemsLayout(d->batchSize)) // layout is done
                 d->batchLayoutTimer.start(0, this); // do a new batch as fast as possible
         }
+    } else { // clear the QBspTree generated by the last layout
+        d->clear();
     }
     QAbstractItemView::doItemsLayout();
     setState(oldState);        // restoring the oldState

tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp

@@ -168,6 +168,7 @@ private slots:
     void taskQTBUG_7232_AllowUserToControlSingleStep();
     void taskQTBUG_51086_skippingIndexesInSelectedIndexes();
     void taskQTBUG_47694_indexOutOfBoundBatchLayout();
+    void moveLastRow();
     void itemAlignment();
     void internalDragDropMove_data();
     void internalDragDropMove();
@@ -2560,6 +2561,222 @@ void tst_QListView::taskQTBUG_47694_indexOutOfBoundBatchLayout()
     view.scrollTo(model.index(batchSize - 1, 0));
 }
 
+class TstMoveItem
+{
+    friend class TstMoveModel;
+public:
+    TstMoveItem(TstMoveItem *parent = nullptr)
+        : parentItem(parent)
+    {
+        if (parentItem)
+            parentItem->childItems.append(this);
+    }
+
+    ~TstMoveItem()
+    {
+        QList<TstMoveItem *> delItms;
+        delItms.swap(childItems);
+        qDeleteAll(delItms);
+
+        if (parentItem)
+            parentItem->childItems.removeAll(this);
+    }
+
+    int row()
+    {
+        if (parentItem)
+            return  parentItem->childItems.indexOf(this);
+        return -1;
+    }
+
+public:
+    TstMoveItem *parentItem = nullptr;
+    QList<TstMoveItem *> childItems;
+    QHash<int, QVariant> data;
+};
+
+/*!
+    Test that removing the last row in an IconView mode QListView
+    doesn't crash. The model is specifically crafted to provoke a
+    stale QBspTree by returning a 0 column count for indexes without
+    children, which changes the column count after moving the last row.
+
+    See QTBUG_95463.
+*/
+class TstMoveModel : public QAbstractItemModel
+{
+    Q_OBJECT
+public:
+    TstMoveModel(QObject *parent = nullptr)
+        : QAbstractItemModel(parent)
+    {
+        rootItem = new TstMoveItem;
+        rootItem->data.insert(Qt::DisplayRole, "root");
+
+        TstMoveItem *itm = new TstMoveItem(rootItem);
+        itm->data.insert(Qt::DisplayRole, "parentItem1");
+
+        TstMoveItem *itmCh = new TstMoveItem(itm);
+        itmCh->data.insert(Qt::DisplayRole, "childItem");
+
+        itm = new TstMoveItem(rootItem);
+        itm->data.insert(Qt::DisplayRole, "parentItem2");
+    }
+
+    ~TstMoveModel()
+    {
+        delete rootItem;
+    }
+
+    QModelIndex index(int row, int column, const QModelIndex &idxPar = QModelIndex()) const override
+    {
+        QModelIndex idx;
+        if (hasIndex(row, column, idxPar)) {
+            TstMoveItem *parentItem = nullptr;
+            if (idxPar.isValid())
+                parentItem = static_cast<TstMoveItem *>(idxPar.internalPointer());
+            else
+                parentItem = rootItem;
+
+            Q_ASSERT(parentItem);
+            TstMoveItem *childItem = parentItem->childItems.at(row);
+            if (childItem)
+                idx = createIndex(row, column, childItem);
+        }
+        return idx;
+    }
+
+    QModelIndex parent(const QModelIndex &index) const override
+    {
+        QModelIndex idxPar;
+        if (index.isValid()) {
+            TstMoveItem *childItem = static_cast<TstMoveItem *>(index.internalPointer());
+            TstMoveItem *parentItem = childItem->parentItem;
+            if (parentItem != rootItem)
+                idxPar = createIndex(parentItem->row(), 0, parentItem);
+        }
+        return idxPar;
+    }
+
+    int columnCount(const QModelIndex &idxPar = QModelIndex()) const override
+    {
+        int cnt = 0;
+        if (idxPar.isValid()) {
+            TstMoveItem *parentItem = static_cast<TstMoveItem *>(idxPar.internalPointer());
+            Q_ASSERT(parentItem);
+            cnt = parentItem->childItems.isEmpty() ? 0 : 1;
+        } else {
+            cnt = rootItem->childItems.isEmpty() ? 0 : 1;
+        }
+        return cnt;
+    }
+
+    int rowCount(const QModelIndex &idxPar = QModelIndex()) const override
+    {
+        int cnt = 0;
+        if (idxPar.isValid()) {
+            TstMoveItem *parentItem = static_cast<TstMoveItem *>(idxPar.internalPointer());
+            Q_ASSERT(parentItem);
+            cnt = parentItem->childItems.count();
+        } else {
+            cnt = rootItem->childItems.count();
+        }
+        return cnt;
+    }
+
+    Qt::ItemFlags flags(const QModelIndex &index) const override
+    {
+        Q_UNUSED(index)
+        return Qt::ItemIsEnabled | Qt::ItemIsSelectable;
+    }
+
+    bool hasChildren(const QModelIndex &parent = QModelIndex()) const override
+    {
+        bool ret = false;
+        if (parent.isValid()) {
+            TstMoveItem *parentItem = static_cast<TstMoveItem *>(parent.internalPointer());
+            Q_ASSERT(parentItem);
+            ret = parentItem->childItems.count() > 0;
+        } else {
+            ret = rootItem->childItems.count() > 0;
+        }
+        return ret;
+    }
+
+    QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override
+    {
+        QVariant dt;
+        if (index.isValid()) {
+            TstMoveItem *item = static_cast<TstMoveItem *>(index.internalPointer());
+            if (item)
+                dt = item->data.value(role);
+        }
+        return dt;
+    }
+
+    bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild) override
+    {
+        TstMoveItem *itmSrcParent = itemAt(sourceParent);
+        TstMoveItem *itmDestParent = itemAt(destinationParent);
+
+        if (itmSrcParent && sourceRow >= 0
+            && sourceRow + count <= itmSrcParent->childItems.count()
+            && itmDestParent && destinationChild <= itmDestParent->childItems.count()) {
+            beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1,
+                          destinationParent, destinationChild);
+            QList<TstMoveItem *> itemsToMove;
+            for (int i = 0; i < count; ++i) {
+                TstMoveItem *itm = itmSrcParent->childItems.at(sourceRow+i);
+                itemsToMove.append(itm);
+            }
+            for (int i = itemsToMove.count() -1; i >= 0; --i) {
+                TstMoveItem *itm = itemsToMove.at(i);
+                itm->parentItem->childItems.removeAll(itm);
+                itm->parentItem = itmDestParent;
+                itmDestParent->childItems.insert(destinationChild, itm);
+            }
+            endMoveRows();
+            return true;
+        }
+        return false;
+    }
+
+private:
+    TstMoveItem *itemAt(const QModelIndex &index) const
+    {
+        TstMoveItem *item = nullptr;
+        if (index.isValid()) {
+            Q_ASSERT(index.model() == this);
+            item = static_cast<TstMoveItem *>(index.internalPointer());
+        } else {
+            item = rootItem;
+        }
+        return item;
+    }
+
+private:
+    TstMoveItem *rootItem = nullptr;
+};
+
+void tst_QListView::moveLastRow()
+{
+    TstMoveModel model;
+    QListView view;
+    view.setModel(&model);
+    view.setRootIndex(model.index(0, 0, QModelIndex()));
+    view.setViewMode(QListView::IconMode);
+    view.show();
+
+    QApplication::setActiveWindow(&view);
+    QVERIFY(QTest::qWaitForWindowActive(&view));
+
+    QModelIndex sourceParent = model.index(0, 0);
+    QModelIndex destinationParent = model.index(1, 0);
+    // must not crash when paint event is processed
+    model.moveRow(sourceParent, 0, destinationParent, 0);
+    QTest::qWait(100);
+}
+
 void tst_QListView::itemAlignment()
 {
     auto item1 = new QStandardItem("111");