Differential D22594 Diff 62212 src/dolphinmainwindow.cpp

Changeset View

Standalone View

View Options

src/dolphinmainwindow.cpp

Context not available.
56#include <KJobWidgets> 56​#include <KJobWidgets>
57#include <KLocalizedString> 57​#include <KLocalizedString>
58#include <KMessageBox> 58​#include <KMessageBox>
59​#include <KNS3/KMoreToolsMenuFactory>
59#include <KProtocolInfo> 60​#include <KProtocolInfo>
60#include <KProtocolManager> 61​#include <KProtocolManager>
61#include <KRun> 62​#include <KRun>
Context not available.
186 toolBar()->installEventFilter(middleClickEventFilter); 187​ toolBar()->installEventFilter(middleClickEventFilter);
187 188​
188 setupWhatsThis(); 189​ setupWhatsThis();
190​
191​ QTimer::singleShot(0, this, &DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction);
189} 192​}
190 193​
191DolphinMainWindow::~DolphinMainWindow() 194​DolphinMainWindow::~DolphinMainWindow()
Context not available.
839 } 842​ }
840} 843​}
841 844​
842void DolphinMainWindow::openTerminal() 845​QString DolphinMainWindow::localPath()
843{ 846​{
844 QString dir(QDir::homePath()); ​
845 ​
846 // If the given directory is not local, it can still be the URL of an ​
847 // ioslave using UDS_LOCAL_PATH which to be converted first. ​
848 KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url()); 847​ KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url());
849 KJobWidgets::setWindow(statJob, this); 848​ KJobWidgets::setWindow(statJob, this);
850 statJob->exec(); 849​ statJob->exec();
851 QUrl url = statJob->mostLocalUrl(); 850​ QUrl url = statJob->mostLocalUrl();
852 ​
853 //If the URL is local after the above conversion, set the directory. ​
854 if (url.isLocalFile()) { 851​ if (url.isLocalFile()) {
855 dir = url.toLocalFile(); 852​ return url.toLocalFile();
853​ }
854​ return QDir::homePath();
855​}
856​
857​QPointer<QAction> DolphinMainWindow::preferredSearchTool()
858​{
859​ m_searchTools.clear();
860​ KMoreToolsMenuFactory("dolphin/search-tools").fillMenuFromGroupingNames(
861​ &m_searchTools, { "files-find" }, QUrl(localPath())
862​ );
863​ QList<QAction*> actions = m_searchTools.actions();
864​ if (actions.isEmpty()) {
865​ return nullptr;
866​ }
867​ QAction* action = actions.first();
868​ if (action->isSeparator()) {
869​ return nullptr;
870​ }
871​ return action;
872​}
873​
874​void DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction() {
elvisangelaccioUnsubmitted
Done

Coding style: opening brace should go to next line

elvisangelaccio: Coding style: opening brace should go to next line
875​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
876​ for (QWidget* widget : openPreferredSearchTool->associatedWidgets()) {
877​ QMenu* menu = dynamic_cast<QMenu*>(widget);
878​ if (menu) {
879​ connect(menu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
880​ }
881​ }
882​ connect(
883​ actionCollection()->action(KStandardAction::name(KStandardAction::KeyBindings)), &QAction::hovered,
884​ this, &DolphinMainWindow::updateOpenPreferredSearchToolAction
885​ );
886​ updateOpenPreferredSearchToolAction();
887​}
888​
889​void DolphinMainWindow::updateOpenPreferredSearchToolAction()
890​{
891​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
892​ if (!openPreferredSearchTool) {
893​ return;
894​ }
elvisangelaccioUnsubmitted
Done

QUrl::fromLocalFile()

elvisangelaccio: `QUrl::fromLocalFile()`
895​ QPointer<QAction> tool = preferredSearchTool();
896​ if (tool) {
897​ openPreferredSearchTool->setVisible(true);
898​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open %1", tool->text()));
899​ openPreferredSearchTool->setIcon(tool->icon());
900​ } else {
901​ openPreferredSearchTool->setVisible(false);
902​ // still visible in Shortcuts configuration window
903​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
904​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
905​ }
906​}
907​
908​void DolphinMainWindow::openPreferredSearchTool()
909​{
910​ QPointer<QAction> tool = preferredSearchTool();
elvisangelaccioUnsubmitted
Done

This might detach the QList container. Either use a temp 'const QList` variable or use qAsConst

elvisangelaccio: This might detach the QList container. Either use a temp 'const QList` variable or use…
911​ if (tool) {
elvisangelaccioUnsubmitted
Done

qobject_cast

elvisangelaccio: `qobject_cast`
912​ tool->trigger();
856 } 913​ }
914​}
857 915​
858 KToolInvocation::invokeTerminal(QString(), dir); 916​void DolphinMainWindow::openTerminal()
917​{
918​ KToolInvocation::invokeTerminal(QString(), localPath());
859} 919​}
elvisangelaccioUnsubmitted
Done

