Differential D22594 Diff 67500 src/dolphinmainwindow.cpp

Changeset View

Standalone View

View Options

src/dolphinmainwindow.cpp

Context not available.
57#include <KJobWidgets> 57​#include <KJobWidgets>
58#include <KLocalizedString> 58​#include <KLocalizedString>
59#include <KMessageBox> 59​#include <KMessageBox>
60​#include <KNS3/KMoreToolsMenuFactory>
60#include <KProtocolInfo> 61​#include <KProtocolInfo>
61#include <KProtocolManager> 62​#include <KProtocolManager>
62#include <KRun> 63​#include <KRun>
Context not available.
195 toolBar()->installEventFilter(middleClickEventFilter); 196​ toolBar()->installEventFilter(middleClickEventFilter);
196 197​
197 setupWhatsThis(); 198​ setupWhatsThis();
199​
200​ QTimer::singleShot(0, this, &DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction);
198} 201​}
199 202​
200DolphinMainWindow::~DolphinMainWindow() 203​DolphinMainWindow::~DolphinMainWindow()
elvisangelaccioUnsubmitted
Done

Coding style: opening brace should go to next line

elvisangelaccio: Coding style: opening brace should go to next line
elvisangelaccioUnsubmitted
Done

QUrl::fromLocalFile()

elvisangelaccio: `QUrl::fromLocalFile()`
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…
elvisangelaccioUnsubmitted
Done

qobject_cast

