From 4d81aabd1ee78c2fca61452ef3a866cfad0c88a5 Mon Sep 17 00:00:00 2001 From: Felix Ernst Date: Tue, 9 Aug 2022 16:02:55 +0200 Subject: [PATCH] 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 | 4 +- src/dolphinmainwindow.h | 8 +-- src/dolphintabwidget.cpp | 126 +++++++++++++++++++++++++++----------- src/dolphintabwidget.h | 39 +++++++----- src/global.cpp | 2 +- src/views/dolphinview.cpp | 10 +++ src/views/dolphinview.h | 7 +++ 7 files changed, 137 insertions(+), 59 deletions(-) diff --git a/src/dolphinmainwindow.cpp b/src/dolphinmainwindow.cpp index 1eed9a016c..305c7f2821 100644 --- a/src/dolphinmainwindow.cpp +++ b/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)); } diff --git a/src/dolphinmainwindow.h b/src/dolphinmainwindow.h index e7355e3ed1..6f922168c0 100644 --- a/src/dolphinmainwindow.h +++ b/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); /** diff --git a/src/dolphintabwidget.cpp b/src/dolphintabwidget.cpp index 6caaf174f8..5a90070129 100644 --- a/src/dolphintabwidget.cpp +++ b/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& dirs, bool splitView, bool skipChildUrls) +void DolphinTabWidget::openDirectories(const QList& dirs, bool splitView) { Q_ASSERT(dirs.size() > 0); @@ -204,25 +204,19 @@ void DolphinTabWidget::openDirectories(const QList& dirs, bool splitView, QList::const_iterator it = dirs.constBegin(); while (it != dirs.constEnd()) { const QUrl& primaryUrl = *(it++); - const QPair indexInfo = indexByUrl(primaryUrl, skipChildUrls ? ReturnIndexForOpenedParentAlso : ReturnIndexForOpenedUrlOnly); - const int index = indexInfo.first; - const bool isInPrimaryView = indexInfo.second; + const std::optional 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& 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 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 dirsThatNeedToBeOpened; + QList 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 DolphinTabWidget::indexByUrl(const QUrl& url, ChildUrlBehavior childUrlBehavior) const +const std::optional 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 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; } diff --git a/src/dolphintabwidget.h b/src/dolphintabwidget.h index 28c51024c8..8342d719d1 100644 --- a/src/dolphintabwidget.h +++ b/src/dolphintabwidget.h @@ -13,6 +13,8 @@ #include #include +#include + 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& dirs, bool splitView, bool skipChildUrls = false); + void openDirectories(const QList& 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 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 indexByUrl(const QUrl& url, ChildUrlBehavior childUrlBehavior = ReturnIndexForOpenedUrlOnly) const; + const std::optional viewShowingItem(const QUrl& item) const; private: QPointer m_lastViewedTab; diff --git a/src/global.cpp b/src/global.cpp index d8132b20ac..cd1409d9e0 100644 --- a/src/global.cpp +++ b/src/global.cpp @@ -90,7 +90,7 @@ bool Dolphin::attachToExistingInstance(const QList& 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); diff --git a/src/views/dolphinview.cpp b/src/views/dolphinview.cpp index 6372266d4f..590fe336b7 100644 --- a/src/views/dolphinview.cpp +++ b/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. diff --git a/src/views/dolphinview.h b/src/views/dolphinview.h index 2ecd75957f..aff4c51a84 100644 --- a/src/views/dolphinview.h +++ b/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) */