I don't get this connection. Can you explain what's needed for?

elvisangelaccio: I don't get this connection. Can you explain what's needed for?
pdabrowskiAuthorUnsubmitted
Done

There is no other way to update the open_preferred_search_tool action *before* the Configure Shortcuts window is shown.
This action is then listed in that window, so it should be up-to-date when it is displayed.
This update is instantaneous if user made no changes to the search tools in the meantime.

pdabrowski: There is no other way to update the open_preferred_search_tool action *before* the Configure…
elvisangelaccioUnsubmitted
Done

Please write this information in a comment above that connect().

elvisangelaccio: Please write this information in a comment above that `connect()`.
860 920​
861void DolphinMainWindow::editSettings() 921​void DolphinMainWindow::editSettings()
elvisangelaccioUnsubmitted
Done

Missing const

elvisangelaccio: Missing `const`
broulikUnsubmitted
Not Done

exec() on a job is generally bad, but looks like it was like this already?

broulik: `exec()` on a job is generally bad, but looks like it was like this already?
pdabrowskiAuthorUnsubmitted
Not Done

Yes, it was like that in DolphinMainWindow::openTerminal() before.
It was needed as explained in this comment:

// If the given directory is not local, it can still be the URL of an
// ioslave using UDS_LOCAL_PATH which to be converted first.
pdabrowski: Yes, it was like that in DolphinMainWindow::openTerminal() before. It was needed as explained…
Context not available.
1029 QMenu* toolsMenu = new QMenu(i18nc("@action:inmenu", "Tools"), menu); 1089​ QMenu* toolsMenu = new QMenu(i18nc("@action:inmenu", "Tools"), menu);
1030 toolsMenu->addAction(ac->action(QStringLiteral("show_filter_bar"))); 1090​ toolsMenu->addAction(ac->action(QStringLiteral("show_filter_bar")));
1031 toolsMenu->addAction(ac->action(QStringLiteral("compare_files"))); 1091​ toolsMenu->addAction(ac->action(QStringLiteral("compare_files")));
1092​ toolsMenu->addAction(ac->action(QStringLiteral("open_preferred_search_tool")));
1032 toolsMenu->addAction(ac->action(QStringLiteral("open_terminal"))); 1093​ toolsMenu->addAction(ac->action(QStringLiteral("open_terminal")));
1033 toolsMenu->addAction(ac->action(QStringLiteral("change_remote_encoding"))); 1094​ toolsMenu->addAction(ac->action(QStringLiteral("change_remote_encoding")));
1034 menu->addMenu(toolsMenu); 1095​ menu->addMenu(toolsMenu);
1096​ connect(toolsMenu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
1035 1097​
1036 // Add "Settings" menu entries 1098​ // Add "Settings" menu entries
1037 addActionToMenu(ac->action(KStandardAction::name(KStandardAction::KeyBindings)), menu); 1099​ addActionToMenu(ac->action(KStandardAction::name(KStandardAction::KeyBindings)), menu);
ngrahamUnsubmitted
Not Done

Since this patch was made, we've re-organized the hamburger menu a bit to simplify it and hide not-commonly-used functionality. I think this feature counts, since it's an alternative to the built-in search, and as such, it won't be useful for the majority of Dolphin's users who use the built-in search instead. Could you remove it from the hamburger menu?

ngraham: Since this patch was made, we've re-organized the hamburger menu a bit to simplify it and hide…
pdabrowskiAuthorUnsubmitted
Not Done

I've added this action everywhere where "Open Terminal" appeared. If you say the hamburger menu should not contain it though, I will remove it from there.
My only concern is that it will be hard for users to notice that it become available as optional toolbar button or direct shortcut. Even if someone anticipated this feature, they may easily miss it and continue not to use it.

pdabrowski: I've added this action everywhere where "Open Terminal" appeared. If you say the hamburger menu…
pdabrowskiAuthorUnsubmitted
Not Done

One more thing to consider: no alternative tool (including KFind) is installed by default (at least in Kubuntu and KDE Neon). So by default this action will not appear at all.
And if user chooses to manually install KFind or another alternative, then they may expect to have it easily accessible from this menu.
What do you think?

pdabrowski: One more thing to consider: no alternative tool (including KFind) is installed by default (at…
Context not available.
1344 compareFiles->setEnabled(false); 1406​ compareFiles->setEnabled(false);
1345 connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles); 1407​ connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles);
1346 1408​
1409​ QAction* openPreferredSearchTool = actionCollection()->addAction(QStringLiteral("open_preferred_search_tool"));
1410​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
1411​ openPreferredSearchTool->setWhatsThis(xi18nc("@info:whatsthis",
1412​ "<para>This opens a preferred search tool for the viewed location.</para>"
1413​ "<para>Use <emphasis>More Search Tools</emphasis> menu to configure it.</para>"));
1414​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
1415​ actionCollection()->setDefaultShortcut(openPreferredSearchTool, Qt::CTRL + Qt::SHIFT + Qt::Key_F);
1416​ connect(openPreferredSearchTool, &QAction::triggered, this, &DolphinMainWindow::openPreferredSearchTool);
1417​
1347#ifdef HAVE_TERMINAL 1418​#ifdef HAVE_TERMINAL
1348 if (KAuthorized::authorize(QStringLiteral("shell_access"))) { 1419​ if (KAuthorized::authorize(QStringLiteral("shell_access"))) {
1349 QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal")); 1420​ QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal"));
Context not available.
broulikUnsubmitted
Not Done

