From 7419cd326902c64b69802ab3f01656076d3c7a97 Mon Sep 17 00:00:00 2001 From: Nate Graham Date: Tue, 11 Nov 2025 10:47:26 -0700 Subject: Revert "Avoid implicitly selecting items" This reverts commit 122fee5625f0285ec4ebda79162c72390989eb2a. This behavior change was well-intentioned, but has significant usability and speed drawbacks that have not been addressed: - Keyboard-driven folder manipulation became slower - Unexpected behavioral differences when opening files with the pointer compared to when opening them with the keyboard introduced inconsistency and cognitive load - Unexpected opening of selection mode during fast operation introduced the potential for confusion and additional errors - Dolphin's behavior became inconsistent with that of other file managers users may be accustomed to The bug tracker, discuss.kde.org, and Reddit are full of complaints about this change. It's been a year now; I think it's clear that many Dolphin users have not gotten used to and accepted it. I have to count myself as one of them, I'm afraid. I've tried to get used to it for a year, but I just have not been able to. I don't believe the benefits of this change outweigh the drawbacks, so let's revert it. 24d859cf19e90fa22ed687b36a68231625c1bd80 was explicitly mentioned as a thing that should also be reverted in this case, but it's already been done. BUG: 494125 BUG: 511966 CCBUG: 424723 CCBUG: 492404 FIXED-IN: 25.12.0 --- src/dolphinviewcontainer.cpp | 3 +-- src/kitemviews/kitemlistcontroller.cpp | 21 ++++----------------- src/tests/dolphinmainwindowtest.cpp | 2 +- src/tests/kitemlistcontrollertest.cpp | 8 -------- 4 files changed, 6 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/src/dolphinviewcontainer.cpp b/src/dolphinviewcontainer.cpp index 0605cb0c8..f6fd01de2 100644 --- a/src/dolphinviewcontainer.cpp +++ b/src/dolphinviewcontainer.cpp @@ -912,8 +912,7 @@ void DolphinViewContainer::slotUrlNavigatorLocationChanged(const QUrl &url) void DolphinViewContainer::slotUrlSelectionRequested(const QUrl &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->markUrlsAsSelected({url}); m_view->markUrlAsCurrent(url); // makes the item scroll into view } diff --git a/src/kitemviews/kitemlistcontroller.cpp b/src/kitemviews/kitemlistcontroller.cpp index 7971de5b7..d889deb4e 100644 --- a/src/kitemviews/kitemlistcontroller.cpp +++ b/src/kitemviews/kitemlistcontroller.cpp @@ -732,9 +732,6 @@ 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; @@ -1674,14 +1671,10 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier // or we just keep going for double-click activation if (singleClickActivation || m_singleClickActivationEnforced) { if (!pressedItemAlreadySelected) { - // An unselected item was clicked directly while deselecting multiple other items so we mark it "current". + // An unselected item was clicked directly while deselecting multiple other items so we select it. + m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Toggle); 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); - } } if (leftClick) { row->setPressed(true); @@ -1756,10 +1749,7 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier break; case SingleSelection: - if (!leftClick || shiftOrControlPressed || (!singleClickActivation && !m_singleClickActivationEnforced)) { - m_selectionManager->setSelected(m_pressedIndex.value()); - } - break; + m_selectionManager->setSelected(m_pressedIndex.value()); case MultiSelection: if (controlPressed && !shiftPressed && leftClick) { @@ -1778,10 +1768,7 @@ bool KItemListController::onPress(const QPointF &pos, const Qt::KeyboardModifier // This will be the start of an item drag-to-copy operation if the user now moves the mouse before releasing the mouse button. } } else if (!shiftPressed || !m_selectionManager->isAnchoredSelectionActive()) { - // Select the pressed item and start a new anchored selection - if (!leftClick || shiftOrControlPressed || (!singleClickActivation && !m_singleClickActivationEnforced)) { - m_selectionManager->setSelected(m_pressedIndex.value(), 1, KItemListSelectionManager::Select); - } + 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 d309b6d9c..fb3c95383 100644 --- a/src/tests/dolphinmainwindowtest.cpp +++ b/src/tests/dolphinmainwindowtest.cpp @@ -672,7 +672,7 @@ void DolphinMainWindowTest::testGoActions() }; 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 + QCOMPARE(m_mainWindow->m_activeViewContainer->view()->selectedItems().count(), 1); // …and it should be selected, too. // 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(); diff --git a/src/tests/kitemlistcontrollertest.cpp b/src/tests/kitemlistcontrollertest.cpp index e9e1e66c2..8fd0f6158 100644 --- a/src/tests/kitemlistcontrollertest.cpp +++ b/src/tests/kitemlistcontrollertest.cpp @@ -1088,7 +1088,6 @@ 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); @@ -1096,7 +1095,6 @@ void KItemListControllerTest::testMouseClickActivation() m_view->event(&mouseReleaseEvent); QCOMPARE(spyItemActivated.count(), 0); spyItemActivated.clear(); - QVERIFY(m_view->controller()->selectionManager()->hasSelection()); // emulation of double click according to https://doc.qt.io/qt-6/qgraphicsscene.html#mouseDoubleClickEvent m_view->event(&mousePressEvent); @@ -1105,7 +1103,6 @@ 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"); // right mouse button should not trigger activation m_view->event(&mouseRightPressEvent); @@ -1127,8 +1124,6 @@ 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); @@ -1136,7 +1131,6 @@ 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); @@ -1144,7 +1138,6 @@ 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); @@ -1152,7 +1145,6 @@ 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); -- cgit v1.3