QSFPM: Fix handling of source model layout change

In sourceLayoutAboutToBeChanged the source model update is ignored if
the affected parents are filtered out anyway.  The same logic is
attempted in the sourceLayoutChanged slot, but there the early-return
logic is applied too late - the mapping is cleared before performing the
early-return.  Because pointers into the mapping are used in the
internalPointer of QModelIndexes in this class, persistent indexes used
later will segfault when attempting to dereference it.

Additionally, if a parent becomes invalid as a result of the
layoutChange, it would be filtered out by the condition in the loop,
resulting in a different result in the comparison of emptiness of the
parents container.

Fix that by persisting the parent's container, and performing the test
for early-return before clearing the mapping.

Task-number: QTBUG-47711
Task-number: QTBUG-32981
Change-Id: If45e8a1c97d39454160f52041bc9ae7e337dce97
Reviewed-by: David Faure <david.faure@kdab.com>
This commit is contained in:
Stephen Kelly 2016-12-17 06:20:06 +00:00
parent 1274a7e419
commit 3bd0fd8f97
2 changed files with 137 additions and 20 deletions

View File

@ -171,6 +171,7 @@ public:
QRowsRemoval itemsBeingRemoved; QRowsRemoval itemsBeingRemoved;
QModelIndexPairList saved_persistent_indexes; QModelIndexPairList saved_persistent_indexes;
QList<QPersistentModelIndex> saved_layoutChange_parents;
QHash<QModelIndex, Mapping *>::const_iterator create_mapping( QHash<QModelIndex, Mapping *>::const_iterator create_mapping(
const QModelIndex &source_parent) const; const QModelIndex &source_parent) const;
@ -1331,23 +1332,23 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutAboutToBeChanged(const QList<Q
Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns
saved_persistent_indexes.clear(); saved_persistent_indexes.clear();
QList<QPersistentModelIndex> parents; saved_layoutChange_parents.clear();
for (const QPersistentModelIndex &parent : sourceParents) { for (const QPersistentModelIndex &parent : sourceParents) {
if (!parent.isValid()) { if (!parent.isValid()) {
parents << QPersistentModelIndex(); saved_layoutChange_parents << QPersistentModelIndex();
continue; continue;
} }
const QModelIndex mappedParent = q->mapFromSource(parent); const QModelIndex mappedParent = q->mapFromSource(parent);
// Might be filtered out. // Might be filtered out.
if (mappedParent.isValid()) if (mappedParent.isValid())
parents << mappedParent; saved_layoutChange_parents << mappedParent;
} }
// All parents filtered out. // All parents filtered out.
if (!sourceParents.isEmpty() && parents.isEmpty()) if (!sourceParents.isEmpty() && saved_layoutChange_parents.isEmpty())
return; return;
emit q->layoutAboutToBeChanged(parents); emit q->layoutAboutToBeChanged(saved_layoutChange_parents);
if (persistent.indexes.isEmpty()) if (persistent.indexes.isEmpty())
return; return;
@ -1359,6 +1360,9 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersisten
Q_Q(QSortFilterProxyModel); Q_Q(QSortFilterProxyModel);
Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns Q_UNUSED(hint); // We can't forward Hint because we might filter additional rows or columns
if (!sourceParents.isEmpty() && saved_layoutChange_parents.isEmpty())
return;
// Optimize: We only actually have to clear the mapping related to the contents of // Optimize: We only actually have to clear the mapping related to the contents of
// sourceParents, not everything. // sourceParents, not everything.
qDeleteAll(source_index_mapping); qDeleteAll(source_index_mapping);
@ -1373,21 +1377,8 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersisten
source_index_mapping.clear(); source_index_mapping.clear();
} }
QList<QPersistentModelIndex> parents; emit q->layoutChanged(saved_layoutChange_parents);
for (const QPersistentModelIndex &parent : sourceParents) { saved_layoutChange_parents.clear();
if (!parent.isValid()) {
parents << QPersistentModelIndex();
continue;
}
const QModelIndex mappedParent = q->mapFromSource(parent);
if (mappedParent.isValid())
parents << mappedParent;
}
if (!sourceParents.isEmpty() && parents.isEmpty())
return;
emit q->layoutChanged(parents);
} }
void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeInserted( void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeInserted(

View File

@ -145,6 +145,8 @@ private slots:
void canDropMimeData(); void canDropMimeData();
void filterHint(); void filterHint();
void sourceLayoutChangeLeavesValidPersistentIndexes();
protected: protected:
void buildHierarchy(const QStringList &data, QAbstractItemModel *model); void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
void checkHierarchy(const QStringList &data, const QAbstractItemModel *model); void checkHierarchy(const QStringList &data, const QAbstractItemModel *model);
@ -4181,5 +4183,129 @@ void tst_QSortFilterProxyModel::filterHint()
QAbstractItemModel::NoLayoutChangeHint); QAbstractItemModel::NoLayoutChangeHint);
} }
/**
Creates a model where each item has one child, to a set depth,
and the last item has no children. For a model created with
setDepth(4):
- 1
- - 2
- - - 3
- - - - 4
*/
class StepTreeModel : public QAbstractItemModel
{
Q_OBJECT
public:
StepTreeModel(QObject * parent = 0)
: QAbstractItemModel(parent), m_depth(0) {}
int columnCount(const QModelIndex& = QModelIndex()) const override { return 1; }
int rowCount(const QModelIndex& parent = QModelIndex()) const override
{
quintptr parentId = (parent.isValid()) ? parent.internalId() : 0;
return (parentId < m_depth) ? 1 : 0;
}
QVariant data(const QModelIndex & index, int role = Qt::DisplayRole) const override
{
if (role != Qt::DisplayRole)
return QVariant();
return QString::number(index.internalId());
}
QModelIndex index(int, int, const QModelIndex& parent = QModelIndex()) const override
{
quintptr parentId = (parent.isValid()) ? parent.internalId() : 0;
if (parentId >= m_depth)
return QModelIndex();
return createIndex(0, 0, parentId + 1);
}
QModelIndex parent(const QModelIndex& index) const override
{
if (index.internalId() == 0)
return QModelIndex();
return createIndex(0, 0, index.internalId() - 1);
}
void setDepth(quintptr depth)
{
int parentIdWithLayoutChange = (m_depth < depth) ? m_depth : depth;
QList<QPersistentModelIndex> parentsOfLayoutChange;
parentsOfLayoutChange.push_back(createIndex(0, 0, parentIdWithLayoutChange));
layoutAboutToBeChanged(parentsOfLayoutChange);
auto existing = persistentIndexList();
QList<QModelIndex> updated;
for (auto idx : existing) {
if (indexDepth(idx) <= depth)
updated.push_back(idx);
else
updated.push_back({});
}
m_depth = depth;
changePersistentIndexList(existing, updated);
layoutChanged(parentsOfLayoutChange);
}
private:
static quintptr indexDepth(QModelIndex const& index)
{
return (index.isValid()) ? 1 + indexDepth(index.parent()) : 0;
}
private:
quintptr m_depth;
};
void tst_QSortFilterProxyModel::sourceLayoutChangeLeavesValidPersistentIndexes()
{
StepTreeModel model;
Q_SET_OBJECT_NAME(model);
model.setDepth(4);
QSortFilterProxyModel proxy1;
proxy1.setSourceModel(&model);
Q_SET_OBJECT_NAME(proxy1);
proxy1.setFilterRegExp("1|2");
// The current state of things:
// model proxy
// - 1 - 1
// - - 2 - - 2
// - - - 3
// - - - - 4
// The setDepth call below removes '4' with a layoutChanged call.
// Because the proxy filters that out anyway, the proxy doesn't need
// to emit any signals or update persistent indexes.
QPersistentModelIndex persistentIndex = proxy1.index(0, 0, proxy1.index(0, 0));
model.setDepth(3);
// Calling parent() causes the internalPointer to be used.
// Before fixing QTBUG-47711, that could be a dangling pointer.
// The use of qDebug here makes sufficient use of the heap to
// cause corruption at runtime with normal use on linux (before
// the fix). valgrind confirms the fix.
qDebug() << persistentIndex.parent();
QVERIFY(persistentIndex.parent().isValid());
}
QTEST_MAIN(tst_QSortFilterProxyModel) QTEST_MAIN(tst_QSortFilterProxyModel)
#include "tst_qsortfilterproxymodel.moc" #include "tst_qsortfilterproxymodel.moc"