From 00ce45efe16ff440651746a94c62e0d5c1f6e435 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Fri, 23 Aug 2024 17:26:04 +0200 Subject: [PATCH] QSortFilterProxyModel: add a protected beginFilterChange If the filter gets changed and invalidated while there is no mapping (perhaps because the model had been invalidated first), then we fail to notice the change and don't emit rowsInserted/Removed. And as the new filter is already in place by the time invalidateFilter gets called, we cannot know what the size of the model was before the change. The only way to fix that is to introduce a beginFilterChange protected function that makes sure that we have a mapping from the source model with the old filter. That is a no-op if a mapping is already in place, costing only the lookup in a hash table. By calling this function, custom models with their own filtering logic can make sure that their model emits the changed-signals as expected. Add test coverage and documentation and fix the relevant examples snippet to use that new protected function as recommended, and to invalidate only the rows filter. [ChangeLog][Core][QSortFilterProxyModel] Added a new protected function beginFilterChange() that subclasses overriding filterAcceptsRow or filterAcceptsColumn should call before the filter parameter is changed. This makes sure that the signals informing about rows or columns changing get correctly emitted. Fixes: QTBUG-115717 Change-Id: Ib73a7119ac9dd9c4bcf220f1274d6b4ed093e7ff Reviewed-by: David Faure --- .../mysortfilterproxymodel.cpp | 6 +- .../itemmodels/qsortfilterproxymodel.cpp | 40 ++++++++++--- .../itemmodels/qsortfilterproxymodel.h | 1 + .../tst_qsortfilterproxymodel.cpp | 59 +++++++++++++++++++ .../tst_qsortfilterproxymodel.h | 2 + 5 files changed, 97 insertions(+), 11 deletions(-) diff --git a/examples/widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp b/examples/widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp index 736d3a40660..841b395bf98 100644 --- a/examples/widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp +++ b/examples/widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp @@ -15,16 +15,18 @@ MySortFilterProxyModel::MySortFilterProxyModel(QObject *parent) //! [1] void MySortFilterProxyModel::setFilterMinimumDate(QDate date) { + beginFilterChange(); minDate = date; - invalidateFilter(); + invalidateRowsFilter(); } //! [1] //! [2] void MySortFilterProxyModel::setFilterMaximumDate(QDate date) { + beginFilterChange(); maxDate = date; - invalidateFilter(); + invalidateRowsFilter(); } //! [2] diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp index 73da00642a7..6c71f131451 100644 --- a/src/corelib/itemmodels/qsortfilterproxymodel.cpp +++ b/src/corelib/itemmodels/qsortfilterproxymodel.cpp @@ -3079,6 +3079,25 @@ void QSortFilterProxyModel::invalidate() emit layoutChanged(); } +/*! + \since 6.9 + + Prepares a change of the filter. + + This function should be called if you are implementing custom filtering + (e.g. filterAcceptsRow()), and your filter parameter is about to be changed. + + \snippet ../widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp 2 + + \sa invalidateFilter(), invalidateColumnsFilter(), invalidateRowsFilter() +*/ + +void QSortFilterProxyModel::beginFilterChange() +{ + Q_D(QSortFilterProxyModel); + d->create_mapping({}); +} + /*! \since 4.3 @@ -3087,9 +3106,12 @@ void QSortFilterProxyModel::invalidate() This function should be called if you are implementing custom filtering (e.g. filterAcceptsRow()), and your filter parameters have changed. - \sa invalidate() - \sa invalidateColumnsFilter() - \sa invalidateRowsFilter() + Before your filter parameters change, call beginFilterChange(). + + \snippet ../widgets/itemviews/customsortfiltermodel/mysortfilterproxymodel.cpp 2 + + \sa invalidate(), invalidateColumnsFilter(), invalidateRowsFilter(), + beginFilterChange() */ void QSortFilterProxyModel::invalidateFilter() { @@ -3109,9 +3131,9 @@ void QSortFilterProxyModel::invalidateFilter() instead of invalidateFilter() if you want to hide or show a column where the rows don't change. - \sa invalidate() - \sa invalidateFilter() - \sa invalidateRowsFilter() + Before your filter parameters change, call beginFilterChange(). + + \sa invalidate(), invalidateRowsFilter(), beginFilterChange() */ void QSortFilterProxyModel::invalidateColumnsFilter() { @@ -3131,9 +3153,9 @@ void QSortFilterProxyModel::invalidateColumnsFilter() instead of invalidateFilter() if you want to hide or show a row where the columns don't change. - \sa invalidate() - \sa invalidateFilter() - \sa invalidateColumnsFilter() + Before your filter parameters change, call beginFilterChange(). + + \sa invalidate(), invalidateFilter(), invalidateColumnsFilter() */ void QSortFilterProxyModel::invalidateRowsFilter() { diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.h b/src/corelib/itemmodels/qsortfilterproxymodel.h index 9d5b2fac9ff..b2d5a320fc4 100644 --- a/src/corelib/itemmodels/qsortfilterproxymodel.h +++ b/src/corelib/itemmodels/qsortfilterproxymodel.h @@ -113,6 +113,7 @@ protected: virtual bool filterAcceptsColumn(int source_column, const QModelIndex &source_parent) const; virtual bool lessThan(const QModelIndex &source_left, const QModelIndex &source_right) const; + void beginFilterChange(); void invalidateFilter(); void invalidateRowsFilter(); void invalidateColumnsFilter(); diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp index 0e027461aae..4d21fcbb593 100644 --- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp +++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp @@ -5497,5 +5497,64 @@ void tst_QSortFilterProxyModel::createPersistentOnLayoutAboutToBeChanged() // QT QCOMPARE(layoutChangedSpy.size(), 1); } +void tst_QSortFilterProxyModel::filterChangeEmitsModelChangedSignals() +{ + QStringListModel model({"1", "2", "3", "4", "5"}); + + class FilterModel : public QSortFilterProxyModel + { + QString m_matchString; + public: + void setFilter(const QString &s) + { + if (m_matchString == s) + return; + + beginFilterChange(); + m_matchString = s; + invalidateFilter(); + } + + bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override + { + const auto index = sourceModel()->index(sourceRow, 0, sourceParent); + if (!index.isValid()) + return false; + + return index.data().value() == m_matchString; + } + }; + + FilterModel filterModel; + + // Reject all source data at the start + filterModel.setFilter("X"); + // Trigger an evaluation + filterModel.sort(0, Qt::AscendingOrder); + filterModel.setSourceModel(&model); + QCOMPARE(filterModel.rowCount(), 0); + filterModel.invalidate(); + + QSignalSpy rowsInsertedSpy(&filterModel, &QSortFilterProxyModel::rowsInserted); + QSignalSpy rowsRemovedSpy(&filterModel, &QSortFilterProxyModel::rowsRemoved); + + filterModel.setFilter("3"); + QCOMPARE(filterModel.rowCount(), 1); + QCOMPARE(rowsInsertedSpy.count(), 1); + rowsInsertedSpy.clear(); + + filterModel.setFilter("2"); + QCOMPARE(filterModel.rowCount(), 1); + QCOMPARE(rowsInsertedSpy.count(), 1); + QCOMPARE(rowsRemovedSpy.count(), 1); + rowsInsertedSpy.clear(); + rowsRemovedSpy.clear(); + + filterModel.setFilter("X"); + QCOMPARE(filterModel.rowCount(), 0); + QCOMPARE(rowsInsertedSpy.count(), 0); + QCOMPARE(rowsRemovedSpy.count(), 1); +} + QTEST_MAIN(tst_QSortFilterProxyModel) #include "tst_qsortfilterproxymodel.moc" diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.h b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.h index 088a5b552e1..78d06f32dab 100644 --- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.h +++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.h @@ -153,6 +153,8 @@ private slots: void filterCaseSensitivityBinding(); void filterRegularExpressionBinding(); + void filterChangeEmitsModelChangedSignals(); + protected: void buildHierarchy(const QStringList &data, QAbstractItemModel *model); void checkHierarchy(const QStringList &data, const QAbstractItemModel *model);