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 <ogoffart@trolltech.com>

2 files changed, 47 insertions, 22 deletions

diff --git a/src/corelib/kernel/qabstractitemmodel.cpp b/src/corelib/kernel/qabstractitemmodel.cpp
index 8e2273d102..10a61cacbf 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 bdc31af1c2..413419d2a1 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<int>("startRow");
     QTest::addColumn<int>("endRow");
     QTest::addColumn<int>("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<bool>("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<QPersistentModelIndex> 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<int>() << 5);
     moveCommand->setStartRow(startRow);
     moveCommand->setEndRow(endRow);
     moveCommand->setDestRow(destRow);
+    if (!topLevel)
+        moveCommand->setDestAncestors(QList<int>() << 5);
     moveCommand->doCommand();
 
     QVariantList beforeSignal = beforeSpy.takeAt(0);
     QVariantList afterSignal = afterSpy.takeAt(0);
 
     QCOMPARE(beforeSignal.size(), 5);
-    QCOMPARE(beforeSignal.at(0).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(beforeSignal.at(0).value<QModelIndex>(), moveParent);
     QCOMPARE(beforeSignal.at(1).toInt(), startRow);
     QCOMPARE(beforeSignal.at(2).toInt(), endRow);
-    QCOMPARE(beforeSignal.at(3).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(beforeSignal.at(3).value<QModelIndex>(), moveParent);
     QCOMPARE(beforeSignal.at(4).toInt(), destRow);
 
     QCOMPARE(afterSignal.size(), 5);
-    QCOMPARE(afterSignal.at(0).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(afterSignal.at(0).value<QModelIndex>(), moveParent);
     QCOMPARE(afterSignal.at(1).toInt(), startRow);
     QCOMPARE(afterSignal.at(2).toInt(), endRow);
-    QCOMPARE(afterSignal.at(3).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(afterSignal.at(3).value<QModelIndex>(), 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<int>("startRow");
     QTest::addColumn<int>("endRow");
     QTest::addColumn<int>("destRow");
+    QTest::addColumn<bool>("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<QPersistentModelIndex> 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<int>() << 5);
     moveCommand->setStartRow(startRow);
     moveCommand->setEndRow(endRow);
     moveCommand->setDestRow(destRow);
+    if (!topLevel)
+        moveCommand->setDestAncestors(QList<int>() << 5);
     moveCommand->doCommand();
 
     QVariantList beforeSignal = beforeSpy.takeAt(0);
     QVariantList afterSignal = afterSpy.takeAt(0);
 
     QCOMPARE(beforeSignal.size(), 5);
-    QCOMPARE(beforeSignal.at(0).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(beforeSignal.at(0).value<QModelIndex>(), moveParent);
     QCOMPARE(beforeSignal.at(1).toInt(), startRow);
     QCOMPARE(beforeSignal.at(2).toInt(), endRow);
-    QCOMPARE(beforeSignal.at(3).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(beforeSignal.at(3).value<QModelIndex>(), moveParent);
     QCOMPARE(beforeSignal.at(4).toInt(), destRow);
 
     QCOMPARE(afterSignal.size(), 5);
-    QCOMPARE(afterSignal.at(0).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(afterSignal.at(0).value<QModelIndex>(), moveParent);
     QCOMPARE(afterSignal.at(1).toInt(), startRow);
     QCOMPARE(afterSignal.at(2).toInt(), endRow);
-    QCOMPARE(afterSignal.at(3).value<QModelIndex>(), QModelIndex());
+    QCOMPARE(afterSignal.at(3).value<QModelIndex>(), 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)