summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorChunLin Wang <wangchunlin@uniontech.com>2021-08-26 21:15:31 +0800
committerQt Cherry-pick Bot <cherrypick_bot@qt-project.org>2021-09-01 17:06:53 +0000
commit8ec406b2296abdbdc8606fa955a2f8feb5254b1f (patch)
tree41abf5671308a58b6b9e5d7177b3afd58417e76d
parent561ba60ea436c06f7aa88ae9c8a847f851851112 (diff)
Fix QListView assert when the last row is moved in IconModev6.2.0-beta4
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>
-rw-r--r--src/widgets/itemviews/qlistview.cpp6
-rw-r--r--tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp217
2 files changed, 221 insertions, 2 deletions
diff --git a/src/widgets/itemviews/qlistview.cpp b/src/widgets/itemviews/qlistview.cpp
index 68d5d7612f..f9e3082962 100644
--- a/src/widgets/itemviews/qlistview.cpp
+++ b/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
diff --git a/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp b/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
index 74198d1cc4..749cb17bf9 100644
--- a/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
+++ b/tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp
@@ -171,6 +171,7 @@ private slots:
void taskQTBUG_58749_adjustToContent();
void taskQTBUG_51086_skippingIndexesInSelectedIndexes();
void taskQTBUG_47694_indexOutOfBoundBatchLayout();
+ void moveLastRow();
void itemAlignment();
void internalDragDropMove_data();
void internalDragDropMove();
@@ -2602,6 +2603,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");