Search web for selected text #15539
Search web for selected text #15539
Conversation
microsoft-github-policy-service
bot
added
the
Issue-Feature
| bool TermControl::HasSelection() const | ||
| { | ||
| return _core.HasSelection(); | ||
| } | ||
| Windows::Foundation::Collections::IVector<winrt::hstring> TermControl::SelectedText(bool trimTrailingWhitespace) const | ||
| { | ||
| return _core.SelectedText(trimTrailingWhitespace); | ||
| } |
There was a problem hiding this comment.
(This comment is mostly for other maintainers. I'm not trying to imply that you need to clean this up yourself.)
This addition introduces some code cruft, because all other parts of TerminalPage get their selection from TermControl when it raises events. We should consider cleaning this up in the future, so that there's only one single path to get the selection contents.
| // make it compact by replacing consecutive whitespaces with a single space | ||
| searchText = std::regex_replace(searchText, std::wregex(LR"(\s+)"), L" "); |
There was a problem hiding this comment.
I'm personally hoping to fully replace std::regex usage in this project with ICU in the future. One reason for this is because it uses ctype facets which are not optimized for wchar_t usage at all. It simply calls GetStringTypeW with CT_CTYPE1 for every single individual input character, which is of course not quite optimal. (They aren't optimized for char usage either, because it doesn't even support UTF-8, but that's beside the point.)
Considering that Unicode whitespace is relatively rare in Terminals, we could simply erase 0x20 whitespace only. This is fairly easy to achieve:
searchText.erase(std::unique(searchText.begin(), searchText.end(), [](const wchar_t a, const wchar_t b) {
return a == L' ' && b == L' ';
}), text.end());I personally believe that would already be sufficient for our use case. Otherwise, we can just keep the regex for now. It's not like this is a performance bottleneck anyways...
There was a problem hiding this comment.
I used the regex to also erase the artificial \r\n which are appended to the selected lines in case of block selection.
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
-Change how to wrap with qoutes -Remove SO and GH search
microsoft-github-policy-service
bot
added
Area-Extensibility
DHowett
added
the
Needs-Discussion
carlos-zamora
removed
the
Needs-Discussion
|
We've just discussed this PR in our team and unfortunately realized that it has a minor problem... Optimally we'd want the functionality to be added to our new context menu here: terminal/src/cascadia/TerminalApp/TerminalPage.cpp Lines 4580 to 4639 in 612b00c But the problem is that it doesn't use "real" actions yet. It uses fake actions which don't have any parameters. This puts us in a difficult spot: To make it work the way you built it we'd have to somehow create a settings schema where any user actions can be hooked up into the context menu automatically. Unfortunately, we most likely won't be able to figure something out in time for shipping this feature, which means we need to make it work without any action parameters. Would you be willing to add a global settings entry here that contains the default query URL? terminal/src/cascadia/TerminalSettingsModel/MTSMSettings.h Lines 21 to 68 in 612b00c I'm not sure what a good name would be, but I suppose something like "searchWebQueryUrlDefault" would work. That way any action without a "queryUrl" parameter could use that as the fallback. To make it work correctly, you'd have to remove the "queryUrl" parameter from the action in Let me know what you think about this! Do you feel like this is a good idea? If you're short on time, we'd be happy to accept the PR as is of course and make any remaining, necessary changes ourselves. 🙂 |
@lhecker How would this work? Let's say I add (if I get it correctly): X(hstring, SearchWebQueryUrlDefault, "queryUrl", L"https://www.bing.com/search?q=")Does it create a global setting, which then we will have to look up if the "queryUrl" parameter is empty here? void TerminalPage::_HandleSearchForText(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (args)
{
if (const auto& realArgs = args.ActionArgs().try_as<SearchForTextArgs>())
{
const auto queryUrl = realArgs.QueryUrl(); |
|
Yep, mostly. You can see how it works for "ShowAdminShield" for instance. It requires an additional modification in The PR is already fine as is, and I don't want to burden a contributor with making any complex changes. So, if you have any trouble making this change, please feel free to just leave the PR as it is, and we'll fix it in post. After all, we only added the context menu quite recently ourselves. 🙂 |
|
Ok I've tried to do that, if I've misinterpreted it I can easily revert and let you finish this off 😄 |
| // make it compact by replacing consecutive whitespaces with a single space | ||
| searchText = std::regex_replace(searchText, std::wregex(LR"(\s+)"), L" "); | ||
|
|
||
| if (realArgs.WrapWithQuotes()) |
There was a problem hiding this comment.
alright, so I've got an idea here. It kills two of my concerns!
When you set up a search engine shortcut in Firefox Microsoft Edge (:wink:) it gives you a special string, %s, that you can insert into the query.
If we use %s just like them, we get some benefits:
- we don't have to append, so people can put the query argument anywhere
- we don't need
wrapWithQuotes, because people can put quotes directly in the URL!
| { | ||
| return winrt::hstring{ | ||
| fmt::format(std::wstring_view(RS_(L"SearchForTextCommandKey")), | ||
| QueryUrl().c_str()) |
There was a problem hiding this comment.
note if we go with the %s idea we may want to revisit how this is phrased
There was a problem hiding this comment.
We could consider automatically generating a short version by parsing the URL and turning it into like.. Search {Uri().Hostname()} => `Search google.com"
(it is not unambiguous, but it could be interesting?)
There was a problem hiding this comment.
changed it, but should we be careful if Uri could throw if queryUrl is malformed?
src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
…minal into feat/10175-web-search-text-2 # Conflicts: # src/cascadia/TerminalSettingsModel/MTSMSettings.h
…minal into feat/10175-web-search-text-2 # Conflicts: # src/cascadia/TerminalApp/AppActionHandlers.cpp # src/cascadia/TerminalSettingsModel/ActionArgs.cpp # src/cascadia/TerminalSettingsModel/MTSMSettings.h
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
|
trying with the context menu as well:
|
This PR adds a `searchWeb` command to search the selected text on the web. Arguments: - `queryUrl`: URL of the web page to launch (the selected text will be inserted where the first `%s` is found in the query string) To make the search text more "compact" and handle multi-line selections, I'm concatenating the selected lines and replacing consecutive whitespaces with a single space (we may change this with something more clever in case). ## Validation Steps Performed Manual testing with single, multi-line, block selections. Closes microsoft#10175 --------- Co-authored-by: Marco Pelagatti <marco.pelagatti@iongroup.com> Co-authored-by: Mike Griese <migrie@microsoft.com>
Added
searchWebcommand to search the selected text on the web.Arguments:
queryUrl: URL of the web page to launch (the selected text will be appended to it)wrapWithQuotes: whether the selected text should be wrapped with quotes (true/false)To make the search text more "compact" and handle multi-line selections, I'm concatenating the selected lines and replacing consecutive whitespaces with a single space (we may change this with something more clever in case).
Validation Steps Performed
Manual testing with single, multi-line, block selections.
Enable/disable the
wrapWithQuotesargument.Closes #10175