DelegateModel: handle roleName invalidation

roleNames are generally guaranteed to be stable (given that QAIM has no
change signal for them), except that resetting the model is allowed to
invalidate them. DelegateModel did so far not take this into account.

Handle this case correctly by snapshotting the current roleNames before
the model is reset. Afterwards, if we detect that roleNames has changed,
we throw the current model set up away and rebuild everything from
scratch – it is unlikely that a more efficient implementation would be
worth it.
If we detect no changes, we simply use the existing logic to handle the
model reset.

Note that some views, e.g. TableView do not use DelegateModel if they
detect that the model is a QAIM. They require a conceptually similar
fix (but can probaly implement this in a more efficient way).

Fixes: QTBUG-32132
Fixes: QTBUG-103220
Pick-to: 6.2 6.5
Change-Id: I6874988cf09b8fe75d2e9750d0bf209915554c45
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

src/qmlmodels/qqmldelegatemodel.cpp

@@ -367,8 +367,8 @@ void QQmlDelegateModelPrivate::connectToAbstractItemModel()
                       q, QQmlDelegateModel, SLOT(_q_dataChanged(QModelIndex,QModelIndex,QVector<int>)));
     qmlobject_connect(aim, QAbstractItemModel, SIGNAL(rowsMoved(QModelIndex,int,int,QModelIndex,int)),
                       q, QQmlDelegateModel, SLOT(_q_rowsMoved(QModelIndex,int,int,QModelIndex,int)));
-    qmlobject_connect(aim, QAbstractItemModel, SIGNAL(modelReset()),
-                      q, QQmlDelegateModel, SLOT(_q_modelReset()));
+
+    QObject::connect(aim, &QAbstractItemModel::modelAboutToBeReset, q, &QQmlDelegateModel::_q_modelAboutToBeReset);
     qmlobject_connect(aim, QAbstractItemModel, SIGNAL(layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
                       q, QQmlDelegateModel, SLOT(_q_layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)));
 }
@@ -397,8 +397,7 @@ void QQmlDelegateModelPrivate::disconnectFromAbstractItemModel()
                         q, SLOT(_q_dataChanged(QModelIndex,QModelIndex,QVector<int>)));
     QObject::disconnect(aim, SIGNAL(rowsMoved(QModelIndex,int,int,QModelIndex,int)),
                         q, SLOT(_q_rowsMoved(QModelIndex,int,int,QModelIndex,int)));
-    QObject::disconnect(aim, SIGNAL(modelReset()),
-                        q, SLOT(_q_modelReset()));
+    QObject::disconnect(aim, &QAbstractItemModel::modelAboutToBeReset, q, &QQmlDelegateModel::_q_modelAboutToBeReset);
     QObject::disconnect(aim, SIGNAL(layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)),
                         q, SLOT(_q_layoutChanged(QList<QPersistentModelIndex>,QAbstractItemModel::LayoutChangeHint)));
 }
@@ -1840,7 +1839,29 @@ void QQmlDelegateModelPrivate::emitChanges()
     }
 }
 
-void QQmlDelegateModel::_q_modelReset()
+void QQmlDelegateModel::_q_modelAboutToBeReset()
+{
+    auto aim = static_cast<QAbstractItemModel *>(sender());
+    auto oldRoleNames = aim->roleNames();
+    // this relies on the fact that modelAboutToBeReset must be followed
+    // by a modelReset signal before any further modelAboutToBeReset can occur
+    QObject::connect(aim, &QAbstractItemModel::modelReset, this, [&, oldRoleNames](){
+        auto aim = static_cast<QAbstractItemModel *>(sender());
+        if (oldRoleNames == aim->roleNames()) {
+            // if the rolenames stayed the same (most common case), then we don't have
+            // to throw away all the setup that we did
+            handleModelReset();
+        } else {
+            // If they did change, we give up and just start from scratch via setMode
+            setModel(QVariant::fromValue(model()));
+            // but we still have to call handleModelReset, otherwise views will
+            // not refresh
+            handleModelReset();
+        }
+    }, Qt::SingleShotConnection);
+}
+
+void QQmlDelegateModel::handleModelReset()
 {
     Q_D(QQmlDelegateModel);
     if (!d->m_delegate)
@@ -2005,7 +2026,7 @@ void QQmlDelegateModel::_q_layoutChanged(const QList<QPersistentModelIndex> &par
         // Ignored
     } else {
         // We don't know what's going on, so reset the model
-        _q_modelReset();
+        handleModelReset();
     }
 }
 

