QQmlTreeModelToTableModel: move logic from modelLayoutChanged() to modelLayoutAboutToBeChanged()

The QQmlTreeModelToTableModel contains a QList of items that
converts the source model (which typically is a tree model)
into a flat list of items that can be be displayed in a TableView.

When the source model is changing the layout (e.g when doing
a sort), we handle this in QQmlTreeModelToTableModel by listening
to the modelLayoutChanged() signal. The problem is that by the
time we receive that signal, all the TreeItems in the list of
items will have their QPersistentModelIndex updated, before we
have updated the layout of the list to be in sync. Since functions
like itemIndex(index) depends on the list model and the tree model
to be in sync, it cannot be trusted at this point.

To handle this problem, we need to divide the current logic into
two functions. The first part will remove all the affected rows
from the list before the source model has changed (while the models
are still in sync), from modelLayoutAboutToBeChanged(). Then we
add them back again once the source model has finished updating the
layout, which is done from modelLayoutChanged().

This will fix a bug in TreeView, where the view will not reflect
the sorting changes done to a QSortFilterProxyModel applied on
a QAbstractItemModel.

Fixes: QTBUG-103877
Change-Id: I9acfc3b7b9a79c2d16e1bc63e56b1d6ea5a53897
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>
(cherry picked from commit 96e70f23666123654528d29d552352d3fddbff18)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

6 files changed, 230 insertions, 40 deletions

