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

QSFPM optimization in dataChanged: don't re-sort if the order didn't change

Browse Source 
We can quickly check if the change affects sorting by checking whether
lessThan(N-1, N) and lessThan(N, N+1) are still true. If this is the case
for all changed rows, then we can skip the whole remove+insert+layoutChanged().

Task-number: QTBUG-1548
Change-Id: Ia778b3e8880cc9909eef1f8a016c84235870353d
Reviewed-by: Stephen Kelly <steveire@gmail.com>
This commit is contained in:
David Faure 2017-03-23 12:48:24 +01:00
parent 95d4ac10e9
commit 83038a7acf
2 changed files with 179 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -302,6 +302,8 @@ public:
virtual void _q_sourceModelDestroyed() Q_DECL_OVERRIDE;
bool needsReorder(const QVector<int> &source_rows, const QModelIndex &source_parent) const;
bool filterAcceptsRowInternal(int source_row, const QModelIndex &source_parent) const;
bool filterRecursiveAcceptsRow(int source_row, const QModelIndex &source_parent) const;
};
@ -1197,6 +1199,33 @@ QSet<int> QSortFilterProxyModelPrivate::handle_filter_changed(
return qVectorToSet(source_items_remove);
}
bool QSortFilterProxyModelPrivate::needsReorder(const QVector<int> &source_rows, const QModelIndex &source_parent) const
{
Q_Q(const QSortFilterProxyModel);
Q_ASSERT(source_sort_column != -1);
const int proxyRowCount = q->rowCount(source_to_proxy(source_parent));
// If any modified proxy row no longer passes lessThan(previous, current) or lessThan(current, next) then we need to reorder.
return std::any_of(source_rows.begin(), source_rows.end(),
[this, q, proxyRowCount, source_parent](int sourceRow) -> bool {
const QModelIndex sourceIndex = model->index(sourceRow, source_sort_column, source_parent);
const QModelIndex proxyIndex = source_to_proxy(sourceIndex);
Q_ASSERT(proxyIndex.isValid()); // caller ensured source_rows were not filtered out
if (proxyIndex.row() > 0) {
const QModelIndex prevProxyIndex = q->sibling(proxyIndex.row() - 1, proxy_sort_column, proxyIndex);
const QModelIndex prevSourceIndex = proxy_to_source(prevProxyIndex);
if (sort_order == Qt::AscendingOrder ? q->lessThan(sourceIndex, prevSourceIndex) : q->lessThan(prevSourceIndex, sourceIndex))
return true;
}
if (proxyIndex.row() < proxyRowCount - 1) {
const QModelIndex nextProxyIndex = q->sibling(proxyIndex.row() + 1, proxy_sort_column, proxyIndex);
const QModelIndex nextSourceIndex = proxy_to_source(nextProxyIndex);
if (sort_order == Qt::AscendingOrder ? q->lessThan(nextSourceIndex, sourceIndex) : q->lessThan(sourceIndex, nextSourceIndex))
return true;
}
return false;
});
}
void QSortFilterProxyModelPrivate::_q_sourceDataChanged(const QModelIndex &source_top_left,
const QModelIndex &source_bottom_right,
const QVector<int> &roles)
@ -1277,18 +1306,20 @@ void QSortFilterProxyModelPrivate::_q_sourceDataChanged(const QModelIndex &sourc
}
if (!source_rows_resort.isEmpty()) {
// Re-sort the rows of this level
QList<QPersistentModelIndex> parents;
parents << q->mapFromSource(source_parent);
emit q->layoutAboutToBeChanged(parents, QAbstractItemModel::VerticalSortHint);
QModelIndexPairList source_indexes = store_persistent_indexes();
remove_source_items(m->proxy_rows, m->source_rows, source_rows_resort,
source_parent, Qt::Vertical, false);
sort_source_rows(source_rows_resort, source_parent);
insert_source_items(m->proxy_rows, m->source_rows, source_rows_resort,
source_parent, Qt::Vertical, false);
update_persistent_indexes(source_indexes);
emit q->layoutChanged(parents, QAbstractItemModel::VerticalSortHint);
if (needsReorder(source_rows_resort, source_parent)) {
// Re-sort the rows of this level
QList<QPersistentModelIndex> parents;
parents << q->mapFromSource(source_parent);
emit q->layoutAboutToBeChanged(parents, QAbstractItemModel::VerticalSortHint);
QModelIndexPairList source_indexes = store_persistent_indexes();
remove_source_items(m->proxy_rows, m->source_rows, source_rows_resort,
source_parent, Qt::Vertical, false);
sort_source_rows(source_rows_resort, source_parent);
insert_source_items(m->proxy_rows, m->source_rows, source_rows_resort,
source_parent, Qt::Vertical, false);
update_persistent_indexes(source_indexes);
emit q->layoutChanged(parents, QAbstractItemModel::VerticalSortHint);
}
// Make sure we also emit dataChanged for the rows
source_rows_change += source_rows_resort;
}

139
tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp
View File

@ -148,6 +148,9 @@ private slots:
void sourceLayoutChangeLeavesValidPersistentIndexes();
void rowMoveLeavesValidPersistentIndexes();
void emitLayoutChangedOnlyIfSortingChanged_data();
void emitLayoutChangedOnlyIfSortingChanged();
protected:
void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
void checkHierarchy(const QStringList &data, const QAbstractItemModel *model);
@ -2057,8 +2060,6 @@ static void checkSortedTableModel(const QAbstractItemModel *model, const QString
void tst_QSortFilterProxyModel::changeSourceDataKeepsStableSorting_qtbug1548()
{
QSKIP("This test will fail, see QTBUG-1548");
// Check that emitting dataChanged from the source model
// for a change of a role which is not the sorting role
// doesn't alter the sorting. In this case, we sort on the DisplayRole,
@ -3568,6 +3569,13 @@ void tst_QSortFilterProxyModel::testParentLayoutChanged()
parentItem = item;
}
}
// item 0
// item 10
// - item 1
// - item 11
// - item 2
// - item 12
// ...
QSortFilterProxyModel proxy;
proxy.sort(0, Qt::AscendingOrder);
@ -3609,11 +3617,12 @@ void tst_QSortFilterProxyModel::testParentLayoutChanged()
QVERIFY(proxy2ParentsChangedSpy.isValid());
QStandardItem *item = model.invisibleRootItem()->child(1)->child(1);
QCOMPARE(item->text(), QStringLiteral("item 11"));
// Ensure mapped:
proxy.mapFromSource(model.indexFromItem(item));
item->setData("Changed");
item->setText("Changed");
QCOMPARE(dataChangedSpy.size(), 1);
QCOMPARE(layoutAboutToBeChangedSpy.size(), 1);
@ -4353,5 +4362,129 @@ void tst_QSortFilterProxyModel::rowMoveLeavesValidPersistentIndexes()
QVERIFY(persistentIndex.parent().isValid());
}
void tst_QSortFilterProxyModel::emitLayoutChangedOnlyIfSortingChanged_data()
{
QTest::addColumn<int>("changedRow");
QTest::addColumn<Qt::ItemDataRole>("changedRole");
QTest::addColumn<QString>("newData");
QTest::addColumn<QString>("expectedSourceRowTexts");
QTest::addColumn<QString>("expectedProxyRowTexts");
QTest::addColumn<int>("expectedLayoutChanged");
// Starting point:
// a source model with 8,7,6,5,4,3,2,1
// a proxy model keeping only even rows and sorting them, therefore showing 2,4,6,8
// When setData changes ordering, layoutChanged should be emitted
QTest::newRow("ordering_change") << 0 << Qt::DisplayRole << "0" << "07654321" << "0246" << 1;
// When setData on visible row doesn't change ordering, layoutChanged should not be emitted
QTest::newRow("no_ordering_change") << 6 << Qt::DisplayRole << "0" << "87654301" << "0468" << 0;
// When setData happens on a filtered out row, layoutChanged should not be emitted
QTest::newRow("filtered_out") << 1 << Qt::DisplayRole << "9" << "89654321" << "2468" << 0;
// When setData makes a row visible, layoutChanged should not be emitted (rowsInserted is emitted instead)
QTest::newRow("make_row_visible") << 7 << Qt::DisplayRole << "0" << "87654320" << "02468" << 0;
// When setData makes a row hidden, layoutChanged should not be emitted (rowsRemoved is emitted instead)
QTest::newRow("make_row_hidden") << 4 << Qt::DisplayRole << "1" << "87651321" << "268" << 0;
// When setData happens on an unrelated role, layoutChanged should not be emitted
QTest::newRow("unrelated_role") << 0 << Qt::DecorationRole << "" << "87654321" << "2468" << 0;
// When many changes happen together... and trigger removal, insertion, and layoutChanged
QTest::newRow("many_changes") << -1 << Qt::DisplayRole << "3,4,2,5,6,0,7,9" << "34256079" << "0246" << 1;
// When many changes happen together... and trigger removal, insertion, but no change in ordering of visible rows => no layoutChanged
QTest::newRow("many_changes_no_layoutChanged") << -1 << Qt::DisplayRole << "7,5,4,3,2,1,0,8" << "75432108" << "0248" << 0;
}
void tst_QSortFilterProxyModel::emitLayoutChangedOnlyIfSortingChanged()
{
QFETCH(int, changedRow);
QFETCH(QString, newData);
QFETCH(Qt::ItemDataRole, changedRole);
QFETCH(QString, expectedSourceRowTexts);
QFETCH(QString, expectedProxyRowTexts);
QFETCH(int, expectedLayoutChanged);
// Custom version of QStringListModel which supports emitting dataChanged for many rows at once
class CustomStringListModel : public QAbstractListModel
{
public:
bool setData(const QModelIndex &index, const QVariant &value, int role) override
{
if (index.row() >= 0 && index.row() < lst.size()
&& (role == Qt::EditRole || role == Qt::DisplayRole)) {
lst.replace(index.row(), value.toString());
emit dataChanged(index, index, {Qt::DisplayRole, Qt::EditRole});
return true;
}
return false;
}
QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override
{
if (role == Qt::DisplayRole || role == Qt::EditRole)
return lst.at(index.row());
return QVariant();
}
int rowCount(const QModelIndex & = QModelIndex()) const override
{
return lst.count();
}
void replaceData(const QStringList &newData)
{
lst = newData;
emit dataChanged(index(0, 0), index(rowCount()-1, 0), {Qt::DisplayRole, Qt::EditRole});
}
void emitDecorationChangedSignal()
{
const QModelIndex idx = index(0, 0);
emit dataChanged(idx, idx, {Qt::DecorationRole});
}
private:
QStringList lst;
};
CustomStringListModel model;
QStringList strings;
for (auto i = 8; i >= 1; --i)
strings.append(QString::number(i));
model.replaceData(strings);
QCOMPARE(rowTexts(&model), QStringLiteral("87654321"));
class FilterEvenRowsProxyModel : public QSortFilterProxyModel
{
public:
bool filterAcceptsRow(int srcRow, const QModelIndex& srcParent) const override
{
return sourceModel()->index(srcRow, 0, srcParent).data().toInt() % 2 == 0;
}
};
FilterEvenRowsProxyModel proxy;
proxy.sort(0);
proxy.setSourceModel(&model);
QCOMPARE(rowTexts(&proxy), QStringLiteral("2468"));
QSignalSpy modelDataChangedSpy(&model, &QAbstractItemModel::dataChanged);
QSignalSpy proxyLayoutChangedSpy(&proxy, &QAbstractItemModel::layoutChanged);
if (changedRole == Qt::DecorationRole)
model.emitDecorationChangedSignal();
else if (changedRow == -1)
model.replaceData(newData.split(QLatin1Char(',')));
else
model.setData(model.index(changedRow, 0), newData, changedRole);
QCOMPARE(rowTexts(&model), expectedSourceRowTexts);
QCOMPARE(rowTexts(&proxy), expectedProxyRowTexts);
QCOMPARE(modelDataChangedSpy.size(), 1);
QCOMPARE(proxyLayoutChangedSpy.size(), expectedLayoutChanged);
}
QTEST_MAIN(tst_QSortFilterProxyModel)
#include "tst_qsortfilterproxymodel.moc"