QAbstractProxyModel: do not access invalid indexes

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>

1 files changed, 128 insertions, 0 deletions

diff --git a/tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp b/tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp
index dced357d61..212a79d490 100644
--- a/tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp
+++ b/tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp
@@ -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");