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

[wifi_info_flutter] Wifi plugin implementation #3143

Merged
merged 11 commits into from Oct 21, 2020
Merged

[wifi_info_flutter] Wifi plugin implementation #3143

merged 11 commits into from Oct 21, 2020

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Oct 12, 2020 •
edited

Description

Implementation of wifi_info_flutter plugin. Mostly a copy of wifi methods and tests from connectivity plugin.

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.

@google-cla google-cla bot added the cla: yes label Oct 12, 2020
@bparrishMines bparrishMines changed the title [WIP] [wifi_info_flutter] Wifi plugin [wifi_info_flutter] Wifi plugin Oct 14, 2020
bparrishMines
bparrishMines commented Oct 14, 2020
View reviewed changes
...info_flutter/android/src/main/java/io/flutter/plugins/wifi_info_flutter/WifiInfoFlutter.java
String ssid = null;
if (wifiInfo != null) ssid = wifiInfo.getSSID();
if (ssid != null) ssid = ssid.replaceAll("\"", ""); // Android returns "SSID"
if (ssid != null && ssid.equals("<unknown ssid>")) ssid = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amirh addresses #3134 (comment)

@bparrishMines bparrishMines changed the title [wifi_info_flutter] Wifi plugin [wifi_info_flutter] Wifi plugin implementation Oct 14, 2020
cyanglaz
cyanglaz approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM! I did a basic scan of the code as I assumed they are copy pasted from connectivity

packages/wifi_info_flutter/wifi_info_flutter/ios/Classes/FLTWifiInfoLocationHandler.h
@@ -0,0 +1,22 @@
// Copyright 2020 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

need an extra line

hamdikahloun
hamdikahloun reviewed Oct 19, 2020
View reviewed changes
packages/wifi_info_flutter/wifi_info_flutter/android/src/main/AndroidManifest.xml
@@ -1,3 +1,4 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="io.flutter.plugins.wifi_info_flutter">
Copy link
Member

Choose a reason for hiding this comment

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

Hi @bparrishMines
To make the plugin work with Android 8.0 and Android 8.1 without additional steps, you can add that too: android.permission.CHANGE_WIFI_STATE .
https://developer.android.com/guide/topics/connectivity/wifi-scan#wifi-scan-permissions
Thank you

hamdikahloun
hamdikahloun reviewed Oct 19, 2020
View reviewed changes
...info_flutter/android/src/main/java/io/flutter/plugins/wifi_info_flutter/WifiInfoFlutter.java Outdated
return ip;
}

private android.net.wifi.WifiInfo getWifiInfo() {
Copy link
Member

Choose a reason for hiding this comment

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

@bparrishMines Why not import android.net.wifi.WifiInfo ?

hamdikahloun
hamdikahloun reviewed Oct 19, 2020
View reviewed changes
.../src/main/java/io/flutter/plugins/wifi_info_flutter/WifiInfoFlutterMethodChannelHandler.java Outdated
private WifiInfoFlutter wifiInfoFlutter;

/**
* Construct the ConnectivityMethodChannelHandler with a {@code connectivity}. The {@code
Copy link
Member

Choose a reason for hiding this comment

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

@bparrishMines It must be updated to Construct the WifiInfoFlutterMethodChannelHandle...

hamdikahloun
hamdikahloun reviewed Oct 19, 2020
View reviewed changes
packages/wifi_info_flutter/wifi_info_flutter/README.md Outdated
```dart
import 'package:connectivity/connectivity.dart';

var wifiBSSID = await (WifiFlutter().getWifiBSSID());
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean var wifiBSSID = await WifiFlutter().getWifiBSSID() without ()?

hamdikahloun
hamdikahloun reviewed Oct 20, 2020
View reviewed changes
packages/wifi_info_flutter/wifi_info_flutter/android/src/main/AndroidManifest.xml Outdated
@@ -1,4 +1,5 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="io.flutter.plugins.wifi_info_flutter">
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />
Copy link
Member

Choose a reason for hiding this comment

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

Please change <uses-permission android:name="android.permission.ACCESS_WIFI_STATE" /> to <uses-permission android:name="android.permission.CHANGE_WIFI_STATE" /> .

Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's best to leave permissions to the developer. I added the permission that was in the connectivity plugin to avoid breaking changes.

@bparrishMines bparrishMines merged commit 48a393c into flutter:master Oct 21, 2020
@cyanglaz
Copy link
Contributor

I saw we have publish:none in pubspec.yaml, when do we publish this? @bparrishMines

@bparrishMines bparrishMines deleted the wifi_plugin branch October 22, 2020 22:22
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@bparrishMines
[wifi_info_flutter] Wifi plugin implementation (flutter#3143)
a80f98e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hamdikahloun hamdikahloun hamdikahloun left review comments

@cyanglaz cyanglaz cyanglaz approved these changes

@amirh amirh Awaiting requested review from amirh

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
3 participants
@bparrishMines @cyanglaz @hamdikahloun