[url_launcher] docs: note about encoding URIs #2172

CoreyCole · 2019-10-09T20:06:05Z

Description

Add a note about encoding URLs passed to the url_launcher in the packages README.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

stuartmorgan-g

encodeFull is not the right API here; if subject contains an ampersand, for instance, you will not get the correct URI. Creating a correctly encoded URI requires knowledge of the structure of the URI you are building, such as what the query components are.

If an example is added, it should be encoding each portion with the correct API. As written, this example encourages people to use the encoding APIs incorrectly.

collinjackson · 2019-12-05T19:21:57Z

@CoreyCole please let us know if you want to keep working on this PR or if you want to abandon it.

If you do want to have it reviewed, please update the CHANGELOG and pubspec.yaml.

stuartmorgan-g · 2019-12-06T02:24:47Z

To summarize here some later discussion in a bug, this is what I think a recommendation should look like for mailto if something like this PR goes forward.

Interestingly, since my earlier comment I actually saw a bug go by where someone was getting "incorrect" results because they were putting a URL (unencoded) that itself had query parameters in the body of a mailto URL, providing a real-world example of the concern I raised; encoding that using encodeFull wouldn't help with the fact that the argument parsing didn't do at all what they wanted, while using proper URL construction as in the comment I've linked to here would have done the right thing.

CoreyCole · 2019-12-18T01:08:34Z

Sorry this took me so long to get around to! Hopefully it looks good now.

Happy holidays :)

also trigger a rebuild because the previous cirrus build timed out while upgrading https://cirrus-ci.com/task/5460390574292992

also re-run the cirrus build it failed randomly again https://github.com/flutter/plugins/runs/413530777

CoreyCole · 2020-01-28T23:42:58Z

@stuartmorgan I think this documentation PR is ready, but every time I re-run the cirrus build something random fails.

stuartmorgan-g · 2020-01-29T00:40:56Z

packages/url_launcher/url_launcher/README.md

