Differential D22594 Diff 66163 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.
187 toolBar()->installEventFilter(middleClickEventFilter); 188​ toolBar()->installEventFilter(middleClickEventFilter);
188 189​
189 setupWhatsThis(); 190​ setupWhatsThis();
191​
192​ QTimer::singleShot(0, this, &DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction);
190} 193​}
191 194​
192DolphinMainWindow::~DolphinMainWindow() 195​DolphinMainWindow::~DolphinMainWindow()
elvisangelaccioUnsubmitted
Done

Coding style: opening brace should go to next line

elvisangelaccio: Coding style: opening brace should go to next line
Context not available.
872 } 875​ }
873} 876​}
874 877​
875void DolphinMainWindow::openTerminal() 878​QString DolphinMainWindow::localPath()
876{ 879​{
877 QString dir(QDir::homePath()); ​
878 ​
879 // If the given directory is not local, it can still be the URL of an ​
880 // ioslave using UDS_LOCAL_PATH which to be converted first. ​
881 KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url()); 880​ KIO::StatJob* statJob = KIO::mostLocalUrl(m_activeViewContainer->url());
882 KJobWidgets::setWindow(statJob, this); 881​ KJobWidgets::setWindow(statJob, this);
883 statJob->exec(); 882​ statJob->exec();
884 QUrl url = statJob->mostLocalUrl(); 883​ QUrl url = statJob->mostLocalUrl();
885 ​
886 //If the URL is local after the above conversion, set the directory. ​
887 if (url.isLocalFile()) { 884​ if (url.isLocalFile()) {
888 dir = url.toLocalFile(); 885​ return url.toLocalFile();
889 } 886​ }
887​ return QDir::homePath();
888​}
890 889​
891 KToolInvocation::invokeTerminal(QString(), dir); 890​QPointer<QAction> DolphinMainWindow::preferredSearchTool()
891​{
892​ m_searchTools.clear();
893​ KMoreToolsMenuFactory("dolphin/search-tools").fillMenuFromGroupingNames(
894​ &m_searchTools, { "files-find" }, QUrl(localPath())
elvisangelaccioUnsubmitted
Done

QUrl::fromLocalFile()

elvisangelaccio: `QUrl::fromLocalFile()`
895​ );
896​ QList<QAction*> actions = m_searchTools.actions();
897​ if (actions.isEmpty()) {
898​ return nullptr;
899​ }
900​ QAction* action = actions.first();
901​ if (action->isSeparator()) {
902​ return nullptr;
903​ }
904​ return action;
905​}
906​
907​void DolphinMainWindow::setupUpdateOpenPreferredSearchToolAction()
908​{
909​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
910​ for (QWidget* widget : openPreferredSearchTool->associatedWidgets()) {
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​ QMenu* menu = dynamic_cast<QMenu*>(widget);
elvisangelaccioUnsubmitted
Done

qobject_cast

elvisangelaccio: `qobject_cast`
912​ if (menu) {
913​ connect(menu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
914​ }
915​ }
916​ connect(
917​ actionCollection()->action(KStandardAction::name(KStandardAction::KeyBindings)), &QAction::hovered,
918​ this, &DolphinMainWindow::updateOpenPreferredSearchToolAction
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()`.
920​ updateOpenPreferredSearchToolAction();
921​}
922​
923​void DolphinMainWindow::updateOpenPreferredSearchToolAction()
924​{
925​ QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
926​ if (!openPreferredSearchTool) {
927​ return;
928​ }
929​ QPointer<QAction> tool = preferredSearchTool();
930​ if (tool) {
931​ openPreferredSearchTool->setVisible(true);
932​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open %1", tool->text()));
933​ openPreferredSearchTool->setIcon(tool->icon());
934​ } else {
935​ openPreferredSearchTool->setVisible(false);
936​ // still visible in Shortcuts configuration window
937​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
938​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
939​ }
940​}
941​
942​void DolphinMainWindow::openPreferredSearchTool()
943​{
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…
944​ QPointer<QAction> tool = preferredSearchTool();
945​ if (tool) {
946​ tool->trigger();
947​ }
948​}
949​
950​void DolphinMainWindow::openTerminal()
951​{
952​ KToolInvocation::invokeTerminal(QString(), localPath());
892} 953​}
893 954​
894void DolphinMainWindow::editSettings() 955​void DolphinMainWindow::editSettings()
elvisangelaccioUnsubmitted
Done

Missing const

elvisangelaccio: Missing `const`
Context not available.
1030 1091​
1031 // Add a curated assortment of items from the "Tools" menu 1092​ // Add a curated assortment of items from the "Tools" menu
1032 addActionToMenu(ac->action(QStringLiteral("show_filter_bar")), menu); 1093​ addActionToMenu(ac->action(QStringLiteral("show_filter_bar")), menu);
1094​ addActionToMenu(ac->action(QStringLiteral("open_preferred_search_tool")), menu);
1033 addActionToMenu(ac->action(QStringLiteral("open_terminal")), menu); 1095​ addActionToMenu(ac->action(QStringLiteral("open_terminal")), menu);
1096​ connect(menu, &QMenu::aboutToShow, this, &DolphinMainWindow::updateOpenPreferredSearchToolAction);
1034 1097​
1035 menu->addSeparator(); 1098​ menu->addSeparator();
1036 1099​
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.
1370 compareFiles->setEnabled(false); 1433​ compareFiles->setEnabled(false);
1371 connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles); 1434​ connect(compareFiles, &QAction::triggered, this, &DolphinMainWindow::compareFiles);
1372 1435​
1436​ QAction* openPreferredSearchTool = actionCollection()->addAction(QStringLiteral("open_preferred_search_tool"));
1437​ openPreferredSearchTool->setText(i18nc("@action:inmenu Tools", "Open Preferred Search Tool"));
1438​ openPreferredSearchTool->setWhatsThis(xi18nc("@info:whatsthis",
1439​ "<para>This opens a preferred search tool for the viewed location.</para>"
1440​ "<para>Use <emphasis>More Search Tools</emphasis> menu to configure it.</para>"));
1441​ openPreferredSearchTool->setIcon(QIcon::fromTheme(QStringLiteral("search")));
1442​ actionCollection()->setDefaultShortcut(openPreferredSearchTool, Qt::CTRL + Qt::SHIFT + Qt::Key_F);
1443​ connect(openPreferredSearchTool, &QAction::triggered, this, &DolphinMainWindow::openPreferredSearchTool);
1444​
1373#ifdef HAVE_TERMINAL 1445​#ifdef HAVE_TERMINAL
1374 if (KAuthorized::authorize(QStringLiteral("shell_access"))) { 1446​ if (KAuthorized::authorize(QStringLiteral("shell_access"))) {
1375 QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal")); 1447​ QAction* openTerminal = actionCollection()->addAction(QStringLiteral("open_terminal"));
Context not available.
2093 QWhatsThisClickedEvent* whatsThisEvent = dynamic_cast<QWhatsThisClickedEvent*>(event); 2165​ QWhatsThisClickedEvent* whatsThisEvent = dynamic_cast<QWhatsThisClickedEvent*>(event);
2094 QDesktopServices::openUrl(QUrl(whatsThisEvent->href())); 2166​ QDesktopServices::openUrl(QUrl(whatsThisEvent->href()));
2095 return true; 2167​ return true;
2168​ } else if (event->type() == QEvent::WindowActivate) {
2169​ updateOpenPreferredSearchToolAction();
2096 } 2170​ }
2097 return KXmlGuiWindow::event(event); 2171​ return KXmlGuiWindow::event(event);
2098} 2172​}
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