TreeModelAdaptor: Take into account childless expanded items

When removing or moving items, we may call lastChildIndex()
with an invalid index because we're not checking for childless
items which can be expanded (think about expanding an empty
directory). Same thing when relayouting an item.

The solution requires testing for the actual children count
since hasChildren() count return true even when the item has
no children.

Change-Id: I0fa6de41e20c6cb2433f8846449e9f1a2989ce91
Reviewed-by: J-P Nurmi <jpnurmi@theqtcompany.com>

2 files changed, 152 insertions, 15 deletions

diff --git a/src/controls/Private/qquicktreemodeladaptor.cpp b/src/controls/Private/qquicktreemodeladaptor.cpp
index dacae676..f4f47a04 100644
--- a/src/controls/Private/qquicktreemodeladaptor.cpp
+++ b/src/controls/Private/qquicktreemodeladaptor.cpp
@@ -473,7 +473,7 @@ void QQuickTreeModelAdaptor::collapseRow(int n)
     }
 
     qDebug() << "collapsing" << n << childrenCount;
-    const QModelIndex &emi = m_model->index(m_model->rowCount(item.index) - 1, 0, item.index);
+    const QModelIndex &emi = m_model->index(childrenCount - 1, 0, item.index);
     int lastIndex = lastChildIndex(emi);
     removeVisibleRows(n + 1, lastIndex);
 }
@@ -503,10 +503,10 @@ int QQuickTreeModelAdaptor::lastChildIndex(const QModelIndex &index)
 
 void QQuickTreeModelAdaptor::removeVisibleRows(int startIndex, int endIndex, bool doRemoveRows)
 {
+    qDebug() << "removing" << startIndex << endIndex;
     if (startIndex < 0 || endIndex < 0 || startIndex > endIndex)
         return;
 
-    qDebug() << "removing" << startIndex << endIndex;
     if (doRemoveRows)
         beginRemoveRows(QModelIndex(), startIndex, endIndex);
     m_items.erase(m_items.begin() + startIndex, m_items.begin() + endIndex + 1);
@@ -582,14 +582,17 @@ void QQuickTreeModelAdaptor::modelLayoutChanged(const QList<QPersistentModelInde
     }
 
     Q_FOREACH (const QPersistentModelIndex &pmi, parents) {
-        if (m_expandedItems.contains(pmi) && m_model->hasChildren(pmi)) {
+        if (m_expandedItems.contains(pmi)) {
             int row = itemIndex(pmi);
             if (row != -1) {
-                const QModelIndex &lmi = m_model->index(m_model->rowCount(pmi) - 1, 0, pmi);
-                int lastRow = lastChildIndex(lmi);
-                removeVisibleRows(row + 1, lastRow, false /*doRemoveRows*/);
-                showModelChildItems(m_items.at(row), 0, m_model->rowCount(pmi) - 1, false /*doInsertRows*/);
-                emit dataChanged(index(row + 1), index(lastRow));
+                int rowCount = m_model->rowCount(pmi);
+                if (rowCount > 0) {
+                    const QModelIndex &lmi = m_model->index(rowCount - 1, 0, pmi);
+                    int lastRow = lastChildIndex(lmi);
+                    removeVisibleRows(row + 1, lastRow, false /*doRemoveRows*/);
+                    showModelChildItems(m_items.at(row), 0, rowCount - 1, false /*doInsertRows*/);
+                    emit dataChanged(index(row + 1), index(lastRow));
+                }
             }
         }
     }
@@ -631,11 +634,17 @@ void QQuickTreeModelAdaptor::modelRowsAboutToBeRemoved(const QModelIndex & paren
         const QModelIndex &smi = m_model->index(start, 0, parent);
         int startIndex = itemIndex(smi);
         const QModelIndex &emi = m_model->index(end, 0, parent);
-        int endIndex = itemIndex(emi);
+        int endIndex = -1;
         if (isExpanded(emi)) {
-            const QModelIndex &idx = m_model->index(m_model->rowCount(emi) - 1, 0, emi);
-            endIndex = lastChildIndex(idx);
+            int rowCount = m_model->rowCount(emi);
+            if (rowCount > 0) {
+                const QModelIndex &idx = m_model->index(rowCount - 1, 0, emi);
+                endIndex = lastChildIndex(idx);
+            }
         }
+        if (endIndex == -1)
+            endIndex = itemIndex(emi);
+
         removeVisibleRows(startIndex, endIndex);
     }
 
@@ -683,10 +692,13 @@ void QQuickTreeModelAdaptor::modelRowsAboutToBeMoved(const QModelIndex & sourceP
 
         int startIndex = itemIndex(m_model->index(sourceStart, 0, sourceParent));
         const QModelIndex &emi = m_model->index(sourceEnd, 0, sourceParent);
-        int endIndex;
-        if (isExpanded(emi))
-            endIndex = lastChildIndex(m_model->index(m_model->rowCount(emi) - 1, 0, emi));
-        else
+        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);
 
         int destIndex = -1;
diff --git a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
index 5bec8c6d..34f1dbe1 100644
--- a/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
+++ b/tests/auto/qquicktreemodeladaptor/tst_qquicktreemodeladaptor.cpp
@@ -69,6 +69,10 @@ private slots:
     void removeRows_data();
     void removeRows();
 
+    void removeRowsChildrenAndParent();
+    void removeChildrenMoveParent();
+    void removeChildrenRelayoutParent();
+
     void insertRows_data();
     void insertRows();
 
@@ -671,6 +675,127 @@ void tst_QQuickTreeModelAdaptor::removeRows()
     }
 }
 
