[Dolphin] Open Preferred Search Tool action
ClosedPublic

Authored by pdabrowski on Jul 20 2019, 7:08 PM.

Details

Summary

Added "Open Preferred Search Tool" action to Tools menu.

It runs preferred (topmost) external search tool as configured in the "More Search Tools" menu.

By default Ctrl+Shift+F shortcut is assigned to this action.

FEATURE: 384798



Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added projects: Dolphin, Documentation. · View Herald TranscriptJul 20 2019, 7:08 PM
Restricted Application added subscribers: kde-doc-english, kfm-devel. · View Herald Transcript
pdabrowski requested review of this revision.Jul 20 2019, 7:08 PM
pdabrowski edited the summary of this revision. (Show Details)Jul 21 2019, 12:55 AM
ngraham requested changes to this revision.Jul 21 2019, 3:37 AM
ngraham added a reviewer: elvisangelaccio.

This is a very well-formed patch, with good code (including correct usage of KUIT markup!), a good commit message, and a good screenshot. Well done! The only thing that could be improved to to use arc for the next one. :) https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist

As for the feature itself, for the past few years I'd been opposed to this, on the basis that people should just use the built-in Baloo-based search instead. But unfortunately the requests keep coming in, and there are use cases that Baloo doesn't work for (e.g. on external disks), so there does seem to be a need, or at least a desire. And we do already have this accessible from the search panel, so this is basically just a shortcut to that. I'll give this patch a fair shake.

In general +1. Here are a few suggestions:

  1. Instead of showing a generic text, how about making it actually say "Search with <name of preferred search tool"?
  2. For the keyboard shortcut let's use Alt instead of Shift. It is an alternate search, after all.
  3. Whenever you change anything in a .rc file, you need to bump the version number that appears at the top of the file.
This revision now requires changes to proceed.Jul 21 2019, 3:37 AM
pdabrowski updated this revision to Diff 62211.Jul 21 2019, 5:50 PM
  1. Whenever you change anything in a .rc file, you need to bump the version number that appears at the top of the file.

Done.

pdabrowski updated this revision to Diff 62212.Jul 21 2019, 5:53 PM
pdabrowski edited the summary of this revision. (Show Details)
  1. Instead of showing a generic text, how about making it actually say "Search with <name of preferred search tool"?

Done.

  1. For the keyboard shortcut let's use Alt instead of Shift. It is an alternate search, after all.

I found Ctrl+Shift+F to be much more popular for "search", "advanced search", "search in all ...", "find in ...", "advanced find", etc.:
https://defkey.com/what-means/ctrl-shift-f (40 hits) vs
https://defkey.com/what-means/ctrl-alt-f (7 hits)

And Ctrl+Shift+F seems to be widely used by virtually all of the most professional applications:
Mozilla Thunderbird, Opera, Sublime, Android Studio, Atom, IntelliJ IDEA, Visual Studio, NetBeans, Notepad++, Qt Creator, Visual Studio Code, Outlook

That's why I have used this shortcut.

My personal opinion is that it also matches Shift+F4 shortcut for "Open Terminal" better (see screenshot).
Both of which open an external tool, which I think goes well with the Shift modifier.

All right, you've convinced me on the shortcut.

The way you've implemented the menu item naming seems a bit fragile though (and in line 1410 of src/dolphinmainwindow.cpp, it's not used at all). In my testing, the name doesn't show up and it falls back to the static name. If you can't find a way to implement the feature reliably, it may be best to just always show static text. But it would be nice to have the name of the app reliably show up there, though.

The way you've implemented the menu item naming seems a bit fragile though

True. It's really hard to update it reliably.
KMoreTools* do not provide an easy way to get notified that the tools changed. Even current solution in "More Search Tools" menu recreates the menu every time it is displayed.
So I tried updating the action every time menu containing it is shown. Connect()ions for this aren't pretty, but this should actually work...

In my testing, the name doesn't show up and it falls back to the static name.

This is strange, because this action should be updated on many occasions, including Dolphin startup.
Or is your testcase installing KFind while Dolphin is open, or doing something similar? (Either way, that should work too).

Please let me know a test scenario that fails.

