From e0be99974d5d66f548543df9b5b919c8e81ade62 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Wed, 10 Jun 2009 18:42:35 +0200 Subject: 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 --- src/gui/itemviews/qstandarditemmodel.cpp | 36 ++++---- .../tst_qsortfilterproxymodel.cpp | 99 +++++++++++++++++----- .../qstandarditemmodel/tst_qstandarditemmodel.cpp | 49 ++++++++++- 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::takeRow(int row) Q_D(QStandardItem); if ((row < 0) || (row >= rowCount())) return QList(); + if (d->model) + d->model->d_func()->rowsAboutToBeRemoved(this, row, row); QList 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::takeColumn(int column) Q_D(QStandardItem); if ((column < 0) || (column >= columnCount())) return QList(); + if (d->model) + d->model->d_func()->columnsAboutToBeRemoved(this, column, column); QList 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(Qt::AscendingOrder) << (QStringList() << "a" << "<" << "b" << "<" << "c" << ">" << ">") << (QStringList() << "a" << "<" << "b" << "<" << "c" << ">" << ">"); - + #if 1 QTest::newRow("hierarchical ascending") << static_cast(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 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 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 sourceIndexes; + QList 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 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 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 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 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 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" -- cgit v0.12