From e6be9c88bc98481936fcba7fa1cfb4e255f6e30b Mon Sep 17 00:00:00 2001 From: Stephen Kelly Date: Fri, 13 Nov 2009 10:58:15 +0100 Subject: Early return for allowMove within a parent QModelIndex If this is not done, the climbing ancestors later in the method uses srcParent.row() as pos causing failure depending on which rows are being moved, and what the row of the parent is. Merge-request: 2072 Reviewed-by: Olivier Goffart --- src/corelib/kernel/qabstractitemmodel.cpp | 6 +-- .../qabstractitemmodel/tst_qabstractitemmodel.cpp | 63 +++++++++++++++------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/src/corelib/kernel/qabstractitemmodel.cpp b/src/corelib/kernel/qabstractitemmodel.cpp index 8e2273d..10a61ca 100644 --- a/src/corelib/kernel/qabstractitemmodel.cpp +++ b/src/corelib/kernel/qabstractitemmodel.cpp @@ -2475,10 +2475,8 @@ void QAbstractItemModel::endRemoveRows() bool QAbstractItemModelPrivate::allowMove(const QModelIndex &srcParent, int start, int end, const QModelIndex &destinationParent, int destinationStart, Qt::Orientation orientation) { // Don't move the range within itself. - if ( ( destinationParent == srcParent ) - && ( destinationStart >= start ) - && ( destinationStart <= end + 1) ) - return false; + if (destinationParent == srcParent) + return !(destinationStart >= start && destinationStart <= end + 1); QModelIndex destinationAncestor = destinationParent; int pos = (Qt::Vertical == orientation) ? destinationAncestor.row() : destinationAncestor.column(); diff --git a/tests/auto/qabstractitemmodel/tst_qabstractitemmodel.cpp b/tests/auto/qabstractitemmodel/tst_qabstractitemmodel.cpp index bdc31af..413419d 100644 --- a/tests/auto/qabstractitemmodel/tst_qabstractitemmodel.cpp +++ b/tests/auto/qabstractitemmodel/tst_qabstractitemmodel.cpp @@ -865,15 +865,22 @@ void tst_QAbstractItemModel::testMoveSameParentDown_data() QTest::addColumn("startRow"); QTest::addColumn("endRow"); QTest::addColumn("destRow"); + // We can't put the actual parent index for the move in here because m_model is not defined until init() is run. + QTest::addColumn("topLevel"); // Move from the start to the middle - QTest::newRow("move01") << 0 << 2 << 8; + QTest::newRow("move01") << 0 << 2 << 8 << true; // Move from the start to the end - QTest::newRow("move02") << 0 << 2 << 10; + QTest::newRow("move02") << 0 << 2 << 10 << true; // Move from the middle to the middle - QTest::newRow("move03") << 3 << 5 << 8; + QTest::newRow("move03") << 3 << 5 << 8 << true; // Move from the middle to the end - QTest::newRow("move04") << 3 << 5 << 10; + QTest::newRow("move04") << 3 << 5 << 10 << true; + + QTest::newRow("move05") << 0 << 2 << 8 << false; + QTest::newRow("move06") << 0 << 2 << 10 << false; + QTest::newRow("move07") << 3 << 5 << 8 << false; + QTest::newRow("move08") << 3 << 5 << 10 << false; } void tst_QAbstractItemModel::testMoveSameParentDown() @@ -881,6 +888,9 @@ void tst_QAbstractItemModel::testMoveSameParentDown() QFETCH( int, startRow); QFETCH( int, endRow); QFETCH( int, destRow); + QFETCH( bool, topLevel); + + QModelIndex moveParent = topLevel ? QModelIndex() : m_model->index(5, 0); QList persistentList; QModelIndexList indexList; @@ -913,33 +923,37 @@ void tst_QAbstractItemModel::testMoveSameParentDown() ModelMoveCommand *moveCommand = new ModelMoveCommand(m_model, this); moveCommand->setNumCols(4); + if (!topLevel) + moveCommand->setAncestorRowNumbers(QList() << 5); moveCommand->setStartRow(startRow); moveCommand->setEndRow(endRow); moveCommand->setDestRow(destRow); + if (!topLevel) + moveCommand->setDestAncestors(QList() << 5); moveCommand->doCommand(); QVariantList beforeSignal = beforeSpy.takeAt(0); QVariantList afterSignal = afterSpy.takeAt(0); QCOMPARE(beforeSignal.size(), 5); - QCOMPARE(beforeSignal.at(0).value(), QModelIndex()); + QCOMPARE(beforeSignal.at(0).value(), moveParent); QCOMPARE(beforeSignal.at(1).toInt(), startRow); QCOMPARE(beforeSignal.at(2).toInt(), endRow); - QCOMPARE(beforeSignal.at(3).value(), QModelIndex()); + QCOMPARE(beforeSignal.at(3).value(), moveParent); QCOMPARE(beforeSignal.at(4).toInt(), destRow); QCOMPARE(afterSignal.size(), 5); - QCOMPARE(afterSignal.at(0).value(), QModelIndex()); + QCOMPARE(afterSignal.at(0).value(), moveParent); QCOMPARE(afterSignal.at(1).toInt(), startRow); QCOMPARE(afterSignal.at(2).toInt(), endRow); - QCOMPARE(afterSignal.at(3).value(), QModelIndex()); + QCOMPARE(afterSignal.at(3).value(), moveParent); QCOMPARE(afterSignal.at(4).toInt(), destRow); for (int i = 0; i < indexList.size(); i++) { QModelIndex idx = indexList.at(i); QModelIndex persistentIndex = persistentList.at(i); - if (idx.parent() == QModelIndex()) + if (idx.parent() == moveParent) { int row = idx.row(); if ( row >= startRow) @@ -976,15 +990,21 @@ void tst_QAbstractItemModel::testMoveSameParentUp_data() QTest::addColumn("startRow"); QTest::addColumn("endRow"); QTest::addColumn("destRow"); + QTest::addColumn("topLevel"); // Move from the middle to the start - QTest::newRow("move01") << 5 << 7 << 0; + QTest::newRow("move01") << 5 << 7 << 0 << true; // Move from the end to the start - QTest::newRow("move02") << 8 << 9 << 0; + QTest::newRow("move02") << 8 << 9 << 0 << true; // Move from the middle to the middle - QTest::newRow("move03") << 5 << 7 << 2; + QTest::newRow("move03") << 5 << 7 << 2 << true; // Move from the end to the middle - QTest::newRow("move04") << 8 << 9 << 5; + QTest::newRow("move04") << 8 << 9 << 5 << true; + + QTest::newRow("move05") << 5 << 7 << 0 << false; + QTest::newRow("move06") << 8 << 9 << 0 << false; + QTest::newRow("move07") << 5 << 7 << 2 << false; + QTest::newRow("move08") << 8 << 9 << 5 << false; } void tst_QAbstractItemModel::testMoveSameParentUp() @@ -993,6 +1013,9 @@ void tst_QAbstractItemModel::testMoveSameParentUp() QFETCH( int, startRow); QFETCH( int, endRow); QFETCH( int, destRow); + QFETCH( bool, topLevel); + + QModelIndex moveParent = topLevel ? QModelIndex() : m_model->index(5, 0); QList persistentList; QModelIndexList indexList; @@ -1026,26 +1049,30 @@ void tst_QAbstractItemModel::testMoveSameParentUp() ModelMoveCommand *moveCommand = new ModelMoveCommand(m_model, this); moveCommand->setNumCols(4); + if (!topLevel) + moveCommand->setAncestorRowNumbers(QList() << 5); moveCommand->setStartRow(startRow); moveCommand->setEndRow(endRow); moveCommand->setDestRow(destRow); + if (!topLevel) + moveCommand->setDestAncestors(QList() << 5); moveCommand->doCommand(); QVariantList beforeSignal = beforeSpy.takeAt(0); QVariantList afterSignal = afterSpy.takeAt(0); QCOMPARE(beforeSignal.size(), 5); - QCOMPARE(beforeSignal.at(0).value(), QModelIndex()); + QCOMPARE(beforeSignal.at(0).value(), moveParent); QCOMPARE(beforeSignal.at(1).toInt(), startRow); QCOMPARE(beforeSignal.at(2).toInt(), endRow); - QCOMPARE(beforeSignal.at(3).value(), QModelIndex()); + QCOMPARE(beforeSignal.at(3).value(), moveParent); QCOMPARE(beforeSignal.at(4).toInt(), destRow); QCOMPARE(afterSignal.size(), 5); - QCOMPARE(afterSignal.at(0).value(), QModelIndex()); + QCOMPARE(afterSignal.at(0).value(), moveParent); QCOMPARE(afterSignal.at(1).toInt(), startRow); QCOMPARE(afterSignal.at(2).toInt(), endRow); - QCOMPARE(afterSignal.at(3).value(), QModelIndex()); + QCOMPARE(afterSignal.at(3).value(), moveParent); QCOMPARE(afterSignal.at(4).toInt(), destRow); @@ -1053,7 +1080,7 @@ void tst_QAbstractItemModel::testMoveSameParentUp() { QModelIndex idx = indexList.at(i); QModelIndex persistentIndex = persistentList.at(i); - if (idx.parent() == QModelIndex()) + if (idx.parent() == moveParent) { int row = idx.row(); if ( row >= destRow) -- cgit v0.12