elvisangelaccio: `qobject_cast`
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()`.
Context not available.
930 } 933​ }
931} 934​}
932 935​
933void DolphinMainWindow::openTerminal() 936​QString DolphinMainWindow::activeContainerLocalPath()
934{ 937​{
935 QString dir(QDir::homePath()); ​
936 ​
937 // If the given directory is not local, it can still be the URL of an ​
938 // ioslave using UDS_LOCAL_PATH which to be converted first. ​
939 KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url()); 938​ KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url());
940 KJobWidgets::setWindow(statJob, this); 939​ KJobWidgets::setWindow(statJob, this);
941 statJob->exec(); 940​ statJob->exec();
942 QUrl url = statJob->mostLocalUrl(); 941​ QUrl url = statJob->mostLocalUrl();
943 ​
944 //If the URL is local after the above conversion, set the directory. ​
945 if (url.isLocalFile()) { 942​ if (url.isLocalFile()) {
946 dir = url.toLocalFile(); 943​ return url.toLocalFile();
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…
947 } 944​ }
945​ return QDir::homePath();
946​}
948 947​
949 KToolInvocation::invokeTerminal(QString(), dir); 948​QPointer<QAction> DolphinMainWindow::preferredSearchTool()
949​{
950​ m_searchTools.clear();
951​ KMoreToolsMenuFactory("dolphin/search-tools").fillMenuFromGroupingNames(
952​ &m_searchTools, { "files-find" }, QUrl::fromLocalFile(activeContainerLocalPath())
953​ );
954​ QList<QAction*> actions = m_searchTools.actions();
955​ if (actions.isEmpty()) {
956​ return nullptr;
957​ }
958​ QAction* action = actions.first();
959​ if (action->isSeparator()) {
960​ return nullptr;
961​ }
962​ return action;
963​}
964​
965​void DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction()
966​{
967​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
968​ QList<QWidget*> widgets = openPreferredSearchTool->associatedWidgets();
elvisangelaccioUnsubmitted
Done

Missing const

elvisangelaccio: Missing `const`
969​ for (QWidget* widget : widgets) {
970​ QMenu* menu = qobject_cast<QMenu*>(widget);
971​ if (menu) {
972​ connect(menu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
973​ }
974​ }
975​ connect(
976​ actionCollection()->action(KStandardAction::name(KStandardAction::KeyBindings)), &QAction::hovered,
977​ this, &DolphinMainWindow::updateOpenPreferredSearchToolAction
978​ );
979​ updateOpenPreferredSearchToolAction();
980​}
981​
982​void DolphinMainWindow::updateOpenPreferredSearchToolAction()
983​{
984​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
985​ if (!openPreferredSearchTool) {
986​ return;
987​ }
988​ QPointer<QAction> tool = preferredSearchTool();
989​ if (tool) {
990​ openPreferredSearchTool->setVisible(true);
991​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open %1", tool->text()));
992​ openPreferredSearchTool->setIcon(tool->icon());
993​ } else {
994​ openPreferredSearchTool->setVisible(false);
995​ // still visible in Shortcuts configuration window
996​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
997​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
998​ }
999​}
1000​
1001​void DolphinMainWindow::openPreferredSearchTool()
1002​{
1003​ QPointer<QAction> tool = preferredSearchTool();
1004​ if (tool) {
1005​ tool->trigger();
1006​ }
1007​}
1008​
1009​void DolphinMainWindow::openTerminal()
1010​{
1011​ KToolInvocation::invokeTerminal(QString(), activeContainerLocalPath());
950} 1012​}
951 1013​
952void DolphinMainWindow::editSettings() 1014​void DolphinMainWindow::editSettings()
Context not available.
1088 1150​
1089 // Add a curated assortment of items from the "Tools" menu 1151​ // Add a curated assortment of items from the "Tools" menu
1090 addActionToMenu(ac->action(QStringLiteral("show_filter_bar")), menu); 1152​ addActionToMenu(ac->action(QStringLiteral("show_filter_bar")), menu);
1153​ addActionToMenu(ac->action(QStringLiteral("open_preferred_search_tool")), menu);
1091 addActionToMenu(ac->action(QStringLiteral("open_terminal")), menu); 1154​ addActionToMenu(ac->action(QStringLiteral("open_terminal")), menu);
1155​ connect(menu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
1092 1156​
1093 menu->addSeparator(); 1157​ menu->addSeparator();
1094 1158​
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.
1458 compareFiles->setEnabled(false); 1522​ compareFiles->setEnabled(false);
1459 connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles); 1523​ connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles);
1460 1524​
1525​ QAction* openPreferredSearchTool = actionCollection()->addAction(QStringLiteral("open_preferred_search_tool"));
1526​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
1527​ openPreferredSearchTool->setWhatsThis(xi18nc("@info:whatsthis",
1528​ "<para>This opens a preferred search tool for the viewed location.</para>"
1529​ "<para>Use <emphasis>More Search Tools</emphasis> menu to configure it.</para>"));
1530​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
1531​ actionCollection()->setDefaultShortcut(openPreferredSearchTool, Qt::CTRL + Qt::SHIFT + Qt::Key_F);
1532​ connect(openPreferredSearchTool, &QAction::triggered, this, &DolphinMainWindow::openPreferredSearchTool);
1533​
1461#ifdef HAVE_TERMINAL 1534​#ifdef HAVE_TERMINAL
1462 if (KAuthorized::authorize(QStringLiteral("shell_access"))) { 1535​ if (KAuthorized::authorize(QStringLiteral("shell_access"))) {
1463 QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal")); 1536​ QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal"));
Context not available.
2181 QWhatsThisClickedEvent* whatsThisEvent = dynamic_cast<QWhatsThisClickedEvent*>(event); 2254​ QWhatsThisClickedEvent* whatsThisEvent = dynamic_cast<QWhatsThisClickedEvent*>(event);
2182 QDesktopServices::openUrl(QUrl(whatsThisEvent->href())); 2255​ QDesktopServices::openUrl(QUrl(whatsThisEvent->href()));
2183 return true; 2256​ return true;
2257​ } else if (event->type() == QEvent::WindowActivate) {
2258​ updateOpenPreferredSearchToolAction();
2184 } 2259​ }
2185 return KXmlGuiWindow::event(event); 2260​ return KXmlGuiWindow::event(event);
2186} 2261​}
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