No, I already had KFind installed. I just opened Dolphin and looked at the menu item, nothing fancy. :/

ngraham accepted this revision.Jul 21 2019, 10:27 PM

Well now I'm embarrassed to admit that I probably didn't. I think I updated it but forgot to compile lol. Sorry for wasting your time!

This looks great to me now. @elvisangelaccio do you agree?

This revision is now accepted and ready to land.Jul 21 2019, 10:27 PM

No problem. I'm glad it works :)

pdabrowski added a comment.EditedJul 21 2019, 10:53 PM

I did some more testing, and everything seems to work fine:

$ sudo su
# cd /usr/share/applications
# sleep 2 && mv org.kde.kfind.desktop org.kde.kfind.desktop_    # no KFind now
# sleep 2 && mv org.kde.kfind.desktop_ org.kde.kfind.desktop    # KFind installed

Actions to test (KFind installed/not installed):

  • menubar -> Tools -> Open KFind/none
  • Control toolbutton (menubar hidden) -> Tools -> Open KFind/none
  • [Settings] -> Configure Shortcuts... -> shortcut for Open KFind/Open Preferred Search Tool in the list

As for the feature itself, for the past few years I'd been opposed to this, on the basis that people should just use the built-in Baloo-based search instead. But unfortunately the requests keep coming in

Where? Do we have an open wish on bugs.kde.org ?

I'd like to remind that the menubar is hidden by default and the average user won't know about the CTRL+Shift+F shortcut.
So is this feature only aimed at advanced users who expect CTRL+Shift+F to do something?

pdabrowski added a comment.EditedJul 29 2019, 8:58 AM

I'd like to remind that the menubar is hidden by default and the average user won't know about the CTRL+Shift+F shortcut.
So is this feature only aimed at advanced users who expect CTRL+Shift+F to do something?

This feature is also available from the Control toolbutton's menu:
[Control] toolbutton (menubar hidden) -> [Tools] -> [Open KFind]
and you can add that entry directly into the Dolphin's toolbar.

pdabrowski added a comment.EditedJul 29 2019, 9:30 AM

As for the feature itself, for the past few years I'd been opposed to this, on the basis that people should just use the built-in Baloo-based search instead. But unfortunately the requests keep coming in

Where? Do we have an open wish on bugs.kde.org ?

I guess @ngraham will be able to tell the full story on this, but here are some links:
https://bugs.kde.org/show_bug.cgi?id=384798
and its duplicates related issues:
https://bugs.kde.org/show_bug.cgi?id=391677
https://bugs.kde.org/show_bug.cgi?id=401317
https://bugs.kde.org/show_bug.cgi?id=400949
Probably some more I didn't find.
And the "More Search Tools" menu was implemented for this exact reason in the first place.
Of course this Revision is also my own Wish in a form of a patch :)

pkloc added a subscriber: pkloc.EditedJul 29 2019, 1:33 PM

As a person who opened https://bugs.kde.org/show_bug.cgi?id=391677 I would like to note that it is not in fact a duplicate of https://bugs.kde.org/show_bug.cgi?id=384798
My reasoning for opening that bug was because baloo is sometimes very unreliable at finding anything on my machine. And thanks to solid state drives there is no issue of waiting for spinning rust so lack of indexing is not an issue, also you could argue that indexing is a privacy risk.

pdabrowski edited the summary of this revision. (Show Details)Aug 1 2019, 3:14 PM
pdabrowski updated this revision to Diff 62910.Aug 1 2019, 4:01 PM

Small fix for the "Open Preferred Search Tool" as a toolbar button.

pdabrowski edited the summary of this revision. (Show Details)Aug 1 2019, 4:17 PM
ngraham accepted this revision.Aug 1 2019, 7:41 PM

I'll be AFK for a week for vacation. Will do a proper review once I'm back ;)

src/dolphinmainwindow.cpp
874

Coding style: opening brace should go to next line

src/dolphinui.rc
2

Please rebase, thanks!

pdabrowski marked 2 inline comments as done.

Done.

Needs one more rebase, sorry

elvisangelaccio requested changes to this revision.Sep 22 2019, 8:59 PM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
894

QUrl::fromLocalFile()

910

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

