Invalid QPersistentIndexes after QStandardItem::takeRow

We need the parent of each potential QPersistentModelIndex in order to
cleanup when removing the rows.
They need not to change in order QSortFilterProxyModel maping to be
still valid.

takeRow must not change the internal data before calling beginRemoveRow.

Same thing for takeColumn

Task-number: 255652
Reviewed-by: Thierry
Reviewed-by: Leo

3 files changed, 148 insertions, 36 deletions

diff --git a/src/gui/itemviews/qstandarditemmodel.cpp b/src/gui/itemviews/qstandarditemmodel.cpp
index 9d0f796..30daed2 100644
--- a/src/gui/itemviews/qstandarditemmodel.cpp
+++ b/src/gui/itemviews/qstandarditemmodel.cpp
@@ -1742,18 +1742,21 @@ QList<QStandardItem*> QStandardItem::takeRow(int row)
     Q_D(QStandardItem);
     if ((row < 0) || (row >= rowCount()))
         return QList<QStandardItem*>();
+    if (d->model)
+        d->model->d_func()->rowsAboutToBeRemoved(this, row, row);
     QList<QStandardItem*> items;
     int index = d->childIndex(row, 0);
-    for (int column = 0; column < d->columnCount(); ++column) {
-        QStandardItem *ch = d->children.at(index);
-        if (ch) {
+    int col_count = d->columnCount();
+    for (int column = 0; column < col_count; ++column) {
+        QStandardItem *ch = d->children.at(index + column);
+        if (ch)
             ch->d_func()->setParentAndModel(0, 0);
-            d->children.replace(index, 0);
-        }
         items.append(ch);
-        ++index;
     }
-    removeRow(row);
+    d->children.remove(index, col_count);
+    d->rows--;
+    if (d->model)
+        d->model->d_func()->rowsRemoved(this, row, 1);
     return items;
 }
 
@@ -1769,18 +1772,21 @@ QList<QStandardItem*> QStandardItem::takeColumn(int column)
     Q_D(QStandardItem);
     if ((column < 0) || (column >= columnCount()))
         return QList<QStandardItem*>();
+    if (d->model)
+        d->model->d_func()->columnsAboutToBeRemoved(this, column, column);
     QList<QStandardItem*> items;
-    int index = d->childIndex(0, column);
-    for (int row = 0; row < d->rowCount(); ++row) {
+
+    for (int row = d->rowCount() - 1; row >= 0; --row) {
+        int index = d->childIndex(row, column);
         QStandardItem *ch = d->children.at(index);
-        if (ch) {
+        if (ch)
             ch->d_func()->setParentAndModel(0, 0);
-            d->children.replace(index, 0);
-        }
-        items.append(ch);
-        index += d->columnCount();
+        d->children.remove(index);
+        items.prepend(ch);
     }
-    removeColumn(column);
+    d->columns--;
+    if (d->model)
+        d->model->d_func()->columnsRemoved(this, column, 1);
     return items;
 }
 
diff --git a/tests/auto/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp b/tests/auto/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
index d7c231f..274b177 100644
--- a/tests/auto/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
+++ b/tests/auto/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
@@ -134,6 +134,7 @@ private slots:
     void task250023_fetchMore();
     void task251296_hiddenChildren();
     void task252507_mapFromToSource();
+    void task255652_removeRowsRecursive();
 
 protected:
     void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
@@ -390,7 +391,7 @@ void tst_QSortFilterProxyModel::sort()
         QModelIndex index = m_proxy->index(row, 0, QModelIndex());
         QCOMPARE(m_proxy->data(index, Qt::DisplayRole).toString(), initial.at(row));
     }
-    
+
 }
 
 void tst_QSortFilterProxyModel::sortHierarchy_data()
@@ -411,7 +412,7 @@ void tst_QSortFilterProxyModel::sortHierarchy_data()
         << static_cast<int>(Qt::AscendingOrder)
         << (QStringList() << "a" << "<" << "b" << "<" << "c" << ">" << ">")
         << (QStringList() << "a" << "<" << "b" << "<" << "c" << ">" << ">");
-    
+
 #if 1
     QTest::newRow("hierarchical ascending")
         << static_cast<int>(Qt::AscendingOrder)
