ngraham requested changes to this revision. ngraham added a reviewer: elvisangelaccio. ngraham added a comment. This revision now requires changes to proceed.
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. REPOSITORY R318 Dolphin REVISION DETAIL https://phabricator.kde.org/D22594 To: pdabrowski, #dolphin, ngraham, elvisangelaccio Cc: kfm-devel, kde-doc-english, aprcela, fprice, gennad, fbampaloukas, alexde, feverfew, meven, spoorun, navarromorales, firef, andrebarros, skadinna, emmanuelp, mikesomov