diff --git a/WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java b/WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java index 0054d21d4227..993b9b694417 100644 --- a/WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java +++ b/WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java @@ -33,6 +33,7 @@ import org.wordpress.android.util.wizard.WizardManager; import org.wordpress.android.fluxc.network.TrackNetworkRequestsInterceptor; import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpNetworkAvailabilityProvider; +import org.wordpress.android.networking.NetworkConnectionMonitor; import org.wordpress.android.viewmodel.helpers.ConnectionStatus; import org.wordpress.android.viewmodel.helpers.ConnectionStatusLiveData; @@ -71,8 +72,8 @@ public static WizardManager provideWizardManager( } @Provides - static LiveData provideConnectionStatusLiveData(@ApplicationContext Context context) { - return new ConnectionStatusLiveData.Factory(context).create(); + static LiveData provideConnectionStatusLiveData(NetworkConnectionMonitor monitor) { + return new ConnectionStatusLiveData(monitor.isConnected()); } @Provides diff --git a/WordPress/src/main/java/org/wordpress/android/networking/NetworkConnectionMonitor.kt b/WordPress/src/main/java/org/wordpress/android/networking/NetworkConnectionMonitor.kt index d32995851773..76834ab71f6c 100644 --- a/WordPress/src/main/java/org/wordpress/android/networking/NetworkConnectionMonitor.kt +++ b/WordPress/src/main/java/org/wordpress/android/networking/NetworkConnectionMonitor.kt @@ -28,6 +28,9 @@ import javax.inject.Singleton * network. During a handover (e.g. Wi-Fi -> cellular) where the replacement is already up, it is added before * the old one is removed, so the set doesn't empty and the handover isn't misreported as a disconnection; a * genuine disconnect empties the set and is reported reliably. + * + * [isConnected] is seeded with the current state in [start], so it always has a value once the monitor is + * running - including when the device starts offline and no callback ever arrives. */ @Singleton class NetworkConnectionMonitor @Inject constructor() { @@ -45,6 +48,13 @@ class NetworkConnectionMonitor @Inject constructor() { val manager = context.applicationContext .getSystemService(Context.CONNECTIVITY_SERVICE) as? ConnectivityManager ?: return + // Publish the current state before registering. A NetworkCallback only reports networks as they appear, + // so on a device that starts with no connectivity nothing is ever delivered and [isConnected] would stay + // null - indistinguishable from "offline" to observers, and leaving the first reconnect looking like an + // initial value rather than a change. The CONNECTIVITY_ACTION broadcast this replaced was sticky and + // delivered the current state at registration; seeding here restores that. + onConnectivityChanged(manager.hasInternetCapability()) + val thread = HandlerThread("NetworkConnectionMonitor").apply { start() } val callback = object : ConnectivityManager.NetworkCallback() { override fun onAvailable(network: Network) = onNetworkAvailable(network) @@ -57,6 +67,11 @@ class NetworkConnectionMonitor @Inject constructor() { started = true } + private fun ConnectivityManager.hasInternetCapability(): Boolean { + val network = activeNetwork ?: return false + return getNetworkCapabilities(network)?.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) == true + } + @VisibleForTesting internal fun onNetworkAvailable(network: Network) { availableNetworks.add(network) @@ -70,10 +85,12 @@ class NetworkConnectionMonitor @Inject constructor() { } /** - * Called on the monitor's background thread. Updates [isConnected] on the first callback and whenever the - * connected state actually changes, so observers aren't spammed while connectivity churns. + * Called from [start] to publish the seeded state, then on the monitor's background thread for each + * callback. Updates [isConnected] on the first call and whenever the connected state actually changes, so + * observers aren't spammed while connectivity churns. */ - private fun onConnectivityChanged(isConnected: Boolean) { + @VisibleForTesting + internal fun onConnectivityChanged(isConnected: Boolean) { if (isFirstCallback || isConnected != wasConnected) { isFirstCallback = false wasConnected = isConnected diff --git a/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java index 9a7124acc522..1c0700b2a82d 100755 --- a/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java @@ -2,16 +2,12 @@ import android.Manifest; import android.app.Activity; -import android.content.BroadcastReceiver; import android.content.ComponentName; import android.content.Context; import android.content.Intent; -import android.content.IntentFilter; import android.content.ServiceConnection; -import android.net.ConnectivityManager; import android.net.Uri; import android.os.Build; -import android.os.Build.VERSION_CODES; import android.os.Bundle; import android.os.IBinder; import android.text.TextUtils; @@ -37,6 +33,7 @@ import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentManager.OnBackStackChangedListener; import androidx.fragment.app.FragmentTransaction; +import androidx.lifecycle.LiveData; import com.google.android.material.tabs.TabLayout; @@ -91,6 +88,7 @@ import org.wordpress.android.util.WPMediaUtils; import org.wordpress.android.util.WPPermissionUtils; import org.wordpress.android.util.analytics.AnalyticsUtils; +import org.wordpress.android.viewmodel.helpers.ConnectionStatus; import org.wordpress.android.widgets.AppReviewManager; import java.util.ArrayList; @@ -130,6 +128,7 @@ public class MediaBrowserActivity extends BaseAppCompatActivity implements Media @Inject JetpackFeatureRemovalHelper mJetpackFeatureRemovalHelper; @Inject ActivityNavigator mActivityNavigator; @Inject WpAppNotifierHandler mWpAppNotifierHandler; + @Inject LiveData mConnectionStatus; private SiteModel mSite; @@ -247,6 +246,20 @@ public void onCreate(@Nullable Bundle savedInstanceState) { doAddMediaItemClicked(AddMenuItem.ITEM_CHOOSE_FILE); mLaunchPhotoPicker = false; } + + observeConnectionStatus(); + } + + /** + * Continue any pending deletes once the connection comes back. The observer is scoped to this Activity, so + * it's only active between onStart and onStop. + */ + private void observeConnectionStatus() { + mConnectionStatus.observe(this, status -> { + if (status == ConnectionStatus.AVAILABLE && mMediaStore.hasSiteMediaToDelete(mSite)) { + startMediaDeleteService(null); + } + }); } @Override @@ -427,15 +440,6 @@ public void onStart() { mWpAppNotifierHandler.addListener(this); - if (Build.VERSION.SDK_INT >= VERSION_CODES.UPSIDE_DOWN_CAKE) { - registerReceiver( - mReceiver, - new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION), - RECEIVER_EXPORTED - ); - } else { - registerReceiver(mReceiver, new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION)); - } mDispatcher.register(this); EventBus.getDefault().register(this); } @@ -461,7 +465,6 @@ protected void onResume() { public void onStop() { mWpAppNotifierHandler.removeListener(this); EventBus.getDefault().unregister(this); - unregisterReceiver(mReceiver); mDispatcher.unregister(this); super.onStop(); } @@ -953,18 +956,6 @@ public void onServiceDisconnected(ComponentName arg0) { } }; - private final BroadcastReceiver mReceiver = new BroadcastReceiver() { - @Override - public void onReceive(Context context, Intent intent) { - if (ConnectivityManager.CONNECTIVITY_ACTION.equals(intent.getAction())) { - // Coming from zero connection. Continue what's pending for delete - if (mMediaStore.hasSiteMediaToDelete(mSite)) { - startMediaDeleteService(null); - } - } - } - }; - public void showAddMediaPopup() { View anchor = findViewById(R.id.menu_new_media); PopupMenu popup = new PopupMenu(this, anchor); diff --git a/WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt b/WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt index 699c64962623..548ff683b8dc 100644 --- a/WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt +++ b/WordPress/src/main/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveData.kt @@ -1,17 +1,7 @@ package org.wordpress.android.viewmodel.helpers -import android.content.BroadcastReceiver -import android.content.Context -import android.content.ContextWrapper -import android.content.Intent -import android.content.IntentFilter -import android.net.ConnectivityManager -import android.os.Build -import android.os.Build.VERSION_CODES import androidx.lifecycle.LiveData -import org.wordpress.android.util.distinct -import org.wordpress.android.viewmodel.helpers.ConnectionStatusLiveData.Factory -import javax.inject.Inject +import androidx.lifecycle.MediatorLiveData enum class ConnectionStatus { AVAILABLE, @@ -21,44 +11,29 @@ enum class ConnectionStatus { /** * A LiveData instance that can be injected to keep track of the network availability. * - * Use [Factory] to create an instance. The Factory guarantees that this only emits if the network availability - * changes and not when the user switches between cellular and wi-fi. + * Backed by [org.wordpress.android.networking.NetworkConnectionMonitor], which observes connectivity through a + * ConnectivityManager.NetworkCallback on a background thread rather than the deprecated CONNECTIVITY_ACTION + * broadcast. + * + * Only emits when the connected state changes. The state the monitor already holds when this instance is created + * is taken as the baseline, so the value LiveData replays to a new observer is swallowed instead of re-running + * that observer's side effect (a refresh, a retry, an upload) every time it starts observing. * * IMPORTANT: It needs to be observed for the changes to be posted. */ -class ConnectionStatusLiveData private constructor(private val context: Context) : LiveData() { - @Suppress("DEPRECATION") - override fun onActive() { - super.onActive() - val intentFilter = IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION) - if (Build.VERSION.SDK_INT >= VERSION_CODES.UPSIDE_DOWN_CAKE) { - context.registerReceiver(networkReceiver, intentFilter, ContextWrapper.RECEIVER_EXPORTED) - } else { - context.registerReceiver(networkReceiver, intentFilter) - } - } - - override fun onInactive() { - super.onInactive() - context.unregisterReceiver(networkReceiver) - } +class ConnectionStatusLiveData(source: LiveData) : MediatorLiveData() { + private var lastStatus: ConnectionStatus? = source.value?.toConnectionStatus() - @Suppress("DEPRECATION") - private val networkReceiver = object : BroadcastReceiver() { - override fun onReceive(context: Context, intent: Intent) { - val connectivityManager = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager? - val networkInfo = connectivityManager?.activeNetworkInfo - - val nextValue: ConnectionStatus = if (networkInfo?.isConnected == true) { - ConnectionStatus.AVAILABLE - } else { - ConnectionStatus.UNAVAILABLE + init { + addSource(source) { isConnected -> + val status = isConnected.toConnectionStatus() + if (status != lastStatus) { + lastStatus = status + value = status } - postValue(nextValue) } } - - class Factory @Inject constructor(private val context: Context) { - fun create() = ConnectionStatusLiveData(context).distinct() - } } + +private fun Boolean.toConnectionStatus() = + if (this) ConnectionStatus.AVAILABLE else ConnectionStatus.UNAVAILABLE diff --git a/WordPress/src/test/java/org/wordpress/android/networking/NetworkConnectionMonitorTest.kt b/WordPress/src/test/java/org/wordpress/android/networking/NetworkConnectionMonitorTest.kt index a340a4524e3d..036048d25157 100644 --- a/WordPress/src/test/java/org/wordpress/android/networking/NetworkConnectionMonitorTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/networking/NetworkConnectionMonitorTest.kt @@ -70,4 +70,30 @@ class NetworkConnectionMonitorTest : BaseUnitTest() { assertThat(monitor.isConnected.value).isTrue assertThat(emissions).containsExactly(true, false, true) } + + @Test + fun `emits the seeded state so isConnected has a value before any callback`() { + monitor.onConnectivityChanged(false) + + assertThat(monitor.isConnected.value).isFalse + assertThat(emissions).containsExactly(false) + } + + @Test + fun `emits connected when a network arrives after seeding offline`() { + // starting offline seeds false; the first network is then a genuine change, not an initial value + monitor.onConnectivityChanged(false) + monitor.onNetworkAvailable(mock()) + + assertThat(monitor.isConnected.value).isTrue + assertThat(emissions).containsExactly(false, true) + } + + @Test + fun `does not re-emit when the first callback matches the seeded state`() { + monitor.onConnectivityChanged(true) + monitor.onNetworkAvailable(mock()) + + assertThat(emissions).containsExactly(true) + } } diff --git a/WordPress/src/test/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveDataTest.kt b/WordPress/src/test/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveDataTest.kt index d11ceda1485e..ec1c66df7e8e 100644 --- a/WordPress/src/test/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveDataTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/viewmodel/helpers/ConnectionStatusLiveDataTest.kt @@ -1,97 +1,69 @@ -@file:Suppress("DEPRECATION") - package org.wordpress.android.viewmodel.helpers -import android.annotation.SuppressLint -import android.content.BroadcastReceiver -import android.content.Context -import android.net.ConnectivityManager -import android.net.NetworkInfo -import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.ExperimentalCoroutinesApi import org.assertj.core.api.Assertions.assertThat -import org.junit.Before import org.junit.Test -import org.mockito.kotlin.any -import org.mockito.kotlin.argumentCaptor -import org.mockito.kotlin.doReturn -import org.mockito.kotlin.mock import org.wordpress.android.BaseUnitTest +import org.wordpress.android.viewmodel.helpers.ConnectionStatus.AVAILABLE +import org.wordpress.android.viewmodel.helpers.ConnectionStatus.UNAVAILABLE @ExperimentalCoroutinesApi class ConnectionStatusLiveDataTest : BaseUnitTest() { - private lateinit var connectionStatusLiveData: LiveData - private lateinit var broadcastReceiver: BroadcastReceiver - - @Before - fun setUp() { - val captor = argumentCaptor() - @SuppressLint("UnspecifiedRegisterReceiverFlag") - val context = mock { - on { registerReceiver(captor.capture(), any()) } doReturn mock() - } - - connectionStatusLiveData = ConnectionStatusLiveData.Factory(context).create() - // Start observing to capture the broadcastReceiver - connectionStatusLiveData.observeForever { } - - broadcastReceiver = captor.firstValue + private val source = MutableLiveData() + + @Test + fun `it does not emit the state the source already holds when observation starts`() { + source.value = true + + val emitted = observe() + + assertThat(emitted).isEmpty() } @Test - fun `it emits a value when receiving a network info change`() { - assertThat(connectionStatusLiveData.value).isNull() + fun `it emits when the connected state changes`() { + source.value = true + val emitted = observe() - broadcastReceiver.onReceive(mockedBroadcastReceiverContext(connectedNetwork = false), mock()) + source.value = false - assertThat(connectionStatusLiveData.value).isEqualTo(ConnectionStatus.UNAVAILABLE) + assertThat(emitted).containsExactly(UNAVAILABLE) } @Test - fun `it emits a value when the network availability changes`() { - // Arrange - broadcastReceiver.onReceive(mockedBroadcastReceiverContext(connectedNetwork = true), mock()) - assertThat(connectionStatusLiveData.value).isEqualTo(ConnectionStatus.AVAILABLE) + fun `it does not emit when the source repeats the same state`() { + source.value = true + val emitted = observe() - // Act - broadcastReceiver.onReceive(mockedBroadcastReceiverContext(connectedNetwork = false), mock()) + repeat(3) { source.value = true } - // Assert - assertThat(connectionStatusLiveData.value).isEqualTo(ConnectionStatus.UNAVAILABLE) + assertThat(emitted).isEmpty() } @Test - fun `it does not emit a value when the network available didn't change`() { - // Arrange - var emitCount = 0 + fun `it emits every change in both directions`() { + source.value = true + val emitted = observe() + + source.value = false + source.value = true - connectionStatusLiveData.observeForever { - emitCount += 1 - } + assertThat(emitted).containsExactly(UNAVAILABLE, AVAILABLE) + } - broadcastReceiver.onReceive(mockedBroadcastReceiverContext(connectedNetwork = true), mock()) + @Test + fun `it emits the first state when the source has none at creation`() { + val emitted = observe() - // Act - repeat(3) { - broadcastReceiver.onReceive(mockedBroadcastReceiverContext(connectedNetwork = true), mock()) - } + source.value = true - // Assert - assertThat(emitCount).isEqualTo(1) - assertThat(connectionStatusLiveData.value).isEqualTo(ConnectionStatus.AVAILABLE) + assertThat(emitted).containsExactly(AVAILABLE) } - @Suppress("DEPRECATION") - private fun mockedBroadcastReceiverContext(connectedNetwork: Boolean): Context { - val networkInfo = mock { - on { isConnected } doReturn connectedNetwork - } - val connectivityManager = mock { - on { activeNetworkInfo } doReturn networkInfo - } - - return mock { - on { getSystemService(any()) } doReturn connectivityManager - } + private fun observe(): List { + val emitted = mutableListOf() + ConnectionStatusLiveData(source).observeForever { emitted.add(it) } + return emitted } }