[url_launcher] Migrate to the new embedding #2157
Conversation
| * | ||
| * <p>It may be null initially, but should be set before calling {@link #launch}. | ||
| */ | ||
| UrlLauncher(@Nullable Context activityContext) { |
There was a problem hiding this comment.
If you really need an Activity Context then I would recommend only accepting an Activity. It will enforce your requirements with the type system and avoid any future developer mistakes by overlooking the significance of activityContext vs context.
There was a problem hiding this comment.
Done. I took a second look at the original code, and I oversimplified. Half the time it was using an activity and half the time it was using application context. Split it out into separate objects and kept the Activity as that class following this suggestion.
| } | ||
|
|
||
| @Override | ||
| public void onDetachedFromActivityForConfigChanges() { |
There was a problem hiding this comment.
Recommendation: consider declaring the methods in the order that they are likely to be called.
There was a problem hiding this comment.
Acknowledged. I prefer bottom line up front style (most "important" methods first), but it's a very subjective ranking.
| * | ||
| * <p>Instantiate this in an add to app scenario to gracefully handle activity and context changes. | ||
| */ | ||
| public final class UrlLauncherPlugin implements FlutterPlugin, ActivityAware { |
There was a problem hiding this comment.
Does this class need a static registerWith() method?
There was a problem hiding this comment.
I put it in a UrlLauncherPluginRegistrar class. I'm not 100% on that being the best place for it, but I wanted to move it as far away from here as possible to try and make it harder to accidentally initialize the "old" vs the "new" version of this.
There was a problem hiding this comment.
Never mind, our plugins infra expects Android and iOS to use the same name for the class that holds registerWith. I moved it back.
|
This should now be ready to land. The latest commit updated the pubspec, added in the Gradle fix to keep this working on stable, and added a simple e2e test. |
| * | ||
| * <p>See {@code dev.flutter.plugins.urllauncherexample.MainActivity} for an example. | ||
| */ | ||
| public UrlLauncherPlugin() {} |
| <activity | ||
| android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection" | ||
| android:hardwareAccelerated="true" | ||
| android:launchMode="singleTop" |
There was a problem hiding this comment.
@matthew-carroll seems to suggest this to be moved.
#2164 (comment)
- Split out `Context` into an `applicationContext` and an `Activity`. - Style nits.
|
also remember to upgrade the flutter version in example app. |
Good catch, done. |
| super.onCreate(savedInstanceState); | ||
| GeneratedPluginRegistrant.registerWith(this); | ||
| public void configureFlutterEngine(FlutterEngine flutterEngine) { | ||
| flutterEngine.getPlugins().add(new UrlLauncherPlugin()); |
There was a problem hiding this comment.
nit: we should have a TODO to remove this when the generated registrant for the v2 embedder is on stable
Migrate android intent to the new embedding. - Refactors the existing plugin logic into two new classes. - Adds an implementation of the plugin for the new embedding. - Adds a simple unit test. It's not a very thorough one but does check that the new embedding's set up handlers work correctly. - Adds a simple e2e test verifying that one of the getter APIs functions correctly.
Migrate android intent to the new embedding. - Refactors the existing plugin logic into two new classes. - Adds an implementation of the plugin for the new embedding. - Adds a simple unit test. It's not a very thorough one but does check that the new embedding's set up handlers work correctly. - Adds a simple e2e test verifying that one of the getter APIs functions correctly.
5 participants
Description
Migrate android intent to the new embedding.
Related Issues
flutter/flutter#41849
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.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?