QSortFilterProxyModel: create mappings on demand again

Calling create_mapping in setSourceModel as introduced by 8455bfee76ed3f1bd3bba8bd3688a7afa94ae0bb
can lead to an early call to filterAcceptsRow, and some existing applications may crash.
It is also an incomplete solution since it was only done for the
toplevel index but not for child indexes.

Instead, go back to creating mappings on demand.
This means coming up with a different fix for QTBUG-87781 (dataChanged
not emitted for indexes that haven't been mapped yet, i.e. not queried
or shown anywhere).

When this happens, we can't know if the index was previously filtered
out or not (for lack of a dataAboutToBeChanged signal...). Creating
the mapping with the new data only gives us the new state of affairs,
there's no reference state to compare to. Therefore, when the mapping
is missing (during dataChanged handling), create it, but skip all the
logic about row insertion/removal, just forward the dataChanged signal
if the row isn't filtered out.

Creating the mapping might require creating first mappings for parents,
recursively, which wasn't done anywhere in QSFPM yet, hence the new
create_mapping_recursive() method.

In addition to all this, the handling of removed items was incorrect,
remove_source_items did nothing if the parent was gone, and then
source_items_removed was trying to adjust indexes in an incorrect list.
If the parent is gone, clear the proxy_to_source list, so there's
nothing to adjust afterwards. This bug actually doesn't happen anymore
in this version of the patch, but the change still seems right and might
prevent repeating a long debugging session in the future.

Thanks to ChunLin Wang for the unittest in this commit.

Done-with: ChunLin Wang
Change-Id: Id543d0cc98f1a03b5852bda01d2f49b980e06be7
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
(cherry picked from commit 4ec5622e62e6fd521e1193f1d6084901f09e7e9e)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

3 files changed, 79 insertions, 10 deletions

diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
index 42b86f08c7..6eb81f7190 100644
--- a/src/corelib/itemmodels/qsortfilterproxymodel.cpp
+++ b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
@@ -307,6 +307,8 @@ public:
 
     QHash<QModelIndex, Mapping *>::const_iterator create_mapping(
         const QModelIndex &source_parent) const;
+    QHash<QModelIndex, Mapping *>::const_iterator create_mapping_recursive(
+            const QModelIndex &source_parent) const;
     QModelIndex proxy_to_source(const QModelIndex &proxyIndex) const;
     QModelIndex source_to_proxy(const QModelIndex &sourceIndex) const;
     bool can_create_mapping(const QModelIndex &source_parent) const;
@@ -533,6 +535,29 @@ IndexMap::const_iterator QSortFilterProxyModelPrivate::create_mapping(
     return it;
 }
 