911

qobject_cast

916–919

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

src/dolphinmainwindow.h
590–595

I know this is the old comment, but it's not clear. How about something like this?

Returns the KIO::StatJob::mostLocalUrl() for the active container URL if it's a local file.
Otherwise returns the user's home path.
596

I'd call this method activeContainerLocalPath() to make it more clear what it is about.

src/dolphinpart.h
237

Hmm, what about KParts::ReadOnlyPart::localFilePath() ? Could we use that?

This revision now requires changes to proceed.Sep 22 2019, 8:59 PM
pdabrowski updated this revision to Diff 67500.Oct 8 2019, 1:30 PM
pdabrowski marked 4 inline comments as done.
pdabrowski marked 2 inline comments as done.
pdabrowski added inline comments.
src/dolphinmainwindow.cpp
916–919

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.

elvisangelaccio requested changes to this revision.Oct 13 2019, 3:19 PM

Almost there!

src/dolphinmainwindow.cpp
916–919

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

968

Missing const

src/dolphinpart.cpp
536

This is an unrelated change actually, isn't it? It makes sense, but it should go in another commit.

This revision now requires changes to proceed.Oct 13 2019, 3:19 PM
pdabrowski updated this revision to Diff 69855.Nov 16 2019, 7:25 PM
pdabrowski marked 5 inline comments as done.
ngraham added inline comments.Nov 16 2019, 9:48 PM
src/dolphinmainwindow.cpp
1162

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?

pdabrowski added inline comments.Nov 17 2019, 8:15 AM
src/dolphinmainwindow.cpp
1162

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 added inline comments.Nov 17 2019, 9:27 AM
src/dolphinmainwindow.cpp
1162

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?

ngraham accepted this revision.Nov 17 2019, 4:11 PM

Okay, that makes sense.

elvisangelaccio accepted this revision.Nov 17 2019, 5:18 PM
This revision is now accepted and ready to land.Nov 17 2019, 5:18 PM
This revision was automatically updated to reflect the committed changes.
broulik added inline comments.
src/dolphinmainwindow.cpp
943

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

2289

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?

pdabrowski added inline comments.May 5 2020, 9:23 AM
src/dolphinmainwindow.cpp
943

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.
2289

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.

broulik added inline comments.May 5 2020, 9:26 AM
src/dolphinmainwindow.cpp
2289

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.

pdabrowski added inline comments.May 5 2020, 9:39 AM
src/dolphinmainwindow.cpp
2289

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

broulik added inline comments.May 5 2020, 9:40 AM
src/dolphinmainwindow.cpp
2289

(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.

pdabrowski added inline comments.May 5 2020, 9:44 AM
src/dolphinmainwindow.cpp
2289

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.

broulik added inline comments.May 5 2020, 9:50 AM
src/dolphinmainwindow.cpp
2289

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 :)

src/dolphinmainwindow.cpp
2289

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.

pdabrowski added inline comments.May 9 2020, 2:43 PM
src/dolphinmainwindow.cpp
2289

D29568: use KSycoca for updating OpenPreferredSearchTool action

firef added a subscriber: firef.Oct 2 2020, 6:13 AM

As for the feature itself, for the past few years I'd been opposed to this, on the basis that people should just use the built-in Baloo-based search instead. But unfortunately the requests keep coming in, and there are use cases that Baloo doesn't work for (e.g. on external disks), so there does seem to be a need, or at least a desire. And we do already have this accessible from the search panel, so this is basically just a shortcut to that. I'll give this patch a fair shake.

I am unsure whether "basis that people should just use the built-in Baloo-based search instead." is a good enough justification for many people.

Note that the KFind-toolbar-button - in 1 click distance - is hugely superior from a usability perspective to a menu entry in 2 click distance!

Having to set up KFind if, say, catfish-search was hypothetically the preset, with KFind on offer only in a secondary settings menu in 3+ click distance, would be a chore escaping 90+ % of users in my estimation.

That fact that Dolphin now has two separate configuration menus (one of the two made for search tools only) may not be 100% ideal from a usability perspective.

But the KFind button is a huge improvement, and I don't mean "huge" in the Trumpian sense. see also : KFind on right-click