TreeView: Track model indexes during selection

As we keep track of the previous user-selected row,
it can happen that that row is no longer part of the
TreeModelAdaptor items if the user collapses it or
any of its ancestors (collapsing a node doesn't update
the current selection). This will result on the row
index pointing to the wrong entry in the model (or a
completely invalid one).

Instead, we now track the selection with QModelIndexes
making it more robust to branch expansion and collapse
in the view. We'll still need to account for model changes
which means that, in the future, we should invalidate
the previous user-selected item.

Change-Id: I54ba2582e65515eef95d5f8ad755a8c68568d7ad
Task-number: QTBUG-46891
Reviewed-by: Caroline Chao <caroline.chao@theqtcompany.com>

6 files changed, 102 insertions, 44 deletions

diff --git a/src/controls/Private/qquicktreemodeladaptor.cpp b/src/controls/Private/qquicktreemodeladaptor.cpp
index c9e31712..dddcdd01 100644
--- a/src/controls/Private/qquicktreemodeladaptor.cpp
+++ b/src/controls/Private/qquicktreemodeladaptor.cpp
@@ -177,7 +177,7 @@ bool QQuickTreeModelAdaptor::setData(const QModelIndex &index, const QVariant &v
     }
 }
 
-int QQuickTreeModelAdaptor::itemIndex(const QModelIndex &index)
+int QQuickTreeModelAdaptor::itemIndex(const QModelIndex &index) const
 {
     // This is basically a plagiarism of QTreeViewPrivate::viewIndex()
     if (!index.isValid() || m_items.isEmpty())
@@ -244,11 +244,15 @@ QModelIndex QQuickTreeModelAdaptor::mapRowToModelIndex(int row) const
     return m_items.at(row).index;
 }
 
-QItemSelection  QQuickTreeModelAdaptor::selectionForRowRange(int from, int to) const
+QItemSelection  QQuickTreeModelAdaptor::selectionForRowRange(const QModelIndex &fromIndex, const QModelIndex &toIndex) const
 {
-    Q_ASSERT(0 <= from && from < m_items.count());
-    Q_ASSERT(0 <= to && to < m_items.count());
-
+    int from = itemIndex(fromIndex);
+    int to = itemIndex(toIndex);
+    if (from == -1) {
+        if (to == -1)
+            return QItemSelection();
+        return QItemSelection(toIndex, toIndex);
+    }
     if (from > to)
         qSwap(from, to);
 
diff --git a/src/controls/Private/qquicktreemodeladaptor_p.h b/src/controls/Private/qquicktreemodeladaptor_p.h
index 8059f681..2297c365 100644
--- a/src/controls/Private/qquicktreemodeladaptor_p.h
+++ b/src/controls/Private/qquicktreemodeladaptor_p.h
@@ -89,12 +89,12 @@ public:
     const QModelIndex &mapToModel(const QModelIndex &index) const;
     Q_INVOKABLE QModelIndex mapRowToModelIndex(int row) const;
 
-    Q_INVOKABLE QItemSelection selectionForRowRange(int from, int to) const;
+    Q_INVOKABLE QItemSelection selectionForRowRange(const QModelIndex &fromIndex, const QModelIndex &toIndex) const;
 
     void showModelTopLevelItems(bool doInsertRows = true);
     void showModelChildItems(const TreeItem &parent, int start, int end, bool doInsertRows = true, bool doExpandPendingRows = true);
 
-    int itemIndex(const QModelIndex &index);
+    int itemIndex(const QModelIndex &index) const;
     void expandPendingRows(bool doInsertRows = true);
     int lastChildIndex(const QModelIndex &index);
     void removeVisibleRows(int startIndex, int endIndex, bool doRemoveRows = true);
@@ -152,7 +152,7 @@ private:
     QList<TreeItem> m_items;
     QSet<QPersistentModelIndex> m_expandedItems;
     QList<TreeItem *> m_itemsToExpand;
-    int m_lastItemIndex;
+    mutable int m_lastItemIndex;
 };
 
 QT_END_NAMESPACE
