QQmlTableInstanceModel: handle model data changes more gracefully

Equal to QQmlDelegateModel, we need to listen for changes done to
existing model items, and notify existing delegate items about it.
Otherwise, they will not stay in sync with the model.

By accident, this sort of worked in QQuickTableView already, since
it would rebuild the whole table for every model update. This
is really slow, and completely unnecessary.

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

7 files changed, 94 insertions, 21 deletions

diff --git a/src/qml/types/qqmltableinstancemodel.cpp b/src/qml/types/qqmltableinstancemodel.cpp
index 6604e009c1..536dd46182 100644
--- a/src/qml/types/qqmltableinstancemodel.cpp
+++ b/src/qml/types/qqmltableinstancemodel.cpp
@@ -437,7 +437,26 @@ void QQmlTableInstanceModel::setModel(const QVariant &model)
     // needs to stay in sync with the model. So we need to drain the pool
     // completely when the model changes.
     drainReusableItemsPool(0);
+    if (auto const aim = abstractItemModel())
+        disconnect(aim, &QAbstractItemModel::dataChanged, this, &QQmlTableInstanceModel::dataChangedCallback);
     m_adaptorModel.setModel(model, this, m_qmlContext->engine());
+    if (auto const aim = abstractItemModel())
+        connect(aim, &QAbstractItemModel::dataChanged, this, &QQmlTableInstanceModel::dataChangedCallback);
+}
+
+void QQmlTableInstanceModel::dataChangedCallback(const QModelIndex &begin, const QModelIndex &end, const QVector<int> &roles)
+{
+    // This function is called when model data has changed. In that case, we tell the adaptor model
+    // to go through all the items we have created, find the ones that are affected, and notify that
+    // their model data has changed. This will in turn update QML bindings inside the delegate items.
+    int numberOfRowsChanged = end.row() - begin.row() + 1;
+    int numberOfColumnsChanged = end.column() - begin.column() + 1;
+
+    for (int column = 0; column < numberOfColumnsChanged; ++column) {
+        const int columnIndex = begin.column() + column;
+        const int rowIndex = begin.row() + (columnIndex * rows());
+        m_adaptorModel.notify(m_modelItems.values(), rowIndex, numberOfRowsChanged, roles);
+    }
 }
 
 QQmlComponent *QQmlTableInstanceModel::delegate() const
diff --git a/src/qml/types/qqmltableinstancemodel_p.h b/src/qml/types/qqmltableinstancemodel_p.h
index 23243e7f0e..71689ce6da 100644
--- a/src/qml/types/qqmltableinstancemodel_p.h
+++ b/src/qml/types/qqmltableinstancemodel_p.h
@@ -142,6 +142,8 @@ private:
     void deleteAllFinishedIncubationTasks();
     QQmlDelegateModelItem *resolveModelItem(int index);
 
+    void dataChangedCallback(const QModelIndex &begin, const QModelIndex &end, const QVector<int> &roles);
+
     static bool isDoneIncubating(QQmlDelegateModelItem *modelItem);
     static void deleteModelItemLater(QQmlDelegateModelItem *modelItem);
 
diff --git a/src/quick/items/qquicktableview.cpp b/src/quick/items/qquicktableview.cpp
index 8e9cc4318e..19e04b212b 100644
--- a/src/quick/items/qquicktableview.cpp
+++ b/src/quick/items/qquicktableview.cpp
@@ -1166,7 +1166,6 @@ void QQuickTableViewPrivate::connectToModel()
         // be our own QQmlTableInstanceModel, which doesn't bother creating change sets at all. For models that are
         // not based on QAIM (like QQmlObjectModel, QQmlListModel, javascript arrays etc), there is currently no way
         // to modify the model at runtime without also re-setting the model on the view.
-        connect(aim, &QAbstractItemModel::dataChanged, this, &QQuickTableViewPrivate::dataChangedCallback);
         connect(aim, &QAbstractItemModel::rowsMoved, this, &QQuickTableViewPrivate::rowsMovedCallback);
         connect(aim, &QAbstractItemModel::columnsMoved, this, &QQuickTableViewPrivate::columnsMovedCallback);
         connect(aim, &QAbstractItemModel::rowsInserted, this, &QQuickTableViewPrivate::rowsInsertedCallback);
@@ -1192,7 +1191,6 @@ void QQuickTableViewPrivate::disconnectFromModel()
     }
 
     if (auto const aim = model->abstractItemModel()) {
-        disconnect(aim, &QAbstractItemModel::dataChanged, this, &QQuickTableViewPrivate::dataChangedCallback);
         disconnect(aim, &QAbstractItemModel::rowsMoved, this, &QQuickTableViewPrivate::rowsMovedCallback);
         disconnect(aim, &QAbstractItemModel::columnsMoved, this, &QQuickTableViewPrivate::columnsMovedCallback);
         disconnect(aim, &QAbstractItemModel::rowsInserted, this, &QQuickTableViewPrivate::rowsInsertedCallback);
@@ -1214,14 +1212,6 @@ void QQuickTableViewPrivate::modelUpdated(const QQmlChangeSet &changeSet, bool r
     invalidateTable();
 }
 
-void QQuickTableViewPrivate::dataChangedCallback(const QModelIndex &begin, const QModelIndex &, const QVector<int> &)
-{
-    if (begin.parent() != QModelIndex())
-        return;
-
-    invalidateTable();
-}
-
 void QQuickTableViewPrivate::rowsMovedCallback(const QModelIndex &parent, int, int, const QModelIndex &, int )
 {
     if (parent != QModelIndex())
diff --git a/src/quick/items/qquicktableview_p_p.h b/src/quick/items/qquicktableview_p_p.h
index 7e8561850a..b61093e55d 100644
--- a/src/quick/items/qquicktableview_p_p.h
+++ b/src/quick/items/qquicktableview_p_p.h
@@ -300,7 +300,6 @@ public:
     void connectToModel();
     void disconnectFromModel();
 
-    void dataChangedCallback(const QModelIndex &begin, const QModelIndex &end, const QVector<int> &roles);
     void rowsMovedCallback(const QModelIndex &parent, int start, int end, const QModelIndex &destination, int row);
     void columnsMovedCallback(const QModelIndex &parent, int start, int end, const QModelIndex &destination, int column);
     void rowsInsertedCallback(const QModelIndex &parent, int begin, int end);
diff --git a/tests/auto/quick/qquicktableview/data/plaintableview.qml b/tests/auto/quick/qquicktableview/data/plaintableview.qml
index 36c28525d5..21373e6732 100644
--- a/tests/auto/quick/qquicktableview/data/plaintableview.qml
+++ b/tests/auto/quick/qquicktableview/data/plaintableview.qml
@@ -71,6 +71,9 @@ Item {
             implicitHeight: delegateHeight
             color: "lightgray"
             border.width: 1
+
+            property string modelDataBinding: modelData
+
             Text {
                 anchors.centerIn: parent
                 text: modelData
diff --git a/tests/auto/quick/qquicktableview/testmodel.h b/tests/auto/quick/qquicktableview/testmodel.h
index ab18af7871..56d5021cec 100644
--- a/tests/auto/quick/qquicktableview/testmodel.h
+++ b/tests/auto/quick/qquicktableview/testmodel.h
@@ -57,10 +57,10 @@ public:
         if (!index.isValid() || role != Qt::DisplayRole)
             return QVariant();
 
-        int cell = index.row() + (index.column() * m_columns);
-        if (selectedCells.contains(cell))
-            return QStringLiteral("selected");
-        return QString("%1").arg(index.row());
+        int serializedIndex = index.row() + (index.column() * m_columns);
+        if (modelData.contains(serializedIndex))
+            return modelData.value(serializedIndex);
+        return QStringLiteral("%1").arg(index.row());
     }
 
     QHash<int, QByteArray> roleNames() const override
@@ -68,12 +68,21 @@ public:
         return { {Qt::DisplayRole, "display"} };
     }
 
-    Q_INVOKABLE void selectCell(int row, int column)
+    Q_INVOKABLE void setModelData(const QPoint &cell, const QSize &span, const QString &prefix)
     {
-        int cell = row + (column * m_columns);
-        selectedCells.insert(cell);
-        auto index = createIndex(row, column, nullptr);
-        emit dataChanged(index, index);
+        for (int c = 0; c < span.width(); ++c) {
+            for (int r = 0; r < span.height(); ++r) {
+                const int changedRow = cell.y() + r;
+                const int changedColumn = cell.x() + c;
+                const int serializedIndex = changedRow + (changedColumn * m_rows);
+                const QString string = prefix + QStringLiteral("%1,%2").arg(changedColumn).arg(changedRow);
+                modelData.insert(serializedIndex, string);
+            }
+        }
+
+        const auto topLeftIndex = createIndex(cell.y(), cell.x(), nullptr);
+        const auto bottomRightIndex = createIndex(cell.y() + span.height() - 1, cell.x() + span.width() - 1, nullptr);
+        emit dataChanged(topLeftIndex, bottomRightIndex);
     }
 
     bool insertRows(int row, int count, const QModelIndex &parent = QModelIndex()) override
@@ -134,7 +143,7 @@ signals:
 private:
     int m_rows = 0;
     int m_columns = 0;
-    QSet<int> selectedCells;
+    QHash<int, QString> modelData;
 };
 
 #define TestModelAsVariant(...) QVariant::fromValue(QSharedPointer<TestModel>(new TestModel(__VA_ARGS__)))
diff --git a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
index 86839b61dc..bc0990862f 100644
--- a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
+++ b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
@@ -51,6 +51,7 @@ using namespace QQuickVisualTestUtil;
 static const char* kTableViewPropName = "tableView";
 static const char* kDelegateObjectName = "tableViewDelegate";
 static const char *kDelegatesCreatedCountProp = "delegatesCreatedCount";
+static const char *kModelDataBindingProp = "modelDataBinding";
 
 Q_DECLARE_METATYPE(QMarginsF);
 
@@ -113,6 +114,7 @@ private slots:
     void flickOvershoot();
     void checkRowColumnCount();
     void modelSignals();
+    void dataChangedSignal();
     void checkIfDelegatesAreReused_data();
     void checkIfDelegatesAreReused();
     void checkContextProperties_data();
@@ -1113,6 +1115,55 @@ void tst_QQuickTableView::modelSignals()
     QCOMPARE(tableView->columns(), 1);
 }
 