src/qmlmodels/qqmldelegatemodel_p.h

@@ -114,7 +114,7 @@ private Q_SLOTS:
     void _q_itemsInserted(int index, int count);
     void _q_itemsRemoved(int index, int count);
     void _q_itemsMoved(int from, int to, int count);
-    void _q_modelReset();
+    void _q_modelAboutToBeReset();
     void _q_rowsInserted(const QModelIndex &,int,int);
     void _q_columnsInserted(const QModelIndex &, int, int);
     void _q_columnsRemoved(const QModelIndex &, int, int);
@@ -126,6 +126,7 @@ private Q_SLOTS:
     void _q_layoutChanged(const QList<QPersistentModelIndex>&, QAbstractItemModel::LayoutChangeHint);
 
 private:
+    void handleModelReset();
     bool isDescendantOf(const QPersistentModelIndex &desc, const QList<QPersistentModelIndex> &parents) const;
 
     Q_DISABLE_COPY(QQmlDelegateModel)

tests/auto/qml/qqmldelegatemodel/data/resetModelData.qml

@@ -0,0 +1,16 @@
+import QtQuick
+
+ListView {
+    id: root
+    anchors.fill: parent
+    property bool success: (currentItem?.mydata ?? 0) === 42
+    height: 300
+    width: 200
+
+    delegate: Rectangle {
+        required property var model
+        implicitWidth: 100
+        implicitHeight: 50
+        property var mydata: model?.foo ?? model.bar
+    }
+}

tests/auto/qml/qqmldelegatemodel/tst_qqmldelegatemodel.cpp

@@ -5,6 +5,7 @@
 #include <QtCore/QConcatenateTablesProxyModel>
 #include <QtGui/QStandardItemModel>
 #include <QtQml/qqmlcomponent.h>
+#include <QtQml/qqmlapplicationengine.h>
 #include <QtQmlModels/private/qqmldelegatemodel_p.h>
 #include <QtQuick/qquickview.h>
 #include <QtQuick/qquickitem.h>
@@ -19,6 +20,7 @@ public:
     tst_QQmlDelegateModel();
 
 private slots:
+    void resettingRolesRespected();
     void valueWithoutCallingObjectFirst_data();
     void valueWithoutCallingObjectFirst();
     void qtbug_86017();
@@ -85,6 +87,61 @@ tst_QQmlDelegateModel::tst_QQmlDelegateModel()
     qmlRegisterType<AbstractItemModel>("Test", 1, 0, "AbstractItemModel");
 }
 
+class TableModel : public QAbstractTableModel
+{
+    Q_OBJECT
+
+public:
+    int rowCount(const QModelIndex & = QModelIndex()) const override
+    {
+        return 1;
+    }
+
+    int columnCount(const QModelIndex & = QModelIndex()) const override
+    {
+        return 1;
+    }
+
+    QVariant data(const QModelIndex &index, int role) const override
+    {
+        switch (role) {
+        case 0:
+            return QString("foo: %1, %2").arg(index.column()).arg(index.row());
+        case 1:
+            return 42;
+        default:
+            break;
+        }
+
+        return QVariant();
+    }
+
+    Q_INVOKABLE void change() { beginResetModel(); toggle = !toggle; endResetModel(); }
+
+    QHash<int, QByteArray> roleNames() const override
+    {
+        if (toggle)
+            return { {0, "foo"} };
+        else
+            return { {1, "bar"} };
+    }
+
+    bool toggle = true;
+};
+
+void tst_QQmlDelegateModel::resettingRolesRespected()
+{
+    auto model = std::make_unique<TableModel>();
+    QQmlApplicationEngine engine;
+    engine.setInitialProperties({ {"model", QVariant::fromValue(model.get()) }} );
+    engine.load(testFileUrl("resetModelData.qml"));
+    QTRY_VERIFY(!engine.rootObjects().isEmpty());
+    QObject *root = engine.rootObjects().constFirst();
+    QVERIFY(!root->property("success").toBool());
+    model->change();
+    QTRY_VERIFY(root->property("success").toBool());
+}
+
 void tst_QQmlDelegateModel::valueWithoutCallingObjectFirst_data()
 {
     QTest::addColumn<QUrl>("qmlFileUrl");