From: Felix Ernst Date: Sat, 18 May 2024 22:18:14 +0000 (+0000) Subject: Avoid implicitly selecting items X-Git-Url: https://cloud.milkyroute.net/gitweb/dolphin.git/commitdiff_plain/122fee5625f0285ec4ebda79162c72390989eb2a Avoid implicitly selecting items Items should only be selected if the user wants to act on them. However, previous to this commit we sometimes selected items even when there is no reason to assume that the user would like to act on them. Such selections are dangerous because they make it more likely that the user manipulates items by accident which they never even explicitly selected. Example: The "Up" action is used to navigate to the parent folder. This will implicitly select the folder one emerged from after opening the parent folder, so just one accidental press of the Delete key will lead to data loss if the press goes unnoticed. This scenario would have been avoided if no folder had been selected automatically. The above example becomes even more dangerous if the user is acting with elevated privileges. The following implicit selections of items are being removed: - Selecting items that are being activated - Selecting folders one emerges from Even though these items will no longer be selected after these actions, they will still be marked as current. The only downside I see is that our indication of which item is "current" is a lot weaker than the selection highlight, so it might be more difficult to spot which folder one has emerged from. However, this could be counter-acted with some other temporary indication if this really turns out to be a problem. The only downside I see is that our indication of which item is "current" is a lot weaker than the selection highlight, so it might be more difficult to spot which folder one has emerged from. However, this could be counter-acted with some other temporary indication if this really turns out to be a problem. BUG: 424723 --- diff --git a/src/dolphinviewcontainer.cpp b/src/dolphinviewcontainer.cpp index ef58abee0..6eff70f9b 100644 --- a/src/dolphinviewcontainer.cpp +++ b/src/dolphinviewcontainer.cpp @@ -868,7 +868,8 @@ void DolphinViewContainer::slotUrlNavigatorLocationChanged(const QUrl &url) void DolphinViewContainer::slotUrlSelectionRequested(const QUrl &url) { - m_view->markUrlsAsSelected({url}); + // We do not want to select any item here because there is no reason to assume that the user wants to edit the folder we are emerging from. BUG: 424723 + m_view->markUrlAsCurrent(url); // makes the item scroll into view } diff --git a/src/kitemviews/kitemlistcontroller.cpp b/src/kitemviews/kitemlistcontroller.cpp index b25c73843..392dc410e 100644 --- a/src/kitemviews/kitemlistcontroller.cpp +++ b/src/kitemviews/kitemlistcontroller.cpp @@ -723,6 +723,9 @@ bool KItemListController::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event, const bool emitItemActivated = index.has_value() && index.value() < m_model->count() && !m_view->isAboveExpansionToggle(index.value(), pos); if (emitItemActivated) { + if (!QApplication::keyboardModifiers()) { + m_selectionManager->clearSelection(); // The user does not want to manage/manipulate the item currently, only activate it. + } Q_EMIT itemActivated(index.value()); } return false; @@ -1661,10 +1664,14 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier // or we just keep going for double-click activation if (m_view->style()->styleHint(QStyle::SH_ItemView_ActivateItemOnSingleClick) || m_singleClickActivationEnforced) { if (!pressedItemAlreadySelected) { - // An unselected item was clicked directly while deselecting multiple other items so we select it. - m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Toggle); + // An unselected item was clicked directly while deselecting multiple other items so we mark it "current". m_selectionManager->setCurrentItem(m_pressedIndex.value()); m_selectionManager->beginAnchoredSelection(m_pressedIndex.value()); + if (!leftClick) { + // We select the item here because this press is not meant to directly activate the item. + // We do not want to select items unless the user wants to edit them. + m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Toggle); + } } return true; // event handled, don't create rubber band } @@ -1731,7 +1738,10 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier break; case SingleSelection: - m_selectionManager->setSelected(m_pressedIndex.value()); + if (!leftClick || shiftOrControlPressed + || (!m_view->style()->styleHint(QStyle::SH_ItemView_ActivateItemOnSingleClick) && !m_singleClickActivationEnforced)) { + m_selectionManager->setSelected(m_pressedIndex.value()); + } break; case MultiSelection: @@ -1752,7 +1762,10 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier } } else if (!shiftPressed || !m_selectionManager->isAnchoredSelectionActive()) { // Select the pressed item and start a new anchored selection - m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Select); + if (!leftClick || shiftOrControlPressed + || (!m_view->style()->styleHint(QStyle::SH_ItemView_ActivateItemOnSingleClick) && !m_singleClickActivationEnforced)) { + m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Select); + } m_selectionManager->beginAnchoredSelection(m_pressedIndex.value()); } break; diff --git a/src/tests/dolphinmainwindowtest.cpp b/src/tests/dolphinmainwindowtest.cpp index f5ece564d..2d90ae52f 100644 --- a/src/tests/dolphinmainwindowtest.cpp +++ b/src/tests/dolphinmainwindowtest.cpp @@ -9,7 +9,10 @@ #include "dolphintabpage.h" #include "dolphintabwidget.h" #include "dolphinviewcontainer.h" +#include "kitemviews/kfileitemmodel.h" #include "kitemviews/kitemlistcontainer.h" +#include "kitemviews/kitemlistcontroller.h" +#include "kitemviews/kitemlistselectionmanager.h" #include "testdir.h" #include @@ -360,7 +363,7 @@ void DolphinMainWindowTest::testGoActions() testDir->createDir("b/b-1"); testDir->createFile("b/b-2"); testDir->createDir("c"); - QUrl childDirUrl(QDir::cleanPath(testDir->url().toString() + "/b")); + const QUrl childDirUrl(QDir::cleanPath(testDir->url().toString() + "/b")); m_mainWindow->openDirectories({childDirUrl}, false); // Open "b" dir m_mainWindow->show(); QVERIFY(QTest::qWaitForWindowExposed(m_mainWindow.data())); @@ -381,24 +384,40 @@ void DolphinMainWindowTest::testGoActions() const QUrl parentDirUrl = m_mainWindow->activeViewContainer()->url(); QVERIFY(parentDirUrl != childDirUrl); - // The item we just emerged from should now have keyboard focus but this doesn't necessarily mean that it is selected. - // To test if it has keyboard focus, we press "Down" to select "c" below and then "Up" so the folder "b" we just emerged from is actually selected. + auto currentItemUrl = [this]() { + const int currentIndex = m_mainWindow->m_activeViewContainer->view()->m_container->controller()->selectionManager()->currentItem(); + const KFileItem currentItem = m_mainWindow->m_activeViewContainer->view()->m_model->fileItem(currentIndex); + return currentItem.url(); + }; + + QCOMPARE(currentItemUrl(), childDirUrl); // The item we just emerged from should now have keyboard focus. + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 0); // The item we just emerged from should not be selected. BUG: 424723 + // Pressing arrow keys should not only move the keyboard focus but also select the item. + // We press "Down" to select "c" below and then "Up" so the folder "b" we just emerged from is selected for the first time. m_mainWindow->actionCollection()->action(QStringLiteral("compact"))->trigger(); QTest::keyClick(m_mainWindow->activeViewContainer()->view()->m_container, Qt::Key::Key_Down, Qt::NoModifier); QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); + QVERIFY2(currentItemUrl() != childDirUrl, "The current item didn't change after pressing the 'Down' key."); QTest::keyClick(m_mainWindow->activeViewContainer()->view()->m_container, Qt::Key::Key_Up, Qt::NoModifier); QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); + QCOMPARE(currentItemUrl(), childDirUrl); // After pressing 'Down' and then 'Up' we should be back where we were. + + // Enter the child folder "b". QTest::keyClick(m_mainWindow->activeViewContainer()->view()->m_container, Qt::Key::Key_Enter, Qt::NoModifier); QVERIFY(spyDirectoryLoadingCompleted.wait()); QCOMPARE(m_mainWindow->activeViewContainer()->url(), childDirUrl); QVERIFY(m_mainWindow->isUrlOpen(childDirUrl.toString())); - // Go back to the parent folder + // Go back to the parent folder. m_mainWindow->actionCollection()->action(KStandardAction::name(KStandardAction::Back))->trigger(); QVERIFY(spyDirectoryLoadingCompleted.wait()); QTest::qWait(100); // Somehow the item we emerged from doesn't have keyboard focus yet if we don't wait a split second. QCOMPARE(m_mainWindow->activeViewContainer()->url(), parentDirUrl); QVERIFY(m_mainWindow->isUrlOpen(parentDirUrl.toString())); + // Going 'Back' means that the view should be in the same state it was in when we left. + QCOMPARE(currentItemUrl(), childDirUrl); // The item we last interacted with in this location should still have keyboard focus. + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().constFirst().url(), childDirUrl); // It should still be selected. // Open a new tab for the "b" child dir and verify that this doesn't interfere with anything. QTest::keyClick(m_mainWindow->activeViewContainer()->view()->m_container, Qt::Key::Key_Enter, Qt::ControlModifier); // Open new inactive tab @@ -412,6 +431,26 @@ void DolphinMainWindowTest::testGoActions() QVERIFY(spyDirectoryLoadingCompleted.wait()); QCOMPARE(m_mainWindow->activeViewContainer()->url(), childDirUrl); QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 0); // There was no action in this view yet that would warrant a selection. + QCOMPARE(currentItemUrl(), QUrl(QDir::cleanPath(testDir->url().toString() + "/b/b-1"))); // The first item in the view should have keyboard focus. + + // Press the 'Down' key in the child folder. + QTest::keyClick(m_mainWindow->activeViewContainer()->view()->m_container, Qt::Key::Key_Down, Qt::NoModifier); + // The second item in the view should have keyboard focus and be selected. + const QUrl secondItemInChildFolderUrl{QDir::cleanPath(testDir->url().toString() + "/b/b-2")}; + QCOMPARE(currentItemUrl(), secondItemInChildFolderUrl); + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().constFirst().url(), secondItemInChildFolderUrl); + + // Go back to the parent folder and then re-enter the child folder. + m_mainWindow->actionCollection()->action(KStandardAction::name(KStandardAction::Back))->trigger(); + QVERIFY(spyDirectoryLoadingCompleted.wait()); + m_mainWindow->actionCollection()->action(KStandardAction::name(KStandardAction::Forward))->trigger(); + QVERIFY(spyDirectoryLoadingCompleted.wait()); + QCOMPARE(m_mainWindow->activeViewContainer()->url(), childDirUrl); + // The state of the view should be identical to how it was before we triggered "Back" and then "Forward". + QTRY_COMPARE(currentItemUrl(), secondItemInChildFolderUrl); + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().constFirst().url(), secondItemInChildFolderUrl); // Go back to the parent folder. m_mainWindow->actionCollection()->action(KStandardAction::name(KStandardAction::Back))->trigger(); diff --git a/src/tests/kitemlistcontrollertest.cpp b/src/tests/kitemlistcontrollertest.cpp index 40b2cecaa..cb921781d 100644 --- a/src/tests/kitemlistcontrollertest.cpp +++ b/src/tests/kitemlistcontrollertest.cpp @@ -628,6 +628,7 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 1); spyItemActivated.clear(); + QVERIFY2(!m_view->controller()->selectionManager()->hasSelection(), "An item should not be implicitly selected during activation. @see bug 424723"); // Set the global setting to "double click activation". m_testStyle->setActivateItemOnSingleClick(false); @@ -635,6 +636,7 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 0); spyItemActivated.clear(); + QVERIFY(m_view->controller()->selectionManager()->hasSelection()); // Enforce single click activation in the controller. m_controller->setSingleClickActivationEnforced(true); @@ -642,6 +644,8 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 1); spyItemActivated.clear(); + constexpr const char *reasonWhySelectionShouldPersist = "An item was selected before this mouse click. The click should not have cleared this selection."; + QVERIFY2(m_view->controller()->selectionManager()->hasSelection(), reasonWhySelectionShouldPersist); // Do not enforce single click activation in the controller. m_controller->setSingleClickActivationEnforced(false); @@ -649,6 +653,7 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 0); spyItemActivated.clear(); + QVERIFY2(m_view->controller()->selectionManager()->hasSelection(), reasonWhySelectionShouldPersist); // Set the global setting back to "single click activation". m_testStyle->setActivateItemOnSingleClick(true); @@ -656,6 +661,7 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 1); spyItemActivated.clear(); + QVERIFY2(m_view->controller()->selectionManager()->hasSelection(), reasonWhySelectionShouldPersist); // Enforce single click activation in the controller. m_controller->setSingleClickActivationEnforced(true); @@ -663,6 +669,7 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 1); spyItemActivated.clear(); + QVERIFY2(m_view->controller()->selectionManager()->hasSelection(), reasonWhySelectionShouldPersist); // Restore previous settings. m_controller->setSingleClickActivationEnforced(true);