From c8347573ba009e1cae96f4569a8838da597849de Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 27 Jan 2022 09:46:56 +0100 Subject: QAbstractItemView: code tidies In preparation for an upcoming fix, refactor an if over an enumerator to a switch (which is how this code should've been to begin with). Change-Id: I11a2de6d66f0359b985b587b7fd37022a7bf56e6 Reviewed-by: David Faure (cherry picked from commit 22ac61e62a1a65bf0049229ab6fcaa44e8688951) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/itemviews/qabstractitemview.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) (limited to 'src/widgets/itemviews/qabstractitemview.cpp') diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp index 80a3ecb09f..92f25e5025 100644 --- a/src/widgets/itemviews/qabstractitemview.cpp +++ b/src/widgets/itemviews/qabstractitemview.cpp @@ -1169,12 +1169,20 @@ QModelIndex QAbstractItemView::rootIndex() const void QAbstractItemView::selectAll() { Q_D(QAbstractItemView); - SelectionMode mode = d->selectionMode; - if (mode == MultiSelection || mode == ExtendedSelection) + const SelectionMode mode = d->selectionMode; + switch (mode) { + case MultiSelection: + case ExtendedSelection: d->selectAll(QItemSelectionModel::ClearAndSelect - |d->selectionBehaviorFlags()); - else if (mode != SingleSelection) + | d->selectionBehaviorFlags()); + break; + case NoSelection: + case ContiguousSelection: d->selectAll(selectionCommand(d->model->index(0, 0, d->root))); + break; + case SingleSelection: + break; + } } /*! -- cgit v1.2.3 From 9a5ab05e2b19d1aebb1e9d8c84b2c8889fe6d5ba Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 27 Jan 2022 09:47:59 +0100 Subject: QAbstractItemView: do not access invalid model indices (1/N) Calling selectAll() on a view with an empty model, with the view in ContiguousSelection, causes an out of bounds access into the model. Guard the access. Change-Id: I3830a979bad760e9e1526c1c8b12d9767d41fc99 Reviewed-by: David Faure (cherry picked from commit 33ad8b6fa9afbe4b8612f26c0bad42d23d94e7b2) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/itemviews/qabstractitemview.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/widgets/itemviews/qabstractitemview.cpp') diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp index 92f25e5025..f2ee9cf09f 100644 --- a/src/widgets/itemviews/qabstractitemview.cpp +++ b/src/widgets/itemviews/qabstractitemview.cpp @@ -1178,7 +1178,8 @@ void QAbstractItemView::selectAll() break; case NoSelection: case ContiguousSelection: - d->selectAll(selectionCommand(d->model->index(0, 0, d->root))); + if (d->model->hasChildren(d->root)) + d->selectAll(selectionCommand(d->model->index(0, 0, d->root))); break; case SingleSelection: break; -- cgit v1.2.3 From 20ae80e33d3ee09ce5871f49fc7454ea1220c7d8 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 27 Jan 2022 09:47:59 +0100 Subject: QAbstractItemView: do not access invalid model indices (2/N) Similar to the parent patch, the private selectAll() was doing two out of bounds accesses on an empty model. Guard it. Change-Id: If0f3ce1e6c44a152791313e47db79985e71ef955 Reviewed-by: David Faure (cherry picked from commit 035babe5020cfeafdaf5e8590f4c5c3ac043a5bf) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/itemviews/qabstractitemview.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/widgets/itemviews/qabstractitemview.cpp') diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp index f2ee9cf09f..e182693a26 100644 --- a/src/widgets/itemviews/qabstractitemview.cpp +++ b/src/widgets/itemviews/qabstractitemview.cpp @@ -4507,6 +4507,8 @@ void QAbstractItemViewPrivate::selectAll(QItemSelectionModel::SelectionFlags com { if (!selectionModel) return; + if (!model->hasChildren(root)) + return; QItemSelection selection; QModelIndex tl = model->index(0, 0, root); -- cgit v1.2.3 From ed6bbda1c97793e0ef128d8e90158953ae1f7115 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Thu, 27 Jan 2022 09:47:59 +0100 Subject: QAbstractItemView: do not access invalid model indices (3/N) The slots connected to rowsAboutToBeRemoved and columnsAboutToBeRemoved attempt to find a new current index by scanning the model around the existing current index -- in case that index is indeed about to be removed. The problem is that the scanning was done without any sorts of bounds checking; instead it was relying on the model's index() to return an invalid index for an out of bounds request. Fix that by adding bounds checking. Since models are not supposed to return invalid indices for in-bounds index() calls, added some warnings if that happens. For some reason, the code handling rows and columns isn't perfectly symmetrical. Rows are searched both forwards and backwards, while columns only backwards, and the related code is slightly different. Filed QTBUG-100273 to try and understand what's going on. Change-Id: I7452d8c521e74daa4408e6cc969ce5a6059f53ea Reviewed-by: David Faure Reviewed-by: Qt CI Bot Reviewed-by: Richard Moe Gustavsen (cherry picked from commit 453f4692162eed17d4f618beddc5eb27e123f6eb) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/itemviews/qabstractitemview.cpp | 48 ++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 7 deletions(-) (limited to 'src/widgets/itemviews/qabstractitemview.cpp') diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp index e182693a26..96d31521f1 100644 --- a/src/widgets/itemviews/qabstractitemview.cpp +++ b/src/widgets/itemviews/qabstractitemview.cpp @@ -3417,15 +3417,39 @@ void QAbstractItemView::rowsAboutToBeRemoved(const QModelIndex &parent, int star } else { int row = end + 1; QModelIndex next; - do { // find the next visible and enabled item + const int rowCount = d->model->rowCount(parent); + bool found = false; + // find the next visible and enabled item + while (row < rowCount && !found) { next = d->model->index(row++, current.column(), current.parent()); - } while (next.isValid() && (isIndexHidden(next) || !d->isIndexEnabled(next))); - if (row > d->model->rowCount(parent)) { +#ifdef QT_DEBUG + if (!next.isValid()) { + qWarning("Model unexpectedly returned an invalid index"); + break; + } +#endif + if (!isIndexHidden(next) && d->isIndexEnabled(next)) { + found = true; + break; + } + } + + if (!found) { row = start - 1; - do { // find the previous visible and enabled item + // find the previous visible and enabled item + while (row >= 0) { next = d->model->index(row--, current.column(), current.parent()); - } while (next.isValid() && (isIndexHidden(next) || !d->isIndexEnabled(next))); +#ifdef QT_DEBUG + if (!next.isValid()) { + qWarning("Model unexpectedly returned an invalid index"); + break; + } +#endif + if (!isIndexHidden(next) && d->isIndexEnabled(next)) + break; + } } + setCurrentIndex(next); } } @@ -3503,9 +3527,19 @@ void QAbstractItemViewPrivate::_q_columnsAboutToBeRemoved(const QModelIndex &par } else { int column = end; QModelIndex next; - do { // find the next visible and enabled item + const int columnCount = model->columnCount(current.parent()); + // find the next visible and enabled item + while (column < columnCount) { next = model->index(current.row(), column++, current.parent()); - } while (next.isValid() && (q->isIndexHidden(next) || !isIndexEnabled(next))); +#ifdef QT_DEBUG + if (!next.isValid()) { + qWarning("Model unexpectedly returned an invalid index"); + break; + } +#endif + if (!q->isIndexHidden(next) && isIndexEnabled(next)) + break; + } q->setCurrentIndex(next); } } -- cgit v1.2.3