Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package dev.flutter.plugins.connectivity;

import android.content.Context;
import android.net.ConnectivityManager;
import android.net.wifi.WifiManager;
import io.flutter.embedding.engine.plugins.FlutterPlugin;
import io.flutter.plugin.common.EventChannel;
import io.flutter.plugin.common.MethodChannel;
import io.flutter.plugins.connectivity.BroadcastReceiverRegistrarImpl;
import io.flutter.plugins.connectivity.Connectivity;
import io.flutter.plugins.connectivity.ConnectivityEventChannelHandler;
import io.flutter.plugins.connectivity.ConnectivityMethodChannelHandler;

/**
* Plugin implementation that uses the new {@code io.flutter.embedding} package.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

*
* <p>Instantiate this in an add to app scenario to gracefully handle activity and context changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

*/
public class ConnectivityPlugin implements FlutterPlugin {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


@Override
public void onAttachedToEngine(FlutterPluginBinding binding) {
final MethodChannel channel =
new MethodChannel(
binding.getFlutterEngine().getDartExecutor(), "plugins.flutter.io/connectivity");
final EventChannel eventChannel =
new EventChannel(
binding.getFlutterEngine().getDartExecutor(), "plugins.flutter.io/connectivity_status");
ConnectivityManager connectivityManager =
(ConnectivityManager)
binding
.getApplicationContext()
.getApplicationContext()
.getSystemService(Context.CONNECTIVITY_SERVICE);
WifiManager wifiManager =
(WifiManager)
binding
.getApplicationContext()
.getApplicationContext()
.getSystemService(Context.WIFI_SERVICE);

Connectivity connectivity = new Connectivity(connectivityManager, wifiManager);

ConnectivityMethodChannelHandler methodChannelHandler =
new ConnectivityMethodChannelHandler(connectivity);
channel.setMethodCallHandler(methodChannelHandler);

BroadcastReceiverRegistrarImpl receiverRegistrar =
new BroadcastReceiverRegistrarImpl(binding.getApplicationContext(), connectivity);
ConnectivityEventChannelHandler eventChannelHandler =
new ConnectivityEventChannelHandler(receiverRegistrar);
eventChannel.setStreamHandler(eventChannelHandler);
}

@Override
public void onDetachedFromEngine(FlutterPluginBinding binding) {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.flutter.plugins.connectivity;

import android.content.BroadcastReceiver;
import androidx.annotation.NonNull;
import io.flutter.plugin.common.EventChannel;

/** Responsible for constructing a BroadcastReceiver as well as registering and unregistering it. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

public interface BroadcastReceiverRegistrar {

/**
* Triggered when it is ready for the BroadcastReceiver to be registered. Register the receiver in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

* this method body.
*
* @param receiver the receiver is going to be registered.
*/
void readyToRegisterBroadcastReceiver(@NonNull BroadcastReceiver receiver);

/**
* Triggered when it is ready for the BroadcastReceiver to be unregistered. Unregister the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here.

* receiver in this method body.
*
* @param receiver the receiver is going to be unregistered.
*/
void readyToUnregisterBroadcastReceiver(@NonNull BroadcastReceiver receiver);

/**
* Creates a Broadcast receiver.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

*
* @param events The events helps the broadcast receiver to dump information to the event channel.
* @return A BroadcastReceiver.
*/
@NonNull
BroadcastReceiver createReceiver(@NonNull final EventChannel.EventSink events);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package io.flutter.plugins.connectivity;

import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.net.ConnectivityManager;
import androidx.annotation.NonNull;
import io.flutter.plugin.common.EventChannel;

/** The BroadcastReceiverRegistrar used for the plugin. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment here about effective prompts and useless documentation.

public class BroadcastReceiverRegistrarImpl implements BroadcastReceiverRegistrar {
private Context context;
private Connectivity connectivity;

/**
* @param context used to register and unregister the broadcastReceiver.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably useless documentation. @mklim do you think this is worth documenting?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I could see it go either way. This seems fairly straightforward to me but having the explanation about what context is used for is also kind of useful.

* @param connectivity used to check connectivity information.
*/
public BroadcastReceiverRegistrarImpl(
@NonNull Context context, @NonNull Connectivity connectivity) {
this.context = context;
this.connectivity = connectivity;
}

@Override
public void readyToRegisterBroadcastReceiver(BroadcastReceiver receiver) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seeing this implementation leads me to believe that this method should be called registerBroadcastReceiver instead of readyToRegisterBroadcastReceiver. It doesn't appear to be an event, but rather a command.

Same for the unregister method.

context.registerReceiver(receiver, new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION));
}

@Override
public void readyToUnregisterBroadcastReceiver(BroadcastReceiver receiver) {
context.unregisterReceiver(receiver);
}

@Override
public BroadcastReceiver createReceiver(final EventChannel.EventSink events) {
return new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
events.success(connectivity.checkNetworkType());
}
};
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
package io.flutter.plugins.connectivity;

import android.net.ConnectivityManager;
import android.net.Network;
import android.net.NetworkCapabilities;
import android.net.NetworkInfo;
import android.net.wifi.WifiInfo;
import android.net.wifi.WifiManager;
import android.os.Build;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;


