192 lines
7.6 KiB
Diff
192 lines
7.6 KiB
Diff
From 0874861bcc70313c343aba5e5566ed30b69eed1c Mon Sep 17 00:00:00 2001
|
|
From: Stephen Kelly <steveire@gmail.com>
|
|
Date: Mon, 19 Dec 2016 21:13:57 +0000
|
|
Subject: [PATCH] QSFPM: Remove data manipulation from move handlers
|
|
|
|
Similar to the fix in the parent commit, incorrect updating of the
|
|
internal data structures during layout changes can lead to dangling
|
|
pointers being dereferenced later. Moves are treated as layoutChanges
|
|
by this proxy by forwarding to the appropriate method. However, data is
|
|
incorrectly cleared prior to that forwarding. Remove that, and let the
|
|
layoutChange handling take appropriate action.
|
|
|
|
Change-Id: Iee951e37152328a4e6a5fb8e5385c32a2fe4c0bd
|
|
Reviewed-by: David Faure <david.faure@kdab.com>
|
|
---
|
|
src/corelib/itemmodels/qsortfilterproxymodel.cpp | 67 ++++------------------
|
|
.../tst_qsortfilterproxymodel.cpp | 46 +++++++++++++++
|
|
2 files changed, 58 insertions(+), 55 deletions(-)
|
|
|
|
diff --git a/src/corelib/itemmodels/qsortfilterproxymodel.cpp b/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
index 3331521..226a240 100644
|
|
--- qtbase/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
+++ qtbase/src/corelib/itemmodels/qsortfilterproxymodel.cpp
|
|
@@ -1418,49 +1418,27 @@ void QSortFilterProxyModelPrivate::_q_sourceRowsRemoved(
|
|
void QSortFilterProxyModelPrivate::_q_sourceRowsAboutToBeMoved(
|
|
const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
|
|
{
|
|
- Q_Q(QSortFilterProxyModel);
|
|
// Because rows which are contiguous in the source model might not be contiguous
|
|
// in the proxy due to sorting, the best thing we can do here is be specific about what
|
|
// parents are having their children changed.
|
|
// Optimize: Emit move signals if the proxy is not sorted. Will need to account for rows
|
|
// being filtered out though.
|
|
|
|
- saved_persistent_indexes.clear();
|
|
-
|
|
QList<QPersistentModelIndex> parents;
|
|
- parents << q->mapFromSource(sourceParent);
|
|
+ parents << sourceParent;
|
|
if (sourceParent != destParent)
|
|
- parents << q->mapFromSource(destParent);
|
|
- emit q->layoutAboutToBeChanged(parents);
|
|
- if (persistent.indexes.isEmpty())
|
|
- return;
|
|
- saved_persistent_indexes = store_persistent_indexes();
|
|
+ parents << destParent;
|
|
+ _q_sourceLayoutAboutToBeChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
|
|
}
|
|
|
|
void QSortFilterProxyModelPrivate::_q_sourceRowsMoved(
|
|
const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
|
|
{
|
|
- Q_Q(QSortFilterProxyModel);
|
|
-
|
|
- // Optimize: We only need to clear and update the persistent indexes which are children of
|
|
- // sourceParent or destParent
|
|
- qDeleteAll(source_index_mapping);
|
|
- source_index_mapping.clear();
|
|
-
|
|
- update_persistent_indexes(saved_persistent_indexes);
|
|
- saved_persistent_indexes.clear();
|
|
-
|
|
- if (dynamic_sortfilter && update_source_sort_column()) {
|
|
- //update_source_sort_column might have created wrong mapping so we have to clear it again
|
|
- qDeleteAll(source_index_mapping);
|
|
- source_index_mapping.clear();
|
|
- }
|
|
-
|
|
QList<QPersistentModelIndex> parents;
|
|
- parents << q->mapFromSource(sourceParent);
|
|
+ parents << sourceParent;
|
|
if (sourceParent != destParent)
|
|
- parents << q->mapFromSource(destParent);
|
|
- emit q->layoutChanged(parents);
|
|
+ parents << destParent;
|
|
+ _q_sourceLayoutChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
|
|
}
|
|
|
|
void QSortFilterProxyModelPrivate::_q_sourceColumnsAboutToBeInserted(
|
|
@@ -1522,42 +1500,21 @@ void QSortFilterProxyModelPrivate::_q_sourceColumnsRemoved(
|
|
void QSortFilterProxyModelPrivate::_q_sourceColumnsAboutToBeMoved(
|
|
const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
|
|
{
|
|
- Q_Q(QSortFilterProxyModel);
|
|
-
|
|
- saved_persistent_indexes.clear();
|
|
-
|
|
QList<QPersistentModelIndex> parents;
|
|
- parents << q->mapFromSource(sourceParent);
|
|
+ parents << sourceParent;
|
|
if (sourceParent != destParent)
|
|
- parents << q->mapFromSource(destParent);
|
|
- emit q->layoutAboutToBeChanged(parents);
|
|
-
|
|
- if (persistent.indexes.isEmpty())
|
|
- return;
|
|
- saved_persistent_indexes = store_persistent_indexes();
|
|
+ parents << destParent;
|
|
+ _q_sourceLayoutAboutToBeChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
|
|
}
|
|
|
|
void QSortFilterProxyModelPrivate::_q_sourceColumnsMoved(
|
|
const QModelIndex &sourceParent, int /* sourceStart */, int /* sourceEnd */, const QModelIndex &destParent, int /* dest */)
|
|
{
|
|
- Q_Q(QSortFilterProxyModel);
|
|
-
|
|
- qDeleteAll(source_index_mapping);
|
|
- source_index_mapping.clear();
|
|
-
|
|
- update_persistent_indexes(saved_persistent_indexes);
|
|
- saved_persistent_indexes.clear();
|
|
-
|
|
- if (dynamic_sortfilter && update_source_sort_column()) {
|
|
- qDeleteAll(source_index_mapping);
|
|
- source_index_mapping.clear();
|
|
- }
|
|
-
|
|
QList<QPersistentModelIndex> parents;
|
|
- parents << q->mapFromSource(sourceParent);
|
|
+ parents << sourceParent;
|
|
if (sourceParent != destParent)
|
|
- parents << q->mapFromSource(destParent);
|
|
- emit q->layoutChanged(parents);
|
|
+ parents << destParent;
|
|
+ _q_sourceLayoutChanged(parents, QAbstractItemModel::NoLayoutChangeHint);
|
|
}
|
|
|
|
/*!
|
|
diff --git a/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp b/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
|
|
index 6b98d9f..7b6c470 100644
|
|
--- qtbase/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
|
|
+++ qtbase/tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
|
|
@@ -146,6 +146,7 @@ private slots:
|
|
void filterHint();
|
|
|
|
void sourceLayoutChangeLeavesValidPersistentIndexes();
|
|
+ void rowMoveLeavesValidPersistentIndexes();
|
|
|
|
protected:
|
|
void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
|
|
@@ -4307,5 +4308,50 @@ void tst_QSortFilterProxyModel::sourceLayoutChangeLeavesValidPersistentIndexes()
|
|
QVERIFY(persistentIndex.parent().isValid());
|
|
}
|
|
|
|
+void tst_QSortFilterProxyModel::rowMoveLeavesValidPersistentIndexes()
|
|
+{
|
|
+ DynamicTreeModel model;
|
|
+ Q_SET_OBJECT_NAME(model);
|
|
+
|
|
+ QList<int> ancestors;
|
|
+ for (auto i = 0; i < 5; ++i)
|
|
+ {
|
|
+ Q_UNUSED(i);
|
|
+ ModelInsertCommand insertCommand(&model);
|
|
+ insertCommand.setAncestorRowNumbers(ancestors);
|
|
+ insertCommand.setStartRow(0);
|
|
+ insertCommand.setEndRow(0);
|
|
+ insertCommand.doCommand();
|
|
+ ancestors.push_back(0);
|
|
+ }
|
|
+
|
|
+ QSortFilterProxyModel proxy1;
|
|
+ proxy1.setSourceModel(&model);
|
|
+ Q_SET_OBJECT_NAME(proxy1);
|
|
+
|
|
+ proxy1.setFilterRegExp("1|2");
|
|
+
|
|
+ auto item5 = model.match(model.index(0, 0), Qt::DisplayRole, "5", 1, Qt::MatchRecursive).first();
|
|
+ auto item3 = model.match(model.index(0, 0), Qt::DisplayRole, "3", 1, Qt::MatchRecursive).first();
|
|
+
|
|
+ Q_ASSERT(item5.isValid());
|
|
+ Q_ASSERT(item3.isValid());
|
|
+
|
|
+ QPersistentModelIndex persistentIndex = proxy1.match(proxy1.index(0, 0), Qt::DisplayRole, "2", 1, Qt::MatchRecursive).first();
|
|
+
|
|
+ ModelMoveCommand moveCommand(&model, 0);
|
|
+ moveCommand.setAncestorRowNumbers(QList<int>{0, 0, 0, 0});
|
|
+ moveCommand.setStartRow(0);
|
|
+ moveCommand.setEndRow(0);
|
|
+ moveCommand.setDestRow(0);
|
|
+ moveCommand.setDestAncestors(QList<int>{0, 0, 0});
|
|
+ moveCommand.doCommand();
|
|
+
|
|
+ // Calling parent() causes the internalPointer to be used.
|
|
+ // Before fixing QTBUG-47711 (moveRows case), that could be
|
|
+ // a dangling pointer.
|
|
+ QVERIFY(persistentIndex.parent().isValid());
|
|
+}
|
|
+
|
|
QTEST_MAIN(tst_QSortFilterProxyModel)
|
|
#include "tst_qsortfilterproxymodel.moc"
|