QSortFilterProxyModel: Don't forward the hint from source's layoutChanged signal

We can't forward a VerticalSortHint or HorizontalSortHint hint, because we might
be filtering extra items.

The documentation of QAbstractItemModel::LayoutChangeHint states:

    Note that VerticalSortHint and HorizontalSortHint carry the meaning that
    items are being moved within the same parent, not moved to a different
    parent in the model, and not filtered out or in.

And some of the views rely on this assumption (QQmlDelegateModel for example)

What happens in the test is the following:
- 'model' emit the dataChanged signal when its data is changed.
- 'proxi1' QSortFilterProxyModelPrivate::_q_sourceDataChanged does not forward
  the dataChanged signal imediatly, it will instead first re-sort the model and
  call layoutAboutToBeChanged / layouChanged with the VerticalSortHint
- 'proxy2' would forward the layoutAboutToBeChanged with the hint, but in
  QSortFilterProxyModelPrivate::_q_sourceLayoutChanged, it will redo the mapping
  which will cause the changed data to be filtered.
So proxy2 can't forward the VerticalSortHint as it removed rows in the process.

Change-Id: I20b6983e9d18bf7509fe6144c74f37d24e4a18c2
Reviewed-by: Tobias Koenig <tobias.koenig@kdab.com>
Reviewed-by: David Faure <david.faure@kdab.com>

2 files changed, 56 insertions, 2 deletions

diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
index 0771fd0e30..f264ad015d 100644
--- a/src/corelib/itemmodels/qsortfilterproxymodel.cpp
+++ b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
@@ -1322,6 +1322,7 @@ void QSortFilterProxyModelPrivate::_q_sourceReset()
 void QSortFilterProxyModelPrivate::_q_sourceLayoutAboutToBeChanged(const QList<QPersistentModelIndex> &sourceParents, QAbstractItemModel::LayoutChangeHint hint)
 {
     Q_Q(QSortFilterProxyModel);
+    Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns
     saved_persistent_indexes.clear();
 
     QList<QPersistentModelIndex> parents;
@@ -1340,7 +1341,7 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutAboutToBeChanged(const QList<Q
     if (!sourceParents.isEmpty() && parents.isEmpty())
         return;
 
-    emit q->layoutAboutToBeChanged(parents, hint);
+    emit q->layoutAboutToBeChanged(parents);
     if (persistent.indexes.isEmpty())
         return;
 
@@ -1350,6 +1351,7 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutAboutToBeChanged(const QList<Q
 void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersistentModelIndex> &sourceParents, QAbstractItemModel::LayoutChangeHint hint)
 {
     Q_Q(QSortFilterProxyModel);
+    Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns
 
     // Optimize: We only actually have to clear the mapping related to the contents of
     // sourceParents, not everything.
@@ -1379,7 +1381,7 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersisten
     if (!sourceParents.isEmpty() && parents.isEmpty())
         return;
 
-    emit q->layoutChanged(parents, hint);
+    emit q->layoutChanged(parents);
 }
 
 void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeInserted(
diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
index 0302ae5cbf..5928ee8688 100644
--- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
+++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
@@ -148,6 +148,7 @@ private slots:
     void noMapAfterSourceDelete();
     void forwardDropApi();
     void canDropMimeData();
+    void filterHint();
 
 protected:
     void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
@@ -3804,6 +3805,12 @@ void tst_QSortFilterProxyModel::moveSourceRows()
     QCOMPARE(filterBeforeParents.size(), 1);
     QCOMPARE(filterAfterParents.size(), 1);
 
+    QCOMPARE(
+        filterBeforeParentLayoutSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+        QAbstractItemModel::NoLayoutChangeHint);
+    QCOMPARE(filterAfterParentLayoutSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+             QAbstractItemModel::NoLayoutChangeHint);
+
     QCOMPARE(filterBothBeforeParentLayoutSpy.size(), 0);
     QCOMPARE(filterBothAfterParentLayoutSpy.size(), 0);
 }
@@ -4124,5 +4131,50 @@ void tst_QSortFilterProxyModel::resortingDoesNotBreakTreeModels()
     QCOMPARE(proxy.rowCount(pi1), 1);
 }
 
+void tst_QSortFilterProxyModel::filterHint()
+{
+    // test that a filtering model does not emit layoutChanged with a hint
+    QStringListModel model(QStringList() << "one"
+                                         << "two"
+                                         << "three"
+                                         << "four"
+                                         << "five"
+                                         << "six");
+    QSortFilterProxyModel proxy1;
+    proxy1.setSourceModel(&model);
+    proxy1.setSortRole(Qt::DisplayRole);
+    proxy1.setDynamicSortFilter(true);
+    proxy1.sort(0);
+
+    QSortFilterProxyModel proxy2;
+    proxy2.setSourceModel(&proxy1);
+    proxy2.setFilterRole(Qt::DisplayRole);
+    proxy2.setFilterRegExp("^[^ ]*$");
+    proxy2.setDynamicSortFilter(true);
+
+    QSignalSpy proxy1BeforeSpy(&proxy1, &QSortFilterProxyModel::layoutAboutToBeChanged);
+    QSignalSpy proxy1AfterSpy(&proxy1, &QSortFilterProxyModel::layoutChanged);
+    QSignalSpy proxy2BeforeSpy(&proxy2, &QSortFilterProxyModel::layoutAboutToBeChanged);
+    QSignalSpy proxy2AfterSpy(&proxy2, &QSortFilterProxyModel::layoutChanged);
+
+    model.setData(model.index(2), QStringLiteral("modified three"), Qt::DisplayRole);
+
+    // The first proxy was re-sorted as one item as changed.
+    QCOMPARE(proxy1BeforeSpy.size(), 1);
+    QCOMPARE(proxy1BeforeSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+             QAbstractItemModel::VerticalSortHint);
+    QCOMPARE(proxy1AfterSpy.size(), 1);
+    QCOMPARE(proxy1AfterSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+             QAbstractItemModel::VerticalSortHint);
+
+    // But the second proxy must not have the VerticalSortHint since an item was filtered
+    QCOMPARE(proxy2BeforeSpy.size(), 1);
+    QCOMPARE(proxy2BeforeSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+             QAbstractItemModel::NoLayoutChangeHint);
+    QCOMPARE(proxy2AfterSpy.size(), 1);
+    QCOMPARE(proxy2AfterSpy.first().at(1).value<QAbstractItemModel::LayoutChangeHint>(),
+             QAbstractItemModel::NoLayoutChangeHint);
+}
+
 QTEST_MAIN(tst_QSortFilterProxyModel)
 #include "tst_qsortfilterproxymodel.moc"