QQuickTableView: sync model and delegate when ready to do so

Doing (silly) things in the delegate, like:

    Component.onCompleted: TableView.view.delegate = null

will lead to a crash. The same if you change the model.
The reason is that you end up changing the model
while e.g a row is half-way loaded. Information needed for
building the row, like model size, will then be invalid.

To protect against this, we insert a "sync" phase to the
code that takes any such changes into effect at a time
when we know it's safe to do so.

Change-Id: I85a992dfc0e04ec6635b10c9768a8ddc140e09da
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

5 files changed, 212 insertions, 58 deletions

diff --git a/src/quick/items/qquicktableview.cpp b/src/quick/items/qquicktableview.cpp
index 25701e82b4..67a9a3a7c2 100644
--- a/src/quick/items/qquicktableview.cpp
+++ b/src/quick/items/qquicktableview.cpp
@@ -1261,10 +1261,6 @@ bool QQuickTableViewPrivate::moveToNextRebuildState()
 
 void QQuickTableViewPrivate::beginRebuildTable()
 {
-    rebuildScheduled = false;
-    rebuildOptions = scheduledRebuildOptions;
-    scheduledRebuildOptions = RebuildOption::None;
-
     if (loadRequest.isActive())
         cancelLoadRequest();
 
@@ -1455,7 +1451,6 @@ void QQuickTableViewPrivate::updatePolish()
     // Whenever something changes, e.g viewport moves, spacing is set to a
     // new value, model changes etc, this function will end up being called. Here
     // we check what needs to be done, and load/unload cells accordingly.
-    Q_Q(QQuickTableView);
 
     Q_TABLEVIEW_ASSERT(!polishing, "recursive updatePolish() calls are not allowed!");
     QBoolBlocker polishGuard(polishing, true);
@@ -1473,15 +1468,9 @@ void QQuickTableViewPrivate::updatePolish()
         return;
     }
 
-    // viewportRect describes the part of the content view that is actually visible. Since a
-    // negative width/height can happen (e.g during start-up), we check for this to avoid rebuilding
-    // the table (and e.g calculate initial row/column sizes) based on a premature viewport rect.
-    viewportRect = QRectF(q->contentX(), q->contentY(), q->width(), q->height());
-    if (!viewportRect.isValid())
-        return;
+    syncWithPendingChanges();
 
-    if (rebuildScheduled) {
-        rebuildState = RebuildState::Begin;
+    if (rebuildState == RebuildState::Begin) {
         processRebuildTable();
         return;
     }
@@ -1562,6 +1551,71 @@ void QQuickTableViewPrivate::itemReusedCallback(int modelIndex, QObject *object)
         emit attached->reused();
 }
 