/** Responsible for checking connectivity information. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can do better here. This class doesn't seem to do much checking of anything. It appears to be far more concerned with reporting wifi properties and status.

public class Connectivity {
private ConnectivityManager connectivityManager;
private WifiManager wifiManager;

/**
* Constructs a ConnectivityChecker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is probably another candidate for "useless documentation"

*
* @param connectivityManager used to check connectivity information.
* @param wifiManager used to check wifi information.
*/
public Connectivity(ConnectivityManager connectivityManager, WifiManager wifiManager) {
this.connectivityManager = connectivityManager;
this.wifiManager = wifiManager;
}

@NonNull
String checkNetworkType() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this method "checking"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any update on this? Does this method need to exist?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No It doesn't. It was copied from the old code. I'll remove this method

return getNetworkType();
}

@Nullable
String getWifiName() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

WifiInfo wifiInfo = getWifiInfo();
String ssid = null;
if (wifiInfo != null) ssid = wifiInfo.getSSID();
if (ssid != null) ssid = ssid.replaceAll("\"", ""); // Android returns "SSID"
return ssid;
}

@Nullable
String getWifiBSSID() {
WifiInfo wifiInfo = getWifiInfo();
String bssid = null;
if (wifiInfo != null) {
bssid = wifiInfo.getBSSID();
}
return bssid;
}

@Nullable
String getWifiIPAddress() {
WifiInfo wifiInfo = null;
if (wifiManager != null) wifiInfo = wifiManager.getConnectionInfo();

String ip = null;
int i_ip = 0;
if (wifiInfo != null) i_ip = wifiInfo.getIpAddress();

if (i_ip != 0)
ip =
String.format(
"%d.%d.%d.%d",
(i_ip & 0xff), (i_ip >> 8 & 0xff), (i_ip >> 16 & 0xff), (i_ip >> 24 & 0xff));

return ip;
}

@Nullable
private WifiInfo getWifiInfo() {
return wifiManager == null ? null : wifiManager.getConnectionInfo();
}

private String getNetworkType() {
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
Network network = connectivityManager.getActiveNetwork();
NetworkCapabilities capabilities = connectivityManager.getNetworkCapabilities(network);
if (capabilities == null) {
return "none";
}
if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)
|| capabilities.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)) {
return "wifi";
}
if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)) {
return "mobile";
}
}

return getNetworkTypeLegacy();
}

@SuppressWarnings("deprecation")
private String getNetworkTypeLegacy() {
// handle type for Android versions less than Android 9
NetworkInfo info = connectivityManager.getActiveNetworkInfo();
if (info == null || !info.isConnected()) {
return "none";
}
int type = info.getType();
switch (type) {
case ConnectivityManager.TYPE_ETHERNET:
case ConnectivityManager.TYPE_WIFI:
case ConnectivityManager.TYPE_WIMAX:
return "wifi";
case ConnectivityManager.TYPE_MOBILE:
case ConnectivityManager.TYPE_MOBILE_DUN:
case ConnectivityManager.TYPE_MOBILE_HIPRI:
return "mobile";
default:
return "none";
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package io.flutter.plugins.connectivity;

import android.content.BroadcastReceiver;
import androidx.annotation.NonNull;
import io.flutter.plugin.common.EventChannel;

/** Handles the event channel for the plugin. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another candidate for "writing prompts for good documentation"

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

public class ConnectivityEventChannelHandler implements EventChannel.StreamHandler {
private final BroadcastReceiverRegistrar broadcastReceiverRegistrar;
private BroadcastReceiver broadcastReceiver;

/**
* Constructs a ConnectivityEventChannelHandler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably "useless documentation"

*
* @param broadcastReceiverRegistrar handling registration of the broadcastReceiver.
*/
public ConnectivityEventChannelHandler(
@NonNull BroadcastReceiverRegistrar broadcastReceiverRegistrar) {
this.broadcastReceiverRegistrar = broadcastReceiverRegistrar;
}

@Override
public void onListen(Object arguments, EventChannel.EventSink events) {
broadcastReceiver = broadcastReceiverRegistrar.createReceiver(events);
broadcastReceiverRegistrar.readyToRegisterBroadcastReceiver(broadcastReceiver);
}

@Override
public void onCancel(Object arguments) {
broadcastReceiverRegistrar.readyToUnregisterBroadcastReceiver(broadcastReceiver);
broadcastReceiver = null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package io.flutter.plugins.connectivity;

import androidx.annotation.NonNull;
import io.flutter.plugin.common.MethodCall;
import io.flutter.plugin.common.MethodChannel;

/** Handles MethodChannel for the plugin. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as the other handler's documentation.

public class ConnectivityMethodChannelHandler implements MethodChannel.MethodCallHandler {

private Connectivity connectivity;

/**
* Construct the ConnectivityMethodChannelHandler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably "useless documentation"

*
* @param connectivity The {@link Connectivity} used to check connectivity information.
*/
public ConnectivityMethodChannelHandler(@NonNull Connectivity connectivity) {
assert (connectivity != null);
this.connectivity = connectivity;
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

switch (call.method) {
case "check":
result.success(connectivity.checkNetworkType());
break;
case "wifiName":
result.success(connectivity.getWifiName());
break;
case "wifiBSSID":
result.success(connectivity.getWifiBSSID());
break;
case "wifiIPAddress":
result.success(connectivity.getWifiIPAddress());
break;
default:
result.notImplemented();
break;
}
}
}
Loading