QItemSelectionModelPrivate: use QObjectPrivate::connect

QItemSelectionModelPrivate::initModel() uses string based connections,
to connect/disconnet its QAbstractItemModel.
The QObject::destroyed signal is connected to modelDestroyed(), which
does not disconnect other signals.

QQuickTableView's selection model binds to its QAbstractItemModel.
The binding also reacts to QObject::destroyed
Eventually, QItemSelectionModel::setModel(nullptr) is called.
At this point, only a QOBject is left from the QAbstractItemModel.
That leads to warnings about disconnecting string based signals, which
belong to QAbstractItemModel.

This patch changes the connect syntax to the QObjectPrivate::connect
API. Instead of keeping a list of string based connections around, the
connections themselves are kept in a list member. Disconnecting happens
based on that list.
Connections are also disconnected in
QAbstractItemModelPrivate::modelDestroyed.

An auto test is added in tst_QItemSelectionModel.

Fixes: QTBUG-116056
Pick-to: 6.6 6.5 6.2
Change-Id: I57e5c0f0a574f154eb312a282003774dd0613dd6
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

src/corelib/itemmodels/qitemselectionmodel.cpp

@@ -550,52 +550,55 @@ void QItemSelection::split(const QItemSelectionRange &range,
 
 void QItemSelectionModelPrivate::initModel(QAbstractItemModel *m)
 {
-    static constexpr auto connections = qOffsetStringArray(
+    Q_Q(QItemSelectionModel);
-        QT_STRINGIFY_SIGNAL(rowsAboutToBeRemoved(QModelIndex,int,int)),
-        QT_STRINGIFY_SLOT(_q_rowsAboutToBeRemoved(QModelIndex,int,int)),
-        QT_STRINGIFY_SIGNAL(columnsAboutToBeRemoved(QModelIndex,int,int)),
-        QT_STRINGIFY_SLOT(_q_columnsAboutToBeRemoved(QModelIndex,int,int)),
-        QT_STRINGIFY_SIGNAL(rowsAboutToBeInserted(QModelIndex,int,int)),
-        QT_STRINGIFY_SLOT(_q_rowsAboutToBeInserted(QModelIndex,int,int)),
-        QT_STRINGIFY_SIGNAL(columnsAboutToBeInserted(QModelIndex,int,int)),
-        QT_STRINGIFY_SLOT(_q_columnsAboutToBeInserted(QModelIndex,int,int)),
-        QT_STRINGIFY_SIGNAL(rowsAboutToBeMoved(QModelIndex,int,int,QModelIndex,int)),
-        QT_STRINGIFY_SLOT(_q_layoutAboutToBeChanged()),
-        QT_STRINGIFY_SIGNAL(columnsAboutToBeMoved(QModelIndex,int,int,QModelIndex,int)),
-        QT_STRINGIFY_SLOT(_q_layoutAboutToBeChanged()),
-        QT_STRINGIFY_SIGNAL(rowsMoved(QModelIndex,int,int,QModelIndex,int)),
-        QT_STRINGIFY_SLOT(_q_layoutChanged()),
-        QT_STRINGIFY_SIGNAL(columnsMoved(QModelIndex,int,int,QModelIndex,int)),
-        QT_STRINGIFY_SLOT(_q_layoutChanged()),
-        QT_STRINGIFY_SIGNAL(layoutAboutToBeChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
-        QT_STRINGIFY_SLOT(_q_layoutAboutToBeChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
-        QT_STRINGIFY_SIGNAL(layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
-        QT_STRINGIFY_SLOT(_q_layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
-        QT_STRINGIFY_SIGNAL(modelReset()),
-        QT_STRINGIFY_SLOT(reset()),
-        QT_STRINGIFY_SIGNAL(destroyed(QObject*)),
-        QT_STRINGIFY_SLOT(_q_modelDestroyed())
-    );
-
     if (model == m)
         return;
 
-    Q_Q(QItemSelectionModel);
+    if (model)
-    if (model.value()) {
+        disconnectModel();
-        for (int i = 0; i < connections.count(); i += 2)
-            QObject::disconnect(model.value(), connections.at(i), q, connections.at(i + 1));
-        q->reset();
-    }
 
     // Caller has to call notify(), unless calling during construction (the common case).
     model.setValueBypassingBindings(m);
 
     if (model.value()) {
-        for (int i = 0; i < connections.count(); i += 2)
+        connections = std::array<QMetaObject::Connection, 12> {
-            QObject::connect(model.value(), connections.at(i), q, connections.at(i + 1));
+        QObjectPrivate::connect(model, &QAbstractItemModel::rowsAboutToBeRemoved,
+                                this, &QItemSelectionModelPrivate::rowsAboutToBeRemoved),
+        QObjectPrivate::connect(model, &QAbstractItemModel::columnsAboutToBeRemoved,
+                                this, &QItemSelectionModelPrivate::columnsAboutToBeRemoved),
+        QObjectPrivate::connect(model, &QAbstractItemModel::rowsAboutToBeInserted,
+                         this, &QItemSelectionModelPrivate::rowsAboutToBeInserted),
+        QObjectPrivate::connect(model, &QAbstractItemModel::columnsAboutToBeInserted,
+                         this, &QItemSelectionModelPrivate::columnsAboutToBeInserted),
+        QObjectPrivate::connect(model, &QAbstractItemModel::rowsAboutToBeMoved,
+                         this, &QItemSelectionModelPrivate::triggerLayoutToBeChanged),
+        QObjectPrivate::connect(model, &QAbstractItemModel::columnsAboutToBeMoved,
+                         this, &QItemSelectionModelPrivate::triggerLayoutToBeChanged),
+        QObjectPrivate::connect(model, &QAbstractItemModel::rowsMoved,
+                         this, &QItemSelectionModelPrivate::triggerLayoutChanged),
+        QObjectPrivate::connect(model, &QAbstractItemModel::columnsMoved,
+                         this, &QItemSelectionModelPrivate::triggerLayoutChanged),
+        QObjectPrivate::connect(model, &QAbstractItemModel::layoutAboutToBeChanged,
+                         this, &QItemSelectionModelPrivate::layoutAboutToBeChanged),
+        QObjectPrivate::connect(model, &QAbstractItemModel::layoutChanged,
+                         this, &QItemSelectionModelPrivate::layoutChanged),
+        QObject::connect(model, &QAbstractItemModel::modelReset,
+                         q, &QItemSelectionModel::reset),
+        QObjectPrivate::connect(model, &QAbstractItemModel::destroyed,
+                         this, &QItemSelectionModelPrivate::modelDestroyed)};
     }
 }
 
+void QItemSelectionModelPrivate::disconnectModel()
+{
+    Q_Q(QItemSelectionModel);
+    for (auto &connection : connections) {
+        QObject::disconnect(connection);
+        connection = QMetaObject::Connection();
+    }
+    q->reset();
+}
+
 /*!
     \internal
 
@@ -638,7 +641,7 @@ QItemSelection QItemSelectionModelPrivate::expandSelection(const QItemSelection
 /*!
     \internal
 */
-void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &parent,
+void QItemSelectionModelPrivate::rowsAboutToBeRemoved(const QModelIndex &parent,
                                                          int start, int end)
 {
     Q_Q(QItemSelectionModel);
@@ -721,7 +724,7 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &pare
 /*!
     \internal
 */
-void QItemSelectionModelPrivate::_q_columnsAboutToBeRemoved(const QModelIndex &parent,
+void QItemSelectionModelPrivate::columnsAboutToBeRemoved(const QModelIndex &parent,
                                                             int start, int end)
 {
     Q_Q(QItemSelectionModel);
@@ -758,7 +761,7 @@ void QItemSelectionModelPrivate::_q_columnsAboutToBeRemoved(const QModelIndex &p
 
     Split selection ranges if columns are about to be inserted in the middle.
 */
-void QItemSelectionModelPrivate::_q_columnsAboutToBeInserted(const QModelIndex &parent,
+void QItemSelectionModelPrivate::columnsAboutToBeInserted(const QModelIndex &parent,
                                                              int start, int end)
 {
     Q_UNUSED(end);
@@ -788,7 +791,7 @@ void QItemSelectionModelPrivate::_q_columnsAboutToBeInserted(const QModelIndex &
 
     Split selection ranges if rows are about to be inserted in the middle.
 */
-void QItemSelectionModelPrivate::_q_rowsAboutToBeInserted(const QModelIndex &parent,
+void QItemSelectionModelPrivate::rowsAboutToBeInserted(const QModelIndex &parent,
                                                           int start, int end)
 {
     Q_Q(QItemSelectionModel);
@@ -829,7 +832,8 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeInserted(const QModelIndex &par
     preparation for the layoutChanged() signal, where the indexes can be
     merged again.
 */
-void QItemSelectionModelPrivate::_q_layoutAboutToBeChanged(const QList<QPersistentModelIndex> &, QAbstractItemModel::LayoutChangeHint hint)
+void QItemSelectionModelPrivate::layoutAboutToBeChanged(const QList<QPersistentModelIndex> &,
+                                                        QAbstractItemModel::LayoutChangeHint hint)
 {
     savedPersistentIndexes.clear();
     savedPersistentCurrentIndexes.clear();
@@ -976,7 +980,7 @@ static QItemSelection mergeIndexes(const QList<QPersistentModelIndex> &indexes)
 /*!
     \internal
 
-    Sort predicate function for QItemSelectionModelPrivate::_q_layoutChanged(),
+    Sort predicate function for QItemSelectionModelPrivate::layoutChanged(),
     sorting by parent first in addition to operator<(). This is to prevent
     fragmentation of the selection by grouping indexes with the same row, column
     of different parents next to each other, which may happen when a selection
@@ -994,7 +998,7 @@ static bool qt_PersistentModelIndexLessThan(const QPersistentModelIndex &i1, con
 
     Merge the selected indexes into selection ranges again.
 */
-void QItemSelectionModelPrivate::_q_layoutChanged(const QList<QPersistentModelIndex> &, QAbstractItemModel::LayoutChangeHint hint)
+void QItemSelectionModelPrivate::layoutChanged(const QList<QPersistentModelIndex> &, QAbstractItemModel::LayoutChangeHint hint)
 {
     // special case for when all indexes are selected
     if (tableSelected && tableColCount == model->columnCount(tableParent)
@@ -1078,9 +1082,10 @@ void QItemSelectionModelPrivate::_q_layoutChanged(const QList<QPersistentModelIn
     We decide to break the new rule, imposed by bindable properties, and not break the old
     rule, because that may break existing code.
 */
-void QItemSelectionModelPrivate::_q_modelDestroyed()
+void QItemSelectionModelPrivate::modelDestroyed()
 {
     model.setValueBypassingBindings(nullptr);
+    disconnectModel();
     model.notify();
 }
 
@@ -1289,7 +1294,7 @@ void QItemSelectionModel::select(const QItemSelection &selection, QItemSelection
     // If d->ranges is non-empty when the source model is reset the persistent indexes
     // it contains will be invalid. We can't clear them in a modelReset slot because that might already
     // be too late if another model observer is connected to the same modelReset slot and is invoked first
-    // it might call select() on this selection model before any such QItemSelectionModelPrivate::_q_modelReset() slot
+    // it might call select() on this selection model before any such QItemSelectionModelPrivate::modelReset() slot
     // is invoked, so it would not be cleared yet. We clear it invalid ranges in it here.
     d->ranges.removeIf(QtFunctionObjects::IsNotValid());
 
@@ -1883,7 +1888,6 @@ void QItemSelectionModel::setModel(QAbstractItemModel *model)
     d->model.removeBindingUnlessInWrapper();
     if (d->model == model)
         return;
-
     d->initModel(model);
     d->model.notify();
 }

src/corelib/itemmodels/qitemselectionmodel.h

@@ -165,13 +165,6 @@ protected:
 
 private:
     Q_DISABLE_COPY(QItemSelectionModel)
-    Q_PRIVATE_SLOT(d_func(), void _q_columnsAboutToBeRemoved(const QModelIndex&, int, int))
-    Q_PRIVATE_SLOT(d_func(), void _q_rowsAboutToBeRemoved(const QModelIndex&, int, int))
-    Q_PRIVATE_SLOT(d_func(), void _q_columnsAboutToBeInserted(const QModelIndex&, int, int))
-    Q_PRIVATE_SLOT(d_func(), void _q_rowsAboutToBeInserted(const QModelIndex&, int, int))
-    Q_PRIVATE_SLOT(d_func(), void _q_layoutAboutToBeChanged(const QList<QPersistentModelIndex> &parents = QList<QPersistentModelIndex>(), QAbstractItemModel::LayoutChangeHint hint = QAbstractItemModel::NoHint))
-    Q_PRIVATE_SLOT(d_func(), void _q_layoutChanged(const QList<QPersistentModelIndex> &parents = QList<QPersistentModelIndex>(), QAbstractItemModel::LayoutChangeHint hint = QAbstractItemModel::NoHint))
-    Q_PRIVATE_SLOT(d_func(), void _q_modelDestroyed())
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(QItemSelectionModel::SelectionFlags)

src/corelib/itemmodels/qitemselectionmodel_p.h

@@ -18,6 +18,7 @@
 #include "qitemselectionmodel.h"
 #include "private/qobject_p.h"
 #include "private/qproperty_p.h"
+#include <array>
 
 QT_REQUIRE_CONFIG(itemmodel);
 
@@ -36,13 +37,23 @@ public:
 
     void initModel(QAbstractItemModel *model);
 
-    void _q_rowsAboutToBeRemoved(const QModelIndex &parent, int start, int end);
+    void rowsAboutToBeRemoved(const QModelIndex &parent, int start, int end);
-    void _q_columnsAboutToBeRemoved(const QModelIndex &parent, int start, int end);
+    void columnsAboutToBeRemoved(const QModelIndex &parent, int start, int end);
-    void _q_rowsAboutToBeInserted(const QModelIndex &parent, int start, int end);
+    void rowsAboutToBeInserted(const QModelIndex &parent, int start, int end);
-    void _q_columnsAboutToBeInserted(const QModelIndex &parent, int start, int end);
+    void columnsAboutToBeInserted(const QModelIndex &parent, int start, int end);
-    void _q_layoutAboutToBeChanged(const QList<QPersistentModelIndex> &parents = QList<QPersistentModelIndex>(), QAbstractItemModel::LayoutChangeHint hint = QAbstractItemModel::NoLayoutChangeHint);
+    void layoutAboutToBeChanged(const QList<QPersistentModelIndex> &parents, QAbstractItemModel::LayoutChangeHint hint);
-    void _q_layoutChanged(const QList<QPersistentModelIndex> &parents = QList<QPersistentModelIndex>(), QAbstractItemModel::LayoutChangeHint hint = QAbstractItemModel::NoLayoutChangeHint);
+    void triggerLayoutToBeChanged()
-    void _q_modelDestroyed();
+    {
+        layoutAboutToBeChanged(QList<QPersistentModelIndex>(), QAbstractItemModel::NoLayoutChangeHint);
+    }
+
+    void layoutChanged(const QList<QPersistentModelIndex> &parents, QAbstractItemModel::LayoutChangeHint hint);
+    void triggerLayoutChanged()
+    {
+        layoutChanged(QList<QPersistentModelIndex>(), QAbstractItemModel::NoLayoutChangeHint);
+    }
+
+    void modelDestroyed();
 
     inline void remove(QList<QItemSelectionRange> &r)
     {
@@ -59,7 +70,8 @@ public:
     }
 
     void setModel(QAbstractItemModel *mod) { q_func()->setModel(mod); }
-    void modelChanged(QAbstractItemModel *mod) { q_func()->modelChanged(mod); }
+    void disconnectModel();
+    void modelChanged(QAbstractItemModel *mod) { emit q_func()->modelChanged(mod); }
     Q_OBJECT_COMPAT_PROPERTY_WITH_ARGS(QItemSelectionModelPrivate, QAbstractItemModel *, model,
                                        &QItemSelectionModelPrivate::setModel,
                                        &QItemSelectionModelPrivate::modelChanged, nullptr)
@@ -76,6 +88,7 @@ public:
     bool tableSelected;
     QPersistentModelIndex tableParent;
     int tableColCount, tableRowCount;
+    std::array<QMetaObject::Connection, 12> connections;
 };
 
 QT_END_NAMESPACE

tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp

@@ -80,7 +80,10 @@ private slots:
 
     void QTBUG93305();
 
+    void testSignalsDisconnection();
+
 private:
+    static void messageHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg);
     QAbstractItemModel *model;
     QItemSelectionModel *selection;
 };
@@ -2917,5 +2920,37 @@ void tst_QItemSelectionModel::QTBUG93305()
     QCOMPARE(spy.size(), 4);
 }
 
+static void (*oldMessageHandler)(QtMsgType, const QMessageLogContext&, const QString&);
+static bool signalError = false;
+
+// detect disconnect warning:
+// qt.core.qobject.connect: QObject::disconnect: No such signal
+void tst_QItemSelectionModel::messageHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg)
+{
+    Q_ASSERT(oldMessageHandler);
+
+    if (type == QtWarningMsg
+            && QString(context.category) == "qt.core.qobject.connect"
+            && msg.contains("No such")) {
+        signalError = true;
+    }
+
+    return oldMessageHandler(type, context, msg);
+}
+
+void tst_QItemSelectionModel::testSignalsDisconnection()
+{
+    oldMessageHandler = qInstallMessageHandler(messageHandler);
+    auto resetMessageHandler = qScopeGuard([] { qInstallMessageHandler(oldMessageHandler); });
+    auto *newModel = new QStandardItemModel(model);
+    selection->setModel(newModel);
+    QSignalSpy spy(newModel, &QObject::destroyed);
+    delete newModel;
+    QTRY_COMPARE(spy.count(), 1);
+    qDebug() << spy;
+    selection->setModel(nullptr);
+    QVERIFY(!signalError);
+}
+
 QTEST_MAIN(tst_QItemSelectionModel)
 #include "tst_qitemselectionmodel.moc"