+void QQuickTableViewPrivate::syncWithPendingChanges()
+{
+    // The application can change properties like the model or the delegate while
+    // we're e.g in the middle of e.g loading a new row. Since this will lead to
+    // unpredicted behavior, and possibly a crash, we need to postpone taking
+    // such assignments into effect until we're in a state that allows it.
+    Q_Q(QQuickTableView);
+    viewportRect = QRectF(q->contentX(), q->contentY(), q->width(), q->height());
+    syncRebuildOptions();
+    syncModel();
+    syncDelegate();
+}
+
+void QQuickTableViewPrivate::syncRebuildOptions()
+{
+    if (!rebuildScheduled)
+        return;
+
+    rebuildState = RebuildState::Begin;
+    rebuildOptions = scheduledRebuildOptions;
+    scheduledRebuildOptions = RebuildOption::None;
+    rebuildScheduled = false;
+}
+
+void QQuickTableViewPrivate::syncDelegate()
+{
+    if (tableModel && assignedDelegate == tableModel->delegate())
+        return;
+
+    if (!tableModel)
+        createWrapperModel();
+
+    tableModel->setDelegate(assignedDelegate);
+}
+
+void QQuickTableViewPrivate::syncModel()
+{
+    if (modelVariant == assignedModel)
+        return;
+
+    if (model)
+        disconnectFromModel();
+
+    modelVariant = assignedModel;
+    QVariant effectiveModelVariant = modelVariant;
+    if (effectiveModelVariant.userType() == qMetaTypeId<QJSValue>())
+        effectiveModelVariant = effectiveModelVariant.value<QJSValue>().toVariant();
+
+    const auto instanceModel = qobject_cast<QQmlInstanceModel *>(qvariant_cast<QObject*>(effectiveModelVariant));
+
+    if (instanceModel) {
+        if (tableModel) {
+            delete tableModel;
+            tableModel = nullptr;
+        }
+        model = instanceModel;
+    } else {
+        if (!tableModel)
+            createWrapperModel();
+        tableModel->setModel(effectiveModelVariant);
+    }
+
+    connectToModel();
+}
+
 void QQuickTableViewPrivate::connectToModel()
 {
     Q_TABLEVIEW_ASSERT(model, "");
@@ -1765,59 +1819,32 @@ void QQuickTableView::setColumnWidthProvider(QJSValue provider)
 
 QVariant QQuickTableView::model() const
 {
-    return d_func()->modelVariant;
+    return d_func()->assignedModel;
 }
 
 void QQuickTableView::setModel(const QVariant &newModel)
 {
     Q_D(QQuickTableView);
+    if (newModel == d->assignedModel)
+        return;
 
-    if (d->model)
-        d->disconnectFromModel();
-
-    d->modelVariant = newModel;
-    QVariant effectiveModelVariant = d->modelVariant;
-    if (effectiveModelVariant.userType() == qMetaTypeId<QJSValue>())
-        effectiveModelVariant = effectiveModelVariant.value<QJSValue>().toVariant();
-
-    const auto instanceModel = qobject_cast<QQmlInstanceModel *>(qvariant_cast<QObject*>(effectiveModelVariant));
-
-    if (instanceModel) {
-        if (d->tableModel) {
-            delete d->tableModel;
-            d->tableModel = nullptr;
-        }
-        d->model = instanceModel;
-    } else {
-        if (!d->tableModel)
-            d->createWrapperModel();
-        d->tableModel->setModel(effectiveModelVariant);
-    }
-
-    d->connectToModel();
+    d->assignedModel = newModel;
     d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::All);
     emit modelChanged();
 }
 
 QQmlComponent *QQuickTableView::delegate() const
 {
-    Q_D(const QQuickTableView);
-    if (d->tableModel)
-        return d->tableModel->delegate();
-
-    return nullptr;
+    return d_func()->assignedDelegate;
 }
 
 void QQuickTableView::setDelegate(QQmlComponent *newDelegate)
 {
     Q_D(QQuickTableView);
-    if (newDelegate == delegate())
+    if (newDelegate == d->assignedDelegate)
         return;
 
-    if (!d->tableModel)
-        d->createWrapperModel();
-
-    d->tableModel->setDelegate(newDelegate);
+    d->assignedDelegate = newDelegate;
     d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::All);
 
     emit delegateChanged();
@@ -1919,16 +1946,6 @@ void QQuickTableView::viewportMoved(Qt::Orientations orientation)
         d->updatePolish();
 }
 
-void QQuickTableView::componentComplete()
-{
-    Q_D(QQuickTableView);
-
-    if (!d->model)
-        setModel(QVariant());
-
-    QQuickFlickable::componentComplete();
-}
-
 #include "moc_qquicktableview_p.cpp"
 
 QT_END_NAMESPACE
diff --git a/src/quick/items/qquicktableview_p.h b/src/quick/items/qquicktableview_p.h
index 6ba91d16a1..45e20027ba 100644
--- a/src/quick/items/qquicktableview_p.h
+++ b/src/quick/items/qquicktableview_p.h
@@ -128,7 +128,6 @@ Q_SIGNALS:
 protected:
     void geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry) override;
     void viewportMoved(Qt::Orientations orientation) override;
-    void componentComplete() override;
 
 private:
     Q_DISABLE_COPY(QQuickTableView)
diff --git a/src/quick/items/qquicktableview_p_p.h b/src/quick/items/qquicktableview_p_p.h
index 9ff2b2e10b..6bf3de76df 100644
--- a/src/quick/items/qquicktableview_p_p.h
+++ b/src/quick/items/qquicktableview_p_p.h
@@ -206,6 +206,11 @@ public:
     QPointer<QQmlTableInstanceModel> tableModel = nullptr;
     QVariant modelVariant;
 
+    // When the applications assignes a new model or delegate to the view, we keep them
+    // around until we're ready to take them into use (syncWithPendingChanges).
+    QVariant assignedModel = QVariant(int(0));
+    QQmlComponent *assignedDelegate = nullptr;
+
     // loadedTable describes the table cells that are currently loaded (from top left
     // row/column to bottom right row/column). loadedTableOuterRect describes the actual
     // pixels that those cells cover, and is matched agains the viewport to determine when
