Fix moving of TreeView items

Property binding for row property in styleData causes an update which
tries to read new value for the index property, but index is changed
afterwards which causes old value to be read. This may lead to crashes
and other unwanted behavior.

Depth changes are now delivered to update item depths in visible
items and model index changes though role instead of looking for a row
change.

Task-number: QTBUG-47523
Change-Id: I540cd06a25281f18e4628f4b030cf969dc8e0a7f
Reviewed-by: Alex Blasche <alexander.blasche@qt.io>

5 files changed, 103 insertions, 8 deletions

diff --git a/src/controls/Private/TreeViewItemDelegateLoader.qml b/src/controls/Private/TreeViewItemDelegateLoader.qml
index 8b8801fc..01492eda 100644
--- a/src/controls/Private/TreeViewItemDelegateLoader.qml
+++ b/src/controls/Private/TreeViewItemDelegateLoader.qml
@@ -78,7 +78,7 @@ TableViewItemDelegateLoader {
         readonly property color textColor: __rowItem ? __rowItem.itemTextColor : "black"
         readonly property string role: __column ? __column.role : ""
         readonly property var value: model && model.hasOwnProperty(role) ? model[role] : ""
-        readonly property var index: __treeModel.mapRowToModelIndex(row)
+        readonly property var index: model ? model["_q_TreeView_ModelIndex"] : __treeModel.index(-1,-1,null)
         readonly property int depth: model && column === 0 ? model["_q_TreeView_ItemDepth"] : 0
         readonly property bool hasChildren: model ? model["_q_TreeView_HasChildren"] : false
         readonly property bool hasSibling: model ? model["_q_TreeView_HasSibling"] : false
diff --git a/src/controls/Private/qquicktreemodeladaptor.cpp b/src/controls/Private/qquicktreemodeladaptor.cpp
index 666fafc9..52b231d0 100644
--- a/src/controls/Private/qquicktreemodeladaptor.cpp
+++ b/src/controls/Private/qquicktreemodeladaptor.cpp
@@ -153,6 +153,7 @@ QHash<int, QByteArray> QQuickTreeModelAdaptor::roleNames() const
     modelRoleNames.insert(ExpandedRole, "_q_TreeView_ItemExpanded");
     modelRoleNames.insert(HasChildrenRole, "_q_TreeView_HasChildren");
     modelRoleNames.insert(HasSiblingRole, "_q_TreeView_HasSibling");
+    modelRoleNames.insert(ModelIndexRole, "_q_TreeView_ModelIndex");
     return modelRoleNames;
 }
 
@@ -177,6 +178,8 @@ QVariant QQuickTreeModelAdaptor::data(const QModelIndex &index, int role) const
         return !(modelIndex.flags() & Qt::ItemNeverHasChildren) && m_model->hasChildren(modelIndex);
     case HasSiblingRole:
         return modelIndex.row() != m_model->rowCount(modelIndex.parent()) - 1;
+    case ModelIndexRole:
+        return modelIndex;
     default:
         return m_model->data(modelIndex, role);
     }
