TableView: ensure we update content size upon model changes

For tables of non-trivial sizes, we usually don't know what the
content size will be unless we load all rows and columns, which
we simply cannot do. Because of this, we have up till now chosen
a strategy where we normally just calculate a predicted content
size up-front, when we table is built, and afterwards just stick
to that prediction.

This strategy works for big tables that fills more than one size
of the viewport, and if the number of rows and column in the model
stays around the same. But for tables that start off smaller than
the viewport, and later expands to grow out of it, it simply fails.
And the failure is such that the tableview can get stuck, with no
way way for the user to flick around to see the rest of the contents.

An example is TreeView that might only show the root node at
start-up, but as you start to expand the tree, it will quickly add
more rows than what fits inside the viewport. And in that case, the
contentHeight will be totally off, and in turn, make the scrollbar
be based on wrong values, and sometimes not work at all (e.g if
it has the flag Flickable::StopAtBounds).

This patch will change the implementation so that we recalculate
the content size whenever it should logially change. That is, if
e.g the model add or remove rows and columns, or if you change
spacing. This still doesn't mean that contentWidth/Height reports
the correct size of the table, but at least it will be a better
guestimate for smaller tables, and at the same time, work
together with Flickable and ScrollBars.

Pick-to: 5.15
Fixes: QTBUG-87680
Change-Id: Ie2d2e7c1f1519dc7a5d5269a6d25e34cf441b3fe
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

3 files changed, 152 insertions, 41 deletions

diff --git a/src/quick/items/qquicktableview.cpp b/src/quick/items/qquicktableview.cpp
index 8f0c783c80..9a0a21cccd 100644
--- a/src/quick/items/qquicktableview.cpp
+++ b/src/quick/items/qquicktableview.cpp
@@ -782,6 +782,15 @@ int QQuickTableViewPrivate::nextVisibleEdgeIndex(Qt::Edge edge, int startIndex)
 
 void QQuickTableViewPrivate::updateContentWidth()
 {
+    // Note that we actually never really know what the content size / size of the full table will
+    // be. Even if e.g spacing changes, and we normally would assume that the size of the table
+    // would increase accordingly, the model might also at some point have removed/hidden/resized
+    // rows/columns outside the viewport. This would also affect the size, but since we don't load
+    // rows or columns outside the viewport, this information is ignored. And even if we did, we
+    // might also have been fast-flicked to a new location at some point, and started a new rebuild
+    // there based on a new guesstimated top-left cell. So the calculated content size should always
+    // be understood as a guesstimate, which sometimes can be really off (as a tradeoff for performance).
+    // When this is not acceptable, the user can always set a custom content size explicitly.
     Q_Q(QQuickTableView);
 
     if (syncHorizontally) {
@@ -1022,7 +1031,7 @@ void QQuickTableViewPrivate::updateExtents()
     }
 }
 
-void QQuickTableViewPrivate::updateAverageEdgeSize()
+void QQuickTableViewPrivate::updateAverageColumnWidth()
 {
     if (explicitContentWidth.isValid()) {
         const qreal accColumnSpacing = (tableSize.width() - 1) * cellSpacing.width();
@@ -1031,7 +1040,10 @@ void QQuickTableViewPrivate::updateAverageEdgeSize()
         const qreal accColumnSpacing = (loadedColumns.count() - 1) * cellSpacing.width();
         averageEdgeSize.setWidth((loadedTableOuterRect.width() - accColumnSpacing) / loadedColumns.count());
     }
+}
 
+void QQuickTableViewPrivate::updateAverageRowHeight()
+{
     if (explicitContentHeight.isValid()) {
         const qreal accRowSpacing = (tableSize.height() - 1) * cellSpacing.height();
         averageEdgeSize.setHeight((explicitContentHeight - accRowSpacing) / tableSize.height());
@@ -1121,9 +1133,15 @@ void QQuickTableViewPrivate::forceLayout()
         // the model is updated, but before we're notified about it.
         rebuildOptions = RebuildOption::All;
     } else {
-        rebuildOptions = checkForVisibilityChanges();
-        if (!rebuildOptions)
-            rebuildOptions = RebuildOption::LayoutOnly;
+        // Resizing a column (or row) can result in the table going from being
+        // e.g completely inside the viewport to go outside. And in the latter
+        // case, the user needs to be able to scroll the viewport, also if
+        // flags such as Flickable.StopAtBounds is in use. So we need to
+        // update contentWidth/Height to support that case.
+        rebuildOptions = RebuildOption::LayoutOnly
+                | RebuildOption::CalculateNewContentWidth
+                | RebuildOption::CalculateNewContentHeight
+                | checkForVisibilityChanges();
     }
 
     scheduleRebuildTable(rebuildOptions);
@@ -2116,20 +2134,13 @@ void QQuickTableViewPrivate::layoutAfterLoadingInitialTable()
     relayoutTableItems();
     syncLoadedTableRectFromLoadedTable();
 
-    if (syncView || rebuildOptions.testFlag(RebuildOption::All)) {
-        // We try to limit how often we update the content size. The main reason is that is has a
-        // tendency to cause flicker in the viewport if it happens while flicking. But another just
-        // as valid reason is that we actually never really know what the size of the full table will
-        // ever be. Even if e.g spacing changes, and we normally would assume that the size of the table
-        // would increase accordingly, the model might also at some point have removed/hidden/resized
-        // rows/columns outside the viewport. This would also affect the size, but since we don't load
-        // rows or columns outside the viewport, this information is ignored. And even if we did, we
-        // might also have been fast-flicked to a new location at some point, and started a new rebuild
-        // there based on a new guesstimated top-left cell. Either way, changing the content size
-        // based on the currently visible row/columns/spacing can be really off. So instead of pretending
-        // that we know what the actual size of the table is, we just keep the first guesstimate.
-        updateAverageEdgeSize();
+    if (rebuildOptions.testFlag(RebuildOption::CalculateNewContentWidth)) {
+        updateAverageColumnWidth();
         updateContentWidth();
+    }
+
+    if (rebuildOptions.testFlag(RebuildOption::CalculateNewContentHeight)) {
+        updateAverageRowHeight();
         updateContentHeight();
     }
 
@@ -2575,6 +2586,8 @@ void QQuickTableViewPrivate::syncRebuildOptions()
     if (rebuildOptions.testFlag(RebuildOption::All)) {
         rebuildOptions.setFlag(RebuildOption::ViewportOnly, false);
         rebuildOptions.setFlag(RebuildOption::LayoutOnly, false);
+        rebuildOptions.setFlag(RebuildOption::CalculateNewContentWidth);
+        rebuildOptions.setFlag(RebuildOption::CalculateNewContentHeight);
     } else if (rebuildOptions.testFlag(RebuildOption::ViewportOnly)) {
         rebuildOptions.setFlag(RebuildOption::LayoutOnly, false);
     }
@@ -2679,10 +2692,15 @@ void QQuickTableViewPrivate::syncSyncView()
     syncHorizontally = syncView && assignedSyncDirection & Qt::Horizontal;
     syncVertically = syncView && assignedSyncDirection & Qt::Vertical;
 
-    if (syncHorizontally)
+    if (syncHorizontally) {
         q->setColumnSpacing(syncView->columnSpacing());
-    if (syncVertically)
+        updateContentWidth();
+    }
+
+    if (syncVertically) {
         q->setRowSpacing(syncView->rowSpacing());
+        updateContentHeight();
+    }
 
     if (syncView && loadedItems.isEmpty() && !tableSize.isEmpty()) {
         // When we have a syncView, we can sometimes temporarily end up with no loaded items.
@@ -2774,7 +2792,9 @@ void QQuickTableViewPrivate::modelUpdated(const QQmlChangeSet &changeSet, bool r
     Q_UNUSED(reset);
 
     Q_TABLEVIEW_ASSERT(!model->abstractItemModel(), "");
-    scheduleRebuildTable(RebuildOption::ViewportOnly);
+    scheduleRebuildTable(RebuildOption::ViewportOnly
+                         | RebuildOption::CalculateNewContentWidth
+                         | RebuildOption::CalculateNewContentHeight);
 }
 
 void QQuickTableViewPrivate::rowsMovedCallback(const QModelIndex &parent, int, int, const QModelIndex &, int )
@@ -2798,7 +2818,7 @@ void QQuickTableViewPrivate::rowsInsertedCallback(const QModelIndex &parent, int
     if (parent != QModelIndex())
         return;
 
-    scheduleRebuildTable(RebuildOption::ViewportOnly);
+    scheduleRebuildTable(RebuildOption::ViewportOnly | RebuildOption::CalculateNewContentHeight);
 }
 
 void QQuickTableViewPrivate::rowsRemovedCallback(const QModelIndex &parent, int, int)
@@ -2806,7 +2826,7 @@ void QQuickTableViewPrivate::rowsRemovedCallback(const QModelIndex &parent, int,
     if (parent != QModelIndex())
         return;
 
-    scheduleRebuildTable(RebuildOption::ViewportOnly);
+    scheduleRebuildTable(RebuildOption::ViewportOnly | RebuildOption::CalculateNewContentHeight);
 }
 
 void QQuickTableViewPrivate::columnsInsertedCallback(const QModelIndex &parent, int, int)
@@ -2814,7 +2834,12 @@ void QQuickTableViewPrivate::columnsInsertedCallback(const QModelIndex &parent,
     if (parent != QModelIndex())
         return;
 
-    scheduleRebuildTable(RebuildOption::ViewportOnly);
+    // Adding a column (or row) can result in the table going from being
+    // e.g completely inside the viewport to go outside. And in the latter
+    // case, the user needs to be able to scroll the viewport, also if
+    // flags such as Flickable.StopAtBounds is in use. So we need to
+    // update contentWidth to support that case.
+    scheduleRebuildTable(RebuildOption::ViewportOnly | RebuildOption::CalculateNewContentWidth);
 }
 
 void QQuickTableViewPrivate::columnsRemovedCallback(const QModelIndex &parent, int, int)
@@ -2822,7 +2847,7 @@ void QQuickTableViewPrivate::columnsRemovedCallback(const QModelIndex &parent, i
     if (parent != QModelIndex())
         return;
 
-    scheduleRebuildTable(RebuildOption::ViewportOnly);
+    scheduleRebuildTable(RebuildOption::ViewportOnly | RebuildOption::CalculateNewContentWidth);
 }
 
 void QQuickTableViewPrivate::layoutChangedCallback(const QList<QPersistentModelIndex> &parents, QAbstractItemModel::LayoutChangeHint hint)
@@ -2853,6 +2878,9 @@ void QQuickTableViewPrivate::scheduleRebuildIfFastFlick()
     // strategy from refilling edges around the current table to instead rebuild the table
     // from scratch inside the new viewport. This will greatly improve performance when flicking
     // a long distance in one go, which can easily happen when dragging on scrollbars.
+    // Note that we don't want to update the content size in this case, since first of all, the
+    // content size should logically not change as a result of flicking. But more importantly, updating
+    // the content size in combination with fast-flicking has a tendency to cause flicker in the viewport.
 
     // Check the viewport moved more than one page vertically
     if (!viewportRect.intersects(QRectF(viewportRect.x(), q->contentY(), 1, q->height()))) {
@@ -2991,7 +3019,8 @@ void QQuickTableView::setRowSpacing(qreal spacing)
         return;
 
     d->cellSpacing.setHeight(spacing);
-    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::LayoutOnly);
+    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::LayoutOnly
+                            | QQuickTableViewPrivate::RebuildOption::CalculateNewContentHeight);
     emit rowSpacingChanged();
 }
 
@@ -3009,7 +3038,8 @@ void QQuickTableView::setColumnSpacing(qreal spacing)
         return;
 
     d->cellSpacing.setWidth(spacing);
-    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::LayoutOnly);
+    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::LayoutOnly
+                            | QQuickTableViewPrivate::RebuildOption::CalculateNewContentWidth);
     emit columnSpacingChanged();
 }
 
@@ -3025,7 +3055,8 @@ void QQuickTableView::setRowHeightProvider(const QJSValue &provider)
         return;
 
     d->rowHeightProvider = provider;
-    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::ViewportOnly);
+    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::ViewportOnly
+                            | QQuickTableViewPrivate::RebuildOption::CalculateNewContentHeight);
     emit rowHeightProviderChanged();
 }
 
@@ -3041,7 +3072,8 @@ void QQuickTableView::setColumnWidthProvider(const QJSValue &provider)
         return;
 
     d->columnWidthProvider = provider;
-    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::ViewportOnly);
+    d->scheduleRebuildTable(QQuickTableViewPrivate::RebuildOption::ViewportOnly
+                            | QQuickTableViewPrivate::RebuildOption::CalculateNewContentWidth);
     emit columnWidthProviderChanged();
 }
 
diff --git a/src/quick/items/qquicktableview_p_p.h b/src/quick/items/qquicktableview_p_p.h
index 876a175d87..6a8c5fa7fa 100644
--- a/src/quick/items/qquicktableview_p_p.h
+++ b/src/quick/items/qquicktableview_p_p.h
@@ -213,8 +213,10 @@ public:
         ViewportOnly = 0x4,
         CalculateNewTopLeftRow = 0x8,
         CalculateNewTopLeftColumn = 0x10,
-        PositionViewAtRow = 0x20,
-        PositionViewAtColumn = 0x40,
+        CalculateNewContentWidth = 0x20,
+        CalculateNewContentHeight = 0x40,
+        PositionViewAtRow = 0x80,
+        PositionViewAtColumn = 0x100,
     };
     Q_DECLARE_FLAGS(RebuildOptions, RebuildOption)
 
@@ -371,7 +373,8 @@ public:
 
     void updateContentWidth();
     void updateContentHeight();
-    void updateAverageEdgeSize();
+    void updateAverageColumnWidth();
+    void updateAverageRowHeight();
     RebuildOptions checkForVisibilityChanges();
     void forceLayout();
 
diff --git a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
index 6453180999..0375b74386 100644
--- a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
+++ b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
@@ -188,6 +188,8 @@ private slots:
     void itemAtCell();
     void leftRightTopBottomProperties_data();
     void leftRightTopBottomProperties();
+    void checkContentSize_data();
+    void checkContentSize();
 };
 
 tst_QQuickTableView::tst_QQuickTableView()
@@ -600,6 +602,12 @@ void tst_QQuickTableView::checkForceLayoutEndUpDoingALayout()
 
     for (auto fxItem : tableViewPrivate->loadedItems)
         QCOMPARE(fxItem->item->height(), newDelegateSize);
+
+    // Check that the content height has been updated as well
+    const qreal rowSpacing = tableView->rowSpacing();
+    const qreal colSpacing = tableView->columnSpacing();
+    QCOMPARE(tableView->contentWidth(), (10 * (newDelegateSize + colSpacing)) - colSpacing);
+    QCOMPARE(tableView->contentHeight(), (9 * (newDelegateSize + rowSpacing)) - rowSpacing);
 }
 
 void tst_QQuickTableView::checkForceLayoutDuringModelChange()
@@ -1305,10 +1313,10 @@ void tst_QQuickTableView::checkSpacingValues()
     tableView->polish();
     WAIT_UNTIL_POLISHED;
 
-    const qreal expectedInitialContentWidth = columnCount * (delegateWidth + tableView->columnSpacing()) - tableView->columnSpacing();
-    const qreal expectedInitialContentHeight = rowCount * (delegateHeight + tableView->rowSpacing()) - tableView->rowSpacing();
-    QCOMPARE(tableView->contentWidth(), expectedInitialContentWidth);
-    QCOMPARE(tableView->contentHeight(), expectedInitialContentHeight);
+    qreal expectedContentWidth = columnCount * (delegateWidth + tableView->columnSpacing()) - tableView->columnSpacing();
+    qreal expectedContentHeight = rowCount * (delegateHeight + tableView->rowSpacing()) - tableView->rowSpacing();
+    QCOMPARE(tableView->contentWidth(), expectedContentWidth);
+    QCOMPARE(tableView->contentHeight(), expectedContentHeight);
 
     // Valid spacing assignment
     tableView->setRowSpacing(42);
@@ -1319,12 +1327,10 @@ void tst_QQuickTableView::checkSpacingValues()
     tableView->polish();
     WAIT_UNTIL_POLISHED;
 
-    // Even after changing spacing, TableView will keep the initial guesstimated content size. The
-    // reason is that deciding the content size based on the currently visible row/columns/spacing
-    // will almost always be at a little bit wrong at best. So instead of pretending that TableView
-    // knows what the size of the full table is, it sticks with the first guesstimate.
-    QCOMPARE(tableView->contentWidth(), expectedInitialContentWidth);
-    QCOMPARE(tableView->contentHeight(), expectedInitialContentHeight);
+    expectedContentWidth = columnCount * (delegateWidth + tableView->columnSpacing()) - tableView->columnSpacing();
+    expectedContentHeight = rowCount * (delegateHeight + tableView->rowSpacing()) - tableView->rowSpacing();
+    QCOMPARE(tableView->contentWidth(), expectedContentWidth);
+    QCOMPARE(tableView->contentHeight(), expectedContentHeight);
 
     // Negative spacing is allowed, and can be used to eliminate double edges
     // in the grid if the delegate is a rectangle with a border.