@@ -332,6 +337,11 @@ public:
     void itemReusedCallback(int modelIndex, QObject *object);
     void modelUpdated(const QQmlChangeSet &changeSet, bool reset);
 
+    inline void syncWithPendingChanges();
+    inline void syncDelegate();
+    inline void syncModel();
+    inline void syncRebuildOptions();
+
     void connectToModel();
     void disconnectFromModel();
 
diff --git a/tests/auto/quick/qquicktableview/data/changemodelordelegateduringupdate.qml b/tests/auto/quick/qquicktableview/data/changemodelordelegateduringupdate.qml
new file mode 100644
index 0000000000..de404be63d
--- /dev/null
+++ b/tests/auto/quick/qquicktableview/data/changemodelordelegateduringupdate.qml
@@ -0,0 +1,81 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the QtQuick module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl-3.0.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or (at your option) the GNU General
+** Public license version 3 or any later version approved by the KDE Free
+** Qt Foundation. The licenses are as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-2.0.html and
+** https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+import QtQuick 2.12
+import QtQuick.Window 2.3
+
+Item {
+    width: 640
+    height: 450
+
+    property alias tableView: tableView
+    property bool changeDelegate: false
+    property bool changeModel: false
+
+    TableView {
+        id: tableView
+        width: 600
+        height: 400
+        clip: true
+        delegate: tableViewDelegate
+    }
+
+    Component {
+        id: tableViewDelegate
+        Rectangle {
+            implicitWidth: 100
+            implicitHeight: 100
+            color: "lightgray"
+            border.width: 1
+
+            Text {
+                anchors.centerIn: parent
+                text: modelData
+            }
+
+            Component.onCompleted: {
+                if (changeDelegate)
+                    TableView.view.delegate = null
+                if (changeModel)
+                    TableView.view.model = null
+            }
+        }
+    }
+
+}
diff --git a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
index 9eb942fc93..a1951bdf90 100644
--- a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
+++ b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
@@ -106,6 +106,8 @@ private slots:
     void checkExplicitContentWidthAndHeight();
     void checkContentXY();
     void noDelegate();
+    void changeDelegateDuringUpdate();
+    void changeModelDuringUpdate();
     void countDelegateItems_data();
     void countDelegateItems();
     void checkLayoutOfEqualSizedDelegateItems_data();
@@ -664,6 +666,51 @@ void tst_QQuickTableView::noDelegate()
     QVERIFY(items.isEmpty());
 }
 
+void tst_QQuickTableView::changeDelegateDuringUpdate()
+{
+    // Check that you can change the delegate (set it to null)
+    // while the TableView is busy loading the table.
+    LOAD_TABLEVIEW("changemodelordelegateduringupdate.qml");
+
+    auto model = TestModelAsVariant(1, 1);
+    tableView->setModel(model);
+    view->rootObject()->setProperty("changeDelegate", true);
+
+    WAIT_UNTIL_POLISHED;
+
+    // We should no longer have a delegate, and no
+    // items should therefore be loaded.
+    QCOMPARE(tableView->delegate(), nullptr);
+    QCOMPARE(tableViewPrivate->loadedItems.size(), 0);
+
+    // Even if the delegate is missing, we still report
+    // the correct size of the model
+    QCOMPARE(tableView->rows(), 1);
+    QCOMPARE(tableView->columns(), 1);
+};
+
+void tst_QQuickTableView::changeModelDuringUpdate()
+{
+    // Check that you can change the model (set it to null)
+    // while the TableView is buzy loading the table.
+    LOAD_TABLEVIEW("changemodelordelegateduringupdate.qml");
+
+    auto model = TestModelAsVariant(1, 1);
+    tableView->setModel(model);
+    view->rootObject()->setProperty("changeModel", true);
+
+    WAIT_UNTIL_POLISHED;
+
+    // We should no longer have a model, and the no
+    // items should therefore be loaded.
+    QVERIFY(tableView->model().isNull());
+    QCOMPARE(tableViewPrivate->loadedItems.size(), 0);
+
+    // The empty model has no rows or columns
+    QCOMPARE(tableView->rows(), 0);
+    QCOMPARE(tableView->columns(), 0);
+};
+
 void tst_QQuickTableView::countDelegateItems_data()
 {
     QTest::addColumn<QVariant>("model");