@@ -192,6 +195,7 @@ bool QQuickTreeModelAdaptor::setData(const QModelIndex &index, const QVariant &v
     case ExpandedRole:
     case HasChildrenRole:
     case HasSiblingRole:
+    case ModelIndexRole:
         return false;
     default: {
         const QModelIndex &pmi = mapToModel(index);
@@ -716,8 +720,11 @@ void QQuickTreeModelAdaptor::modelRowsAboutToBeMoved(const QModelIndex & sourceP
             destIndex = itemIndex(m_model->index(destinationRow, 0, destinationParent));
         }
 
-        beginMoveRows(QModelIndex(), startIndex, endIndex, QModelIndex(), destIndex);
         int totalMovedCount = endIndex - startIndex + 1;
+
+        const bool visibleRowsMoved = startIndex != destIndex &&
+            beginMoveRows(QModelIndex(), startIndex, endIndex, QModelIndex(), destIndex);
+
         const QList<TreeItem> &buffer = m_items.mid(startIndex, totalMovedCount);
         int bufferCopyOffset;
         if (destIndex > endIndex) {
@@ -726,6 +733,7 @@ void QQuickTreeModelAdaptor::modelRowsAboutToBeMoved(const QModelIndex & sourceP
             }
             bufferCopyOffset = destIndex - totalMovedCount;
         } else {
+            // NOTE: we will not enter this loop if startIndex == destIndex
             for (int i = startIndex - 1; i >= destIndex; i--) {
                 m_items.swap(i, i + totalMovedCount); // Fast move from 1st to 2nd position
             }
@@ -736,14 +744,49 @@ void QQuickTreeModelAdaptor::modelRowsAboutToBeMoved(const QModelIndex & sourceP
             item.depth += depthDifference;
             m_items.replace(bufferCopyOffset + i, item);
         }
-        endMoveRows();
+
+        if (visibleRowsMoved)
+            endMoveRows();
+
+        if (depthDifference != 0) {
+            const QModelIndex &topLeft = index(bufferCopyOffset, 0, QModelIndex());
+            const QModelIndex &bottomRight = index(bufferCopyOffset + totalMovedCount - 1, 0, QModelIndex());
+            const QVector<int> changedRole(1, DepthRole);
+            emit dataChanged(topLeft, bottomRight, changedRole);
+        }
     }
 }
 
 void QQuickTreeModelAdaptor::modelRowsMoved(const QModelIndex & sourceParent, int sourceStart, int sourceEnd, const QModelIndex & destinationParent, int destinationRow)
 {
-    if (!childrenVisible(sourceParent) && childrenVisible(destinationParent))
-        modelRowsInserted(destinationParent, destinationRow, destinationRow + sourceEnd - sourceStart);
+    if (childrenVisible(destinationParent)) {
+        if (!childrenVisible(sourceParent))
+            modelRowsInserted(destinationParent, destinationRow, destinationRow + sourceEnd - sourceStart);
+        else {
+            int destIndex = -1;
+            if (destinationRow == m_model->rowCount(destinationParent)) {
+                const QModelIndex &emi = m_model->index(destinationRow - 1, 0, destinationParent);
+                destIndex = lastChildIndex(emi) + 1;
+            } else {
+                destIndex = itemIndex(m_model->index(destinationRow, 0, destinationParent));
+            }
+
+            const QModelIndex &emi = m_model->index(destinationRow + sourceEnd - sourceStart, 0, destinationParent);
+            int endIndex = -1;
+            if (isExpanded(emi)) {
+                int rowCount = m_model->rowCount(emi);
+                if (rowCount > 0)
+                    endIndex = lastChildIndex(m_model->index(rowCount - 1, 0, emi));
+            }
+            if (endIndex == -1)
+                endIndex = itemIndex(emi);
+
+            const QModelIndex &topLeft = index(destIndex, 0, QModelIndex());
+            const QModelIndex &bottomRight = index(endIndex, 0, QModelIndex());
+            const QVector<int> changedRole(1, ModelIndexRole);
+            emit dataChanged(topLeft, bottomRight, changedRole);
+        }
+    }
     ASSERT_CONSISTENCY();
 }
 
diff --git a/src/controls/Private/qquicktreemodeladaptor_p.h b/src/controls/Private/qquicktreemodeladaptor_p.h
index 3eefbe77..56c6ea08 100644
--- a/src/controls/Private/qquicktreemodeladaptor_p.h
+++ b/src/controls/Private/qquicktreemodeladaptor_p.h
@@ -74,10 +74,11 @@ public:
     void resetRootIndex();
 
     enum {
-        DepthRole = Qt::UserRole - 4,
+        DepthRole = Qt::UserRole - 5,
         ExpandedRole,
         HasChildrenRole,
-        HasSiblingRole
+        HasSiblingRole,
+        ModelIndexRole
     };
 
     QHash<int, QByteArray> roleNames() const;
diff --git a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
index 13a92ea7..34c8f7cc 100644
--- a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
+++ b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
@@ -81,6 +81,7 @@ private slots:
 
     void moveRows_data();
     void moveRows();
+    void reparentOnSameRow();
 
     void selectionForRowRange();
 
@@ -1160,6 +1161,56 @@ void tst_QQuickTreeModelAdaptor::moveRows()
     compareModels(tma, model);
 }
 
