1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QAbstractProxyModel: do not access invalid indexes

Browse Source 
QAbstractProxyModel::headerData tries to do the "smart" thing and
map sections in the proxy to sections in the source. However there's
no "mapSectionToSource" virtual. Instead, to map horizontal headers, the
code builds a proxy index at row 0 and section N, maps it to the source,
and finds out which source column it gets mapped to. (Same story
for the vertical headers).

... in general this can obviously fail, say you've got a "horizontal
scrambling" proxy model, but in the common case this is OK.

Except, if the proxy is empty (e.g. 0 rows or columns). In this case,
it asks for an illegal index, and if you reimplemented index() yourself
(which you must, since it's a pure virtual in QAPM) and you do bounds
checking, you'll not be pleased at the result.

This turns out to be a massive API liability. To fix this somehow properly,
we can decide that empty models don't get the section remapped (easy).
Less easy is the fact that, when the model does get some data, we have to
emit headerDataChanged() otherwise the views will get broken. So add
this logic too.

Note that QAPM does not normally forward any source model's signal -- a
subclass has to connect to them and handle them explicitly. That's
*another* API liability, all over the place -- data(), headerData(),
flags(), etc.

What I mean by this is that one can create a valid QAPM (by implementing
its pure virtuals) that however is immediately broken by the convenience
that QAPM provides for the rest (data(), headerData(), etc.).

This commit doesn't try and change this in any way, but I'm less and
less convinced of the usefulness of QAPM in its current shape.

Change-Id: I45a8c2139f2c1917ffbf429910fdb92f005f4feb
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: David Faure <david.faure@kdab.com>
This commit is contained in:
Giuseppe D'Angelo 2022-03-02 17:45:20 +01:00
parent fd8441a97c
commit 33c88f86b5
4 changed files with 240 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
src/corelib/itemmodels/qabstractproxymodel.cpp
View File

@ -91,6 +91,69 @@ void QAbstractProxyModelPrivate::_q_sourceModelDestroyed()
model = QAbstractItemModelPrivate::staticEmptyModel();
}
void QAbstractProxyModelPrivate::_q_sourceModelRowsAboutToBeInserted(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
sourceHadZeroRows = model->rowCount() == 0;
}
void QAbstractProxyModelPrivate::_q_sourceModelRowsInserted(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
if (sourceHadZeroRows) {
Q_Q(QAbstractProxyModel);
const int columnCount = q->columnCount();
if (columnCount > 0)
emit q->headerDataChanged(Qt::Horizontal, 0, columnCount - 1);
}
}
void QAbstractProxyModelPrivate::_q_sourceModelRowsRemoved(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
if (model->rowCount() == 0) {
Q_Q(QAbstractProxyModel);
const int columnCount = q->columnCount();
if (columnCount > 0)
emit q->headerDataChanged(Qt::Horizontal, 0, columnCount - 1);
}
}
void QAbstractProxyModelPrivate::_q_sourceModelColumnsAboutToBeInserted(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
sourceHadZeroColumns = model->columnCount() == 0;
}
void QAbstractProxyModelPrivate::_q_sourceModelColumnsInserted(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
if (sourceHadZeroColumns) {
Q_Q(QAbstractProxyModel);
const int rowCount = q->rowCount();
if (rowCount > 0)
emit q->headerDataChanged(Qt::Vertical, 0, rowCount - 1);
}
}
void QAbstractProxyModelPrivate::_q_sourceModelColumnsRemoved(const QModelIndex &parent, int, int)
{
if (parent.isValid())
return;
if (model->columnCount() == 0) {
Q_Q(QAbstractProxyModel);
const int rowCount = q->rowCount();
if (rowCount > 0)
emit q->headerDataChanged(Qt::Vertical, 0, rowCount - 1);
}
}
/*!
Constructs a proxy model with the given \a parent.
*/
@ -134,13 +197,29 @@ void QAbstractProxyModel::setSourceModel(QAbstractItemModel *sourceModel)
// notifications.
if (!sourceModel && d->model == QAbstractItemModelPrivate::staticEmptyModel())
return;
static const struct {
const char *signalName;
const char *slotName;
} connectionTable[] = {
{ SIGNAL(destroyed()), SLOT(_q_sourceModelDestroyed()) },
{ SIGNAL(rowsAboutToBeInserted(QModelIndex, int, int)), SLOT(_q_sourceModelRowsAboutToBeInserted(QModelIndex, int, int)) },
{ SIGNAL(rowsInserted(QModelIndex, int, int)), SLOT(_q_sourceModelRowsInserted(QModelIndex, int, int)) },
{ SIGNAL(rowsRemoved(QModelIndex, int, int)), SLOT(_q_sourceModelRowsRemoved(QModelIndex, int, int)) },
{ SIGNAL(columnsAboutToBeInserted(QModelIndex, int, int)), SLOT(_q_sourceModelColumnsAboutToBeInserted(QModelIndex, int, int)) },
{ SIGNAL(columnsInserted(QModelIndex, int, int)), SLOT(_q_sourceModelColumnsInserted(QModelIndex, int, int)) },
{ SIGNAL(columnsRemoved(QModelIndex, int, int)), SLOT(_q_sourceModelColumnsRemoved(QModelIndex, int, int)) }
};
if (sourceModel != d->model) {
if (d->model)
disconnect(d->model, SIGNAL(destroyed()), this, SLOT(_q_sourceModelDestroyed()));
if (d->model) {
for (const auto &c : connectionTable)
disconnect(d->model, c.signalName, this, c.slotName);
}
if (sourceModel) {
d->model.setValueBypassingBindings(sourceModel);
connect(d->model, SIGNAL(destroyed()), this, SLOT(_q_sourceModelDestroyed()));
for (const auto &c : connectionTable)
connect(d->model, c.signalName, this, c.slotName);
} else {
d->model.setValueBypassingBindings(QAbstractItemModelPrivate::staticEmptyModel());
}
@ -253,13 +332,17 @@ QVariant QAbstractProxyModel::data(const QModelIndex &proxyIndex, int role) cons
QVariant QAbstractProxyModel::headerData(int section, Qt::Orientation orientation, int role) const
{
Q_D(const QAbstractProxyModel);
int sourceSection;
int sourceSection = section;
if (orientation == Qt::Horizontal) {
const QModelIndex proxyIndex = index(0, section);
sourceSection = mapToSource(proxyIndex).column();
if (rowCount() > 0) {
const QModelIndex proxyIndex = index(0, section);
sourceSection = mapToSource(proxyIndex).column();
}
} else {
const QModelIndex proxyIndex = index(section, 0);
sourceSection = mapToSource(proxyIndex).row();
if (columnCount() > 0) {
const QModelIndex proxyIndex = index(section, 0);
sourceSection = mapToSource(proxyIndex).row();
}
}
return d->model->headerData(sourceSection, orientation, role);
}

6
src/corelib/itemmodels/qabstractproxymodel.h
View File

@ -111,6 +111,12 @@ private:
Q_DECLARE_PRIVATE(QAbstractProxyModel)
Q_DISABLE_COPY(QAbstractProxyModel)
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelDestroyed())
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelRowsAboutToBeInserted(QModelIndex, int, int))
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelRowsInserted(QModelIndex, int, int))
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelRowsRemoved(QModelIndex, int, int))
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelColumnsAboutToBeInserted(QModelIndex, int, int))
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelColumnsInserted(QModelIndex, int, int))
Q_PRIVATE_SLOT(d_func(), void _q_sourceModelColumnsRemoved(QModelIndex, int, int))
};
QT_END_NAMESPACE