+final Uri _emailLaunchUri = Uri(
+  scheme: 'mailto',
+  userInfo: 'support',
+  host: 'flutter.dev',


The use of userInfo/host here is not correct for mailto, per the comment linked earlier in this discussion. Per your own testing, this would fail.

Ah that is my mistake, I'm not actively working on the app using the mailto url anymore. Fixed in 854f6a3

as recommended flutter/flutter#42279 (comment)

stuartmorgan-g

I left a bunch of specific nits, but at a high level I think adding this to the docs is a good idea given that it's been reported as a bug quite a few times, indicating that many people don't realize it's necessary (and especially given that constructing mailto correctly as a URI required me to go read the core URI spec, so is definitely non-obvious).

I don't think I need to review this again, but a flutter/plugins maintainer should give it a final approval.

stuartmorgan-g · 2020-02-04T18:29:10Z

packages/url_launcher/url_launcher/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 5.4.2
+
+* Add documentation suggesting how to encode urls with special characters.


@collinjackson Should this still be revving the version given that it's just a README update now?

stuartmorgan-g · 2020-02-04T18:29:55Z

packages/url_launcher/url_launcher/README.md

@@ -53,6 +53,28 @@ Common schemes supported by both iOS and Android:

 More details can be found here for [iOS](https://developer.apple.com/library/content/featuredarticles/iPhoneURLScheme_Reference/Introduction/Introduction.html) and [Android](https://developer.android.com/guide/components/intents-common.html)

+### Encoding URLs
+
+URLs should be safely encoded, espeically when including spaces or other special characters. We can do this using dart's `Uri` helper methods included in `dart:core`:


Nit: I would suggest "must be properly encoded" rather than "should be safely encoded" (it's not really optional, and "safely" is somewhat ambiguous).

stuartmorgan-g · 2020-02-04T18:31:18Z

packages/url_launcher/url_launcher/README.md

@@ -53,6 +53,28 @@ Common schemes supported by both iOS and Android:

 More details can be found here for [iOS](https://developer.apple.com/library/content/featuredarticles/iPhoneURLScheme_Reference/Introduction/Introduction.html) and [Android](https://developer.android.com/guide/components/intents-common.html)

+### Encoding URLs
+
+URLs should be safely encoded, espeically when including spaces or other special characters. We can do this using dart's `Uri` helper methods included in `dart:core`:


Nit: the rest of this README appears to use "we" to mean the plugin maintainers, but this is a step that would be done by clients of the plugin, so it would be clearer to say "This can be done using" rather than "We can do this using".

stuartmorgan-g · 2020-02-04T18:33:55Z

packages/url_launcher/url_launcher/README.md

@@ -53,6 +53,28 @@ Common schemes supported by both iOS and Android:

 More details can be found here for [iOS](https://developer.apple.com/library/content/featuredarticles/iPhoneURLScheme_Reference/Introduction/Introduction.html) and [Android](https://developer.android.com/guide/components/intents-common.html)

+### Encoding URLs
+
+URLs should be safely encoded, espeically when including spaces or other special characters. We can do this using dart's `Uri` helper methods included in `dart:core`:


Nit: "dart"->"Dart". And maybe provide a link rather than just mentioning the library.

Combined with the above, I would suggest changing this whole sentence to:

This can be done using the Uri class:

stuartmorgan-g · 2020-02-04T18:38:45Z

packages/url_launcher/url_launcher/README.md

+
+// ...
+
+launch(_emailLaunchString);


Maybe collapse this last section to just:

launch(_emailLaunchUri.toString());

Extracting it to a variable if someone wants to do other things with the string first is something they can do trivially, so having shorter example code seems like the better option.

stuartmorgan-g · 2020-02-04T18:43:30Z

but every time I re-run the cirrus build something random fails

Don't worry about that part; once it's ready to go whoever is landing it can deal with the infrastructure flake.

stuartmorgan-g · 2020-06-07T01:49:35Z

@cyanglaz Can you give this a review? This continues to be a source of confusion and bug reports, and it would be really good to have something in the readme about it.

cyanglaz

LGTM

stuartmorgan-g · 2020-06-17T17:30:39Z

@CoreyCole Now that this has all the approvals it needs, if you update to the latest master and resolve the conflicts, I can land and publish it.

CoreyCole · 2020-06-17T18:10:20Z

Done. I removed the version bump because this is only a documentation change as @stuartmorgan stated

…mentation in the README

CoreyCole · 2020-06-17T18:24:04Z

On second thought, it looks like the version has been bumped in the past for README changes. I bumped the version to 5.4.11.

stuartmorgan-g

Thanks for bearing with the long delays here. Hopefully this will help more people use url_launcher successfully!

CoreyCole · 2020-06-17T18:42:53Z

Happy to help out! 😄

stuartmorgan-g · 2020-06-17T20:29:12Z

The new README is live 🙂

Add a note about encoding URLs passed to the url_launcher in the packages README.

docs(url_launcher): note about encoding URIs

c5623aa

googlebot added the cla: yes label Oct 9, 2019

docs(url_launcher): consistent static types

7fc9d3e

stuartmorgan-g reviewed Oct 9, 2019

View reviewed changes

docs: update to suggested URI usage

eb362ba

CoreyCole added 9 commits January 27, 2020 15:27

[url_launcher] docs: spacing

8951995

[url_launcher] docs: fix typo

53566e3

[url_launcher] docs: generic flutter url

0826e8c

[url_launcher] docs: explicitly import dart:core

c884717

Merge branch 'master' into patch-1

303c310

[url_launcher] chore: upgrade version to 5.4.1+1 and prepend changelog

5509c58

[url_launcher] fix: update to allowed version number

809d9fe

[url_launcher] docs: add missing bullet point to changelog

6a28275

also trigger a rebuild because the previous cirrus build timed out while upgrading https://cirrus-ci.com/task/5460390574292992

[url_launcher] docs: add period to changelog statement

5a1d471

also re-run the cirrus build it failed randomly again https://github.com/flutter/plugins/runs/413530777

stuartmorgan-g reviewed Jan 29, 2020

View reviewed changes

[url_launcher] fix: mailto example with path

854f6a3

as recommended flutter/flutter#42279 (comment)

stuartmorgan-g reviewed Feb 4, 2020

View reviewed changes

CoreyCole added 4 commits February 4, 2020 10:50

[url_launcher] docs: update readme with suggestions from @stuartmorgan

cb6a89b

Merge branch 'patch-1' of github.com:CoreyCole/plugins into patch-1

b719351

[url_launcher] docs: fix merge issue back to userInfo and host format

cb15f23

[url_launcher] docs: fix merge issue to path format

c3b9c15

stuartmorgan-g requested a review from collinjackson February 4, 2020 18:56

stuartmorgan-g requested review from cyanglaz and removed request for collinjackson June 7, 2020 01:48

cyanglaz approved these changes Jun 8, 2020

View reviewed changes

Merge branch 'master' into patch-1

715fa56

[url_launcher] docs: bump version to 5.4.11 for the URL encoding docu…

1cd6c49

…mentation in the README

stuartmorgan-g approved these changes Jun 17, 2020

View reviewed changes

stuartmorgan-g merged commit 957fb75 into flutter:master Jun 17, 2020

agent3bood pushed a commit to agent3bood/flutter-plugins that referenced this pull request Jul 10, 2020

[url_launcher] docs: note about encoding URIs (flutter#2172)

adfcd86

Add a note about encoding URLs passed to the url_launcher in the packages README.

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[url_launcher] docs: note about encoding URIs (flutter#2172)

2bc1873

Add a note about encoding URLs passed to the url_launcher in the packages README.

Delgan mentioned this pull request Oct 17, 2020

[url_launcher] Uri.toString() for "mailto" does not encode spaces properly (+ instead of %20) flutter/flutter#68406

Closed

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[url_launcher] docs: note about encoding URIs (flutter#2172)

64e1529

Add a note about encoding URLs passed to the url_launcher in the packages README.

		@@ -1,3 +1,7 @@
		## 5.4.2

		* Add documentation suggesting how to encode urls with special characters.


		// ...

		launch(_emailLaunchString);

[url_launcher] docs: note about encoding URIs #2172

[url_launcher] docs: note about encoding URIs #2172

Uh oh!

Conversation

CoreyCole commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

collinjackson commented Dec 5, 2019

Uh oh!

stuartmorgan-g commented Dec 6, 2019

Uh oh!

CoreyCole commented Dec 18, 2019

Uh oh!

CoreyCole commented Jan 28, 2020

Uh oh!

stuartmorgan-g Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

CoreyCole Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 4, 2020

Uh oh!

stuartmorgan-g commented Jun 7, 2020

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jun 17, 2020

Uh oh!

CoreyCole commented Jun 17, 2020

Uh oh!

CoreyCole commented Jun 17, 2020

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

CoreyCole commented Jun 17, 2020

Uh oh!

stuartmorgan-g commented Jun 17, 2020

Uh oh!

Uh oh!

CoreyCole commented Oct 9, 2019 •

edited

Loading

stuartmorgan-g Feb 4, 2020 •

edited

Loading