search

infinum / android_connectionbuddy

Utility library for handling connectivity change events.
Apache License 2.0
94 stars 17 forks source link

Maintenance #91

Closed frankot-inf closed 4 years ago

frankot-inf commented 4 years ago

Null safety

Added @NonNull and @Nullable annotations where appropriate. The only places I didn't know how to resolve were the init method in ConnectionBuddy and onReceive overrides of BroadcastReceiver.

The init method receives a ConnectionBuddyConfiguration parameter. This parameter shouldn't be null, as there is a check for this inside the method:

if (configuration == null) {
    throw new IllegalArgumentException();
}

However, if I put the @NonNull annotation to the parameter, then AS marks the if condition and says Condition 'configuration == null' is always 'false'. I don't like the idea of putting @Nullable on this parameter, as obviously it's not supposed to be null. (The configuration field is marked as @Nullable, though.) Options are to ignore the warning, to remove the check, or to remove the annotation. I've chosen to remove the annotation, but I want your opinion on this.

Regarding onReceive, the intent parameter seems unannotated by the SDK, so I've left it unannotated as well. I think that, in Kotlin, when you override this, it puts a nullable type. What do you think?

These annotations have enabled much better null-analysis, and I've gone through the warnings and added null-checks.

Wi-Fi logic

Android 10

In Android 10, it is not possible to manipulate WifiConfiguration objects anymore using addNetwork, enableNetwork, and the likes. You cannot access them through the getConfiguredNetworks call, either.

The way to connect to Wi-Fi is entirely through suggestions and requests. CHANGE_NETWORK_STATE permission is needed for connecting to Wi-Fi using ConnectivityManager APIs like requestNetwork.

I've kept the connectToWifiConfiguration names, as they're public API. If you think it's fine to rename them, comment on this.

A new private connectToWifiUsingNetworkSpecifier method has been added, that uses a WifiNetworkSpecifier API to connect to the desired network. This is called on Android 10 when connectToWifiConfiguration is called by the library user.

When using this API, a system dialog is displayed asking the user to connect to the network. When connected this way, the connection is only available while using the app, and once the process is killed, the connection is killed as well. It also seems to not provide internet access by default. (I get an exclamation mark over my Wi-Fi icon in the system tray.) I've added a bindProcessToNetwork call when establishing the connection, so traffic should go over this network anyway. I've not tested whether you can really access the internet after this, though.

It seems that another API is supposed to be used if internet access is desired, and it is the WifiNetworkSuggestion. I've not used this API, so I'm not sure how it behaves, but it seems that the device ultimately decides whether to connect or not, if it thinks this is really the best connection.

Apparently, we are no longer going to be able to explicitly connect to any Wi-Fi network, and our control over this is going away. Perhaps we should rethink the API.

What do you think about this?

Pre- Android 10

In WifiScanResultReceiver, we've had the following logic:

if (wifiConfiguration == null) {
    wifiConfiguration = new WifiConfiguration();
    wifiConfiguration.SSID = "\"" + networkSsid + "\"";
    wifiConfiguration.preSharedKey = "\"" + networkPassword + "\"";
    networkId = wifiManager.addNetwork(wifiConfiguration);
} else {
    // Set new password
    wifiConfiguration.preSharedKey = "\"" + networkPassword + "\"";
    networkId = wifiConfiguration.networkId;
}

This had the following problems. In the else branch, it seemingly updated the preSharedKey. This did not happen, however, it only updated the WifiConfiguration object local to the process. The real object is inside the Wi-Fi service process, and to update it, updateNetwork would need to be called here. (or addNetwork, as internally, they both call addOrUpdateNetwork.)

This call can still fail, as well as any of the methods that manipulate the configurations. addNetwork call remembers the package name of the creator. If the package doesn't match, no configuration manipulation can be done. (Except for enableNetwork when called with true to attempt connection. It seems to allow connecting to a configuration regardless of the creator. This is entirely experimental information, however, I don't see it in the documentation.)

This is unfortunate, because that means if the user connected to a Wi-Fi network using the system settings, then we cannot manipulate the configuration and update the password. Nothing we can do about it.

Either way, the connection is attempted.

Another problem this code had was the way the configuration is created in the if branch. It seems to work at times, but the configuration needs to be constructed with many other parameters available from the scan results. This is why a new helper method createWifiConfiguration has been created.

Further, in WifiConnectionStateChangedReceiver, the receiver would be unregistered after the first onReceive call. This is incorrect, as this receiver is notified about every step of the connection attempt. This is an example of a series of calls to onReceive:

State:             DISCONNECTED
Detailed state:    DISCONNECTED
SSID:              <unknown ssid>

State:             DISCONNECTED
Detailed state:    DISCONNECTED
SSID:              <unknown ssid>

State:             DISCONNECTED
Detailed state:    DISCONNECTED
SSID:              <unknown ssid>

State:             CONNECTING
Detailed state:    CONNECTING
SSID:              "Infinum"

State:             CONNECTING
Detailed state:    AUTHENTICATING
SSID:              "Infinum"

State:             CONNECTING
Detailed state:    AUTHENTICATING
SSID:              "Infinum"

State:             CONNECTING
Detailed state:    CONNECTING
SSID:              "Infinum"

State:             CONNECTING
Detailed state:    OBTAINING_IPADDR
SSID:              "Infinum"

State:             CONNECTING
Detailed state:    OBTAINING_IPADDR
SSID:              "Infinum"

State:             CONNECTED
Detailed state:    CONNECTED
SSID:              "Infinum"

State:             CONNECTED
Detailed state:    CONNECTED
SSID:              "Infinum"

It can even be called with DISCONNECED at the beginning (though, not always), even though a connection attempt is in progress.

It now waits to enter the CONNECTING state. After this, if CONNECTED is received, the connection is considered a success, and if DISCONNECTED or SUSPENDED is received, the connection is considered a failure.

ConnectionBuddyConfiguration changes

frankot-inf commented 4 years ago

@zplesac I've checked the demo project. Everything works fine, except Wi-Fi on Android 10, which is expected. A small change had to be introduced to the URL used for checking the network connection, though:

private static final String NETWORK_CHECK_URL = "https://clients3.google.com/generate_204";

Previously, it was http, now it's https. It would throw an IOException saying that cleartext traffic is not permitted.

I also noticed that the output was not readable in the demo app, so I've added toString overrides in the models.

The following method has been added to address your comment about extracting the null-check:

private static void assertNotNull(ConnectionBuddyConfiguration configuration) {
    if (configuration == null) {
        throw new IllegalStateException(NOT_INITIALIZED_ERROR);
    }
}

However, this had the side-effect that the IDE would not recognise the null-check in the calling method. For example, consider:

public void registerForConnectivityEvents(@NonNull Object object, @NonNull ConnectivityChangeListener listener) {
    assertNotNull(configuration);
    registerForConnectivityEvents(object, configuration.isNotifyImmediately(), listener);
}

The IDE would issue a warning at configuration.isNotifyImmediately() saying that it might result in a NPE.

Fortunately, there exists JetBrains' annotation library: https://github.com/JetBrains/java-annotations. This introduces the @Contract annotation that solves the problem. The library is included with compileOnly, so this should only have an effect on the library during compile-time.

Compile-only dependencies are distinctly different than regular compile dependencies. They are not included on the runtime classpath and they are non-transitive, meaning they are not included in dependent projects. This is true when using Gradle project dependencies and also when publishing to Maven or Ivy repositories. In the latter case, compile-only dependencies are simply omitted from published metadata.

frankot-inf commented 4 years ago

Modified Readme.md.

frankot-inf commented 4 years ago

Maybe we should set compileSdkVersion and targetSdkVersion back to 28, as we don't really properly support Android 10?

I think this is a good time to think about ConnectionBuddy 3.0.0 with breaking API changes.

zplesac commented 4 years ago

@frankot-inf yeah, I agree.

Let's go with this a introduce breaking changes with 3.0.0, I have a couple of ideas which I would like to introduce.

frankot-inf commented 4 years ago

@zplesac I've had some ideas as well. I'd be happy to discuss further in about a month, when I get back to work after my exams, if that's fine with you.

I've set targetSdkVersion to 28. The explicit buildToolsVersion setting has been removed, as this is automatically set by the Gradle plugin since version 3.0.0.

The buildscript block in build.gradle of :sampleapp has been removed, as I think this shouldn't be there. Can you double check if this is okay, please?

Gradle has been updated to 6.1.1 and Gradle plugin to 4.0.0 to keep up to date with the latest Android Studio 4.0.

This made the FindBugs plugin obsolete, and it had to be removed. Gradle recommends SpotBugs instead: https://docs.gradle.org/current/userguide/upgrading_version_5.html#the_findbugs_plugin_has_been_removed. You should decide if this should be included.

For the record, there's also a deprecation warning in the build scan here: https://scans.gradle.com/s/ux2yv6fvqujc4.