This repository was archived by the owner on Feb 22, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[url_launcher] Update README discussion of permissions #5424
Merged
fluttergithubbot
merged 6 commits into
flutter:main
from
stuartmorgan-g:url-launcher-readme-query-cleanup
Jun 7, 2022
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bec6daf
[url_launcher] Update README discussion of permissions
stuartmorgan-g fe9f2c8
Update Android queries
stuartmorgan-g 6df47ea
Merge branch 'main' into url-launcher-readme-query-cleanup
stuartmorgan-g a6f7167
Update manifest to match
stuartmorgan-g 685906b
Merge branch 'flutter:main' into url-launcher-readme-query-cleanup
stuartmorgan f0fbf7c
Re-add https query permission for tests
stuartmorgan-g File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
[url_launcher] Update README discussion of permissions
Improves the README section about query permissions: - Removes http/https from the examples; we don't actually expect people to query for http(s) support in general (since generally a browser can assumed to be available), so it's an odd thing to show. - Updates the comments in the Android example to clarify that it's about querying for support, not launching. - Makes the iOS and Android sections have parallel structure so they are easier to read. Fixes flutter/flutter#102630
- Loading branch information
commit bec6dafda7f36cbe9d02c35c72d1ef390166dbe5
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GaryQian Are these actually right? They were added by a community-submitted PR that was based on comments from plugin clients in issue reports, but looking at them again after having more familiarity with the Android implementation it seems questionable.
Since the implementation of
canLaunchUrlis always justandroid.intent.action.VIEWwith a URL, shouldn't all of the queries be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only been recently added as owner of this plugin, so I am actually unclear about this particular question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blasten Are you familiar with the details of how query specification is supposed to work? Is my expectation that it should match the actual intent query we are going to make (here) correct?
Alternately, @GaryQian you could try experimenting locally to see if you can reproduce
canLaunchUrlfailing for various cases listed in this README section, and then if the query permission format I suggested above is all we need in each case to make it work? It could be a way to get to know this part of the plugin first-hand :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree: https://github.com/flutter/plugins/blob/main/packages/url_launcher/url_launcher_android/android/src/main/java/io/flutter/plugins/urllauncher/UrlLauncher.java#L41
It looks like we always hardcode ACTION_VIEW, so this DIAL intent may not be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated to always use
VIEWthen, matching our implementation. PTAL.