You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
253 lines
8.7 KiB
253 lines
8.7 KiB
From 3bd0fd8f97e7a33a874929a383a42e6c710bfff3 Mon Sep 17 00:00:00 2001
|
|
From: Stephen Kelly <steveire@gmail.com>
|
|
Date: Sat, 17 Dec 2016 06:20:06 +0000
|
|
Subject: [PATCH] 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>
|
|
---
|
|
src/corelib/itemmodels/qsortfilterproxymodel.cpp | 31 ++---
|
|
.../tst_qsortfilterproxymodel.cpp | 126 +++++++++++++++++++++
|
|
2 files changed, 137 insertions(+), 20 deletions(-)
|
|
|
|
diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
index b0ddfa8..3331521 100644
|
|
--- a/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
+++ b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
@@ -171,6 +171,7 @@ public:
|
|
QRowsRemoval itemsBeingRemoved;
|
|
|
|
QModelIndexPairList saved_persistent_indexes;
|
|
+ QList<QPersistentModelIndex> saved_layoutChange_parents;
|
|
|
|
QHash<QModelIndex, Mapping *>::const_iterator create_mapping(
|
|
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
|
|
saved_persistent_indexes.clear();
|
|
|
|
- QList<QPersistentModelIndex> parents;
|
|
+ saved_layoutChange_parents.clear();
|
|
for (const QPersistentModelIndex &parent : sourceParents) {
|
|
if (!parent.isValid()) {
|
|
- parents << QPersistentModelIndex();
|
|
+ saved_layoutChange_parents << QPersistentModelIndex();
|
|
continue;
|
|
}
|
|
const QModelIndex mappedParent = q->mapFromSource(parent);
|
|
// Might be filtered out.
|
|
if (mappedParent.isValid())
|
|
- parents << mappedParent;
|
|
+ saved_layoutChange_parents << mappedParent;
|
|
}
|
|
|
|
// All parents filtered out.
|
|
- if (!sourceParents.isEmpty() && parents.isEmpty())
|
|
+ if (!sourceParents.isEmpty() && saved_layoutChange_parents.isEmpty())
|
|
return;
|
|
|
|
- emit q->layoutAboutToBeChanged(parents);
|
|
+ emit q->layoutAboutToBeChanged(saved_layoutChange_parents);
|
|
if (persistent.indexes.isEmpty())
|
|
return;
|
|
|
|
@@ -1359,6 +1360,9 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersisten
|
|
Q_Q(QSortFilterProxyModel);
|
|
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
|
|
// sourceParents, not everything.
|
|
qDeleteAll(source_index_mapping);
|
|
@@ -1373,21 +1377,8 @@ void QSortFilterProxyModelPrivate::_q_sourceLayoutChanged(const QList<QPersisten
|
|
source_index_mapping.clear();
|
|
}
|
|
|
|
- QList<QPersistentModelIndex> parents;
|
|
- for (const QPersistentModelIndex &parent : sourceParents) {
|
|
- 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);
|
|
+ emit q->layoutChanged(saved_layoutChange_parents);
|
|
+ saved_layoutChange_parents.clear();
|
|
}
|
|
|
|
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 38e3c68..6b98d9f 100644
|
|
--- a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
|
|
+++ b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
|
|
@@ -145,6 +145,8 @@ private slots:
|
|
void canDropMimeData();
|
|
void filterHint();
|
|
|
|
+ void sourceLayoutChangeLeavesValidPersistentIndexes();
|
|
+
|
|
protected:
|
|
void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
|
|
void checkHierarchy(const QStringList &data, const QAbstractItemModel *model);
|
|
@@ -4181,5 +4183,129 @@ void tst_QSortFilterProxyModel::filterHint()
|
|
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)
|
|
#include "tst_qsortfilterproxymodel.moc"
|
|
--
|
|
2.7.4
|
|
|
|
|