diff --git a/src/controls/TreeView.qml b/src/controls/TreeView.qml
index 84c30edd..c97930f3 100644
--- a/src/controls/TreeView.qml
+++ b/src/controls/TreeView.qml
@@ -118,10 +118,11 @@ BasicTableView {
         // the flickable from eating our mouse press events
         preventStealing: !Settings.hasTouchScreen
 
-        property int clickedRow: -1
-        property int pressedRow: -1
+        property var clickedIndex: undefined
+        property var pressedIndex: undefined
         property int pressedColumn: -1
         readonly property alias currentRow: root.__currentRow
+        readonly property alias currentIndex: root.currentIndex
 
         // Handle vertical scrolling whem dragging mouse outside boundaries
         property int autoScroll: 0 // 0 -> do nothing; 1 -> increment; 2 -> decrement
@@ -132,7 +133,7 @@ BasicTableView {
             interval: 20
             repeat: true
             onTriggered: {
-                var oldPressedRow = mouseArea.pressedRow
+                var oldPressedIndex = mouseArea.pressedIndex
                 var row
                 if (mouseArea.autoScroll === 1) {
                     __listView.incrementCurrentIndexBlocking();
@@ -144,28 +145,33 @@ BasicTableView {
                     row = __listView.indexAt(0, __listView.contentY)
                 }
 
-                if (row !== oldPressedRow) {
-                    mouseArea.pressedRow = row
+                var index = modelAdaptor.mapRowToModelIndex(row)
+                if (index !== oldPressedIndex) {
+                    mouseArea.pressedIndex = index
                     var modifiers = mouseArea.shiftPressed ? Qt.ShiftModifier : Qt.NoModifier
-                    mouseArea.mouseSelect(row, modifiers, true /* drag */)
+                    mouseArea.mouseSelect(index, modifiers, true /* drag */)
                 }
             }
         }
 
-        function mouseSelect(row, modifiers, drag) {
+        function mouseSelect(modelIndex, modifiers, drag) {
             if (!selection) {
                 maybeWarnAboutSelectionMode()
                 return
             }
 
             if (selectionMode) {
-                var modelIndex = modelAdaptor.mapRowToModelIndex(row)
                 selection.setCurrentIndex(modelIndex, ItemSelectionModel.NoUpdate)
                 if (selectionMode === SelectionMode.SingleSelection) {
                     selection.select(modelIndex, ItemSelectionModel.ClearAndSelect)
                 } else {
-                    var itemSelection = clickedRow === row ? modelIndex
-                                        : modelAdaptor.selectionForRowRange(clickedRow, row)
+                    var selectRowRange = (drag && (selectionMode === SelectionMode.MultiSelection
+                                                   || (selectionMode === SelectionMode.ExtendedSelection
+                                                       && modifiers & Qt.ControlModifier)))
+                                         || modifiers & Qt.ShiftModifier
+                    var itemSelection = !selectRowRange || clickedIndex === modelIndex ? modelIndex
+                                        : modelAdaptor.selectionForRowRange(clickedIndex, modelIndex)
+
                     if (selectionMode === SelectionMode.MultiSelection
                         || selectionMode === SelectionMode.ExtendedSelection && modifiers & Qt.ControlModifier) {
                         if (drag)
@@ -175,7 +181,7 @@ BasicTableView {
                     } else if (modifiers & Qt.ShiftModifier) {
                         selection.select(itemSelection, ItemSelectionModel.SelectCurrent)
                     } else {
-                        clickedRow = row // Needed only when drag is true
+                        clickedIndex = modelIndex // Needed only when drag is true
                         selection.select(modelIndex, ItemSelectionModel.ClearAndSelect)
                     }
                 }
@@ -185,9 +191,9 @@ BasicTableView {
         function keySelect(keyModifiers) {
             if (selectionMode) {
                 if (!keyModifiers)
-                    clickedRow = currentRow
+                    clickedIndex = currentIndex
                 if (!(keyModifiers & Qt.ControlModifier))
-                    mouseSelect(currentRow, keyModifiers, keyModifiers & Qt.ShiftModifier)
+                    mouseSelect(currentIndex, keyModifiers, keyModifiers & Qt.ShiftModifier)
             }
         }
 
@@ -227,22 +233,23 @@ BasicTableView {
         }
 
         onPressed: {
-            pressedRow = __listView.indexAt(0, mouseY + __listView.contentY)
+            var pressedRow = __listView.indexAt(0, mouseY + __listView.contentY)
+            pressedIndex = modelAdaptor.mapRowToModelIndex(pressedRow)
             pressedColumn = __listView.columnAt(mouseX)
             __listView.forceActiveFocus()
             if (pressedRow > -1 && !Settings.hasTouchScreen
                 && !branchDecorationContains(mouse.x, mouse.y)) {
                 __listView.currentIndex = pressedRow
-                if (clickedRow === -1)
-                    clickedRow = pressedRow
-                mouseSelect(pressedRow, mouse.modifiers, false)
+                if (!clickedIndex)
+                    clickedIndex = pressedIndex
+                mouseSelect(pressedIndex, mouse.modifiers, false)
                 if (!mouse.modifiers)
-                    clickedRow = pressedRow
+                    clickedIndex = pressedIndex
             }
         }
 
         onReleased: {
-            pressedRow = -1
+            pressedIndex = undefined
             pressedColumn = -1
             autoScroll = 0
         }
@@ -261,23 +268,24 @@ BasicTableView {
             }
 
             if (pressed && containsMouse) {
-                var oldPressedRow = pressedRow
-                pressedRow = __listView.indexAt(0, mouseY + __listView.contentY)
+                var oldPressedIndex = pressedIndex
+                var pressedRow = __listView.indexAt(0, mouseY + __listView.contentY)
+                pressedIndex = modelAdaptor.mapRowToModelIndex(pressedRow)
                 pressedColumn = __listView.columnAt(mouseX)
-                if (pressedRow > -1 && oldPressedRow !== pressedRow) {
+                if (pressedRow > -1 && oldPressedIndex !== pressedIndex) {
                     __listView.currentIndex = pressedRow
-                    mouseSelect(pressedRow, mouse.modifiers, true /* drag */)
+                    mouseSelect(pressedIndex, mouse.modifiers, true /* drag */)
                 }
             }
         }
 
         onExited: {
-            pressedRow = -1
+            pressedIndex = undefined
             pressedColumn = -1
         }
 
         onCanceled: {
-            pressedRow = -1
+            pressedIndex = undefined
             pressedColumn = -1
             autoScroll = 0
         }
diff --git a/tests/auto/controls/data/tst_treeview.qml b/tests/auto/controls/data/tst_treeview.qml
index 9db58f14..6e17e318 100644
--- a/tests/auto/controls/data/tst_treeview.qml
+++ b/tests/auto/controls/data/tst_treeview.qml
@@ -785,5 +785,34 @@ Item {
             compare(treeIndex.column, modelIndex.column)
             compare(treeIndex.internalId, modelIndex.internalId)
         }
+
+        function test_QTBUG_46891_selection_collapse_parent()
+        {
+            var component = Qt.createComponent("treeview/treeview_1.qml")
+            compare(component.status, Component.Ready)
+            var tree = component.createObject(container);
+            verify(tree !== null, "tree created is null")
+            var model = tree.model
+            model.removeRows(1, 9)
+            model.removeRows(1, 9, model.index(0, 0))
+            waitForRendering(tree)
+
+            var selectionModel = Qt.createQmlObject(testCase.instance_selectionModel, container, '')
+            selectionModel.model = tree.model
+            tree.selection = selectionModel
+            tree.selectionMode = SelectionMode.ExtendedSelection
+
+            var parentItem = tree.model.index(0, 0)
+            tree.expand(parentItem)
+            verify(tree.isExpanded(parentItem))
+
+            wait(100)
+            mouseClick(tree, semiIndent + 50, 20 + 100, Qt.LeftButton)
+            verify(selectionModel.isSelected(tree.currentIndex))
+
+            tree.collapse(parentItem)
+            mouseClick(tree, semiIndent + 50, 20 + 50, Qt.LeftButton)
+            verify(selectionModel.isSelected(parentItem))
+        }
     }
 }
diff --git a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
index 503b9e6f..b484665d 100644
--- a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
+++ b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
@@ -1138,7 +1138,8 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
 
     for (int i = 0; i < ModelRowCount; i += ModelRowCountLoopStep) {
         // Single row selection
-        const QItemSelection &sel = tma.selectionForRowRange(i, i);
+        const QModelIndex &idx = model.index(i, 0);
+        const QItemSelection &sel = tma.selectionForRowRange(idx, idx);
         QCOMPARE(sel.count(), 1);
         const QItemSelectionRange &range = sel.first();
         QCOMPARE(QModelIndex(range.topLeft()), model.index(i, 0));
@@ -1147,7 +1148,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
 
     for (int i = 0; i < ModelRowCount - ModelRowCountLoopStep; i += ModelRowCountLoopStep) {
         // Single range selection
-        const QItemSelection &sel = tma.selectionForRowRange(i, i + ModelRowCountLoopStep);
+        const QModelIndex &from = model.index(i, 0);
+        const QModelIndex &to = model.index(i + ModelRowCountLoopStep, 0);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 1);
         const QItemSelectionRange &range = sel.first();
         QCOMPARE(QModelIndex(range.topLeft()), model.index(i, 0));
@@ -1155,7 +1158,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     }
 
     {   // Select all, no branch expanded
-        const QItemSelection &sel = tma.selectionForRowRange(0, ModelRowCount - 1);
+        const QModelIndex &from = model.index(0, 0);
+        const QModelIndex &to = model.index(ModelRowCount - 1, 0);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 1);
         const QItemSelectionRange &range = sel.first();
         QCOMPARE(QModelIndex(range.topLeft()), model.index(0, 0));
@@ -1167,7 +1172,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     tma.expand(parent);
 
     {   // 1st item expanded, select first 5 rows
-        const QItemSelection &sel = tma.selectionForRowRange(0, 4);
+        const QModelIndex &from = tma.mapRowToModelIndex(0);
+        const QModelIndex &to = tma.mapRowToModelIndex(4);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 2);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
@@ -1182,7 +1189,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     }
 
     {   // 1st item expanded, select first 5 top-level items
-        const QItemSelection &sel = tma.selectionForRowRange(0, 4 + ModelRowCount);
+        const QModelIndex &from = tma.mapRowToModelIndex(0);
+        const QModelIndex &to = tma.mapRowToModelIndex(4 + ModelRowCount);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 2);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
@@ -1201,7 +1210,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     tma.expand(parent2);
 
     {   // 1st two items expanded, select first 5 top-level items
-        const QItemSelection &sel = tma.selectionForRowRange(0, 4 + 2 * ModelRowCount);
+        const QModelIndex &from = tma.mapRowToModelIndex(0);
+        const QModelIndex &to = tma.mapRowToModelIndex(4 + 2 * ModelRowCount);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 3);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
@@ -1222,7 +1233,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     tma.expand(parent3);
 
     {   // 1st two items, and 1st child of 1st item expanded, select first 5 rows
-        const QItemSelection &sel = tma.selectionForRowRange(0, 4);
+        const QModelIndex &from = tma.mapRowToModelIndex(0);
+        const QModelIndex &to = tma.mapRowToModelIndex(4);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 3);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
@@ -1239,7 +1252,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     }
 
     {   // 1st two items, and 1st child of 1st item expanded, select all
-        const QItemSelection &sel = tma.selectionForRowRange(0, 4 * ModelRowCount - 1);
+        const QModelIndex &from = tma.mapRowToModelIndex(0);
+        const QModelIndex &to = tma.mapRowToModelIndex(4 * ModelRowCount - 1);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 4);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
@@ -1258,7 +1273,9 @@ void tst_QQuickTreeModelAdaptor::selectionForRowRange()
     }
 
     {   // 1st two items, and 1st child of 1st item expanded, select rows across branches
-        const QItemSelection &sel = tma.selectionForRowRange(8, 23);
+        const QModelIndex &from = tma.mapRowToModelIndex(8);
+        const QModelIndex &to = tma.mapRowToModelIndex(23);
+        const QItemSelection &sel = tma.selectionForRowRange(from, to);
         QCOMPARE(sel.count(), 4);
         // We don't know in which order the selection ranges are
         // being added, so we iterate and try to find what we expect.
diff --git a/tests/auto/shared/testmodel.h b/tests/auto/shared/testmodel.h
index 0bc06757..00e74129 100644
--- a/tests/auto/shared/testmodel.h
+++ b/tests/auto/shared/testmodel.h
@@ -124,7 +124,7 @@ public:
         return false;
     }
 
-    Q_INVOKABLE QModelIndex index(int row, int column, const QModelIndex &parent = QModelIndex()) const
+    QModelIndex index(int row, int column, const QModelIndex &parent = QModelIndex()) const
     {
         if (row < 0 || column < 0 || (level(parent) > levels) || column >= cols)
             return QModelIndex();
@@ -199,7 +199,7 @@ public:
         emit layoutChanged(parents);
     }
 
-    bool removeRows(int row, int count, const QModelIndex &parent)
+    Q_INVOKABLE bool removeRows(int row, int count, const QModelIndex &parent = QModelIndex())
     {
         beginRemoveRows(parent, row, row + count - 1);
         Node *n = (Node *)parent.internalPointer();