[Dolphin] Open Preferred Search Tool action
Needs RevisionPublic

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

Details

Reviewers
ngraham
elvisangelaccio
Group Reviewers
Dolphin
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
pdabrowski created this revision.Jul 20 2019, 7:08 PM
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
880

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
579–584

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

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.Tue, Oct 8, 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.Sun, Oct 13, 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.Sun, Oct 13, 3:19 PM