diff options
author | Andreas Buhr <andreas.buhr@qt.io> | 2021-06-02 16:29:44 +0200 |
---|---|---|
committer | Andreas Buhr <andreas.buhr@qt.io> | 2021-07-15 18:09:12 +0200 |
commit | 0c2125458a9fdddaf3385b257ba4350da872a1d1 (patch) | |
tree | 65c30aaf3436ec6e7571d43be24b2ec1c2c682eb | |
parent | 4cd2cca553ba2fdcd6cd59c2cd055fc8e6e83ae4 (diff) |
Consistent handling of disabled items in QItemSelectionModel
In QItemSelectionModel, items which are disabled or marked as not
selectable should not be considered as selected. But this was
not handled consistently.
The following methods considered only items which are enabled and
marked selectable: selectedIndexes(), rowIntersectsSelection(), and
columnIntersectsSelection(). The following methods considered only
items which are marked selectable, but did not check whether they
are enabled: selectedRows(), selectedColumns(), isRowSelected(),
isColumnSelected(), isSelected(). Finally there is hasSelection(),
which did not check for enabled nor for selectable.
This patch introduces consistent behavior. All methods check
both whether the items are enabled and whether they are selectable now.
[ChangeLog][QtCore][QItemSelectionModel][Important Behavior Changes]
All methods in QItemSelectionModel now consider only items which
are marked as enabled and selectable as part of the selection.
Fixes: QTBUG-93829
Pick-to: 6.2
Change-Id: I4725243ea6b0db4f289ce34ada22c7a9d3282713
Reviewed-by: David Faure <david.faure@kdab.com>
-rw-r--r-- | src/corelib/itemmodels/qabstractitemmodel.cpp | 7 | ||||
-rw-r--r-- | src/corelib/itemmodels/qabstractitemmodel_p.h | 3 | ||||
-rw-r--r-- | src/corelib/itemmodels/qitemselectionmodel.cpp | 68 | ||||
-rw-r--r-- | src/widgets/itemviews/qtreewidget.cpp | 12 | ||||
-rw-r--r-- | src/widgets/itemviews/qtreewidget_p.h | 1 | ||||
-rw-r--r-- | tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp | 2 |
6 files changed, 70 insertions, 23 deletions
diff --git a/src/corelib/itemmodels/qabstractitemmodel.cpp b/src/corelib/itemmodels/qabstractitemmodel.cpp index 8dda51e5f5..02ffd0e085 100644 --- a/src/corelib/itemmodels/qabstractitemmodel.cpp +++ b/src/corelib/itemmodels/qabstractitemmodel.cpp @@ -3018,6 +3018,13 @@ bool QAbstractItemModelPrivate::allowMove(const QModelIndex &srcParent, int star } /*! + \internal + + see QTBUG-94546 + */ +void QAbstractItemModelPrivate::executePendingOperations() const { } + +/*! \since 4.6 Begins a row move operation. diff --git a/src/corelib/itemmodels/qabstractitemmodel_p.h b/src/corelib/itemmodels/qabstractitemmodel_p.h index dbab493c9c..e8f00975ed 100644 --- a/src/corelib/itemmodels/qabstractitemmodel_p.h +++ b/src/corelib/itemmodels/qabstractitemmodel_p.h @@ -99,6 +99,9 @@ public: void itemsMoved(const QModelIndex &srcParent, int srcFirst, int srcLast, const QModelIndex &destinationParent, int destinationChild, Qt::Orientation orientation); bool allowMove(const QModelIndex &srcParent, int srcFirst, int srcLast, const QModelIndex &destinationParent, int destinationChild, Qt::Orientation orientation); + // ugly hack for QTreeModel, see QTBUG-94546 + virtual void executePendingOperations() const; + inline QModelIndex createIndex(int row, int column, void *data = nullptr) const { return q_func()->createIndex(row, column, data); } diff --git a/src/corelib/itemmodels/qitemselectionmodel.cpp b/src/corelib/itemmodels/qitemselectionmodel.cpp index 4653bd26ba..ad24b5c9d8 100644 --- a/src/corelib/itemmodels/qitemselectionmodel.cpp +++ b/src/corelib/itemmodels/qitemselectionmodel.cpp @@ -1,6 +1,6 @@ /**************************************************************************** ** -** Copyright (C) 2020 The Qt Company Ltd. +** Copyright (C) 2021 The Qt Company Ltd. ** Contact: https://www.qt.io/licensing/ ** ** This file is part of the QtCore module of the Qt Toolkit. @@ -41,6 +41,7 @@ #include "qitemselectionmodel_p.h" #include <private/qitemselectionmodel_p.h> +#include <private/qabstractitemmodel_p.h> #include <private/qduplicatetracker_p.h> #include <qdebug.h> @@ -285,6 +286,11 @@ static void rowLengthsFromRange(const QItemSelectionRange &range, QList<QPair<QP } } +static bool isSelectableAndEnabled(Qt::ItemFlags flags) +{ + return flags.testFlags(Qt::ItemIsSelectable | Qt::ItemIsEnabled); +} + template<typename ModelIndexContainer> static void indexesFromRange(const QItemSelectionRange &range, ModelIndexContainer &result) { @@ -296,8 +302,7 @@ static void indexesFromRange(const QItemSelectionRange &range, ModelIndexContain const QModelIndex columnLeader = topLeft.sibling(row, topLeft.column()); for (int column = topLeft.column(); column <= right; ++column) { QModelIndex index = columnLeader.sibling(row, column); - Qt::ItemFlags flags = range.model()->flags(index); - if ((flags & Qt::ItemIsSelectable) && (flags & Qt::ItemIsEnabled)) + if (isSelectableAndEnabled(range.model()->flags(index))) result.push_back(index); } } @@ -314,7 +319,9 @@ static ModelIndexContainer qSelectionIndexes(const QItemSelection &selection) } /*! - Returns \c true if the selection range contains no selectable item + Returns \c true if the selection range contains either no items + or only items which are either disabled or marked as not selectable. + \since 4.7 */ @@ -326,8 +333,7 @@ bool QItemSelectionRange::isEmpty() const for (int column = left(); column <= right(); ++column) { for (int row = top(); row <= bottom(); ++row) { QModelIndex index = model()->index(row, column, parent()); - Qt::ItemFlags flags = model()->flags(index); - if ((flags & Qt::ItemIsSelectable) && (flags & Qt::ItemIsEnabled)) + if (isSelectableAndEnabled(model()->flags(index))) return false; } } @@ -440,7 +446,7 @@ void QItemSelection::select(const QModelIndex &topLeft, const QModelIndex &botto bool QItemSelection::contains(const QModelIndex &index) const { - if (index.flags() & Qt::ItemIsSelectable) { + if (isSelectableAndEnabled(index.flags())) { QList<QItemSelectionRange>::const_iterator it = begin(); for (; it != end(); ++it) if ((*it).contains(index)) @@ -1475,10 +1481,8 @@ bool QItemSelectionModel::isSelected(const QModelIndex &index) const selected = d->currentSelection.contains(index); } - if (selected) { - Qt::ItemFlags flags = d->model->flags(index); - return flags.testAnyFlag(Qt::ItemIsSelectable); - } + if (selected) + return isSelectableAndEnabled(d->model->flags(index)); return false; } @@ -1524,8 +1528,7 @@ bool QItemSelectionModel::isRowSelected(int row, const QModelIndex &parent) cons } auto isSelectable = [&](int row, int column) { - Qt::ItemFlags flags = d->model->index(row, column, parent).flags(); - return (flags & Qt::ItemIsSelectable); + return isSelectableAndEnabled(d->model->index(row, column, parent).flags()); }; const int colCount = d->model->columnCount(parent); @@ -1603,8 +1606,7 @@ bool QItemSelectionModel::isColumnSelected(int column, const QModelIndex &parent } auto isSelectable = [&](int row, int column) { - Qt::ItemFlags flags = d->model->index(row, column, parent).flags(); - return (flags & Qt::ItemIsSelectable); + return isSelectableAndEnabled(d->model->index(row, column, parent).flags()); }; const int rowCount = d->model->rowCount(parent); int unselectable = 0; @@ -1662,8 +1664,7 @@ bool QItemSelectionModel::rowIntersectsSelection(int row, const QModelIndex &par int right = range.right(); if (top <= row && bottom >= row) { for (int j = left; j <= right; j++) { - const Qt::ItemFlags flags = d->model->index(row, j, parent).flags(); - if ((flags & Qt::ItemIsSelectable) && (flags & Qt::ItemIsEnabled)) + if (isSelectableAndEnabled(d->model->index(row, j, parent).flags())) return true; } } @@ -1698,8 +1699,7 @@ bool QItemSelectionModel::columnIntersectsSelection(int column, const QModelInde int right = range.right(); if (left <= column && right >= column) { for (int j = top; j <= bottom; j++) { - const Qt::ItemFlags flags = d->model->index(j, column, parent).flags(); - if ((flags & Qt::ItemIsSelectable) && (flags & Qt::ItemIsEnabled)) + if (isSelectableAndEnabled(d->model->index(j, column, parent).flags())) return true; } } @@ -1709,20 +1709,44 @@ bool QItemSelectionModel::columnIntersectsSelection(int column, const QModelInde } /*! + \internal + + Check whether the selection is empty. + In contrast to selection.isEmpty(), this takes into account + whether items are enabled and whether they are selectable. +*/ +static bool selectionIsEmpty(const QItemSelection &selection) +{ + return std::all_of(selection.begin(), selection.end(), + [](const QItemSelectionRange &r) { return r.isEmpty(); }); +} + +/*! \since 4.2 - Returns \c true if the selection model contains any selection ranges; + Returns \c true if the selection model contains any selected item, otherwise returns \c false. */ bool QItemSelectionModel::hasSelection() const { Q_D(const QItemSelectionModel); + + // QTreeModel unfortunately sorts itself lazily. + // When it sorts itself, it emits are layoutChanged signal. + // This layoutChanged signal invalidates d->ranges here. + // So QTreeModel must not sort itself while we are iterating over + // d->ranges here. It sorts itself in executePendingOperations, + // thus preventing the sort to happen inside of selectionIsEmpty below. + // Sad story, read more in QTBUG-94546 + auto model_p = static_cast<const QAbstractItemModelPrivate *>(QObjectPrivate::get(model())); + model_p->executePendingOperations(); + if (d->currentCommand & (Toggle | Deselect)) { QItemSelection sel = d->ranges; sel.merge(d->currentSelection, d->currentCommand); - return !sel.isEmpty(); + return !selectionIsEmpty(sel); } else { - return !(d->ranges.isEmpty() && d->currentSelection.isEmpty()); + return !(selectionIsEmpty(d->ranges) && selectionIsEmpty(d->currentSelection)); } } diff --git a/src/widgets/itemviews/qtreewidget.cpp b/src/widgets/itemviews/qtreewidget.cpp index 6f35fe8764..e54bb5eea0 100644 --- a/src/widgets/itemviews/qtreewidget.cpp +++ b/src/widgets/itemviews/qtreewidget.cpp @@ -112,7 +112,9 @@ public: */ QTreeModel::QTreeModel(int columns, QTreeWidget *parent) - : QAbstractItemModel(parent), rootItem(new QTreeWidgetItem), headerItem(new QTreeWidgetItem) + : QAbstractItemModel(*new QTreeModelPrivate, parent), + rootItem(new QTreeWidgetItem), + headerItem(new QTreeWidgetItem) { rootItem->view = parent; rootItem->itemFlags = Qt::ItemIsDropEnabled; @@ -3365,6 +3367,14 @@ bool QTreeWidget::event(QEvent *e) return QTreeView::event(e); } +/*! + see QTBUG-94546 +*/ +void QTreeModelPrivate::executePendingOperations() const +{ + q_func()->executePendingSort(); +} + QT_END_NAMESPACE #include "moc_qtreewidget.cpp" diff --git a/src/widgets/itemviews/qtreewidget_p.h b/src/widgets/itemviews/qtreewidget_p.h index 1f10b06358..65d52bf2bc 100644 --- a/src/widgets/itemviews/qtreewidget_p.h +++ b/src/widgets/itemviews/qtreewidget_p.h @@ -181,6 +181,7 @@ QT_END_INCLUDE_NAMESPACE class QTreeModelPrivate : public QAbstractItemModelPrivate { Q_DECLARE_PUBLIC(QTreeModel) + void executePendingOperations() const override; }; class QTreeWidgetItemPrivate diff --git a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp index b7e13c8853..7c4dbaedbf 100644 --- a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp +++ b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp @@ -2070,8 +2070,10 @@ void tst_QItemSelectionModel::unselectable() selectionModel.select(QItemSelection(model.index(0, 0), model.index(9, 0)), QItemSelectionModel::Select); QCOMPARE(selectionModel.selectedIndexes().count(), 10); QCOMPARE(selectionModel.selectedRows().count(), 10); + QVERIFY(selectionModel.hasSelection()); for (int j = 0; j < 10; ++j) model.item(j)->setFlags({ }); + QVERIFY(!selectionModel.hasSelection()); QCOMPARE(selectionModel.selectedIndexes().count(), 0); QCOMPARE(selectionModel.selectedRows().count(), 0); } |