This causes quite severe bug https://bugs.kde.org/show_bug.cgi?id=420911
Why is this needed anyway, we already monitor aboutToShow for all the relevant menus, no?

broulik: This causes quite severe bug https://bugs.kde.org/show_bug.cgi?id=420911 Why is this needed…
pdabrowskiAuthorUnsubmitted
Not Done

It was added to handle this action as a button in toolbar (can be added by user, and this was requested in #384798).

I didn't notice this causes such a problem with remote connections, sorry.

It is a rare occasion that user changes preferred search tools, but I guess they would like to have it updated all over Dolphin when they do.

pdabrowski: It was added to handle this action as a button in toolbar (can be added by user, and this was…
broulikUnsubmitted
Not Done

It is a rare occasion that user changes preferred search tools,

So, the URL does not have any impact on the search tool chosen? I thought that was the main reason for using the URL and updating it all the time.

broulik: > It is a rare occasion that user changes preferred search tools, So, the URL does not have…
pdabrowskiAuthorUnsubmitted
Not Done

So, the URL does not have any impact on the search tool chosen? I thought that was the main reason for using the URL and updating it all the time.

This depends on KMoreToolsMenuFactory. But it doesn't seem to choose different tools for different URLs.

The main reason for the updates was so that all the Open Preferred Search Tool actions reflect the user choice of preferred tool (can be modified with More Search Tools -> More -> Configure)
See: https://phabricator.kde.org/D22594#499652

pdabrowski: >So, the URL does not have any impact on the search tool chosen? I thought that was the main…
broulikUnsubmitted
Not Done

(can be modified with More Search Tools -> More -> Configure)

Then there should be a signal in KMoreTools a user of this class can connect to to get notified when user preference changes. Or, the actions were actually governed by KMoreTools and automatically updated.

broulik: > (can be modified with More Search Tools -> More -> Configure) Then there should be a signal…
pdabrowskiAuthorUnsubmitted
Not Done

It is still impossible to detect (un)installation of such tool.
"More Search Tools" menu currently recreates the menu from KMoreToolsMenuFactory when it is shown.

pdabrowski: It is still impossible to detect (un)installation of such tool. "More Search Tools" menu…
broulikUnsubmitted
Not Done

It is still impossible to detect (un)installation of such tool.

There's signals when the KSycoca service database changes which we use in the application menu to update when apps are installed/uninstalled.
Might not work for non-gui tools without a desktop file, but I don't think KMoreTools offers to open command line tools.
Still better than updating the menu all the time :)

broulik: > It is still impossible to detect (un)installation of such tool. There's signals when the…
elvisangelaccioUnsubmitted
Not Done

I agree with @broulik. This is a niche use case anyway, so if we really need to support it, we should do it right without nasty side effects.

elvisangelaccio: I agree with @broulik. This is a niche use case anyway, so if we really need to support it, we…
pdabrowskiAuthorUnsubmitted
Not Done

D29568: use KSycoca for updating OpenPreferredSearchTool action

pdabrowski: D29568: use KSycoca for updating OpenPreferredSearchTool action