16
src/corelib/itemmodels/qabstractproxymodel_p.h
View File

@ -63,7 +63,11 @@ class Q_CORE_EXPORT QAbstractProxyModelPrivate : public QAbstractItemModelPrivat
{
Q_DECLARE_PUBLIC(QAbstractProxyModel)
public:
QAbstractProxyModelPrivate() : QAbstractItemModelPrivate() { }
QAbstractProxyModelPrivate()
: QAbstractItemModelPrivate(),
sourceHadZeroRows(false),
sourceHadZeroColumns(false)
{}
void setModelForwarder(QAbstractItemModel *sourceModel)
{
q_func()->setSourceModel(sourceModel);
@ -79,8 +83,18 @@ public:
&QAbstractProxyModelPrivate::modelChangedForwarder,
&QAbstractProxyModelPrivate::getModelForwarder, nullptr)
virtual void _q_sourceModelDestroyed();
void _q_sourceModelRowsAboutToBeInserted(const QModelIndex &parent, int first, int last);
void _q_sourceModelRowsInserted(const QModelIndex &parent, int first, int last);
void _q_sourceModelRowsRemoved(const QModelIndex &parent, int first, int last);
void _q_sourceModelColumnsAboutToBeInserted(const QModelIndex &parent, int first, int last);
void _q_sourceModelColumnsInserted(const QModelIndex &parent, int first, int last);
void _q_sourceModelColumnsRemoved(const QModelIndex &parent, int first, int last);
void mapDropCoordinatesToSource(int row, int column, const QModelIndex &parent,
int *source_row, int *source_column, QModelIndex *source_parent) const;
unsigned int sourceHadZeroRows : 1;
unsigned int sourceHadZeroColumns : 1;
};
QT_END_NAMESPACE

128
tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp
View File

