From 298c57bb91082b2f6c21b45488cbc2a8ba88607c Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 13 Dec 2023 12:47:36 +0100 Subject: [PATCH] QAIV: reset state if removing index while editing or committing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the commit of data to a model index result in that index getting filtered out, then the editor will be removed as part of committing the data, and the index that the editor is associated with becomes invalid. The subsequent call to closeEditor() will find that the index for the editor is no longer valid. This should not warn, and it should also not abort the clean-up process early. Instead, identify that the editor that we want to closed is already hidden. In that case, skip the warning and most of the cleanup process, and proceed directly to the EndEditHint handling further down. Add a test that simulates the two scenarios where this can happen: either the committing of data results in the index being filtered out by the existing filter; or the filter changes while editing, and the index being edited gets removed. In both cases, we don't want to see a warning, and the state of the item view should be reset correctly. Pick-to: 6.5 Fixes: QTBUG-115765 Change-Id: If247172fdac9a1a9279dae96c078d32553d4ee5d Reviewed-by: Axel Spoerl Reviewed-by: 🌴 Alexey Edelev 🌴 (cherry picked from commit 04415264489cd96a4a542a2ae7db1c14558397a5) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 97dd51e677779f73d75171acba00e4ea1ae7c8aa) --- src/widgets/itemviews/qabstractitemview.cpp | 73 +++++++++++-------- .../tst_qabstractitemview.cpp | 58 +++++++++++++++ 2 files changed, 99 insertions(+), 32 deletions(-) diff --git a/src/widgets/itemviews/qabstractitemview.cpp b/src/widgets/itemviews/qabstractitemview.cpp index c04a699a56c..f557abd75ec 100644 --- a/src/widgets/itemviews/qabstractitemview.cpp +++ b/src/widgets/itemviews/qabstractitemview.cpp @@ -2924,41 +2924,50 @@ void QAbstractItemView::closeEditor(QWidget *editor, QAbstractItemDelegate::EndE // Close the editor if (editor) { - bool isPersistent = d->persistent.contains(editor); - bool hadFocus = editor->hasFocus(); - QModelIndex index = d->indexForEditor(editor); + const bool isPersistent = d->persistent.contains(editor); + const QModelIndex index = d->indexForEditor(editor); if (!index.isValid()) { - qWarning("QAbstractItemView::closeEditor called with an editor that does not belong to this view"); - return; // the editor was not registered - } - - // start a timer that expires immediately when we return to the event loop - // to identify whether this close was triggered by a mousepress-initiated - // focus event - d->pressClosedEditorWatcher.start(0, this); - d->lastEditedIndex = index; - - if (!isPersistent) { - setState(NoState); - QModelIndex index = d->indexForEditor(editor); - editor->removeEventFilter(itemDelegateForIndex(index)); - d->removeEditor(editor); - } - if (hadFocus) { - if (focusPolicy() != Qt::NoFocus) - setFocus(); // this will send a focusLost event to the editor - else - editor->clearFocus(); + if (!editor->isVisible()) { + // The commit might have removed the index (e.g. it might get filtered), in + // which case the editor is already hidden and scheduled for deletion. We + // don't have to do anything, except reset the state, and continue with + // EndEditHint processing. + if (!isPersistent) + setState(NoState); + } else { + qWarning("QAbstractItemView::closeEditor called with an editor that does not belong to this view"); + return; + } } else { - d->checkPersistentEditorFocus(); + const bool hadFocus = editor->hasFocus(); + // start a timer that expires immediately when we return to the event loop + // to identify whether this close was triggered by a mousepress-initiated + // focus event + d->pressClosedEditorWatcher.start(0, this); + d->lastEditedIndex = index; + + if (!isPersistent) { + setState(NoState); + QModelIndex index = d->indexForEditor(editor); + editor->removeEventFilter(itemDelegateForIndex(index)); + d->removeEditor(editor); + } + if (hadFocus) { + if (focusPolicy() != Qt::NoFocus) + setFocus(); // this will send a focusLost event to the editor + else + editor->clearFocus(); + } else { + d->checkPersistentEditorFocus(); + } + + QPointer ed = editor; + QCoreApplication::sendPostedEvents(editor, 0); + editor = ed; + + if (!isPersistent && editor) + d->releaseEditor(editor, index); } - - QPointer ed = editor; - QCoreApplication::sendPostedEvents(editor, 0); - editor = ed; - - if (!isPersistent && editor) - d->releaseEditor(editor, index); } // The EndEditHint part diff --git a/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp b/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp index 22d4d5b66d0..15a7d2319ed 100644 --- a/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp +++ b/tests/auto/widgets/itemviews/qabstractitemview/tst_qabstractitemview.cpp @@ -149,6 +149,7 @@ private slots: void selectionAutoScrolling(); void testSpinBoxAsEditor_data(); void testSpinBoxAsEditor(); + void removeIndexWhileEditing(); private: static QAbstractItemView *viewFromString(const QByteArray &viewType, QWidget *parent = nullptr) @@ -3463,5 +3464,62 @@ void tst_QAbstractItemView::testSpinBoxAsEditor() QCOMPARE(model.data(model.index(0, 1)).toInt(), 1); } +void tst_QAbstractItemView::removeIndexWhileEditing() +{ + QTreeView view; + QStandardItemModel treeModel; + auto editableItem1 = new QStandardItem("aa"); + auto editableItem2 = new QStandardItem("ab"); + auto editableItem3 = new QStandardItem("ac"); + auto item = new QStandardItem("a"); + item->appendRow(editableItem1); + item->appendRow(editableItem2); + item->appendRow(editableItem3); + treeModel.setItem(0, 0, item); + QSortFilterProxyModel filterModel; + filterModel.setSourceModel(&treeModel); + view.setModel(&filterModel); + view.show(); + + QVERIFY(QTest::qWaitForWindowExposed(&view)); + + view.setExpanded(item->index(), true); + + filterModel.setFilterRegularExpression("a.*"); + + QTest::failOnWarning(QRegularExpression("QAbstractItemView::closeEditor called with an editor " + "that does not belong to this view")); + + // Verify that we shut editing down cleanly if the index we are editing is + // filtered out after committing + { + const QModelIndex filteredIndex = filterModel.mapFromSource(editableItem1->index()); + QVERIFY(filteredIndex.isValid()); + view.edit(filteredIndex); + QCOMPARE(view.state(), QAbstractItemView::EditingState); + QPointer lineEdit = qobject_cast(QApplication::focusWidget()); + QVERIFY(lineEdit); + lineEdit->setText("c"); + QTest::keyClick(lineEdit, Qt::Key_Enter); + QTRY_VERIFY(!lineEdit); + QCOMPARE(editableItem1->data(Qt::DisplayRole), "c"); + QCOMPARE(view.state(), QAbstractItemView::NoState); + } + + // If we change the filter while we edit, then we should clean up state as well + { + const QModelIndex filteredIndex = filterModel.mapFromSource(editableItem2->index()); + QVERIFY(filteredIndex.isValid()); + view.edit(filteredIndex); + QCOMPARE(view.state(), QAbstractItemView::EditingState); + QPointer lineEdit = qobject_cast(QApplication::focusWidget()); + QVERIFY(lineEdit); + filterModel.setFilterFixedString("c"); + QVERIFY(!filterModel.mapFromSource(editableItem2->index()).isValid()); + QTRY_VERIFY(!lineEdit); + QCOMPARE(view.state(), QAbstractItemView::NoState); + } +} + QTEST_MAIN(tst_QAbstractItemView) #include "tst_qabstractitemview.moc"