Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Add url_launcher_windows #3015

Merged
merged 6 commits into from Sep 12, 2020

Conversation

stuartmorgan
Copy link
Contributor

Description

Adds a federated Windows implementation of url_launcher (not yet endorsed).

Since this is the first C++ plugin, this also adds a .clang-format file to the repo root for formatting C++ plugins.

Related Issues

Part of flutter/flutter#41721 (will need follow-up to endorse it in url_launcher)

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

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 stuartmorgan marked this pull request as draft September 10, 2020 22:05
@stuartmorgan stuartmorgan marked this pull request as ready for review September 10, 2020 22:30
Copy link
Contributor

@csells csells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

## Backward compatible 1.0.0 version is coming
The plugin has reached a stable API, we guarantee that version `1.0.0` will be backward compatible with `0.0.y+z`. If you use
url_launcher_windows directly, rather than as an implementation detail
of `url_launcher`, please use `url_launcher_windows: '>=0.0.y+x <2.0.0'`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I ask why we're recommending this instead of putting the right version of the plugin as a dependency of url_launcher?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

answering my own question, I assume we're doing this as a temporary message until Windows hits alpha.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To endorse a federated plugin (i.e., make it a dependency of the main plugin), the plugin already has to be published. So our flow for a new federated implementation is:

  1. Make a PR for the federated implementation (e.g., foo_windows).
  2. Land foo_windows.
  3. Publish foo_windows.
  4. Make a PR (against foo) to endorse the new federated implementation.
  5. Land foo.
  6. Publish foo.

This README covers steps 3 through 5, where this plugin exists in the wild as a published plugin but isn't endorsed. In step 6, the foo_windows readme gets updated to say that it's included automatically by foo. (Then that gets published as a minor, README-only change to foo_windows).

It's cumbersome, but the alternative (putting everything in one PR) wouldn't pass CI because the build of the main plugin would fail due to the federated implementation not having been published.

Future<void> _launched;

Future<void> _launchInBrowser(String url) async {
if (await UrlLauncherPlatform.instance.canLaunch(url)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR but still -

This saddens me, I really want us to come up with a way to share relevant pieces of the example app and e2e tests (and not have an example app thats using the platform interface).

I'm planning to be focusing on these issues in the coming weeks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my recent comments is Discord, I think using the platform interface rather than the app-facing plugin is a better way to write the driver tests; maybe we can continue the discussion there.

The role of the example app in a federated implementation is questionable though; it's only going to be an example in the (rare?) cases where there is extra API in the platform implementation. Having it replicate things that are in the main example—which is what people should look at in almost every case—isn't really useful as an example. But it is useful to have a self-contained build when working on the implementation.

Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stuartmorgan stuartmorgan removed the request for review from clarkezone September 12, 2020 18:12
@stuartmorgan stuartmorgan merged commit c194de2 into flutter:master Sep 12, 2020
@stuartmorgan stuartmorgan deleted the url-launcher-windows branch September 12, 2020 18:14
danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Sep 18, 2020
Adds a federated Windows implementation of url_launcher (not yet endorsed).

Since this is the first C++ plugin, this also adds a .clang-format file to the repo root for formatting C++ plugins.

Part of flutter/flutter#41721 (will need follow-up to endorse it in url_launcher)
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
Adds a federated Windows implementation of url_launcher (not yet endorsed).

Since this is the first C++ plugin, this also adds a .clang-format file to the repo root for formatting C++ plugins.

Part of flutter/flutter#41721 (will need follow-up to endorse it in url_launcher)
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Adds a federated Windows implementation of url_launcher (not yet endorsed).

Since this is the first C++ plugin, this also adds a .clang-format file to the repo root for formatting C++ plugins.

Part of flutter/flutter#41721 (will need follow-up to endorse it in url_launcher)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants