Fix item highlighting through DBus

Before this commit, even items that are distant children of
currently open views were considered selectable. This lead to the
bug that items meant to be highlighted through DBus would not be
highlighted if any ancestor of the item was open in any view.

This was fixed by only considering items in view if they can be
seen by scrolling. Main difficulty here was to make this also work
for the details view mode which allows expanding.

To implement this cleanly, some refactoring seemed necessary
because the logic to determine if an item is already in view
was previously intertwined with the logic to identify already open
directories.

This commit also contains the following refactorings aiming to
make the code more readable:
- A magic value (-1) is replaced using std::optional.
- A boolean trap is removed.
- A QPair is replaced by a struct with named variables.
- More and improved documentation

src/dolphinmainwindow.cpp

@@ -2680,7 +2680,7 @@ bool DolphinMainWindow::isUrlOpen(const QString &url)
     return m_tabWidget->isUrlOpen(QUrl::fromUserInput(url));
 }
 
-bool DolphinMainWindow::isUrlOrParentOpen(const QString &url)
+bool DolphinMainWindow::isItemVisibleInAnyView(const QString &urlOfItem)
 {
-    return m_tabWidget->isUrlOrParentOpen(QUrl::fromUserInput(url));
+    return m_tabWidget->isItemVisibleInAnyView(QUrl::fromUserInput(urlOfItem));
 }

src/dolphinmainwindow.h

@@ -153,13 +153,11 @@ public Q_SLOTS:
     bool isUrlOpen(const QString &url);
 
     /**
-     * Determines if a URL or it's parent is open in any tab.
+     * @return Whether the item with @p url can be found in any view only by switching
+     * between already open tabs and scrolling in their primary or secondary view.
      * @note Use of QString instead of QUrl is required to be callable via DBus.
-     *
-     * @param url URL to look for
-     * @returns true if url or it's parent is currently open in a tab, false otherwise.
      */
-    bool isUrlOrParentOpen(const QString &url);
+    bool isItemVisibleInAnyView(const QString &urlOfItem);
 
 
     /**

src/dolphintabwidget.cpp

@@ -114,12 +114,12 @@ void DolphinTabWidget::refreshViews()
 
 bool DolphinTabWidget::isUrlOpen(const QUrl &url) const
 {
-    return indexByUrl(url).first >= 0;
+    return viewOpenAtDirectory(url).has_value();
 }
 
-bool DolphinTabWidget::isUrlOrParentOpen(const QUrl &url) const
+bool DolphinTabWidget::isItemVisibleInAnyView(const QUrl &urlOfItem) const
 {
-    return indexByUrl(url, ReturnIndexForOpenedParentAlso).first >= 0;
+    return viewShowingItem(urlOfItem).has_value();
 }
 
 void DolphinTabWidget::openNewActivatedTab()
@@ -195,7 +195,7 @@ void DolphinTabWidget::openNewTab(const QUrl& primaryUrl, const QUrl& secondaryU
     }
 }
 
-void DolphinTabWidget::openDirectories(const QList<QUrl>& dirs, bool splitView, bool skipChildUrls)
+void DolphinTabWidget::openDirectories(const QList<QUrl>& dirs, bool splitView)
 {
     Q_ASSERT(dirs.size() > 0);
 
@@ -204,25 +204,19 @@ void DolphinTabWidget::openDirectories(const QList<QUrl>& dirs, bool splitView,
     QList<QUrl>::const_iterator it = dirs.constBegin();
     while (it != dirs.constEnd()) {
         const QUrl& primaryUrl = *(it++);
-        const QPair<int, bool> indexInfo = indexByUrl(primaryUrl, skipChildUrls ? ReturnIndexForOpenedParentAlso : ReturnIndexForOpenedUrlOnly);
-        const int index = indexInfo.first;
-        const bool isInPrimaryView = indexInfo.second;
+        const std::optional<ViewIndex> alreadyOpenDirectory = viewOpenAtDirectory(primaryUrl);
 
-        // When the user asks for a URL that's already open (or it's parent is open if skipChildUrls is set),
+        // When the user asks for a URL that's already open,
         // activate it instead of opening a new tab
-        if (index >= 0) {
+        if (alreadyOpenDirectory.has_value()) {
             somethingWasAlreadyOpen = true;
-            activateTab(index);
-            const auto tabPage = tabPageAt(index);
-            if (isInPrimaryView) {
+            activateTab(alreadyOpenDirectory->tabIndex);
+            const auto tabPage = tabPageAt(alreadyOpenDirectory->tabIndex);
+            if (alreadyOpenDirectory->isInPrimaryView) {
                 tabPage->primaryViewContainer()->setActive(true);
             } else {
                 tabPage->secondaryViewContainer()->setActive(true);
             }
-            // BUG: 417230
-            // Required for updateViewState() call in openFiles() to work as expected
-            // If there is a selection, updateViewState() calls are effectively a no-op
-            tabPage->activeViewContainer()->view()->clearSelection();
         } else if (splitView && (it != dirs.constEnd())) {
             const QUrl& secondaryUrl = *(it++);
             if (somethingWasAlreadyOpen) {
@@ -244,19 +238,40 @@ void DolphinTabWidget::openFiles(const QList<QUrl>& files, bool splitView)
 {
     Q_ASSERT(files.size() > 0);
 
-    // Get all distinct directories from 'files' and open a tab
-    // for each directory. If the "split view" option is enabled, two
-    // directories are shown inside one tab (see openDirectories()).
-    QList<QUrl> dirs;
-    for (const QUrl& url : files) {
-        const QUrl dir(url.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash));
-        if (!dirs.contains(dir)) {
-            dirs.append(dir);
+    // Get all distinct directories from 'files'.
+    QList<QUrl> dirsThatNeedToBeOpened;
+    QList<QUrl> dirsThatWereAlreadyOpen;
+    for (const QUrl& file : files) {
+        const QUrl dir(file.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash));
+        if (dirsThatNeedToBeOpened.contains(dir) || dirsThatWereAlreadyOpen.contains(dir)) {
+            continue;
+        }
+
+        // The selecting of files that we do later will not work in views that already have items selected.
+        // So we check if dir is already open and clear the selection if it is. BUG: 417230
+        // We also make sure the view will be activated.
+        auto viewIndex = viewShowingItem(file);
+        if (viewIndex.has_value()) {
+            activateTab(viewIndex->tabIndex);
+            if (viewIndex->isInPrimaryView) {
+                tabPageAt(viewIndex->tabIndex)->primaryViewContainer()->view()->clearSelection();
+                tabPageAt(viewIndex->tabIndex)->primaryViewContainer()->setActive(true);
+            } else {
+                tabPageAt(viewIndex->tabIndex)->secondaryViewContainer()->view()->clearSelection();
+                tabPageAt(viewIndex->tabIndex)->secondaryViewContainer()->setActive(true);
+            }
+            dirsThatWereAlreadyOpen.append(dir);
+        } else {
+            dirsThatNeedToBeOpened.append(dir);
         }
     }
 
     const int oldTabCount = count();
-    openDirectories(dirs, splitView, true);
+    // Open a tab for each directory. If the "split view" option is enabled,
+    // two directories are shown inside one tab (see openDirectories()).
+    if (dirsThatNeedToBeOpened.size() > 0) {
+        openDirectories(dirsThatNeedToBeOpened, splitView);
+    }
     const int tabCount = count();
 
     // Select the files. Although the files can be split between several
@@ -499,29 +514,70 @@ QString DolphinTabWidget::tabName(DolphinTabPage* tabPage) const
     return name.replace('&', QLatin1String("&&"));
 }
 
-QPair<int, bool> DolphinTabWidget::indexByUrl(const QUrl& url, ChildUrlBehavior childUrlBehavior) const
+const std::optional<const DolphinTabWidget::ViewIndex> DolphinTabWidget::viewOpenAtDirectory(const QUrl& directory) const
 {
     int i = currentIndex();
     if (i < 0) {
-        return qMakePair(-1, false);
+        return std::nullopt;
     }
     // loop over the tabs starting from the current one
     do {
         const auto tabPage = tabPageAt(i);
-        if (tabPage->primaryViewContainer()->url() == url ||
-                (childUrlBehavior == ReturnIndexForOpenedParentAlso && tabPage->primaryViewContainer()->url().isParentOf(url))) {
-            return qMakePair(i, true);
+        if (tabPage->primaryViewContainer()->url() == directory) {
+            return std::optional(ViewIndex{i, true});
         }
 
-        if (tabPage->splitViewEnabled() &&
-                (url == tabPage->secondaryViewContainer()->url() ||
-                 (childUrlBehavior == ReturnIndexForOpenedParentAlso && tabPage->secondaryViewContainer()->url().isParentOf(url)))) {
-            return qMakePair(i, false);
+        if (tabPage->splitViewEnabled() && tabPage->secondaryViewContainer()->url() == directory) {
+            return std::optional(ViewIndex{i, false});
         }
 
         i = (i + 1) % count();
     }
     while (i != currentIndex());
 
-    return qMakePair(-1, false);
+    return std::nullopt;
+}
+
+const std::optional<const DolphinTabWidget::ViewIndex> DolphinTabWidget::viewShowingItem(const QUrl& item) const
+{
+    // The item might not be loaded yet even though it exists. So instead
+    // we check if the folder containing the item is showing its contents.
+    const QUrl dirContainingItem(item.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash));
+
+    // The dirContainingItem is either open directly or expanded in a tree-style view mode.
+    // Is dirContainingitem the base url of a view?
+    auto viewOpenAtContainingDirectory = viewOpenAtDirectory(dirContainingItem);
+    if (viewOpenAtContainingDirectory.has_value()) {
+        return viewOpenAtContainingDirectory;
+    }
+
+    // Is dirContainingItem expanded in some tree-style view?
+    // The rest of this method is about figuring this out.
+
+    int i = currentIndex();
+    if (i < 0) {
+        return std::nullopt;
+    }
+    // loop over the tabs starting from the current one
+    do {
+        const auto tabPage = tabPageAt(i);
+        if (tabPage->primaryViewContainer()->url().isParentOf(item)) {
+            const KFileItem fileItemContainingItem = tabPage->primaryViewContainer()->view()->items().findByUrl(dirContainingItem);
+            if (!fileItemContainingItem.isNull() && tabPage->primaryViewContainer()->view()->isExpanded(fileItemContainingItem)) {
+                return std::optional(ViewIndex{i, true});
+            }
+        }
+
+        if (tabPage->splitViewEnabled() && tabPage->secondaryViewContainer()->url().isParentOf(item)) {
+            const KFileItem fileItemContainingItem = tabPage->secondaryViewContainer()->view()->items().findByUrl(dirContainingItem);
+            if (!fileItemContainingItem.isNull() && tabPage->secondaryViewContainer()->view()->isExpanded(fileItemContainingItem)) {
+                return std::optional(ViewIndex{i, false});
+            }
+        }
+
+        i = (i + 1) % count();
+    }
+    while (i != currentIndex());
+
+    return std::nullopt;
 }

src/dolphintabwidget.h

@@ -13,6 +13,8 @@
 #include <QTabWidget>
 #include <QUrl>
 
+#include <optional>
+
 class DolphinViewContainer;
 class KConfigGroup;
 
@@ -75,10 +77,10 @@ public:
     bool isUrlOpen(const QUrl& url) const;
 
     /**
-     * @return Whether any of the tab pages has @p url or it's parent opened
-     * in their primary or secondary view.
+     * @return Whether the item with @p url can be found in any view only by switching
+     * between already open tabs and scrolling in their primary or secondary view.
      */
-    bool isUrlOrParentOpen(const QUrl& url) const;
+    bool isItemVisibleInAnyView(const QUrl& urlOfItem) const;
 
 Q_SIGNALS:
     /**
@@ -127,10 +129,9 @@ public Q_SLOTS:
     /**
      * Opens each directory in \p dirs in a separate tab unless it is already open.
      * If \a splitView is set, 2 directories are collected within one tab.
-     * If \a skipChildUrls is set, do not open a directory if it's parent is already open.
      * \pre \a dirs must contain at least one url.
      */
-    void openDirectories(const QList<QUrl>& dirs, bool splitView, bool skipChildUrls = false);
+    void openDirectories(const QList<QUrl>& dirs, bool splitView);
 
     /**
      * Opens the directories which contain the files \p files and selects all files.
@@ -221,21 +222,27 @@ private:
      */
     QString tabName(DolphinTabPage* tabPage) const;
 
-    enum ChildUrlBehavior {
-        ReturnIndexForOpenedUrlOnly,
-        ReturnIndexForOpenedParentAlso
+    struct ViewIndex {
+        const int tabIndex;
+        const bool isInPrimaryView;
     };
+    /**
+     * Get the position of the view within this widget that is open at @p directory.
+     * @param directory The URL of the directory we want to find.
+     * @return a small struct containing the tab index of the view and whether it is
+     * in the primary view. A std::nullopt is returned if there is no view open for @p directory.
+     */
+    const std::optional<const ViewIndex> viewOpenAtDirectory(const QUrl& directory) const;
 
     /**
-     * @param url The URL that we would like
-     * @param childUrlBehavior Whether a tab with opened parent of the URL can be returned too
-     * @return a QPair with:
-     * First containing the index of the tab with the desired URL or -1 if not found.
-     * Second says true if URL is in primary view container, false otherwise.
-     * False means the URL is in the secondary view container, unless first == -1.
-     * In that case the value of second is meaningless.
+     * Get the position of the view within this widget that has @p item in the view.
+     * This means that the item can be seen by the user in that view when scrolled to the right position.
+     * If the view has folders expanded and @p item is one of them, the view will also be returned.
+     * @param item The URL of the item we want to find.
+     * @return a small struct containing the tab index of the view and whether it is
+     * in the primary view. A std::nullopt is returned if there is no view open that has @p item visible anywhere.
      */
-    QPair<int, bool> indexByUrl(const QUrl& url, ChildUrlBehavior childUrlBehavior = ReturnIndexForOpenedUrlOnly) const;
+    const std::optional<const ViewIndex> viewShowingItem(const QUrl& item) const;
 
 private:
     QPointer<DolphinTabPage> m_lastViewedTab;

src/global.cpp

@@ -90,7 +90,7 @@ bool Dolphin::attachToExistingInstance(const QList<QUrl>& inputUrls, bool openFi
         do {
             auto &interface = dolphinInterfaces[i];
 
-            auto isUrlOpenReply = openFiles ? interface.first->isUrlOrParentOpen(url) : interface.first->isUrlOpen(url);
+            auto isUrlOpenReply = openFiles ? interface.first->isItemVisibleInAnyView(url) : interface.first->isUrlOpen(url);
             isUrlOpenReply.waitForFinished();
             if (!isUrlOpenReply.isError() && isUrlOpenReply.value()) {
                 interface.second.append(url);

src/views/dolphinview.cpp

@@ -1490,6 +1490,16 @@ bool DolphinView::itemsExpandable() const
     return m_mode == DetailsView;
 }
 
+bool DolphinView::isExpanded(const KFileItem& item) const
+{
+    Q_ASSERT(item.isDir());
+    Q_ASSERT(items().contains(item));
+    if (!itemsExpandable()) {
+        return false;
+    }
+    return m_model->isExpanded(m_model->index(item));
+}
+
 void DolphinView::restoreState(QDataStream& stream)
 {
     // Read the version number of the view state and check if the version is supported.

src/views/dolphinview.h

@@ -291,6 +291,13 @@ public:
      */
     bool itemsExpandable() const;
 
+    /**
+     * @returns true if the @p item is one of the items() of this view and
+     * is currently expanded. false otherwise.
+     * Only directories in view modes that allow expanding can ever be expanded.
+     */
+    bool isExpanded(const KFileItem &item) const;
+
     /**
      * Restores the view state (current item, contents position, details view expansion state)
      */