Skip to content

WIP: Reactivex wifi state listener

Peter Serwylo requested to merge pserwylo/fdroidclient:reactivex--wifi-state-listener into master

(This is clearly not a tiny change, so I've tried to be comprehensive with my description of it. It should realistically not change much other than swap code, and the largest number of added lines of code are explained below (i.e. "Dumb Wrappers" and "Tests"). Please ask if you have any questions.)

Overview

Previous Behaviour

Previously, F-Droid would eagerly try to identify the IP address of the device it was running on when started. This was for the reason that when a swap session was started - it would already know:

  • Whether it was connected to a network
  • The IP of the device
  • The SSID/BSSID of the network it was connected to

And it could use this information to snappily display to the user without them having to wait.

Once the information was figured out, it would store it in static variables in FDroidApp.

It was smart enough to know that if the Android WifiManager was unable to proactively tell us the network information, then it may be able to be ascertained by procedurally querying the network interface related classes provided by Android.

Problems

The code to ascertain the network information was a little bit difficult to reason about:

  • It could have either been invoked in response to a WifiManager broadcast from the Android system, or explicitly by F-Droid
  • The way in which it decided which mode it was in was not transparent
  • There Was a background thread which would usually terminate nicely, but sometimes would continue running (see #522 (closed))
  • Code that wanted to be notified of network state changes would need to:
  • Firstly, listen for broadcasts from the WifiStateChangeService.
  • Secondly, know to query the FDroidApp.ipAddressString static variable and its friends to get the relevant info.

Solution

By and large, this entire change is about making the process of figuring out the network state easier to reason about and more functional. The end result is a reduction in the amount of state in F-Droid, and also code that is testable (with tests!).

The WifiStateChangeService has been rewritten into three classes:

  • WifiState is the class you can ask for information about the network state
  • WifiNetworkState is a dumb class that contains information about the current network state. It is what is sent to code which asked WifiState for the current network setup.
  • WifiStateCalculator figures out what the current WifiNetworkState is. It first tries to do this by querying the Android WifiManager if it tells us it is WIFI_STATE_CONNECTED. If that is not successful, then it will defer to procedurally querying the network interface API, as was done before. This is the class that is covered by the test suite.

There is no potential for endless loops in the new code, which should prevent #522 (closed)

The static FDroidApp.* variables about the network state are now completely removed. Code accessing the network state no longer needs to understand the relationship between this global state (and how it does or doesn't get set).

When code requires access to the network state, it has two options:

  • If it requires the network state only once, then it can call WifiState.getState(...)
  • If it wants to be notified of network state changes, then it can call WifiState.subscribe(...)

Note that getting the state via WifiState.getState(...) is a little bit more verbose than the previous FDroidApp.ipAddress + FDroidApp.ssid approach. However, it should be more clear that you will always get a sensible response from it, even if F-Droid is starting and has not yet obtained its network state.

WifiState.getState(new Action1<WifiNetworkState>() {
	@Override
	public void call(WifiNetworkState state) {
		doStuffWithState(state);
	}
});

The verbosity is in part due to the lack of support for lambda expressions in the current version of Java supported by Android, or else this would be rewritten as:

WifiState.getState((state) -> doStuffWithState(state))

Dumb wrapper classes

A handful of classes have been added to the ord.fdroid.fdroid.net.wifi.wrappers package. These are dumb wrappers around existing functionality in the Android API. The only reason they exist, is because it was difficult to mock them in order to write unit tests due to:

  • Package-private constructors
  • final classes
  • Residing in a java.* package (this was new to me, but you can't put classes, even test classes, in this package)

You will note that each interface in the org.fdroid.fdroid.net.wifi.wrappers package has a corresponding *Impl in both the F-Droid source code, and a mock implementation for the tests.

git diff --stat master -- F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/

 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/InterfaceAddressWrapper.java     | 12 ++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/InterfaceAddressWrapperImpl.java | 22 ++++++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/NetworkInterfaceManager.java     |  8 ++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/NetworkInterfaceManagerImpl.java | 20 ++++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/NetworkInterfaceWrapper.java     | 23 +++++++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/NetworkInterfaceWrapperImpl.java | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/WifiInfoWrapper.java             | 18 ++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/WifiInfoWrapperImpl.java         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/WifiManagerWrapper.java          | 17 +++++++++++++++++
 F-Droid/src/org/fdroid/fdroid/net/wifi/wrappers/WifiManagerWrapperImpl.java      | 35 +++++++++++++++++++++++++++++++++++
 10 files changed, 264 insertions(+)

Tests for new WifiStateCalculator class

These are by no means comprehensive, but they should test the following features of the WifiStateCalculator class:

  • WIFI_STATE_ENABLED should return relevant network info.
  • WIFI_STATE_ENABLING should return not connected because we expect a WIFI_STATE_ENABLED in the near future.
  • Any other of the WIFI_STATE_* states from WifiManager should defer to the network interface devices:
  • Any NIC other than 'eth0', 'ap0', or 'wlan0' should be not connected
  • 'eth0', 'ap0', or 'wlan0' are tested both with valid IP + SSID info, and also without.

git diff --stat master -- F-Droid/test

 F-Droid/test/src/android/net/MockDhcpInfo.java                               |   4 ++
 F-Droid/test/src/android/net/wifi/MockWifiInfo.java                          |  36 ++++++++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/mock/MockInterfaceAddress.java            |  24 ++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/MockInterfaceAddressWrapper.java |  49 +++++++++++++++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/MockNetworkInterfaceManager.java |  22 +++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/MockNetworkInterfaceWrapper.java |  60 ++++++++++++++++++++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/MockNetworkUtils.java            | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/MockWifiManagerWrapper.java      |  34 +++++++++++++++++
 F-Droid/test/src/org/fdroid/fdroid/net/wifi/TestWifiState.java               | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 517 insertions(+)

Other Changes

While removing the dependency on FDroidApp.ssid + FDroidApp.bssid global state, I noticed that the NFC sharing of repo URLs via RepoDetailsActivity was busted. It was sharing all repos (not just swap repos) with the following format:

fdroidrepo://server/path/to/fdroid/repo?fingerprint=...&isSwap=1&bssid=...&ssid=...

The new code now shares with the following format:

fdroidrepo://server/path/to/fdroid/repo?fingerprint=...

And in the process, I also renamed Utils.getSharingUri(...) to Utils.getSwapUri(...).

Merge request reports