AuroraMiddleware/qt5base-lts
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

QSortFilterProxyModel: create mappings on demand again

Browse Source 
Calling create_mapping in setSourceModel as introduced by 8455bfee76
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
Pick-to: 6.1 6.0 5.15
Change-Id: Id543d0cc98f1a03b5852bda01d2f49b980e06be7
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
This commit is contained in:
David Faure 2021-05-17 18:37:58 +02:00
parent 05a04c80e0
commit 4ec5622e62
3 changed files with 79 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
src/corelib/itemmodels/qsortfilterproxymodel.cpp
View File

@ -276,6 +276,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;
@ -550,6 +552,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())
@ -765,8 +790,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);
@ -1423,11 +1450,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
@ -1437,7 +1473,7 @@ void QSortFilterProxyModelPrivate::_q_sourceDataChanged(const QModelIndex &sourc
QList<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
@ -1587,7 +1623,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();
@ -1653,8 +1688,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);
@ -1773,8 +1808,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);
}
@ -2142,7 +2177,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();
}

34
tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.cpp
View File

@ -2196,6 +2196,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 };

1
tests/auto/corelib/itemmodels/qsortfilterproxymodel_common/tst_qsortfilterproxymodel.h
View File

@ -91,6 +91,7 @@ private slots:
void changeSourceDataKeepsStableSorting_qtbug1548();
void changeSourceDataForwardsRoles_qtbug35440();
void changeSourceDataProxySendDataChanged_qtbug87781();
void changeSourceDataTreeModel();
void changeSourceDataProxyFilterSingleColumn();
void changeSourceDataProxyFilterMultipleColumns();
void resortingDoesNotBreakTreeModels();