Add Search Web to selection controls on iOS #43324
Conversation
There was a problem hiding this comment.
LGTM with nits 👍
A question that crossed my mind while looking at this PR is: why not generically expose openURL and do the x-web-search:// stuff in Dart?
I think the answer is that exposing openURl is a much greater maintenance burden. If someone wants to use it, their use case is better covered by a plugin. If a user wants to create another context menu button that does something slightly different with openURL, they can easily create their own button in the framework and use their own plugin. There is no need to modify what Flutter is doing with openURL.
This might not be true for every platform API that we use in the Framework, but it is here.
All that to say that I agree with this approach 👍 . I just wanted to write down my thought process because this kind of thing will probably come up again.
| } else { | ||
| result(FlutterMethodNotImplemented); | ||
| } | ||
| } | ||
|
|
||
| - (void)handleSearchWebWithSelection:(NSString*)selection { | ||
| NSString* searchURLPrefix = @"x-web-search://?"; |
There was a problem hiding this comment.
Nit: Maybe define this outside the method body as a constant somewhere?
| [[[FlutterPlatformPlugin alloc] initWithEngine:_weakFactory->GetWeakPtr()] autorelease]; | ||
| FlutterPlatformPlugin* mockPlugin = OCMPartialMock(plugin); | ||
| FlutterMethodCall* methodCall = [FlutterMethodCall methodCallWithMethodName:@"SearchWeb.initiate" | ||
| arguments:@"Test"]; |
There was a problem hiding this comment.
Nit: Maybe you could also try testing that a string with some characters that need to be escaped get escaped? If you think it's useful.
| } else { | ||
| result(FlutterMethodNotImplemented); | ||
| } | ||
| } | ||
|
|
||
| - (void)handleSearchWebWithSelection:(NSString*)selection { |
There was a problem hiding this comment.
Nit: Maybe renamed this to something like searchWeb and the parameter to url?
I think that using the word selection might be overly specific. It implies that it knows that the given string is selected in the UI, but that's probably an irrelevant detail at this level.
Also losing the word handle makes it sound more generic and less like it must only be called when the "Search web" button is pressed on some selected text.
That is if you agree that the method should be generic. The method channel name SearchWeb.initiate sounds generic, so maybe it makes sense that this method is too.
|
Can you do the framework part of Lookup feature first before this? Just wanna make sure the lookup feature is indeed working before adding another feature. |
It already works locally, do you want me to make a PR for the framework part of lookup first? |
Ya wanna make sure the whole thing is good. but up to you |
|
Are we making progress on this? |
Yes sort of - as recommended Im working on the framework portion of the corresponding Look Up feature first |
|
Sounds good. Adding the WIP tag so triagers don't bother you. |
…sehsu/engine into add-searchweb-to-selection-controls
| [[UIApplication sharedApplication] openURL:[NSURL URLWithString:searchURL] | ||
| options:@{} | ||
| completionHandler:^(BOOL success) { | ||
| result = success; |
There was a problem hiding this comment.
is this completion block sync or async?
If async, then updating result here won't change what this function returns.
There was a problem hiding this comment.
Ah, it's async
But somehow the test passes 👀 should I remove this check then?
There was a problem hiding this comment.
If async, then this result always returns false.
What is this result used for? Is it piped back to the framework? If so, you may wanna have searchWeb function to take a block param, rather than returning a BOOL.
There was a problem hiding this comment.
the result isnt used for anything, I just wanted to be able to unit test it
There was a problem hiding this comment.
Gotcha. Can remove it if you don't need the result.
LouiseHsu
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 43324, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
LouiseHsu
added
the
autosubmit
…sions) (#131830) Manual roll requested by [email protected] flutter/engine@9304c00...4c1157b 2023-08-03 [email protected] Revert Android Hardware Texture PRs (flutter/engine#44310) 2023-08-03 [email protected] Roll Dart SDK from 87df1bbcea5e to 3b9af2825d47 (2 revisions) (flutter/engine#44308) 2023-08-03 [email protected] Roll Fuchsia Mac SDK from Hx7ap5qcoqRIknnnG... to WwFjJuQF_rpToCYPJ... (flutter/engine#44306) 2023-08-03 [email protected] Check whether the lookup of android.hardware.HardwareBuffer found a class (flutter/engine#44304) 2023-08-02 [email protected] [Impeller] Add placeholder filter input. (flutter/engine#44290) 2023-08-02 [email protected] Roll ANGLE from 6c1bab070220 to 6a09e41ce6ea (1 revision) (flutter/engine#44300) 2023-08-02 [email protected] Roll Skia from fd5bd67d532f to c0956a252f30 (1 revision) (flutter/engine#44296) 2023-08-02 [email protected] [iOS] Fix use-after-free in setBinaryMessenger (flutter/engine#44294) 2023-08-02 [email protected] Add Search Web to selection controls on iOS (flutter/engine#43324) 2023-08-02 [email protected] Improve logging in the clang-tidy script (flutter/engine#44228) 2023-08-02 [email protected] Roll ANGLE from 335c6b86d70b to 6c1bab070220 (1 revision) (flutter/engine#44291) 2023-08-02 [email protected] Roll Skia from 25f5a32367ad to fd5bd67d532f (2 revisions) (flutter/engine#44289) 2023-08-02 [email protected] Be sure to clear exceptions after a failed JNI lookup (flutter/engine#44293) 2023-08-02 [email protected] Handle deprecation of Dart_TimelineEvent Embedder API (flutter/engine#42497) 2023-08-02 [email protected] Roll Skia from ccc17f784e5d to 25f5a32367ad (4 revisions) (flutter/engine#44283) 2023-08-02 [email protected] Roll ANGLE from 01ee134bb223 to 335c6b86d70b (2 revisions) (flutter/engine#44287) 2023-08-02 [email protected] Roll Skia from 7104d0e8863f to ccc17f784e5d (2 revisions) (flutter/engine#44279) 2023-08-02 [email protected] [ios][autocorrection]disable auto-correction highlight in iOS 17 (flutter/engine#44176) 2023-08-02 [email protected] Reland Introduce TextureRegistry.ImageTexture and HardwareBufferExternalTextureGL (flutter/engine#44278) 2023-08-02 [email protected] Roll Dart SDK from afbaf4216fc8 to 87df1bbcea5e (1 revision) (flutter/engine#44276) 2023-08-02 [email protected] Roll Skia from 93764a98b866 to 7104d0e8863f (4 revisions) (flutter/engine#44273) 2023-08-02 [email protected] [Impeller] Fix leak of wrapped TextureMTL objects in the Metal embedder API (flutter/engine#44245) 2023-08-02 [email protected] Revert "Listen to window notifications to update application lifecycle" (flutter/engine#44275) 2023-08-02 [email protected] Roll Skia from 514c66ce0471 to 93764a98b866 (1 revision) (flutter/engine#44270) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from Hx7ap5qcoqRI to WwFjJuQF_rpT If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This PR adds framework support for the Search Web feature in iOS. https://github.com/flutter/flutter/assets/36148254/c159f0d9-8f14-45e7-b295-e065b0826fab The corresponding merged engine PR can be found [here](flutter/engine#43324). This PR addresses #82907 More details are available in this [design doc](https://docs.google.com/document/d/1QizXwBiO-2REIcEovl5pK06BaLPOWYmNwOE5jactJZA/edit?resourcekey=0-1pb9mJiAq29Gesmt25GAug)
In native iOS, users are able to select text and initiate a web search on the selected text. Specifically, this will launch a search using the users selected default search engine. Apple provides a custom url scheme in the form of "x-web-search://?[term]" which will automatically launch a search using the user's preferred browser in Safari. Additionally, this also correctly handles the country code top level domain eg. a user with a selected region in the UK with a preferred Google search engine will automatically have the .co.uk domain appended to the end of the search. This PR is the engine portion of the changes that will allow Search Web to be implemented This PR addresses flutter/flutter#82907 More details are available in this [design doc](https://github.com/flutter/engine/pull/flutter.dev/go/add-missing-features-to-selection-controls)
In native iOS, users are able to select text and initiate a web search on the selected text. Specifically, this will launch a search using the users selected default search engine. Apple provides a custom url scheme in the form of "x-web-search://?[term]" which will automatically launch a search using the user's preferred browser in Safari. Additionally, this also correctly handles the country code top level domain eg. a user with a selected region in the UK with a preferred Google search engine will automatically have the .co.uk domain appended to the end of the search.
This PR is the engine portion of the changes that will allow Search Web to be implemented
This PR addresses flutter/flutter#82907
More details are available in this design doc
Pre-launch Checklist
///).