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
ngraham | |
elvisangelaccio |
Dolphin |
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
Lint Skipped |
Unit Tests Skipped |
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:
- Whenever you change anything in a .rc file, you need to bump the version number that appears at the top of the file.
Done.
- Instead of showing a generic text, how about making it actually say "Search with <name of preferred search tool"?
Done.
- 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. :/
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?
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):
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?
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.
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 :)
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.
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! |
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? |
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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
1157 | 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? |
src/dolphinmainwindow.cpp | ||
---|---|---|
1157 | 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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
1157 | 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. |
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 |
src/dolphinmainwindow.cpp | ||
---|---|---|
943 | Yes, it was like that in DolphinMainWindow::openTerminal() before. // 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. |
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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
2289 |
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) |
src/dolphinmainwindow.cpp | ||
---|---|---|
2289 |
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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
2289 | It is still impossible to detect (un)installation of such tool. |
src/dolphinmainwindow.cpp | ||
---|---|---|
2289 |
There's signals when the KSycoca service database changes which we use in the application menu to update when apps are installed/uninstalled. |
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