+void tst_QQuickTreeModelAdaptor::reparentOnSameRow()
+{
+    TestModel model(2, 1);
+    model.alternateChildlessRows = false;
+    QQuickTreeModelAdaptor tma;
+    tma.setModel(&model);
+
+    const QModelIndex &destParent = model.index(0, 0);
+    const QModelIndex &sourceParent = QModelIndex();
+    QVERIFY(destParent.isValid());
+    tma.expand(destParent);
+    QVERIFY(tma.isExpanded(destParent));
+
+    QSignalSpy dataChangedSpy(&tma, SIGNAL(dataChanged(QModelIndex,QModelIndex,QVector<int>)));
+    QSignalSpy rowsMovedSpy(&tma, SIGNAL(rowsMoved(QModelIndex,int,int,QModelIndex,int)));
+    QVERIFY(rowsMovedSpy.isValid());
+    QVERIFY(dataChangedSpy.isValid());
+
+    QVERIFY(model.moveRows(sourceParent, 1, 1, destParent, 2));
+
+    QModelIndex movedIndex = tma.index(3, 0, QModelIndex());
+    QVERIFY(movedIndex.isValid());
+    QCOMPARE(movedIndex.data(QQuickTreeModelAdaptor::DepthRole).toInt(), 1);
+    QCOMPARE(tma.data(movedIndex, QQuickTreeModelAdaptor::ModelIndexRole).toModelIndex(), model.index(2, 0, destParent));
+
+    // at least DepthRole and ModeIndexRole changes should have happened for the affected row
+    bool depthChanged = false;
+    bool modelIndexChanged = false;
+    QList<QList<QVariant> > &changes = dataChangedSpy;
+    foreach (QList<QVariant> change, changes) {
+        if (change.at(0) == movedIndex) {
+            if (change.at(2).value<QVector<int> >().contains(QQuickTreeModelAdaptor::DepthRole))
+                depthChanged = true;
+            if (change.at(2).value<QVector<int> >().contains(QQuickTreeModelAdaptor::ModelIndexRole))
+                modelIndexChanged = true;
+        }
+    }
+
+    QCOMPARE(depthChanged, true);
+    QCOMPARE(modelIndexChanged, true);
+
+    QCOMPARE(rowsMovedSpy.count(), 0);
+
+    model.moveRow(destParent, 2, QModelIndex(), 1);
+
+    QCOMPARE(rowsMovedSpy.count(), 0);
+    QVERIFY(tma.testConsistency());
+    compareModels(tma, model);
+}
+
 void tst_QQuickTreeModelAdaptor::selectionForRowRange()
 {
     const int ModelRowCount = 9;
diff --git a/tests/auto/shared/testmodel.h b/tests/auto/shared/testmodel.h
index 00e74129..d1f49a4d 100644
--- a/tests/auto/shared/testmodel.h
+++ b/tests/auto/shared/testmodel.h
@@ -238,7 +238,7 @@ public:
     bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild)
     {
         Q_ASSERT_X(sourceRow >= 0 && sourceRow < rowCount(sourceParent)
-                   && count > 0 && sourceRow + count < rowCount(sourceParent)
+                   && count > 0 && sourceRow + count - 1 < rowCount(sourceParent)
                    && destinationChild >= 0 && destinationChild <= rowCount(destinationParent),
                    Q_FUNC_INFO, "Rows out of range.");
         Q_ASSERT_X(!(sourceParent == destinationParent && destinationChild >= sourceRow && destinationChild < sourceRow + count),