frontend/android: refactor network related code #3673
Conversation
| private final BroadcastReceiver networkStateReceiver = new BroadcastReceiver() { | ||
| @Override | ||
| public void onReceive(Context context, Intent intent) { | ||
| Mobileserver.usingMobileDataChanged(); |
There was a problem hiding this comment.
This is deleted with no replacement, so it seems the mobile data warning banner will never be shown?
There was a problem hiding this comment.
I think in my tests it was displayed anyway. But in the meantime I have heavily refactored the code anyway.
Beerosagos
force-pushed
the
android-helpers
branch
from
4539984 to
c1150e7
Compare
Beerosagos
changed the title
Beerosagos
force-pushed
the
android-helpers
branch
3 times, most recently
from
dfb5813 to
2383f8b
Compare
|
@benma I upgraded min sdk to 24 to match the code requirements and tweaked a bit the PR to make the whole feature more reliable. PTAL |
| // To avoid false positives, if we can't obtain connectivity info, | ||
| // we return true. | ||
| // Note: this should never happen per Android documentation, as: | ||
| // - these can not be null it come from the onCapabilitiesChanged callback. | ||
| // - when obtained with getNetworkCapabilities(network), they can only be null if the | ||
| // network is null or unknown, but we guard against both in the caller. |
There was a problem hiding this comment.
This comment and the comment below are very useful, any reason you did not copy them to the new file like the test of this function?
There was a problem hiding this comment.
I don't think this applies anymore in the proposed implementation: checkConnectivity() gets the active network and verifies its capabilities. We don't have guaranties that that network capabilities are not null, and if they are null we should consider it a signal of missing connectivity, not a false positive.
I tested this implementation on various Android versions and I never experienced false positives (or negatives) with this approach.
There was a problem hiding this comment.
For the comment below, about the VALIDATED capability: I don't think that the comment matches closely the API docs. I wanted to replace it but eventually I forgot about it, I'll make a new one trying to clarify 🙏
|
|
||
| ## Unreleased | ||
| - Add feedback link to guide and about settings | ||
| - Android: fix connectivity misdetection when switching between WIFI and cellular network. |
There was a problem hiding this comment.
Mention dropped support for Android <6 too?
| Util.log("hasInternetConnectivity"); | ||
| if (capabilities == null) { | ||
| Util.log("hasInternetConnectivity: null capabilities"); | ||
| return false; |
There was a problem hiding this comment.
In the original PR where @bznein introduced the connectivity check, we carefully returned true in all the failure cases, so that the user does not get false banners about being offline:
// To avoid false positives, if we can't obtain connectivity info,
// we return true.
It seems you have changed back all these defaults. Please change it back.
See also
There was a problem hiding this comment.
Answered in my other comment about this 😇
| // Fetches the active network and verifies if that provides internet access. | ||
| public void checkConnectivity() { | ||
| if (connectivityManager == null) { | ||
| Mobileserver.setOnline(false); |
There was a problem hiding this comment.
Ah yeah, it makes sense here to behave as if we are connected 👍 Thanks
frontends/android/BitBoxApp/app/src/main/java/ch/shiftcrypto/bitboxapp/GoViewModel.java
Show resolved
Hide resolved
Beerosagos
force-pushed
the
android-helpers
branch
from
2383f8b to
13f251c
Compare
Some of the calls used in the code (e.g. those that fetch the native locale of the device) require sdk versions 24.
Our MainActivity code is quite complicated and mixes a lot of different things. This change moves the code related to network management (e.g. to display alerts when the network is not available) to a dedicated helper class and declutters the main activity a bit. It also fixes a bug that caused the app to misdetect mobile connection or missing connectivity when switching between WIFI and cellular connection. The bug was caused by the fact that the "onLost" call is not guaranteed to be executed after the network is actually lost. This can bring both to false positive and false negatives results, depending on race conditions. This caused to occasionally not display the "connection lost" banner when needed, or to e.g. display both the "connection lost" and the "mobile connection" banner at the same time. Adding a 250ms delay in the execution of the "onLost" handler is not nice, but seems to reliably fix the issue on all the tested Android versions. This also drops the `netWorkStateReceiver`, as it was registered as a broadcast receiver for the `CONNECTIVITY_ACTION`, which is now deprecated. See https://developer.android.com/reference/android/net/ConnectivityManager#CONNECTIVITY_ACTION The `usingMobileDataChanged` call, which is used to force the mobile server to fetch again the network source (wifi/mobile) and optionally display the "mobile connection" banner, is moved inside the `checkNetworkConnectivity` call, so that it's executed every time we are reassessing the connectivity status.
Beerosagos
force-pushed
the
android-helpers
branch
from
13f251c to
162eb99
Compare
|
@benma I updated the PR, added some more details to the commit message and hopefully answered all the comments :) PTAL 🙏 |
2 participants
Our MainActivity code is quite complicated and mixes a lot of different things. This change moves the code related to network management (e.g. to display alerts when the network is not available) to a dedicated helper class and declutters the main activity a bit.