@@ -571,7 +572,7 @@ void tst_QSortFilterProxyModel::insertRows()
         QModelIndex index = m_proxy->index(row, 0, QModelIndex());
         QCOMPARE(m_proxy->data(index, Qt::DisplayRole).toString(), initial.at(row));
     }
-    
+
     // insert the row
     m_proxy->insertRows(position, insert.count(), QModelIndex());
     QCOMPARE(m_model->rowCount(QModelIndex()), expected.count());
@@ -615,7 +616,7 @@ void tst_QSortFilterProxyModel::prependRow()
     QStandardItem sub2("sub2");
     sub2.appendRow(new QStandardItem("sub3"));
     item.insertRow(0, &sub2);
-    
+
     QModelIndex index_sub2 = proxy.mapFromSource(model.indexFromItem(&sub2));
 
     QCOMPARE(sub2.rowCount(), proxy.rowCount(index_sub2));
@@ -626,7 +627,7 @@ void tst_QSortFilterProxyModel::prependRow()
 /*
 void tst_QSortFilterProxyModel::insertColumns_data()
 {
-    
+
 }
 
 void tst_QSortFilterProxyModel::insertColumns()
@@ -923,7 +924,7 @@ void tst_QSortFilterProxyModel::removeRows()
     // prepare model
     foreach (QString s, initial)
         model.appendRow(new QStandardItem(s));
-    
+
     // remove the rows
     QCOMPARE(proxy.removeRows(position, count, QModelIndex()), success);
     QCOMPARE(model.rowCount(QModelIndex()), expectedSource.count());
@@ -1164,10 +1165,10 @@ void tst_QSortFilterProxyModel::removeColumns()
     proxy.setSourceModel(&model);
     if (!filter.isEmpty())
         proxy.setFilterRegExp(QRegExp(filter));
-    
+
     // prepare model
     model.setHorizontalHeaderLabels(initial);
-    
+
     // remove the columns
     QCOMPARE(proxy.removeColumns(position, count, QModelIndex()), success);
     QCOMPARE(model.columnCount(QModelIndex()), expectedSource.count());
@@ -1266,7 +1267,7 @@ void tst_QSortFilterProxyModel::filter_data()
                            << "golf"
                            << "quebec"
                            << "foxtrot"
-                           << "india" 
+                           << "india"
                            << "romeo"
                            << "november"
                            << "oskar"
@@ -1274,9 +1275,9 @@ void tst_QSortFilterProxyModel::filter_data()
                            << "kilo"
                            << "whiskey"
                            << "mike"
-                           << "papa" 
+                           << "papa"
                            << "sierra"
-                           << "xray" 
+                           << "xray"
                            << "viktor")
                        << (QStringList()
                            << "delta"
@@ -1334,7 +1335,7 @@ void tst_QSortFilterProxyModel::filterHierarchy_data()
             << "foo" << "boo" << "baz" << "moo" << "laa" << "haa")
         << (QStringList()
             << "foo" << "boo" << "moo");
-    
+
     QTest::newRow("simple hierarchy") << "b.*z"
         << (QStringList() << "baz" << "<" << "boz" << "<" << "moo" << ">" << ">")
         << (QStringList() << "baz" << "<" << "boz" << ">");
@@ -1671,7 +1672,7 @@ void tst_QSortFilterProxyModel::removeSourceRows()
     QSignalSpy insertSpy(&proxy, SIGNAL(rowsInserted(QModelIndex, int, int)));
     QSignalSpy aboutToRemoveSpy(&proxy, SIGNAL(rowsAboutToBeRemoved(QModelIndex, int, int)));
     QSignalSpy aboutToInsertSpy(&proxy, SIGNAL(rowsAboutToBeInserted(QModelIndex, int, int)));
-    
+
     model.removeRows(start, count, QModelIndex());
 
     QCOMPARE(aboutToRemoveSpy.count(), expectedRemovedProxyIntervals.count());
@@ -1827,7 +1828,7 @@ void tst_QSortFilterProxyModel::changeFilter()
     QFETCH(IntPairList, finalRemoveIntervals);
     QFETCH(IntPairList, insertIntervals);
     QFETCH(QStringList, finalProxyItems);
-    
+
     QStandardItemModel model;
     QSortFilterProxyModel proxy;
 
@@ -1880,7 +1881,7 @@ void tst_QSortFilterProxyModel::changeFilter()
 
 #ifdef Q_OS_IRIX
     QEXPECT_FAIL("filter (2)", "Not reliable on IRIX", Abort);
-#endif    
+#endif
     QCOMPARE(finalInsertSpy.count(), insertIntervals.count());
     for (int i = 0; i < finalInsertSpy.count(); ++i) {
         QList<QVariant> args = finalInsertSpy.at(i);
@@ -2068,7 +2069,7 @@ void tst_QSortFilterProxyModel::sortFilterRole()
         model.setData(index, sourceItems.at(i).first, Qt::DisplayRole);
         model.setData(index, sourceItems.at(i).second, Qt::UserRole);
     }
-        
+
     proxy.setFilterRegExp("2");
     QCOMPARE(proxy.rowCount(), 0); // Qt::DisplayRole is default role
 
@@ -2358,7 +2359,7 @@ void tst_QSortFilterProxyModel::sourceInsertRows()
       model.insertColumns(0, 1, parent);
       model.insertRows(0, 1, parent);
     }
-   
+
     model.insertRows(0, 1, QModelIndex());
     model.insertRows(0, 1, QModelIndex());
 
@@ -2462,7 +2463,7 @@ void tst_QSortFilterProxyModel::task236755_hiddenColumns()
     QVERIFY(view.isColumnHidden(0));
 }
 
-void tst_QSortFilterProxyModel::task247867_insertRowsSort() 
+void tst_QSortFilterProxyModel::task247867_insertRowsSort()
 {
     QStandardItemModel model(2,2);
     QSortFilterProxyModel proxyModel;
@@ -2567,7 +2568,7 @@ void tst_QSortFilterProxyModel::task248868_dynamicSorting()
     QStringList initial2 = initial;
     initial2.replaceInStrings("bateau", "girafe");
     model1.setStringList(initial2); //this will cause a reset
-    
+
     QStringList expected2 = initial2;
     expected2.sort();
 
@@ -2648,7 +2649,7 @@ class QtTestModel: public QAbstractItemModel
         {
             if (!idx.isValid())
                 return QVariant();
-            
+
             if (role == Qt::DisplayRole) {
                 if (idx.row() < 0 || idx.column() < 0 || idx.column() >= cols || idx.row() >= rows) {
                     wrongIndex = true;
@@ -2756,5 +2757,63 @@ void tst_QSortFilterProxyModel::task252507_mapFromToSource()
     QCOMPARE(proxy.mapFromSource(proxy.index(6, 2)), QModelIndex());
 }
 
+static QStandardItem *addEntry(QStandardItem* pParent, const QString &description)
+{
+    QStandardItem* pItem = new QStandardItem(description);
+    pParent->appendRow(pItem);
+    return pItem;
+}
+
+
+void tst_QSortFilterProxyModel::task255652_removeRowsRecursive()
+{
+    QStandardItemModel pModel;
+    QStandardItem *pItem1    = new QStandardItem("root");
+    pModel.appendRow(pItem1);
+    QList<QStandardItem *> items;
+
+    QStandardItem *pItem11   = addEntry(pItem1,"Sub-heading");
+    items << pItem11;
+    QStandardItem *pItem111 = addEntry(pItem11,"A");
+    items << pItem111;
+    items << addEntry(pItem111,"A1");
+    items << addEntry(pItem111,"A2");
+    QStandardItem *pItem112 = addEntry(pItem11,"B");
+    items << pItem112;
+    items << addEntry(pItem112,"B1");
+    items << addEntry(pItem112,"B2");
+    QStandardItem *pItem1123 = addEntry(pItem112,"B3");
+    items << pItem1123;
+    items << addEntry(pItem1123,"B3-");
+
+    QSortFilterProxyModel proxy;
+    proxy.setSourceModel(&pModel);
+
+    QList<QPersistentModelIndex> sourceIndexes;
+    QList<QPersistentModelIndex> proxyIndexes;
+    foreach (QStandardItem *item, items) {
+        QModelIndex idx = item->index();
+        sourceIndexes << idx;
+        proxyIndexes << proxy.mapFromSource(idx);
+    }
+
+    foreach (const QPersistentModelIndex &pidx, sourceIndexes)
+        QVERIFY(pidx.isValid());
+    foreach (const QPersistentModelIndex &pidx, proxyIndexes)
+        QVERIFY(pidx.isValid());
+
+    QList<QStandardItem*> itemRow = pItem1->takeRow(0);
+
+    QCOMPARE(itemRow.count(), 1);
+    QCOMPARE(itemRow.first(), pItem11);
+
+    foreach (const QPersistentModelIndex &pidx, sourceIndexes)
+        QVERIFY(!pidx.isValid());
+    foreach (const QPersistentModelIndex &pidx, proxyIndexes)
+        QVERIFY(!pidx.isValid());
+
+    delete pItem11;
+}
+
 QTEST_MAIN(tst_QSortFilterProxyModel)
 #include "tst_qsortfilterproxymodel.moc"
diff --git a/tests/auto/qstandarditemmodel/tst_qstandarditemmodel.cpp b/tests/auto/qstandarditemmodel/tst_qstandarditemmodel.cpp
index 84bda92..8067554 100644
--- a/tests/auto/qstandarditemmodel/tst_qstandarditemmodel.cpp
+++ b/tests/auto/qstandarditemmodel/tst_qstandarditemmodel.cpp
@@ -135,6 +135,7 @@ private slots:
 
     void rootItemFlags();
     void treeDragAndDrop();
+    void removeRowsAndColumns();
 
 private:
     QAbstractItemModel *m_model;
@@ -1403,7 +1404,7 @@ bool tst_QStandardItemModel::compareItems(QStandardItem *item1, QStandardItem *i
         return true;
     if (!item1 || !item2)
         return false;
-    if (item1->text() != item2->text()){  
+    if (item1->text() != item2->text()){
         qDebug() << item1->text() << item2->text();
         return false;
     }
@@ -1606,6 +1607,52 @@ void tst_QStandardItemModel::treeDragAndDrop()
     }
 }
 
+void tst_QStandardItemModel::removeRowsAndColumns()
+{
+#define VERIFY_MODEL \
+    for (int c = 0; c < col_list.count(); c++) \
+        for (int r = 0; r < row_list.count(); r++) \
+            QCOMPARE(model.item(r,c)->text() , row_list[r] + "x" + col_list[c]);
+
+    QVector<QString> row_list = QString("1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20").split(',').toVector();
+    QVector<QString> col_list = row_list;
+    QStandardItemModel model;
+    for (int c = 0; c < col_list.count(); c++)
+        for (int r = 0; r < row_list.count(); r++)
+            model.setItem(r, c, new QStandardItem(row_list[r] + "x" + col_list[c]));
+    VERIFY_MODEL
+
+    row_list.remove(3);
+    model.removeRow(3);
+    VERIFY_MODEL
+
+    col_list.remove(5);
+    model.removeColumn(5);
+    VERIFY_MODEL
+
+    row_list.remove(2, 5);
+    model.removeRows(2, 5);
+    VERIFY_MODEL
+
+    col_list.remove(1, 6);
+    model.removeColumns(1, 6);
+    VERIFY_MODEL
+
+    QList<QStandardItem *> row_taken = model.takeRow(6);
+    QCOMPARE(row_taken.count(), col_list.count());
+    for (int c = 0; c < col_list.count(); c++)
+        QCOMPARE(row_taken[c]->text() , row_list[6] + "x" + col_list[c]);
+    row_list.remove(6);
+    VERIFY_MODEL
+
+    QList<QStandardItem *> col_taken = model.takeColumn(10);
+    QCOMPARE(col_taken.count(), row_list.count());
+    for (int r = 0; r < row_list.count(); r++)
+        QCOMPARE(col_taken[r]->text() , row_list[r] + "x" + col_list[10]);
+    col_list.remove(10);
+    VERIFY_MODEL
+}
+
 
 QTEST_MAIN(tst_QStandardItemModel)
 #include "tst_qstandarditemmodel.moc"