Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper Android 10 support #36

Closed
wants to merge 6 commits into from
Closed

Proper Android 10 support #36

wants to merge 6 commits into from

Conversation

eliaslecomte
Copy link
Collaborator

@eliaslecomte eliaslecomte commented Jan 29, 2020

Starting Android 10, it's no longer possible to connect (as an app) to wifi networks like before.

What can an app still do:

  1. Temporary connect to a wifi access point (for example to setup an IoT device)
  2. Suggest a network, but the OS can choose what to do with it.

With this PR, i've switched the connection logic to use WifiUtils which just released support for option 1.

Advantages of using WifiUtils:

  • Works well on different android versions
  • Only resolves when the app is connected to the network
  • Has a good retry mechanism built in
  • Well written code

@Rapsssito
Copy link
Collaborator

I really like this PR and appreciate your work. However, as discussed in ThanosFisherman/WifiUtils#47, WifiUtils Android 10 compatibility has not been properly tested.

@eliaslecomte, have you tested this changes on a real Android device?

Comment on lines 179 to 190
/**
* Send the SSID and password of a Wifi network into this to connect to the network.
* Example: wifi.findAndConnect(ssid, password);
* After 10 seconds, a post telling you whether you are connected will pop up.
* Callback returns true if ssid is in the range
* Use this to connect with a wifi network.
* Example: wifi.findAndConnect(ssid, password, false);
* The promise will resolve with the message 'connected' when the user is connected on Android.
*
* @param SSID name of the network to connect with
* @param password password of the network to connect with
* @param isWep required for iOS
* @param promise
* @param isWep only for iOS
* @param promise to send success/error feedback
*/
@ReactMethod
public void connectToProtectedSSID(@NonNull final String SSID, @NonNull final String password, final boolean isWep, final Promise promise) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this method is now compatible with open WiFi networks, we should document it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume on Android you could always connect to an open network with using an empty string as password. In my opinition we should also have the same connectTo methods on iOS and Android.

@eliaslecomte
Copy link
Collaborator Author

eliaslecomte commented Jan 31, 2020

I agree more testing should be done. Specially edge cases. I have tested the happy flow on a Pixel 4.
And also this might break some of the other functionalities (apart from connectToProtectedSSID).

@Rapsssito
Copy link
Collaborator

@eliaslecomte, if someone uses this module to connect a network indefinitely, does this PR break that functionality?

@eliaslecomte
Copy link
Collaborator Author

@eliaslecomte, if someone uses this module to connect a network indefinitely, does this PR break that functionality?

I fear for it. I don't see how with the changes for Android 10.
And even when you target Android 9 or previous, the connect to wifi functionality no longer works on Android 10.

@Rapsssito
Copy link
Collaborator

@eliaslecomte, while this module targets API 28, it will still work on Android 10. I am currently using it on my personal device on Android 10.

The option 2 from your original comment looks more like the scope of this project. The module suggest a network and user decides to connect. This is how iOS handles the connections at the moment.

Copy link
Owner

@JuanSeBestia JuanSeBestia left a comment

Choose a reason for hiding this comment

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

I love the simplicity of using this library.
I know we will lose control a little. (is the trade-off)

I request add a commit with guidelines of semmantic-release
BREAKING CHANGE: [Android] Use com.thanosfisherman.wifiutils to connect to WiFi

@eliaslecomte
Copy link
Collaborator Author

I love the simplicity of using this library.
I know we will lose control a little. (is the trade-off)

I request add a commit with guidelines of semmantic-release
BREAKING CHANGE: [Android] Use com.thanosfisherman.wifiutils to connect to WiFi

I'll will go through the change requests on thursday.

@eliaslecomte
Copy link
Collaborator Author

@eliaslecomte, while this module targets API 28, it will still work on Android 10. I am currently using it on my personal device on Android 10.

The option 2 from your original comment looks more like the scope of this project. The module suggest a network and user decides to connect. This is how iOS handles the connections at the moment.

Problem is that even when targetting sdk 28 it won't work on all Android 10 devices. For example it doesn't work on the Samsung s10.

@eliaslecomte
Copy link
Collaborator Author

I've opened a new PR as I didn't want to rewrite all of the history here (changing commit messages..); #46

@eliaslecomte eliaslecomte deleted the feature/use-wifiutils-for-android branch March 18, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants