From d58532963bee6af2ccccd9882cda18e7bc9b2430 Mon Sep 17 00:00:00 2001 From: Richard Moe Gustavsen Date: Thu, 6 May 2021 11:20:53 +0200 Subject: qquicktableview: upon forceLayout(), check for visible rows/columns at the origin There is a bug in TableView which will stop the user from scrolling/flicking back to the first column if it has become visible after first being hidden. The reason is that this is somewhat of a special case that happens only if the current left column is already at the origin of the viewport, since that will fool tableview into thinking that there can be no more columns in front of it. This patch add an extra section to the function that checks for visibility changes, to detect this special case. Fixes: QTBUG-93264 Change-Id: Ieaad507b45ea11dc231519e9f49cbf182d6443ba Reviewed-by: Mitch Curtis (cherry picked from commit 546df684e272bbbc5e8b871ae9b224fdb34a4cfa) Reviewed-by: Qt Cherry-pick Bot --- src/quick/items/qquicktableview.cpp | 78 +++++++++++++--------- .../quick/qquicktableview/tst_qquicktableview.cpp | 78 ++++++++++++++++++++++ 2 files changed, 126 insertions(+), 30 deletions(-) diff --git a/src/quick/items/qquicktableview.cpp b/src/quick/items/qquicktableview.cpp index 63bedb9734..3f06684f25 100644 --- a/src/quick/items/qquicktableview.cpp +++ b/src/quick/items/qquicktableview.cpp @@ -1105,43 +1105,61 @@ QQuickTableViewPrivate::RebuildOptions QQuickTableViewPrivate::checkForVisibilit return RebuildOption::None; } - // Go through all columns from first to last, find the columns that used - // to be hidden and not loaded, and check if they should become visible - // (and vice versa). If there is a change, we need to rebuild. RebuildOptions rebuildOptions = RebuildOption::None; - for (int column = leftColumn(); column <= rightColumn(); ++column) { - const bool wasVisibleFromBefore = loadedColumns.contains(column); - const bool isVisibleNow = !qFuzzyIsNull(getColumnWidth(column)); - if (wasVisibleFromBefore == isVisibleNow) - continue; - - // A column changed visibility. This means that it should - // either be loaded or unloaded. So we need a rebuild. - qCDebug(lcTableViewDelegateLifecycle) << "Column" << column << "changed visibility to" << isVisibleNow; + if (loadedTableOuterRect.x() == origin.x() && leftColumn() != 0) { + // Since the left column is at the origin of the viewport, but still not the first + // column in the model, we need to calculate a new left column since there might be + // columns in front of it that used to be hidden, but should now be visible (QTBUG-93264). rebuildOptions.setFlag(RebuildOption::ViewportOnly); - if (column == leftColumn()) { - // The first loaded column should now be hidden. This means that we - // need to calculate which column should now be first instead. - rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftColumn); + rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftColumn); + } else { + // Go through all loaded columns from first to last, find the columns that used + // to be hidden and not loaded, and check if they should become visible + // (and vice versa). If there is a change, we need to rebuild. + for (int column = leftColumn(); column <= rightColumn(); ++column) { + const bool wasVisibleFromBefore = loadedColumns.contains(column); + const bool isVisibleNow = !qFuzzyIsNull(getColumnWidth(column)); + if (wasVisibleFromBefore == isVisibleNow) + continue; + + // A column changed visibility. This means that it should + // either be loaded or unloaded. So we need a rebuild. + qCDebug(lcTableViewDelegateLifecycle) << "Column" << column << "changed visibility to" << isVisibleNow; + rebuildOptions.setFlag(RebuildOption::ViewportOnly); + if (column == leftColumn()) { + // The first loaded column should now be hidden. This means that we + // need to calculate which column should now be first instead. + rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftColumn); + } + break; } - break; } - // Go through all rows from first to last, and do the same as above - for (int row = topRow(); row <= bottomRow(); ++row) { - const bool wasVisibleFromBefore = loadedRows.contains(row); - const bool isVisibleNow = !qFuzzyIsNull(getRowHeight(row)); - if (wasVisibleFromBefore == isVisibleNow) - continue; - - // A row changed visibility. This means that it should - // either be loaded or unloaded. So we need a rebuild. - qCDebug(lcTableViewDelegateLifecycle) << "Row" << row << "changed visibility to" << isVisibleNow; + if (loadedTableOuterRect.y() == origin.y() && topRow() != 0) { + // Since the top row is at the origin of the viewport, but still not the first + // row in the model, we need to calculate a new top row since there might be + // rows in front of it that used to be hidden, but should now be visible (QTBUG-93264). rebuildOptions.setFlag(RebuildOption::ViewportOnly); - if (row == topRow()) - rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftRow); - break; + rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftRow); + } else { + // Go through all loaded rows from first to last, find the rows that used + // to be hidden and not loaded, and check if they should become visible + // (and vice versa). If there is a change, we need to rebuild. + for (int row = topRow(); row <= bottomRow(); ++row) { + const bool wasVisibleFromBefore = loadedRows.contains(row); + const bool isVisibleNow = !qFuzzyIsNull(getRowHeight(row)); + if (wasVisibleFromBefore == isVisibleNow) + continue; + + // A row changed visibility. This means that it should + // either be loaded or unloaded. So we need a rebuild. + qCDebug(lcTableViewDelegateLifecycle) << "Row" << row << "changed visibility to" << isVisibleNow; + rebuildOptions.setFlag(RebuildOption::ViewportOnly); + if (row == topRow()) + rebuildOptions.setFlag(RebuildOption::CalculateNewTopLeftRow); + break; + } } return rebuildOptions; diff --git a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp index 005aae0639..60ba28ec08 100644 --- a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp +++ b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp @@ -169,6 +169,8 @@ private slots: void checkTableviewInsideAsyncLoader(); void hideRowsAndColumns_data(); void hideRowsAndColumns(); + void hideAndShowFirstColumn(); + void hideAndShowFirstRow(); void checkThatRevisionedPropertiesCannotBeUsedInOldImports(); void checkSyncView_rootView_data(); void checkSyncView_rootView(); @@ -2423,6 +2425,82 @@ void tst_QQuickTableView::hideRowsAndColumns() QVERIFY(!columnsToHideList.contains(column)); } +void tst_QQuickTableView::hideAndShowFirstColumn() +{ + // Check that if we hide the first column, it will move + // the second column to the origin of the viewport. Then check + // that if we show the first column again, it will reappear at + // the origin of the viewport, and as such, pushing the second + // column to the right of it. + LOAD_TABLEVIEW("hiderowsandcolumns.qml"); + + const int modelSize = 5; + auto model = TestModelAsVariant(modelSize, modelSize); + tableView->setModel(model); + + // Start by making the first column hidden + const auto columnsToHideList = QList() << 0; + view->rootObject()->setProperty("columnsToHide", QVariant::fromValue(columnsToHideList)); + + WAIT_UNTIL_POLISHED; + + const int expectedColumnCount = modelSize - columnsToHideList.count(); + QCOMPARE(tableViewPrivate->loadedColumns.count(), expectedColumnCount); + QCOMPARE(tableViewPrivate->leftColumn(), 1); + QCOMPARE(tableView->contentX(), 0); + QCOMPARE(tableViewPrivate->loadedTableOuterRect.x(), 0); + + // Make the first column in the model visible again + const auto emptyList = QList(); + view->rootObject()->setProperty("columnsToHide", QVariant::fromValue(emptyList)); + tableView->forceLayout(); + + WAIT_UNTIL_POLISHED; + + QCOMPARE(tableViewPrivate->loadedColumns.count(), modelSize); + QCOMPARE(tableViewPrivate->leftColumn(), 0); + QCOMPARE(tableView->contentX(), 0); + QCOMPARE(tableViewPrivate->loadedTableOuterRect.x(), 0); +} + +void tst_QQuickTableView::hideAndShowFirstRow() +{ + // Check that if we hide the first row, it will move + // the second row to the origin of the viewport. Then check + // that if we show the first row again, it will reappear at + // the origin of the viewport, and as such, pushing the second + // row below it. + LOAD_TABLEVIEW("hiderowsandcolumns.qml"); + + const int modelSize = 5; + auto model = TestModelAsVariant(modelSize, modelSize); + tableView->setModel(model); + + // Start by making the first row hidden + const auto rowsToHideList = QList() << 0; + view->rootObject()->setProperty("rowsToHide", QVariant::fromValue(rowsToHideList)); + + WAIT_UNTIL_POLISHED; + + const int expectedRowsCount = modelSize - rowsToHideList.count(); + QCOMPARE(tableViewPrivate->loadedRows.count(), expectedRowsCount); + QCOMPARE(tableViewPrivate->topRow(), 1); + QCOMPARE(tableView->contentY(), 0); + QCOMPARE(tableViewPrivate->loadedTableOuterRect.y(), 0); + + // Make the first row in the model visible again + const auto emptyList = QList(); + view->rootObject()->setProperty("rowsToHide", QVariant::fromValue(emptyList)); + tableView->forceLayout(); + + WAIT_UNTIL_POLISHED; + + QCOMPARE(tableViewPrivate->loadedRows.count(), modelSize); + QCOMPARE(tableViewPrivate->topRow(), 0); + QCOMPARE(tableView->contentY(), 0); + QCOMPARE(tableViewPrivate->loadedTableOuterRect.y(), 0); +} + void tst_QQuickTableView::checkThatRevisionedPropertiesCannotBeUsedInOldImports() { // Check that if you use a QQmlAdaptorModel together with a Repeater, the -- cgit v1.2.3