QAbstractItemModel: fix persistent index corruption when moving columns

QHeaderView creates persistent indexes in _q_sectionsAboutToBeChanged(), called by the slot connected to rowsAboutToBeMoved/columnsAboutToBeMoved. In the case of rows, QAbstractItemModel emits the signal *before* preparing to update persistent indexes in itemsAboutToBeMoved(), so it can see the ones newly created by QHeaderView, all is well. In the case of columns, the emit was done *after* calling itemsAboutToBeMoved(), so the additional persistent indexes created by QHeaderView were ignored, and in endMoveRows() we could end up with: ASSERT failure in QPersistentModelIndex::~QPersistentModelIndex: "persistent model indexes corrupted" This bug has been there since the very beginning of beginMoveColumns(), but was undetected because moving columns in a model is pretty rare (in my case there's a QTransposeProxyModel that turns columns into rows in the underlying model, and a proxy that handles dropMimeData...) Pick-to: 6.3 6.2 5.15 Change-Id: I74bad137594019a04c2a19c2abb351ff3065c25a Reviewed-by: Andreas Buhr <[email protected]> Reviewed-by: Qt CI Bot <[email protected]> Reviewed-by: Giuseppe D'Angelo <[email protected]>
petrmanek · Feb 28, 2022 · 74a4d88 · 74a4d88
1 parent af05f27
commit 74a4d88
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 3 deletions.
diff --git a/src/corelib/itemmodels/qabstractitemmodel.cpp b/src/corelib/itemmodels/qabstractitemmodel.cpp
@@ -3354,9 +3354,8 @@ bool QAbstractItemModel::beginMoveColumns(const QModelIndex &sourceParent, int s
     destinationChange.needsAdjust = destinationParent.isValid() && destinationParent.row() >= sourceLast && destinationParent.parent() == sourceParent;
     d->changes.push(destinationChange);
 
-    d->itemsAboutToBeMoved(sourceParent, sourceFirst, sourceLast, destinationParent, destinationChild, Qt::Horizontal);
-
     emit columnsAboutToBeMoved(sourceParent, sourceFirst, sourceLast, destinationParent, destinationChild, QPrivateSignal());
+    d->itemsAboutToBeMoved(sourceParent, sourceFirst, sourceLast, destinationParent, destinationChild, Qt::Horizontal);
     return true;
 }
 
@@ -3389,7 +3388,6 @@ void QAbstractItemModel::endMoveColumns()
       adjustedSource = createIndex(adjustedSource.row(), adjustedSource.column() + numMoved, adjustedSource.internalPointer());
 
     d->itemsMoved(adjustedSource, removeChange.first, removeChange.last, adjustedDestination, insertChange.first, Qt::Horizontal);
-
     emit columnsMoved(adjustedSource, removeChange.first, removeChange.last, adjustedDestination, insertChange.first, QPrivateSignal());
 }
 

diff --git a/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp b/tests/auto/widgets/itemviews/qheaderview/tst_qheaderview.cpp
@@ -249,6 +249,7 @@ private slots:
     void testResetCachedSizeHint();
     void statusTips();
     void testRemovingColumnsViaLayoutChanged();
+    void testModelMovingColumns();
 
 protected:
     void setupTestData(bool use_reset_model = false);
@@ -350,6 +351,12 @@ class QtTestModel: public QAbstractTableModel
         endRemoveColumns();
     }
 
+    void moveColumn(int from, int to)
+    {
+        beginMoveColumns(QModelIndex(), from, from, QModelIndex(), to);
+        endMoveColumns();
+    }
+
     void cleanup()
     {
         emit layoutAboutToBeChanged();
@@ -3635,5 +3642,18 @@ void tst_QHeaderView::testRemovingColumnsViaLayoutChanged()
     // The main point of this test is that the section-size restoring code didn't go out of bounds.
 }
 
+void tst_QHeaderView::testModelMovingColumns()
+{
+    QtTestModel model(10, 10);
+    QHeaderView hv(Qt::Horizontal);
+    hv.setModel(&model);
+    hv.resizeSections(QHeaderView::ResizeToContents);
+    hv.show();
+
+    QPersistentModelIndex index3 = model.index(0, 3);
+    model.moveColumn(3, 1);
+    QCOMPARE(index3.column(), 1);
+}
+
 QTEST_MAIN(tst_QHeaderView)
 #include "tst_qheaderview.moc"