[android_intent] Migrate to the new embedding #2143

mklim · 2019-10-03T19:39:17Z

Description

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 unit test.

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.

cyanglaz · 2019-10-03T20:03:57Z

...roid_intent/android/src/main/java/dev/flutter/plugins/androidintent/AndroidIntentPlugin.java

+  }
+
+  /** This exists for legacy compatibility purposes. */
+  public static void registerWith(Registrar registrar) {


Doesn't seem to be called in this PR.
We might not need this. The io MainAcitivity can be left unchanged.

Ah, right. I was thinking there was some magic here but since I didn't delete the io. class this isn't necessary. Deleted.

cyanglaz · 2019-10-03T20:18:04Z

...ges/android_intent/android/src/main/java/dev/flutter/plugins/androidintent/IntentSender.java

+   * <p>Also resets the cached {@code activity} to null, since the given activity should no longer
+   * be valid after applicationContext changes.
+   */
+  void setApplicationContext(@Nullable Context applicationContext) {


How do we force people to set the ApplicationContext first and then Activity?
For example:
I want to set a new application context and a new activity. I have to know to set the application context first before calling set activity.

I might be missing some context (no pun intended), but why would we not be able to cache both? The app context is always there. It outlives all Activitys. Then an Activity comes and goes.

I was thinking that if the application context changed for some reason we know that any previous activity reference we've set is invalid, because the application context should outlive the activities. But @cyanglaz has a good point that a dev may try to call both and happen to call this first so this is kind of a footgun. I removed the setActivity(null) logic from here.

Can an application context change? I believe the application context is the Application class, which I believe is instantiated exactly one time at process launch. Do you know if that's right?

That sounds roughly right to me, but I'm not confident enough to totally rule it changing out. I'm suspicious of what would happen in a multiprocess app. I felt like it was safer to just always treat it as changed once we got a new binding in onAttachedToEngine.

matthew-carroll

Generally LGTM. Thanks for migrating, and thanks for adding channel message tests.

...roid_intent/android/src/main/java/dev/flutter/plugins/androidintent/AndroidIntentPlugin.java

...ges/android_intent/android/src/main/java/dev/flutter/plugins/androidintent/IntentSender.java

matthew-carroll · 2019-10-03T20:50:07Z

...ges/android_intent/android/src/main/java/dev/flutter/plugins/androidintent/IntentSender.java

+   * <p>Also resets the cached {@code activity} to null, since the given activity should no longer
+   * be valid after applicationContext changes.
+   */
+  void setApplicationContext(@Nullable Context applicationContext) {


I might be missing some context (no pun intended), but why would we not be able to cache both? The app context is always there. It outlives all Activitys. Then an Activity comes and goes.

matthew-carroll · 2019-10-03T20:50:49Z

...id_intent/android/src/main/java/dev/flutter/plugins/androidintent/MethodCallHandlerImpl.java

+import java.util.Map;
+
+/** Forwards incoming {@link MethodCall}s to {@link IntentSender#send}. */
+public class MethodCallHandlerImpl implements MethodCallHandler {


Given that this is public, would it make sense to specify what kind of method calls this is handling? e.g., IntentMethodCallHandlerImpl?

I think that's already implied from the package name, but I don't feel super strongly on this if you'd still prefer a rename.

Ok. You may want to unify the approach on the team. The connectivity plugin chose to prefix:
#2142

Either seems acceptable, but it also seems like one of those details that doesn't have any cause to randomly change between 1P plugins.

...id_intent/android/src/main/java/dev/flutter/plugins/androidintent/MethodCallHandlerImpl.java

...ntent/android/src/test/java/dev/flutter/plugins/androidintent/MethodCallHandlerImplTest.java

mklim · 2019-10-04T00:06:12Z

I updated this PR with the two extra changes we discussed offline: diff.

matthew-carroll

LGTM

matthew-carroll · 2019-10-11T21:07:42Z

...droid_intent/android/src/main/java/io/flutter/plugins/androidintent/AndroidIntentPlugin.java

+  @Override
+  public void onAttachedToEngine(@NonNull FlutterPluginBinding binding) {
+    sender.setApplicationContext(binding.getApplicationContext());
+    sender.setActivity(null);


Would it make more senes to set activity to null in the appropriate ActivityAware methods? The execution of this method doesn't really imply anything about the Activity...

Would a user potentially be changing the plugin's engine, but expecting it to hold on to an existing activity reference? I was assuming that if the engine changes, the activity itself is also invalid. I also figured that the Activity callbacks should be called anyway so I am updating the Activity there too, but I wanted to add an extra check here just to guarantee that they never got out of state based on some callback issue.

You can add an extra call, but if that extra call ever has an effect, it indicates a bug in the embedding. The Activity calls are supposed to be strictly between engine calls:

attach engine
attach activity
detach activity
detach engine

Split out the business logic into a new IntentSender class. Split out the MethodCall parsing into MethodCallHandlerImpl.

Create a new dev.Plugin class with the new embedding. Add it to the manifest and use it in a new dev.MainActivity.

1. Move all classes into `dev`. 2. Clean up the MethodChannel based on the engine lifecycle handlers.

cyanglaz

LGTM!

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 unit test.

mklim added the WIP label Oct 3, 2019

mklim assigned cyanglaz and matthew-carroll Oct 3, 2019

googlebot added the cla: yes label Oct 3, 2019

cyanglaz reviewed Oct 3, 2019

View reviewed changes

matthew-carroll approved these changes Oct 3, 2019

View reviewed changes

matthew-carroll approved these changes Oct 11, 2019

View reviewed changes

Michael Klimushyn added 7 commits October 11, 2019 16:40

Refactor the existing logic

5a82ef0

Split out the business logic into a new IntentSender class. Split out the MethodCall parsing into MethodCallHandlerImpl.

Migrate the plugin to the new embedding

c26003e

Create a new dev.Plugin class with the new embedding. Add it to the manifest and use it in a new dev.MainActivity.

Add a unit test.

7cca6f9

Review feedback.

dd6079c

Changes from chat discussion

a5caa9f

1. Move all classes into `dev`. 2. Clean up the MethodChannel based on the engine lifecycle handlers.

Naming changes based on offline discussion.

0a0e3e3

Last migration fixes

5434ffb

mklim removed the WIP label Oct 12, 2019

cyanglaz approved these changes Oct 14, 2019

View reviewed changes

mklim merged commit 0359295 into flutter:master Oct 14, 2019

mklim deleted the android_intent_migrate branch October 14, 2019 17:24

[android_intent] Migrate to the new embedding #2143

[android_intent] Migrate to the new embedding #2143

Uh oh!

Conversation

mklim commented Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim Oct 4, 2019 • 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!

matthew-carroll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mklim commented Oct 4, 2019

Uh oh!

matthew-carroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mklim commented Oct 3, 2019 •

edited

Loading

mklim Oct 4, 2019 •

edited

Loading