@ -44,6 +44,7 @@ private slots:
void flags();
void headerData_data();
void headerData();
void headerDataInBounds();
void itemData_data();
void itemData();
void mapFromSource_data();
@ -174,6 +175,133 @@ void tst_QAbstractProxyModel::headerData()
QCOMPARE(model.headerData(section, orientation, role), headerData);
}
class SimpleTableReverseColumnsProxy : public QAbstractProxyModel
{
public:
QModelIndex index(int row, int column, const QModelIndex &parent = QModelIndex()) const override
{
if (parent.isValid())
return {};
if (row < 0 || row >= rowCount() || column < 0 || column >= columnCount())
qFatal("error"); // cannot QFAIL here
return createIndex(row, column);
}
int rowCount(const QModelIndex &parent = QModelIndex()) const override
{
if (parent.isValid())
return 0;
return sourceModel()->rowCount();
}
int columnCount(const QModelIndex &parent = QModelIndex()) const override
{
if (parent.isValid())
return 0;
return sourceModel()->columnCount();
}
QModelIndex parent(const QModelIndex &) const override
{
return QModelIndex();
}
QModelIndex mapToSource(const QModelIndex &idx) const override
{
if (!idx.isValid())
return QModelIndex();
return sourceModel()->index(idx.row(), columnCount() - 1 - idx.column());
}
QModelIndex mapFromSource(const QModelIndex &idx) const override
{
if (idx.parent().isValid())
return QModelIndex();
return createIndex(idx.row(), columnCount() - 1 - idx.column());
}
};
void tst_QAbstractProxyModel::headerDataInBounds()
{
QStandardItemModel qsim(0, 5);
qsim.setHorizontalHeaderLabels({"Col1", "Col2", "Col3", "Col4", "Col5"});
SimpleTableReverseColumnsProxy proxy;
QSignalSpy headerDataChangedSpy(&proxy, &QAbstractItemModel::headerDataChanged);
QVERIFY(headerDataChangedSpy.isValid());
proxy.setSourceModel(&qsim);
QCOMPARE(proxy.rowCount(), 0);
QCOMPARE(proxy.columnCount(), 5);
for (int i = 0; i < proxy.columnCount(); ++i) {
QString expected = QString("Col%1").arg(i + 1);
QCOMPARE(proxy.headerData(i, Qt::Horizontal).toString(), expected);
}
qsim.appendRow({
new QStandardItem("A"),
new QStandardItem("B"),
new QStandardItem("C"),
new QStandardItem("D"),
new QStandardItem("E")
});
QCOMPARE(proxy.rowCount(), 1);
QCOMPARE(proxy.columnCount(), 5);
QCOMPARE(headerDataChangedSpy.count(), 1);
QCOMPARE(headerDataChangedSpy[0][0].value<Qt::Orientation>(), Qt::Horizontal);
QCOMPARE(headerDataChangedSpy[0][1].value<int>(), 0);
QCOMPARE(headerDataChangedSpy[0][2].value<int>(), 4);
for (int i = 0; i < proxy.columnCount(); ++i) {
QString expected = QString("Col%1").arg(proxy.columnCount() - i);
QCOMPARE(proxy.headerData(i, Qt::Horizontal).toString(), expected);
}
qsim.appendRow({
new QStandardItem("A"),
new QStandardItem("B"),
new QStandardItem("C"),
new QStandardItem("D"),
new QStandardItem("E")
});
QCOMPARE(proxy.rowCount(), 2);
QCOMPARE(proxy.columnCount(), 5);
QCOMPARE(headerDataChangedSpy.count(), 1);
for (int i = 0; i < proxy.columnCount(); ++i) {
QString expected = QString("Col%1").arg(proxy.columnCount() - i);
QCOMPARE(proxy.headerData(i, Qt::Horizontal).toString(), expected);
}
QVERIFY(qsim.removeRows(0, 1));
QCOMPARE(proxy.rowCount(), 1);
QCOMPARE(proxy.columnCount(), 5);
QCOMPARE(headerDataChangedSpy.count(), 1);
for (int i = 0; i < proxy.columnCount(); ++i) {
QString expected = QString("Col%1").arg(proxy.columnCount() - i);
QCOMPARE(proxy.headerData(i, Qt::Horizontal).toString(), expected);
}
QVERIFY(qsim.removeRows(0, 1));
QCOMPARE(proxy.rowCount(), 0);
QCOMPARE(proxy.columnCount(), 5);
QCOMPARE(headerDataChangedSpy.count(), 2);
QCOMPARE(headerDataChangedSpy[1][0].value<Qt::Orientation>(), Qt::Horizontal);
QCOMPARE(headerDataChangedSpy[1][1].value<int>(), 0);
QCOMPARE(headerDataChangedSpy[1][2].value<int>(), 4);
for (int i = 0; i < proxy.columnCount(); ++i) {
QString expected = QString("Col%1").arg(i + 1);
QCOMPARE(proxy.headerData(i, Qt::Horizontal).toString(), expected);
}
}
void tst_QAbstractProxyModel::itemData_data()
{
QTest::addColumn<QModelIndex>("index");