+void tst_QQuickTreeModelAdaptor::removeRowsChildrenAndParent()
+{
+    TestModel model(ModelRowCount, 1);
+    QQuickTreeModelAdaptor tma;
+    tma.setModel(&model);
+
+    // Expand the first node
+    const QModelIndex &parent = model.index(0, 0);
+    tma.expand(parent);
+    QVERIFY(tma.isExpanded(parent));
+
+    QSignalSpy rowsAboutToBeRemovedSpy(&tma, SIGNAL(rowsAboutToBeRemoved(const QModelIndex&, int, int)));
+    QSignalSpy rowsRemovedSpy(&tma, SIGNAL(rowsRemoved(const QModelIndex&, int, int)));
+
+    // Remove the first node children
+    int expectedRemovedCount = model.rowCount(parent);
+    int tmaItemIdx = tma.itemIndex(model.index(0, 0, parent));
+    QCOMPARE(tmaItemIdx, tma.itemIndex(parent) + 1);
+    model.removeRows(0, expectedRemovedCount, parent);
+    QCOMPARE(rowsAboutToBeRemovedSpy.count(), 1);
+    QCOMPARE(rowsRemovedSpy.count(), 1);
+    QVariantList rowsAboutToBeRemovedArgs = rowsAboutToBeRemovedSpy.first();
+    QVariantList rowsRemovedArgs = rowsRemovedSpy.first();
+    QCOMPARE(rowsAboutToBeRemovedArgs, rowsRemovedArgs);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(0).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(1).toInt(), tmaItemIdx);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(2).toInt(), tmaItemIdx + expectedRemovedCount - 1);
+
+    // Remove the first node
+    rowsAboutToBeRemovedSpy.clear();
+    rowsRemovedSpy.clear();
+    model.removeRows(0, 1, QModelIndex());
+    QCOMPARE(rowsAboutToBeRemovedSpy.count(), 1);
+    QCOMPARE(rowsRemovedSpy.count(), 1);
+    rowsAboutToBeRemovedArgs = rowsAboutToBeRemovedSpy.first();
+    rowsRemovedArgs = rowsRemovedSpy.first();
+    QCOMPARE(rowsAboutToBeRemovedArgs, rowsRemovedArgs);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(0).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(1).toInt(), 0);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(2).toInt(), 0);
+}
+
+void tst_QQuickTreeModelAdaptor::removeChildrenMoveParent()
+{
+    TestModel model(ModelRowCount, 1);
+    QQuickTreeModelAdaptor tma;
+    tma.setModel(&model);
+
+    // Expand the first node
+    const QModelIndex &parent = model.index(0, 0);
+    tma.expand(parent);
+    QVERIFY(tma.isExpanded(parent));
+
+    // Remove the first node children
+    QSignalSpy rowsAboutToBeRemovedSpy(&tma, SIGNAL(rowsAboutToBeRemoved(const QModelIndex&, int, int)));
+    QSignalSpy rowsRemovedSpy(&tma, SIGNAL(rowsRemoved(const QModelIndex&, int, int)));
+    int expectedRemovedCount = model.rowCount(parent);
+    int tmaItemIdx = tma.itemIndex(model.index(0, 0, parent));
+    QCOMPARE(tmaItemIdx, tma.itemIndex(parent) + 1);
+    model.removeRows(0, expectedRemovedCount, parent);
+    QCOMPARE(rowsAboutToBeRemovedSpy.count(), 1);
+    QCOMPARE(rowsRemovedSpy.count(), 1);
+    QVariantList rowsAboutToBeRemovedArgs = rowsAboutToBeRemovedSpy.first();
+    QVariantList rowsRemovedArgs = rowsRemovedSpy.first();
+    QCOMPARE(rowsAboutToBeRemovedArgs, rowsRemovedArgs);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(0).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(1).toInt(), tmaItemIdx);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(2).toInt(), tmaItemIdx + expectedRemovedCount - 1);
+
+    // Move the first node
+    QSignalSpy rowsAboutToBeMovedSpy(&tma, SIGNAL(rowsAboutToBeMoved(QModelIndex,int,int,QModelIndex,int)));
+    QSignalSpy rowsMovedSpy(&tma, SIGNAL(rowsMoved(QModelIndex,int,int,QModelIndex,int)));
+    model.moveRows(QModelIndex(), 0, 1, QModelIndex(), 3);
+    QCOMPARE(rowsAboutToBeMovedSpy.count(), 1);
+    QCOMPARE(rowsRemovedSpy.count(), 1);
+    QVariantList rowsAboutToBeMovedArgs = rowsAboutToBeMovedSpy.first();
+    QVariantList rowsMovedArgs = rowsMovedSpy.first();
+    QCOMPARE(rowsAboutToBeMovedArgs, rowsMovedArgs);
+    QCOMPARE(rowsAboutToBeMovedArgs.at(0).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeMovedArgs.at(1).toInt(), 0);
+    QCOMPARE(rowsAboutToBeMovedArgs.at(2).toInt(), 0);
+    QCOMPARE(rowsAboutToBeMovedArgs.at(3).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeMovedArgs.at(4).toInt(), 3);
+}
+
+void tst_QQuickTreeModelAdaptor::removeChildrenRelayoutParent()
+{
+    TestModel model(ModelRowCount, 1);
+    QQuickTreeModelAdaptor tma;
+    tma.setModel(&model);
+
+    // Expand the first node
+    const QModelIndex &parent = model.index(0, 0);
+    tma.expand(parent);
+    QVERIFY(tma.isExpanded(parent));
+
+    QSignalSpy rowsAboutToBeRemovedSpy(&tma, SIGNAL(rowsAboutToBeRemoved(const QModelIndex&, int, int)));
+    QSignalSpy rowsRemovedSpy(&tma, SIGNAL(rowsRemoved(const QModelIndex&, int, int)));
+
+    // Remove the first node children
+    int expectedRemovedCount = model.rowCount(parent);
+    int tmaItemIdx = tma.itemIndex(model.index(0, 0, parent));
+    QCOMPARE(tmaItemIdx, tma.itemIndex(parent) + 1);
+    model.removeRows(0, expectedRemovedCount, parent);
+    QCOMPARE(rowsAboutToBeRemovedSpy.count(), 1);
+    QCOMPARE(rowsRemovedSpy.count(), 1);
+    QVariantList rowsAboutToBeRemovedArgs = rowsAboutToBeRemovedSpy.first();
+    QVariantList rowsRemovedArgs = rowsRemovedSpy.first();
+    QCOMPARE(rowsAboutToBeRemovedArgs, rowsRemovedArgs);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(0).toModelIndex(), QModelIndex());
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(1).toInt(), tmaItemIdx);
+    QCOMPARE(rowsAboutToBeRemovedArgs.at(2).toInt(), tmaItemIdx + expectedRemovedCount - 1);
+
+    // Relayout the first node
+    QSignalSpy dataChanged(&tma, SIGNAL(dataChanged(QModelIndex,QModelIndex,QVector<int>)));
+    QList<QPersistentModelIndex> parents;
+    parents << parent;
+    model.changeLayout(parents);
+    QCOMPARE(dataChanged.count(), 0);
+}
+
 void tst_QQuickTreeModelAdaptor::insertRows_data()
 {
     QTest::addColumn<int>("insertFromRow");