@@ -3113,6 +3119,76 @@ void tst_QQuickTableView::leftRightTopBottomProperties()
     QCOMPARE(bottomSpy.count(), expectedSignalCount.bottom());
 }
 
+void tst_QQuickTableView::checkContentSize_data()
+{
+    QTest::addColumn<int>("rowCount");
+    QTest::addColumn<int>("colCount");
+
+    QTest::newRow("4x4") << 4 << 4;
+    QTest::newRow("100x100") << 100 << 100;
+    QTest::newRow("0x0") << 0 << 0;
+}
+
+void tst_QQuickTableView::checkContentSize()
+{
+    QFETCH(int, rowCount);
+    QFETCH(int, colCount);
+
+    // Check that the content size is initially correct, and that
+    // it updates when we change e.g the model or spacing (QTBUG-87680)
+    LOAD_TABLEVIEW("plaintableview.qml");
+
+    TestModel model(rowCount, colCount);
+    tableView->setModel(QVariant::fromValue(&model));
+    tableView->setRowSpacing(1);
+    tableView->setColumnSpacing(2);
+
+    WAIT_UNTIL_POLISHED;
+
+    const qreal delegateWidth = 100;
+    const qreal delegateHeight = 50;
+    qreal colSpacing = tableView->columnSpacing();
+    qreal rowSpacing = tableView->rowSpacing();
+
+    // Check that content size has the exepected initial values
+    QCOMPARE(tableView->contentWidth(), colCount == 0 ? 0 : (colCount * (delegateWidth + colSpacing)) - colSpacing);
+    QCOMPARE(tableView->contentHeight(), rowCount == 0 ? 0 : (rowCount * (delegateHeight + rowSpacing)) - rowSpacing);
+
+    // Set no spacing
+    rowSpacing = 0;
+    colSpacing = 0;
+    tableView->setRowSpacing(rowSpacing);
+    tableView->setColumnSpacing(colSpacing);
+    WAIT_UNTIL_POLISHED;
+    QCOMPARE(tableView->contentWidth(), colCount * delegateWidth);
+    QCOMPARE(tableView->contentHeight(), rowCount * delegateHeight);
+
+    // Set typical spacing values
+    rowSpacing = 2;
+    colSpacing = 3;
+    tableView->setRowSpacing(rowSpacing);
+    tableView->setColumnSpacing(colSpacing);
+    WAIT_UNTIL_POLISHED;
+    QCOMPARE(tableView->contentWidth(), colCount == 0 ? 0 : (colCount * (delegateWidth + colSpacing)) - colSpacing);
+    QCOMPARE(tableView->contentHeight(), rowCount == 0 ? 0 : (rowCount * (delegateHeight + rowSpacing)) - rowSpacing);
+
+    // Add a row and a column
+    model.insertRow(0);
+    model.insertColumn(0);
+    rowCount = model.rowCount();
+    colCount = model.columnCount();
+    WAIT_UNTIL_POLISHED;
+    QCOMPARE(tableView->contentWidth(), (colCount * (delegateWidth + colSpacing)) - colSpacing);
+    QCOMPARE(tableView->contentHeight(), (rowCount * (delegateHeight + rowSpacing)) - rowSpacing);
+
+    // Remove a row (this should also make affect contentWidth if rowCount becomes 0)
+    model.removeRow(0);
+    rowCount = model.rowCount();
+    WAIT_UNTIL_POLISHED;
+    QCOMPARE(tableView->contentWidth(), rowCount == 0 ? 0 : (colCount * (delegateWidth + colSpacing)) - colSpacing);
+    QCOMPARE(tableView->contentHeight(), rowCount == 0 ? 0 : (rowCount * (delegateHeight + rowSpacing)) - rowSpacing);
+}
+
 QTEST_MAIN(tst_QQuickTableView)
 
 #include "tst_qquicktableview.moc"