diff --git a/src/qmlmodels/qqmltreemodeltotablemodel.cpp b/src/qmlmodels/qqmltreemodeltotablemodel.cpp
index 810ea2cde8..930b113811 100644
--- a/src/qmlmodels/qqmltreemodeltotablemodel.cpp
+++ b/src/qmlmodels/qqmltreemodeltotablemodel.cpp
@@ -672,18 +672,61 @@ void QQmlTreeModelToTableModel::modelDataChanged(const QModelIndex &topLeft, con
 
 void QQmlTreeModelToTableModel::modelLayoutAboutToBeChanged(const QList<QPersistentModelIndex> &parents, QAbstractItemModel::LayoutChangeHint hint)
 {
-    ASSERT_CONSISTENCY();
-    Q_UNUSED(parents)
     Q_UNUSED(hint)
+
+    // Since the m_items is a list of TreeItems that contains QPersistentModelIndexes, we
+    // cannot wait until we get a modelLayoutChanged() before we remove the affected rows
+    // from that list. After the layout has changed, the list (or, the persistent indexes
+    // that it contains) is no longer in sync with the model (after all, that is what we're
+    // supposed to correct in modelLayoutChanged()).
+    // This means that vital functions, like itemIndex(index), cannot be trusted at that point.
+    // Therefore we need to do the update in two steps; First remove all the affected rows
+    // from here (while we're still in sync with the model), and then add back the
+    // affected rows, and notify about it, from modelLayoutChanged().
+    m_modelLayoutChanged = false;
+
+    if (parents.isEmpty() || !parents[0].isValid()) {
+        // Update entire model
+        emit layoutAboutToBeChanged();
+        m_modelLayoutChanged = true;
+        m_items.clear();
+        return;
+    }
+
+    for (const QPersistentModelIndex &pmi : parents) {
+        if (!m_expandedItems.contains(pmi))
+            continue;
+        const int row = itemIndex(pmi);
+        if (row == -1)
+            continue;
+        const int rowCount = m_model->rowCount(pmi);
+        if (rowCount == 0)
+            continue;
+
+        if (!m_modelLayoutChanged) {
+            emit layoutAboutToBeChanged();
+            m_modelLayoutChanged = true;
+        }
+
+        const QModelIndex &lmi = m_model->index(rowCount - 1, 0, pmi);
+        const int lastRow = lastChildIndex(lmi);
+        removeVisibleRows(row + 1, lastRow, false /*doRemoveRows*/);
+    }
+
+    ASSERT_CONSISTENCY();
 }
 
 void QQmlTreeModelToTableModel::modelLayoutChanged(const QList<QPersistentModelIndex> &parents, QAbstractItemModel::LayoutChangeHint hint)
 {
     Q_UNUSED(hint)
 
-    if (parents.isEmpty()) {
-        emit layoutAboutToBeChanged();
-        m_items.clear();
+    if (!m_modelLayoutChanged) {
+        // No relevant changes done from modelLayoutAboutToBeChanged()
+        return;
+    }
+
+    if (m_items.isEmpty()) {
+        // Entire model has changed. Add back all rows.
         showModelTopLevelItems(false /*doInsertRows*/);
         const QModelIndex &mi = m_model->index(0, 0);
         const int columnCount = m_model->columnCount(mi);
@@ -692,30 +735,24 @@ void QQmlTreeModelToTableModel::modelLayoutChanged(const QList<QPersistentModelI
         return;
     }
 
-    bool shouldEmitLayoutChanged = false;
     for (const QPersistentModelIndex &pmi : parents) {
-        if (m_expandedItems.contains(pmi)) {
-            int row = itemIndex(pmi);
-            if (row != -1) {
-                if (!shouldEmitLayoutChanged) {
-                    shouldEmitLayoutChanged = true;
-                    emit layoutAboutToBeChanged();
-                }
-                int rowCount = m_model->rowCount(pmi);
-                if (rowCount > 0) {
-                    const QModelIndex &lmi = m_model->index(rowCount - 1, 0, pmi);
-                    const int lastRow = lastChildIndex(lmi);
-                    const int columnCount = m_model->columnCount(lmi);
-                    removeVisibleRows(row + 1, lastRow, false /*doRemoveRows*/);
-                    showModelChildItems(m_items.at(row), 0, rowCount - 1, false /*doInsertRows*/);
-                    emit dataChanged(index(row + 1, 0), index(lastRow, columnCount - 1));
-                }
-            }
-        }
+        if (!m_expandedItems.contains(pmi))
+            continue;
+        const int row = itemIndex(pmi);
+        if (row == -1)
+            continue;
+        const int rowCount = m_model->rowCount(pmi);
+        if (rowCount == 0)
+            continue;
+
+        const QModelIndex &lmi = m_model->index(rowCount - 1, 0, pmi);
+        const int columnCount = m_model->columnCount(lmi);
+        showModelChildItems(m_items.at(row), 0, rowCount - 1, false /*doInsertRows*/);
+        const int lastRow = lastChildIndex(lmi);
+        emit dataChanged(index(row + 1, 0), index(lastRow, columnCount - 1));
     }
 
-    if (shouldEmitLayoutChanged)
-        emit layoutChanged();
+    emit layoutChanged();
 
     ASSERT_CONSISTENCY();
 }
diff --git a/src/qmlmodels/qqmltreemodeltotablemodel_p_p.h b/src/qmlmodels/qqmltreemodeltotablemodel_p_p.h
index dd5857e3a6..55ef45912a 100644
--- a/src/qmlmodels/qqmltreemodeltotablemodel_p_p.h
+++ b/src/qmlmodels/qqmltreemodeltotablemodel_p_p.h
@@ -165,6 +165,7 @@ private:
     QList<TreeItem> m_itemsToExpand;
     mutable int m_lastItemIndex = 0;
     bool m_visibleRowsMoved = false;
+    bool m_modelLayoutChanged = false;
     int m_signalAggregatorStack = 0;
     QVector<DataChangedParams> m_queuedDataChanged;
     int m_column = 0;
diff --git a/tests/auto/qml/qqmltreemodeltotablemodel/tst_qqmltreemodeltotablemodel.cpp b/tests/auto/qml/qqmltreemodeltotablemodel/tst_qqmltreemodeltotablemodel.cpp
index 3efc369b57..cd9aed4b30 100644
--- a/tests/auto/qml/qqmltreemodeltotablemodel/tst_qqmltreemodeltotablemodel.cpp
+++ b/tests/auto/qml/qqmltreemodeltotablemodel/tst_qqmltreemodeltotablemodel.cpp
@@ -8,6 +8,10 @@
 
 #include "testmodel.h"
 
+/*
+ * Note: Out of practical reasons, QQmlTreeModelToTableModel is by and large
+ * tested from tst_qquicktreeview.cpp, where TreeView is available.
+ */
 class tst_QQmlTreeModelToTableModel : public QObject {
     Q_OBJECT
 
diff --git a/tests/auto/quick/qquicktreeview/testmodel.cpp b/tests/auto/quick/qquicktreeview/testmodel.cpp
index 50e326e108..49bbc5a458 100644
--- a/tests/auto/quick/qquicktreeview/testmodel.cpp
+++ b/tests/auto/quick/qquicktreeview/testmodel.cpp
@@ -38,8 +38,14 @@ void TestModel::createTreeRecursive(TreeItem *item, int childCount, int currentD
         for (int col = 0; col < m_columnCount; ++col)
             childItem->m_entries << QVariant(QString("%1, %2").arg(row).arg(col));
         item->m_childItems.append(childItem);
-        if (row == childCount - 1)
+        if (row == childCount - 2 && currentDepth != maxDepth()) {
+            // Add a branch that doesn't recurse
+            createTreeRecursive(childItem, childCount, maxDepth());
+        }
+        if (row == childCount - 1) {
+            // Add a branch that recurses
             createTreeRecursive(childItem, childCount, currentDepth + 1);
+        }
     }
 }
 
diff --git a/tests/auto/quick/qquicktreeview/testmodel.h b/tests/auto/quick/qquicktreeview/testmodel.h
index e35a6e4f6a..4cb80d0eab 100644
--- a/tests/auto/quick/qquicktreeview/testmodel.h
+++ b/tests/auto/quick/qquicktreeview/testmodel.h
@@ -45,4 +45,6 @@ private:
     int m_columnCount = 5;
 };
 
+#define TestModelAsVariant(...) QVariant::fromValue(QSharedPointer<TestModel>(new TestModel(__VA_ARGS__)))
+
 #endif // TESTMODEL_H
diff --git a/tests/auto/quick/qquicktreeview/tst_qquicktreeview.cpp b/tests/auto/quick/qquicktreeview/tst_qquicktreeview.cpp
index 1a775ed3c7..776e7c4968 100644
--- a/tests/auto/quick/qquicktreeview/tst_qquicktreeview.cpp
+++ b/tests/auto/quick/qquicktreeview/tst_qquicktreeview.cpp
@@ -79,6 +79,10 @@ private slots:
     void selectionBehaviorRows();
     void selectionBehaviorColumns();
     void selectionBehaviorDisabled();
+    void sortTreeModel_data();
+    void sortTreeModel();
+    void sortTreeModelDynamic_data();
+    void sortTreeModelDynamic();
 };
 
 tst_qquicktreeview::tst_qquicktreeview()
@@ -297,7 +301,7 @@ void tst_qquicktreeview::requiredPropertiesChildren()
         QCOMPARE(viewProp, treeView);
         QCOMPARE(isTreeNode, true);
         QCOMPARE(expanded, row == 4);
-        QCOMPARE(hasChildren, row == 4 || row == 8);
+        QCOMPARE(hasChildren, model->hasChildren(treeView->modelIndex(0, row)));
         QCOMPARE(depth, row <= 4 ? 1 : 2);
     }
 }
