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 ef58abee0eef3f7dff7f5e267bfe4c9b901d6076..6eff70f9becf79b5d3ca364c41623f9eaceb989d 100644 (file)
--- 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 b25c73843c1e47df1a82a36e5b6b22887cf8bf7f..392dc410e605a9b4a069dd985071732933eed807 100644 (file)
--- 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 f5ece564d33e32f725c6c369746878fd2e3d4125..2d90ae52f9bda448a3e442c728025d1fd7559f9f 100644 (file)
--- 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 <KActionCollection>
@@ -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 40b2cecaad8fad5cc27ef7810e14976ff5cc90cf..cb921781dff133e861d226b2708381a4664682f0 100644 (file)
--- 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);