[Connectivity] migrate to the new android embedding #2142

cyanglaz · 2019-10-03T17:22:37Z

Description

Migrate connectivity to the new android embedding.

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.

amirh · 2019-10-03T17:53:34Z

packages/connectivity/example/android/app/build.gradle

 }

 dependencies {
+    implementation 'androidx.lifecycle:lifecycle-runtime:2.1.0'


remove these deps, the engine POM makes sure these are pulled in (once flutter/flutter#41885 lands)

matthew-carroll · 2019-10-03T21:22:49Z

.../connectivity/android/src/main/java/dev/flutter/plugins/connectivity/ConnectivityPlugin.java

+import io.flutter.plugin.common.MethodChannel;
+import io.flutter.plugins.connectivity.ConnectivityMethodChannelHandler;
+
+public class ConnectivityPlugin implements FlutterPlugin {


Would you mind add Javadocs to all public, non-trivial classes/methods/fields?

New classes members/methods added should definitely be added with Javadocs.

Regarding all existing code - this is definitely tech debt we should be paying, and is welcomed if the author is interested in doing it as part of this PR, though it shouldn't block it. For the purpose of supporting the v2 embedder in all plugins I suggest we focus PRs and reviews on that goal. Please do leave any improvement suggestions of existing code as non-blocking nits.

I think this fits with the spirit of the Flutter code review guidelines:

In general, reviewers should favor approving a PR once it is in a state where it definitely improves the overall code health of the system being worked on, even if the PR isn't perfect.

matthew-carroll · 2019-10-03T21:23:17Z

.../connectivity/android/src/main/java/dev/flutter/plugins/connectivity/ConnectivityPlugin.java

+
+  @Override
+  public void onAttachedToEngine(FlutterPluginBinding binding) {
+    this.pluginBinding = binding;


You don't need to cache the binding if you only use it within these 2 methods.

matthew-carroll · 2019-10-03T21:32:59Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+  private ConnectivityManager manager;
+  private BroadcastReceiver receiver;
+
+  public ConnectivityMethodChannelHandler(Context context) {


Recommendation: If you explicitly pass a ConnectivityManager instance, and if you create an interface around broadcast registration, passing an instance of that interface, instead of passing a Context, then you'll be a much better position to test interactions in this class.

matthew-carroll · 2019-10-03T23:15:40Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+  }
+
+  private void handleWifiIPAddress(MethodCall call, final MethodChannel.Result result) {
+    WifiManager wifiManager =


Recommendation: passing this as a constructor argument allows you to mock for tests.

matthew-carroll · 2019-10-03T23:16:40Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+    }
+  }
+
+  private BroadcastReceiver createReceiver(final EventChannel.EventSink events) {


Recommendation: If I were migrating this, I would probably restructure such that this BroadcastReceiver is separated from this MethodCallHandler and then bring them together in a third class.

matthew-carroll · 2019-10-03T23:17:44Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+  }
+
+  @Override
+  public void onListen(Object arguments, EventChannel.EventSink events) {


Please add nullability annotations to all input parameters and return values for each public and private method.

matthew-carroll · 2019-10-03T23:19:26Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+  }
+
+  @Override
+  public void onMethodCall(MethodCall call, MethodChannel.Result result) {


Would you mind adding unit tests for this class to ensure that incoming messages are correctly parsed, and outgoing messages are correctly serialized?

I think that Dart e2e integration tests would be acceptable as an alternative to unit tests.

cyanglaz · 2019-10-04T22:42:54Z

@matthew-carroll Updated the code per some of our recommendation. I didn't add any JUnit tests nor Nullability notation to the existing code. Could you take another look and let me know if the new refactoring looks good.

matthew-carroll · 2019-10-04T22:47:34Z

.../connectivity/android/src/main/java/dev/flutter/plugins/connectivity/ConnectivityPlugin.java

+import io.flutter.plugins.connectivity.ConnectivityMethodChannelHandler;
+
+/**
+ * Plugin implementation that uses the new {@code io.flutter.embedding} package.


The most important thing in this javadoc is probably what the plugin does, and possibly how to use it, rather than a reference to the new embedding.

Also, through this migration, we should probably avoid talking about "new" vs "old". That language makes sense to us right now, but it naturally becomes stale. Is this still the "new" embedding a year from now?

matthew-carroll · 2019-10-04T22:49:10Z

.../connectivity/android/src/main/java/dev/flutter/plugins/connectivity/ConnectivityPlugin.java

+/**
+ * Plugin implementation that uses the new {@code io.flutter.embedding} package.
+ *
+ * <p>Instantiate this in an add to app scenario to gracefully handle activity and context changes.


How should one instantiate this in an add-to-app scenario? I would either omit this line, or I would refer people to some kind of centralized documentation that all plugins can point to for setup instructions.

matthew-carroll · 2019-10-04T22:52:10Z

...tivity/android/src/main/java/io/flutter/plugins/connectivity/BroadcastReceiverRegistrar.java

+import androidx.annotation.NonNull;
+import io.flutter.plugin.common.EventChannel;
+
+/** Responsible for constructing a BroadcastReceiver as well as registering and unregistering it. */


While this statement is true, it may not be useful for developers. When a developer reads this doc, what kinds of questions might that developer be attempting to answer?

Flutter style guide: Writing prompts for good documentation
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#writing-prompts-for-good-documentation

matthew-carroll · 2019-10-04T22:53:33Z

...tivity/android/src/main/java/io/flutter/plugins/connectivity/BroadcastReceiverRegistrar.java

+public interface BroadcastReceiverRegistrar {
+
+  /**
+   * Triggered when it is ready for the BroadcastReceiver to be registered. Register the receiver in


Triggered when what is ready? This method probably involves a broader context. Can you fill in the reader on what is happening when this method is called, or why it exists? @mklim might have some suggestions here.

I think this is a little unclear because it's phrased using passive voice, so it's not obvious who is doing what. https://developers.google.com/style/voice

I'd rephrase the first sentence to <Object> triggers this when it's ready for the BroadcastReceiver to be registered. With how it is now I think the worry is that you the developer need to trigger it somehow in some other place. I think the second sentence is clear as-is.

matthew-carroll · 2019-10-04T22:53:40Z

...tivity/android/src/main/java/io/flutter/plugins/connectivity/BroadcastReceiverRegistrar.java

+  void readyToRegisterBroadcastReceiver(@NonNull BroadcastReceiver receiver);
+
+  /**
+   * Triggered when it is ready for the BroadcastReceiver to be unregistered. Unregister the


matthew-carroll · 2019-10-04T22:58:41Z

packages/connectivity/android/src/main/java/io/flutter/plugins/connectivity/Connectivity.java

+  }
+
+  @Nullable
+  String getWifiName() {


Should all of these methods being with "/* package */"? @mklim for style input

It's one of those totally subjective style things, like annotating primitive arguments with /*name=*/. It's fine either way IMHO. From what I can tell the existing code doesn't use it, so it probably makes more sense to leave it off to be consistent.

matthew-carroll · 2019-10-04T22:59:24Z

...y/android/src/main/java/io/flutter/plugins/connectivity/ConnectivityEventChannelHandler.java

+import androidx.annotation.NonNull;
+import io.flutter.plugin.common.EventChannel;
+
+/** Handles the event channel for the plugin. */


Another candidate for "writing prompts for good documentation"

What does it mean to "handle"? What is "the event channel"?

matthew-carroll · 2019-10-04T22:59:33Z

...y/android/src/main/java/io/flutter/plugins/connectivity/ConnectivityEventChannelHandler.java

+  private BroadcastReceiver broadcastReceiver;
+
+  /**
+   * Constructs a ConnectivityEventChannelHandler


Probably "useless documentation"

matthew-carroll · 2019-10-04T22:59:53Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+import io.flutter.plugin.common.MethodCall;
+import io.flutter.plugin.common.MethodChannel;
+
+/** Handles MethodChannel for the plugin. */


Same comment as the other handler's documentation.

matthew-carroll · 2019-10-04T23:00:01Z

.../android/src/main/java/io/flutter/plugins/connectivity/ConnectivityMethodChannelHandler.java

+  private Connectivity connectivity;
+
+  /**
+   * Construct the ConnectivityMethodChannelHandler


Probably "useless documentation"

cyanglaz · 2019-10-07T17:23:53Z

@matthew-carroll Updated per Friday's discussion.

matthew-carroll · 2019-10-07T21:46:28Z

packages/connectivity/example/android/app/src/main/AndroidManifest.xml

+            android:hardwareAccelerated="true"
+            android:windowSoftInputMode="adjustResize">
+        </activity>
+        <activity android:name="dev.flutter.plugins.connectivity_exmaple.MainActivity"


FYI, in Java/Android, packages usually don't include underscores. You might choose plugins.connectivity.example or plugins.connectivityexample.

using connectivityexample.

matthew-carroll

Still a few small comments, but generally LGTM.

cyanglaz · 2019-10-14T23:00:07Z

@collinjackson Could you review the e2e bits in this PR? Although ./gradlew connectedAnroidTest still fails on my local.

collinjackson

lgtm!

This reverts commit 67697ab.

Chris Yang added 2 commits October 2, 2019 18:20

draft

ae42d12

migrate connectivity

67f4f70

googlebot added the cla: yes label Oct 3, 2019

Chris Yang added 4 commits October 3, 2019 10:25

revert some changes

6aebaf7

revert testing code

f682eb7

add

8922d5b

revert

b24cfc9

amirh reviewed Oct 3, 2019

View reviewed changes

remove deps

7267b11

matthew-carroll mentioned this pull request Oct 3, 2019

[android_intent] Migrate to the new embedding #2143

Merged

13 tasks

matthew-carroll reviewed Oct 3, 2019

View reviewed changes

Chris Yang added 6 commits October 3, 2019 17:13

javadoc

4926557

refacotring and testing

60dde9e

add nonnull notation

24d949a

remove empty line

13c60dd

formatting

1516b81

review fixes

b3abbe4

matthew-carroll reviewed Oct 4, 2019

View reviewed changes

Chris Yang added 2 commits October 4, 2019 17:23

some javadoc rewrites and update broadcast receiver related structure

a212fc9

more javadoc fixes

dfcd8c9

Chris Yang added 2 commits October 7, 2019 11:54

add lisence header

98c2d09

merge new and old plugins

a58e305

matthew-carroll reviewed Oct 7, 2019

View reviewed changes

matthew-carroll approved these changes Oct 7, 2019

View reviewed changes

Chris Yang added 4 commits October 7, 2019 15:26

review fixes

ba9a2fb

refactor

43bfaea

rename activities

ba0bb22

non public

ef9762e

Chris Yang and others added 3 commits October 11, 2019 13:51

fix gradle script

e118ec6

version, e2e and more

5194df3

merge

17213fd

cyanglaz requested a review from collinjackson October 14, 2019 22:59

Chris Yang added 2 commits October 14, 2019 16:44

formatting

1ee15d8

Update pubspec.yaml

114435d

collinjackson approved these changes Oct 15, 2019

View reviewed changes

cyanglaz merged commit 67697ab into flutter:master Oct 15, 2019

cyanglaz deleted the connectivity_migrate branch October 15, 2019 19:33

cyanglaz pushed a commit that referenced this pull request Oct 18, 2019

Revert "[Connectivity] migrate to the new android embedding (#2142)"

ba9d448

This reverts commit 67697ab.

cyanglaz mentioned this pull request Oct 18, 2019

Revert "[Connectivity] migrate to the new android embedding" #2214

Closed

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019

[Connectivity] migrate to the new android embedding (flutter#2142)

fdcb5b7

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

[Connectivity] migrate to the new android embedding (flutter#2142)

ad3a966

[Connectivity] migrate to the new android embedding #2142

[Connectivity] migrate to the new android embedding #2142

Uh oh!

Conversation

cyanglaz 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!

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 commented Oct 4, 2019

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!

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!

cyanglaz commented Oct 7, 2019

Uh oh!

Choose a reason for hiding this comment

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!

cyanglaz commented Oct 14, 2019

Uh oh!

collinjackson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cyanglaz commented Oct 3, 2019 •

edited

Loading