@@ -508,7 +512,7 @@ void tst_qquicktreeview::expandRecursivelyChild()
     const bool rowToExpandDepth = treeView->depth(rowToExpand);
     const int effectiveMaxDepth = depth != -1 ? rowToExpandDepth + depth : model->maxDepth();
 
-    // Check that all rows before rowToExpand is not expanded
+    // Check that none of the rows before rowToExpand are expanded
     // (except the root node)
     for (int currentRow = 1; currentRow < rowToExpand; ++currentRow)
         QVERIFY(!treeView->isExpanded(currentRow));
@@ -519,8 +523,8 @@ void tst_qquicktreeview::expandRecursivelyChild()
     else
         QVERIFY(!treeView->isExpanded(rowToExpand));
 
-    // Check that all rows after rowToExpand that is also
-    // children of that row is expanded (down to depth)
+    // Check that any row after rowToExpand, that is also
+    // a child of that row, is expanded (down to depth)
     for (int currentRow = rowToExpand + 1; currentRow < treeView->rows(); ++currentRow) {
         const int currentDepth = treeView->depth(currentRow);
         const bool isChild = currentDepth > rowToExpandDepth;
@@ -562,7 +566,9 @@ void tst_qquicktreeview::collapseRecursivelyRoot()
     WAIT_UNTIL_POLISHED;
 
     // Verify that the tree is now fully expanded
-    const int expectedRowCount = 1 + (model->maxDepth() * 4); // root + 4 children per level
+    // The number of rows should be the root, + 4 children per level. All parents
+    // (minus the root), will also have a node with 4 non-recursive children.
+    const int expectedRowCount = 1 + (model->maxDepth() * 8) - 4;
     QCOMPARE(treeView->rows(), expectedRowCount);
 
     QSignalSpy spy(treeView, SIGNAL(collapsed(int, bool)));
@@ -607,14 +613,18 @@ void tst_qquicktreeview::collapseRecursivelyChild()
     WAIT_UNTIL_POLISHED;
 
     // Verify that the tree is now fully expanded
-    const int expectedRowCount = 1 + (model->maxDepth() * 4); // root + 4 children per level
+    // The number of rows should be the root, + 4 children per level. All parents
+    // (minus the root), will also have a node with 4 non-recursive children.
+    const int expectedRowCount = 1 + (model->maxDepth() * 8) - 4;
     QCOMPARE(treeView->rows(), expectedRowCount);
 
     QSignalSpy spy(treeView, SIGNAL(collapsed(int, bool)));
 
-    // Collapse the 4th child recursive
-    const int rowToCollapse = 4;
-    QCOMPARE(model->data(treeView->modelIndex(0, rowToCollapse), Qt::DisplayRole), QStringLiteral("3, 0"));
+    // Collapse the 8th child recursive
+    const int rowToCollapse = 8;
+    const QModelIndex collapseIndex = treeView->modelIndex(0, rowToCollapse);
+    const auto expectedLabel = model->data(collapseIndex, Qt::DisplayRole);
+    QCOMPARE(expectedLabel, QStringLiteral("3, 0"));
     treeView->collapseRecursively(rowToCollapse);
 
     QCOMPARE(spy.count(), 1);
@@ -624,7 +634,7 @@ void tst_qquicktreeview::collapseRecursivelyChild()
 
     WAIT_UNTIL_POLISHED;
 
-    QCOMPARE(treeView->rows(), 5); // root + 4 children
+    QCOMPARE(treeView->rows(), 9); // root + 4 children + 4 grand children of the 3rd row
 
     // We need to check that all descendants are collapsed as well. But since they're
     // now no longer visible in the view, we need to expand each parent one by one again to make
@@ -634,9 +644,15 @@ void tst_qquicktreeview::collapseRecursivelyChild()
     while (currentRow < treeView->rows()) {
         const QModelIndex currentIndex = treeView->modelIndex(0, currentRow);
         if (model->hasChildren(currentIndex)) {
-            QVERIFY(!treeView->isExpanded(currentRow));
-            treeView->expand(currentRow);
-            WAIT_UNTIL_POLISHED;
+            if (treeView->depth(currentRow) == 1 && currentIndex.row() == 2) {
+                // We did only recursively expand the 4th child, so the
+                // third should still be expanded
+                QVERIFY(treeView->isExpanded(currentRow));
+            } else {
+                QVERIFY(!treeView->isExpanded(currentRow));
+                treeView->expand(currentRow);
+                WAIT_UNTIL_POLISHED;
+            }
         }
         currentRow++;
     }
@@ -986,6 +1002,130 @@ void tst_qquicktreeview::selectionBehaviorDisabled()
     QCOMPARE(selectionModel->hasSelection(), false);
 }
 
+void tst_qquicktreeview::sortTreeModel_data()
+{
+    QTest::addColumn<QSharedPointer<QAbstractItemModel>>("treeModel");
+
+    const auto stringList = QStringList() << "1" << "2" << "3";
+    QTest::newRow("TreeModel") << QSharedPointer<QAbstractItemModel>(new TestModel());
+    QTest::newRow("Number model") << QSharedPointer<QAbstractItemModel>(new QStringListModel(stringList));
+}
+
+void tst_qquicktreeview::sortTreeModel()
+{
+    // Check that if you assign a QSortFilterProxyModel to to a TreeView, the
+    // view will end up sorted correctly if the proxy model is sorted.
+    QFETCH(QSharedPointer<QAbstractItemModel>, treeModel);
+    LOAD_TREEVIEW("normaltreeview.qml");
+
+    QSortFilterProxyModel proxyModel;
+    proxyModel.setSourceModel(treeModel.data());
+    treeView->setModel(QVariant::fromValue(&proxyModel));
+
+    // Expand some nodes
+    treeView->expand(0);
+    treeView->expand(4);
+    treeView->expand(3);
+
+    WAIT_UNTIL_POLISHED;
+
+    // Go through all rows in the view, and check that display in the model
+    // is the same as in the view. That means that QQmlTreeModelToTableModel
+    // and QSortFilterProxyModel are in sync.
+    for (int row = 0; row < treeView->rows(); ++row) {
+        const auto index = treeView->modelIndex(0, row);
+        const QString modelDisplay = proxyModel.data(index, Qt::DisplayRole).toString();
+        const auto childFxItem = treeViewPrivate->loadedTableItem(QPoint(0, row));
+        QVERIFY(childFxItem);
+        const auto childItem = childFxItem->item;
+        QVERIFY(childItem);
+        const auto context = qmlContext(childItem.data());
+        const auto itemDisplay = context->contextProperty("display").toString();
+        QCOMPARE(itemDisplay, modelDisplay);
+    }
+
+    // Now sort the model, and do the same test again
+    proxyModel.sort(0, Qt::DescendingOrder);
+    WAIT_UNTIL_POLISHED;
+
+    for (int row = 0; row < treeView->rows(); ++row) {
+        const auto index = treeView->modelIndex(0, row);
+        const QString modelDisplay = proxyModel.data(index, Qt::DisplayRole).toString();
+        const auto childFxItem = treeViewPrivate->loadedTableItem(QPoint(0, row));
+        QVERIFY(childFxItem);
+        const auto childItem = childFxItem->item;
+        QVERIFY(childItem);
+        const auto context = qmlContext(childItem.data());
+        const auto itemDisplay = context->contextProperty("display").toString();
+        QCOMPARE(itemDisplay, modelDisplay);
+    }
+}
+
+void tst_qquicktreeview::sortTreeModelDynamic_data()
+{
+    QTest::addColumn<QSharedPointer<QAbstractItemModel>>("treeModel");
+    QTest::addColumn<int>("row");
+
+    const auto stringList = QStringList() << "1" << "2" << "3";
+    QTest::newRow("TreeModel 0") << QSharedPointer<QAbstractItemModel>(new TestModel()) << 0;
+    QTest::newRow("TreeModel 1") << QSharedPointer<QAbstractItemModel>(new TestModel()) << 1;
+    QTest::newRow("TreeModel 3") << QSharedPointer<QAbstractItemModel>(new TestModel()) << 3;
+    QTest::newRow("TreeModel 6") << QSharedPointer<QAbstractItemModel>(new TestModel()) << 6;
+    QTest::newRow("Number model") << QSharedPointer<QAbstractItemModel>(new QStringListModel(stringList)) << 1;
+}
+
+void tst_qquicktreeview::sortTreeModelDynamic()
+{
+    // Check that if you assign a QSortFilterProxyModel to a TreeView, and
+    // set DynamicSortFilter to true, the view will end up sorted correctly
+    // if you change the text for one of the items.
+    QFETCH(QSharedPointer<QAbstractItemModel>, treeModel);
+    QFETCH(int, row);
+    LOAD_TREEVIEW("normaltreeview.qml");
+
+    QSortFilterProxyModel proxyModel;
+    proxyModel.setSourceModel(treeModel.data());
+    proxyModel.setDynamicSortFilter(true);
+    proxyModel.sort(Qt::AscendingOrder);
+    treeView->setModel(QVariant::fromValue(&proxyModel));
+
+    // Expand some nodes
+    treeView->expand(0);
+    treeView->expand(4);
+    treeView->expand(3);
+
+    // Go through all rows in the view, and check that display in the model
+    // is the same as in the view. That means that QQmlTreeModelToTableModel
+    // and QSortFilterProxyModel are in sync.
+    for (int row = 0; row < treeView->rows(); ++row) {
+        const auto index = treeView->modelIndex(0, row);
+        const QString modelDisplay = proxyModel.data(index, Qt::DisplayRole).toString();
+        const auto childFxItem = treeViewPrivate->loadedTableItem(QPoint(0, row));
+        QVERIFY(childFxItem);
+        const auto childItem = childFxItem->item;
+        QVERIFY(childItem);
+        const auto context = qmlContext(childItem.data());
+        const auto itemDisplay = context->contextProperty("display").toString();
+        QCOMPARE(itemDisplay, modelDisplay);
+    }
+
+    // Now change the text in one of the items. This will trigger
+    // a sort for only one of the parents in the model.
+    proxyModel.setData(treeView->modelIndex(0, row), u"xxx"_qs, Qt::DisplayRole);
+
+    for (int row = 0; row < treeView->rows(); ++row) {
+        const auto index = treeView->modelIndex(0, row);
+        const QString modelDisplay = proxyModel.data(index, Qt::DisplayRole).toString();
+        const auto childFxItem = treeViewPrivate->loadedTableItem(QPoint(0, row));
+        QVERIFY(childFxItem);
+        const auto childItem = childFxItem->item;
+        QVERIFY(childItem);
+        const auto context = qmlContext(childItem.data());
+        const auto itemDisplay = context->contextProperty("display").toString();
+        QCOMPARE(itemDisplay, modelDisplay);
+    }
+}
+
 QTEST_MAIN(tst_qquicktreeview)
 
 #include "tst_qquicktreeview.moc"