+void tst_QQuickTableView::dataChangedSignal()
+{
+    // Check that bindings to the model inside a delegate gets updated
+    // when the model item they bind to changes.
+    LOAD_TABLEVIEW("plaintableview.qml");
+
+    const QString prefix(QStringLiteral("changed"));
+
+    TestModel model(10, 10);
+    tableView->setModel(QVariant::fromValue(&model));
+
+    WAIT_UNTIL_POLISHED;
+
+    for (auto fxItem : tableViewPrivate->loadedItems) {
+        const auto item = tableViewPrivate->loadedTableItem(fxItem->cell)->item;
+        const QString modelDataBindingProperty = item->property(kModelDataBindingProp).toString();
+        QString expectedModelData = QString::number(fxItem->cell.y());
+        QCOMPARE(modelDataBindingProperty, expectedModelData);
+    }
+
+    // Change one cell in the model
+    model.setModelData(QPoint(0, 0), QSize(1, 1), prefix);
+
+    for (auto fxItem : tableViewPrivate->loadedItems) {
+        const QPoint cell = fxItem->cell;
+        const auto modelIndex = model.index(cell.y(), cell.x());
+        QString expectedModelData = model.data(modelIndex, Qt::DisplayRole).toString();
+
+        const auto item = tableViewPrivate->loadedTableItem(fxItem->cell)->item;
+        const QString modelDataBindingProperty = item->property(kModelDataBindingProp).toString();
+
+        QCOMPARE(modelDataBindingProperty, expectedModelData);
+    }
+
+    // Change four cells in one go
+    model.setModelData(QPoint(1, 0), QSize(2, 2), prefix);
+
+    for (auto fxItem : tableViewPrivate->loadedItems) {
+        const QPoint cell = fxItem->cell;
+        const auto modelIndex = model.index(cell.y(), cell.x());
+        QString expectedModelData = model.data(modelIndex, Qt::DisplayRole).toString();
+
+        const auto item = tableViewPrivate->loadedTableItem(fxItem->cell)->item;
+        const QString modelDataBindingProperty = item->property(kModelDataBindingProp).toString();
+
+        QCOMPARE(modelDataBindingProperty, expectedModelData);
+    }
+}
+
 void tst_QQuickTableView::checkIfDelegatesAreReused_data()
 {
     QTest::addColumn<bool>("reuseItems");