+// Go up the tree, creating mappings, unless of course the parent is filtered out
+IndexMap::const_iterator QSortFilterProxyModelPrivate::create_mapping_recursive(const QModelIndex &source_parent) const
+{
+    if (source_parent.isValid()) {
+        const QModelIndex source_grand_parent = source_parent.parent();
+        IndexMap::const_iterator it = source_index_mapping.constFind(source_grand_parent);
+        IndexMap::const_iterator end = source_index_mapping.constEnd();
+        if (it == end) {
+            it = create_mapping_recursive(source_grand_parent);
+            end = source_index_mapping.constEnd();
+            if (it == end)
+                return end;
+        }
+        Mapping *gm = it.value();
+        if (gm->proxy_rows.at(source_parent.row()) == -1 ||
+                gm->proxy_columns.at(source_parent.column()) == -1) {
+            // Can't do, parent is filtered
+            return end;
+        }
+    }
+    return create_mapping(source_parent);
+}
+
 QModelIndex QSortFilterProxyModelPrivate::proxy_to_source(const QModelIndex &proxy_index) const
 {
     if (!proxy_index.isValid())
@@ -751,8 +776,10 @@ void QSortFilterProxyModelPrivate::remove_source_items(
 {
     Q_Q(QSortFilterProxyModel);
     QModelIndex proxy_parent = q->mapFromSource(source_parent);
-    if (!proxy_parent.isValid() && source_parent.isValid())
+    if (!proxy_parent.isValid() && source_parent.isValid()) {
+        proxy_to_source.clear();
         return; // nothing to do (already removed)
+    }
 
     const auto proxy_intervals = proxy_intervals_for_source_items(
         source_to_proxy, source_items);
@@ -1404,11 +1431,20 @@ void QSortFilterProxyModelPrivate::_q_sourceDataChanged(const QModelIndex &sourc
         const QModelIndex &source_bottom_right = data_changed.bottomRight;
         const QModelIndex source_parent = source_top_left.parent();
 
+        bool change_in_unmapped_parent = false;
         IndexMap::const_iterator it = source_index_mapping.constFind(source_parent);
         if (it == source_index_mapping.constEnd()) {
-            // Don't care, since we don't have mapping for this index
-            continue;
+            // We don't have mapping for this index, so we cannot know how things
+            // changed (in case the change affects filtering) in order to forward
+            // the change correctly.
+            // But we can at least forward the signal "as is", if the row isn't
+            // filtered out, this is better than nothing.
+            it = create_mapping_recursive(source_parent);
+            if (it == source_index_mapping.constEnd())
+                continue;
+            change_in_unmapped_parent = true;
         }
+
         Mapping *m = it.value();
 
         // Figure out how the source changes affect us
@@ -1418,7 +1454,7 @@ void QSortFilterProxyModelPrivate::_q_sourceDataChanged(const QModelIndex &sourc
         QVector<int> source_rows_resort;
         int end = qMin(source_bottom_right.row(), m->proxy_rows.count() - 1);
         for (int source_row = source_top_left.row(); source_row <= end; ++source_row) {
-            if (dynamic_sortfilter) {
+            if (dynamic_sortfilter && !change_in_unmapped_parent) {
                 if (m->proxy_rows.at(source_row) != -1) {
                     if (!filterAcceptsRowInternal(source_row, source_parent)) {
                         // This source row no longer satisfies the filter, so it must be removed
@@ -1568,7 +1604,6 @@ void QSortFilterProxyModelPrivate::_q_sourceReset()
     _q_clearMapping();
     // All internal structures are deleted in clear()
     q->endResetModel();
-    create_mapping(QModelIndex());
     update_source_sort_column();
     if (dynamic_sortfilter && update_source_sort_column())
         sort();
@@ -1634,8 +1669,8 @@ void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeInserted(
 
     const bool toplevel = !source_parent.isValid();
     const bool recursive_accepted = filter_recursive && !toplevel && filterAcceptsRowInternal(source_parent.row(), source_parent.parent());
-    //Force the creation of a mapping now, even if its empty.
-    //We need it because the proxy can be acessed at the moment it emits rowsAboutToBeInserted in insert_source_items
+    //Force the creation of a mapping now, even if it's empty.
+    //We need it because the proxy can be accessed at the moment it emits rowsAboutToBeInserted in insert_source_items
     if (!filter_recursive || toplevel || recursive_accepted) {
         if (can_create_mapping(source_parent))
             create_mapping(source_parent);
@@ -1754,8 +1789,8 @@ void QSortFilterProxyModelPrivate::_q_sourceColumnsAboutToBeInserted(
 {
     Q_UNUSED(start);
     Q_UNUSED(end);
-    //Force the creation of a mapping now, even if its empty.
-    //We need it because the proxy can be acessed at the moment it emits columnsAboutToBeInserted in insert_source_items
+    //Force the creation of a mapping now, even if it's empty.
+    //We need it because the proxy can be accessed at the moment it emits columnsAboutToBeInserted in insert_source_items
     if (can_create_mapping(source_parent))
         create_mapping(source_parent);
 }
@@ -2149,7 +2184,6 @@ void QSortFilterProxyModel::setSourceModel(QAbstractItemModel *sourceModel)
     connect(d->model, SIGNAL(modelReset()), this, SLOT(_q_sourceReset()));
 
     endResetModel();
-    d->create_mapping(QModelIndex());
     if (d->update_source_sort_column() && d->dynamic_sortfilter)
         d->sort();
 }
diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.cpp b/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.cpp
index 6f608854ae..a6e1f49be2 100644
--- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.cpp
+++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.cpp
@@ -2220,6 +2220,40 @@ void tst_QSortFilterProxyModel::changeSourceDataProxySendDataChanged_qtbug87781(
     QCOMPARE(afterDataChangedSpy.size(), 1);
 }
 
+void tst_QSortFilterProxyModel::changeSourceDataTreeModel()
+{
+    QStandardItemModel treeModel;
+    QSortFilterProxyModel treeProxyModelBefore;
+    QSortFilterProxyModel treeProxyModelAfter;
+
+    QSignalSpy treeBaseDataChangedSpy(&treeModel, &QStandardItemModel::dataChanged);
+    QSignalSpy treeBeforeDataChangedSpy(&treeProxyModelBefore, &QSortFilterProxyModel::dataChanged);
+    QSignalSpy treeAfterDataChangedSpy(&treeProxyModelAfter, &QSortFilterProxyModel::dataChanged);
+
+    QVERIFY(treeBaseDataChangedSpy.isValid());
+    QVERIFY(treeBeforeDataChangedSpy.isValid());
+    QVERIFY(treeAfterDataChangedSpy.isValid());
+
+    treeProxyModelBefore.setSourceModel(&treeModel);
+    QStandardItem treeNode1("data1");
+    QStandardItem treeNode11("data11");
+    QStandardItem treeNode111("data111");
+
+    treeNode1.appendRow(&treeNode11);
+    treeNode11.appendRow(&treeNode111);
+    treeModel.appendRow(&treeNode1);
+    treeProxyModelAfter.setSourceModel(&treeModel);
+
+    QCOMPARE(treeBaseDataChangedSpy.size(), 0);
+    QCOMPARE(treeBeforeDataChangedSpy.size(), 0);
+    QCOMPARE(treeAfterDataChangedSpy.size(), 0);
+
+    treeNode111.setData(QStringLiteral("new data"), Qt::DisplayRole);
+    QCOMPARE(treeBaseDataChangedSpy.size(), 1);
+    QCOMPARE(treeBeforeDataChangedSpy.size(), 1);
+    QCOMPARE(treeAfterDataChangedSpy.size(), 1);
+}
+
 void tst_QSortFilterProxyModel::changeSourceDataProxyFilterSingleColumn()
 {
     enum modelRow { Row0, Row1, RowCount };
diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.h b/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.h
index 7bf87a86c2..97862e804b 100644
--- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.h
+++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.h
@@ -91,6 +91,7 @@ private slots:
     void changeSourceDataKeepsStableSorting_qtbug1548();
     void changeSourceDataForwardsRoles_qtbug35440();
     void changeSourceDataProxySendDataChanged_qtbug87781();
+    void changeSourceDataTreeModel();
     void changeSourceDataProxyFilterSingleColumn();
     void changeSourceDataProxyFilterMultipleColumns();
     void resortingDoesNotBreakTreeModels();