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>

3 files changed, 112 insertions, 9 deletions

diff --git a/src/corelib/itemmodels/qabstractproxymodel.cpp b/src/corelib/itemmodels/qabstractproxymodel.cpp
index a1622dbfe1..aec5f52e6b 100644
--- a/src/corelib/itemmodels/qabstractproxymodel.cpp
+++ b/src/corelib/itemmodels/qabstractproxymodel.cpp
@@ -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);
 }
diff --git a/src/corelib/itemmodels/qabstractproxymodel.h b/src/corelib/itemmodels/qabstractproxymodel.h
index bb8c5400dc..c9c4eed3ec 100644
--- a/src/corelib/itemmodels/qabstractproxymodel.h
+++ b/src/corelib/itemmodels/qabstractproxymodel.h
@@ -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
diff --git a/src/corelib/itemmodels/qabstractproxymodel_p.h b/src/corelib/itemmodels/qabstractproxymodel_p.h
index 2ddd2428dc..f6e3fdbd39 100644
--- a/src/corelib/itemmodels/qabstractproxymodel_p.h
+++ b/src/corelib/itemmodels/qabstractproxymodel_p.h
@@ -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