From 022a29820b377a78e02d10e441f3d11a92e507f2 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Mon, 26 Sep 2022 18:03:44 +0000 Subject: [PATCH] [SB Refactor] Listen to the WIFI_STATE_CHANGED_ACTION broadcasts and re-fetch `isWifiEnabled` whenever it happens. Bug: 238425913 Test: manual: Go into airplane mode and verify that we get logs for the WIFI_STATE_CHANGED_ACTION intent and for `enabled` being updated; leave airplane mode and verify the same Test: statusbar.pipeline tests Change-Id: I1eba794e98a288683bc4cfe0405a98c48a2b2c03 --- .../shared/ConnectivityPipelineLogger.kt | 38 ++++++ .../wifi/data/repository/WifiRepository.kt | 32 +++-- .../shared/ConnectivityPipelineLoggerTest.kt | 37 ++++++ .../data/repository/WifiRepositoryImplTest.kt | 125 ++++++++++++++---- 4 files changed, 190 insertions(+), 42 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt index 3a7ff167af18d..dbb1aa54d8ee6 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLogger.kt @@ -31,6 +31,20 @@ import kotlinx.coroutines.flow.onEach class ConnectivityPipelineLogger @Inject constructor( @StatusBarConnectivityLog private val buffer: LogBuffer, ) { + /** + * Logs a change in one of the **raw inputs** to the connectivity pipeline. + * + * Use this method for inputs that don't have any extra information besides their callback name. + */ + fun logInputChange(callbackName: String) { + buffer.log( + SB_LOGGING_TAG, + LogLevel.INFO, + { str1 = callbackName }, + { "Input: $str1" } + ) + } + /** * Logs a change in one of the **raw inputs** to the connectivity pipeline. */ @@ -127,6 +141,30 @@ class ConnectivityPipelineLogger @Inject constructor( companion object { const val SB_LOGGING_TAG = "SbConnectivity" + /** + * Log a change in one of the **inputs** to the connectivity pipeline. + */ + fun Flow.logInputChange( + logger: ConnectivityPipelineLogger, + inputParamName: String, + ): Flow { + return this.onEach { logger.logInputChange(inputParamName) } + } + + /** + * Log a change in one of the **inputs** to the connectivity pipeline. + * + * @param prettyPrint an optional function to transform the value into a readable string. + * [toString] is used if no custom function is provided. + */ + fun Flow.logInputChange( + logger: ConnectivityPipelineLogger, + inputParamName: String, + prettyPrint: (T) -> String = { it.toString() } + ): Flow { + return this.onEach {logger.logInputChange(inputParamName, prettyPrint(it)) } + } + /** * Log a change in one of the **outputs** to the connectivity pipeline. * diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepository.kt index 1400032ea0d31..681cf7254ae75 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepository.kt @@ -17,6 +17,7 @@ package com.android.systemui.statusbar.pipeline.wifi.data.repository import android.annotation.SuppressLint +import android.content.IntentFilter import android.net.ConnectivityManager import android.net.Network import android.net.NetworkCapabilities @@ -30,12 +31,14 @@ import android.net.wifi.WifiManager import android.net.wifi.WifiManager.TrafficStateCallback import android.util.Log import com.android.settingslib.Utils +import com.android.systemui.broadcast.BroadcastDispatcher import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCallbackFlow import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.SB_LOGGING_TAG +import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.logInputChange import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.logOutputChange import com.android.systemui.statusbar.pipeline.wifi.data.model.WifiNetworkModel import com.android.systemui.statusbar.pipeline.wifi.shared.model.WifiActivityModel @@ -44,6 +47,7 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted @@ -52,6 +56,7 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.mapLatest +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.stateIn /** Provides data related to the wifi state. */ @@ -67,10 +72,12 @@ interface WifiRepository { } /** Real implementation of [WifiRepository]. */ +@Suppress("EXPERIMENTAL_IS_NOT_ENABLED") @OptIn(ExperimentalCoroutinesApi::class) @SysUISingleton @SuppressLint("MissingPermission") class WifiRepositoryImpl @Inject constructor( + broadcastDispatcher: BroadcastDispatcher, connectivityManager: ConnectivityManager, logger: ConnectivityPipelineLogger, @Main mainExecutor: Executor, @@ -78,23 +85,22 @@ class WifiRepositoryImpl @Inject constructor( wifiManager: WifiManager?, ) : WifiRepository { - /** - * A flow that emits [Unit] whenever the wifi state may have changed. - * - * Because [WifiManager] doesn't expose a wifi state change listener, we do it internally by - * emitting to this flow whenever we think the state may have changed. - * - * TODO(b/238425913): We also need to emit to this flow whenever the WIFI_STATE_CHANGED_ACTION - * intent is triggered. - */ - private val _wifiStateChangeEvents: MutableSharedFlow = + private val wifiStateChangeEvents: Flow = broadcastDispatcher.broadcastFlow( + IntentFilter(WifiManager.WIFI_STATE_CHANGED_ACTION) + ) + .logInputChange(logger, "WIFI_STATE_CHANGED_ACTION intent") + + private val wifiNetworkChangeEvents: MutableSharedFlow = MutableSharedFlow(extraBufferCapacity = 1) override val isWifiEnabled: StateFlow = if (wifiManager == null) { MutableStateFlow(false).asStateFlow() } else { - _wifiStateChangeEvents + // Because [WifiManager] doesn't expose a wifi enabled change listener, we do it + // internally by fetching [WifiManager.isWifiEnabled] whenever we think the state may + // have changed. + merge(wifiNetworkChangeEvents, wifiStateChangeEvents) .mapLatest { wifiManager.isWifiEnabled } .distinctUntilChanged() .logOutputChange(logger, "enabled") @@ -115,7 +121,7 @@ class WifiRepositoryImpl @Inject constructor( ) { logger.logOnCapabilitiesChanged(network, networkCapabilities) - _wifiStateChangeEvents.tryEmit(Unit) + wifiNetworkChangeEvents.tryEmit(Unit) val wifiInfo = networkCapabilitiesToWifiInfo(networkCapabilities) if (wifiInfo?.isPrimary == true) { @@ -138,7 +144,7 @@ class WifiRepositoryImpl @Inject constructor( override fun onLost(network: Network) { logger.logOnLost(network) - _wifiStateChangeEvents.tryEmit(Unit) + wifiNetworkChangeEvents.tryEmit(Unit) val wifi = currentWifi if (wifi is WifiNetworkModel.Active && wifi.networkId == network.getNetId()) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLoggerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLoggerTest.kt index d3d8d542e0785..0e75c74ef6f5b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLoggerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityPipelineLoggerTest.kt @@ -23,6 +23,7 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.dump.DumpManager import com.android.systemui.log.LogBufferFactory import com.android.systemui.log.LogcatEchoTracker +import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.logInputChange import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.logOutputChange import com.google.common.truth.Truth.assertThat import java.io.PrintWriter @@ -89,6 +90,42 @@ class ConnectivityPipelineLoggerTest : SysuiTestCase() { job.cancel() } + @Test + fun logInputChange_unit_printsInputName() = runBlocking(IMMEDIATE) { + val flow: Flow = flowOf(Unit, Unit) + + val job = flow + .logInputChange(logger, "testInputs") + .launchIn(this) + + val stringWriter = StringWriter() + buffer.dump(PrintWriter(stringWriter), tailLength = 0) + val actualString = stringWriter.toString() + + assertThat(actualString).contains("testInputs") + + job.cancel() + } + + @Test + fun logInputChange_any_printsValuesAndNulls() = runBlocking(IMMEDIATE) { + val flow: Flow = flowOf(null, 2, "threeString") + + val job = flow + .logInputChange(logger, "testInputs") + .launchIn(this) + + val stringWriter = StringWriter() + buffer.dump(PrintWriter(stringWriter), tailLength = 0) + val actualString = stringWriter.toString() + + assertThat(actualString).contains("null") + assertThat(actualString).contains("2") + assertThat(actualString).contains("threeString") + + job.cancel() + } + companion object { private const val NET_1_ID = 100 private val NET_1 = com.android.systemui.util.mockito.mock().also { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepositoryImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepositoryImplTest.kt index c962315adb51b..0ba0bd623c391 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepositoryImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/wifi/data/repository/WifiRepositoryImplTest.kt @@ -28,6 +28,7 @@ import android.net.wifi.WifiManager import android.net.wifi.WifiManager.TrafficStateCallback import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase +import com.android.systemui.broadcast.BroadcastDispatcher import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.wifi.data.model.WifiNetworkModel import com.android.systemui.statusbar.pipeline.wifi.data.repository.WifiRepositoryImpl.Companion.ACTIVITY_DEFAULT @@ -37,6 +38,7 @@ import com.android.systemui.util.concurrency.FakeExecutor import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.argumentCaptor import com.android.systemui.util.mockito.mock +import com.android.systemui.util.mockito.nullable import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat import java.util.concurrent.Executor @@ -44,23 +46,28 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Before import org.junit.Test +import org.mockito.ArgumentMatchers.anyInt import org.mockito.Mock import org.mockito.Mockito.verify import org.mockito.Mockito.`when` as whenever import org.mockito.MockitoAnnotations +@Suppress("EXPERIMENTAL_IS_NOT_ENABLED") @OptIn(ExperimentalCoroutinesApi::class) @SmallTest class WifiRepositoryImplTest : SysuiTestCase() { private lateinit var underTest: WifiRepositoryImpl + @Mock private lateinit var broadcastDispatcher: BroadcastDispatcher @Mock private lateinit var logger: ConnectivityPipelineLogger @Mock private lateinit var connectivityManager: ConnectivityManager @Mock private lateinit var wifiManager: WifiManager @@ -70,16 +77,17 @@ class WifiRepositoryImplTest : SysuiTestCase() { @Before fun setUp() { MockitoAnnotations.initMocks(this) + whenever( + broadcastDispatcher.broadcastFlow( + any(), + nullable(), + anyInt(), + nullable(), + ) + ).thenReturn(flowOf(Unit)) executor = FakeExecutor(FakeSystemClock()) scope = CoroutineScope(IMMEDIATE) - - underTest = WifiRepositoryImpl( - connectivityManager, - logger, - executor, - scope, - wifiManager, - ) + underTest = createRepo() } @After @@ -89,13 +97,7 @@ class WifiRepositoryImplTest : SysuiTestCase() { @Test fun isWifiEnabled_nullWifiManager_getsFalse() = runBlocking(IMMEDIATE) { - underTest = WifiRepositoryImpl( - connectivityManager, - logger, - executor, - scope, - wifiManager = null, - ) + underTest = createRepo(wifiManagerToUse = null) assertThat(underTest.isWifiEnabled.value).isFalse() } @@ -104,13 +106,7 @@ class WifiRepositoryImplTest : SysuiTestCase() { fun isWifiEnabled_initiallyGetsWifiManagerValue() = runBlocking(IMMEDIATE) { whenever(wifiManager.isWifiEnabled).thenReturn(true) - underTest = WifiRepositoryImpl( - connectivityManager, - logger, - executor, - scope, - wifiManager - ) + underTest = createRepo() assertThat(underTest.isWifiEnabled.value).isTrue() } @@ -159,6 +155,72 @@ class WifiRepositoryImplTest : SysuiTestCase() { enabledJob.cancel() } + @Test + fun isWifiEnabled_intentsReceived_valueUpdated() = runBlocking(IMMEDIATE) { + val intentFlow = MutableSharedFlow() + whenever( + broadcastDispatcher.broadcastFlow( + any(), + nullable(), + anyInt(), + nullable(), + ) + ).thenReturn(intentFlow) + underTest = createRepo() + + val job = underTest.isWifiEnabled.launchIn(this) + + whenever(wifiManager.isWifiEnabled).thenReturn(true) + intentFlow.emit(Unit) + + assertThat(underTest.isWifiEnabled.value).isTrue() + + whenever(wifiManager.isWifiEnabled).thenReturn(false) + intentFlow.emit(Unit) + + assertThat(underTest.isWifiEnabled.value).isFalse() + + job.cancel() + } + + @Test + fun isWifiEnabled_bothIntentAndNetworkUpdates_valueAlwaysUpdated() = runBlocking(IMMEDIATE) { + val intentFlow = MutableSharedFlow() + whenever( + broadcastDispatcher.broadcastFlow( + any(), + nullable(), + anyInt(), + nullable(), + ) + ).thenReturn(intentFlow) + underTest = createRepo() + + val networkJob = underTest.wifiNetwork.launchIn(this) + val enabledJob = underTest.isWifiEnabled.launchIn(this) + + whenever(wifiManager.isWifiEnabled).thenReturn(false) + intentFlow.emit(Unit) + assertThat(underTest.isWifiEnabled.value).isFalse() + + whenever(wifiManager.isWifiEnabled).thenReturn(true) + getNetworkCallback().onLost(NETWORK) + assertThat(underTest.isWifiEnabled.value).isTrue() + + whenever(wifiManager.isWifiEnabled).thenReturn(false) + getNetworkCallback().onCapabilitiesChanged( + NETWORK, createWifiNetworkCapabilities(PRIMARY_WIFI_INFO) + ) + assertThat(underTest.isWifiEnabled.value).isFalse() + + whenever(wifiManager.isWifiEnabled).thenReturn(true) + intentFlow.emit(Unit) + assertThat(underTest.isWifiEnabled.value).isTrue() + + networkJob.cancel() + enabledJob.cancel() + } + @Test fun wifiNetwork_initiallyGetsDefault() = runBlocking(IMMEDIATE) { var latest: WifiNetworkModel? = null @@ -581,13 +643,7 @@ class WifiRepositoryImplTest : SysuiTestCase() { @Test fun wifiActivity_nullWifiManager_receivesDefault() = runBlocking(IMMEDIATE) { - underTest = WifiRepositoryImpl( - connectivityManager, - logger, - executor, - scope, - wifiManager = null, - ) + underTest = createRepo(wifiManagerToUse = null) var latest: WifiActivityModel? = null val job = underTest @@ -666,6 +722,17 @@ class WifiRepositoryImplTest : SysuiTestCase() { job.cancel() } + private fun createRepo(wifiManagerToUse: WifiManager? = wifiManager): WifiRepositoryImpl { + return WifiRepositoryImpl( + broadcastDispatcher, + connectivityManager, + logger, + executor, + scope, + wifiManagerToUse, + ) + } + private fun getTrafficStateCallback(): TrafficStateCallback { val callbackCaptor = argumentCaptor() verify(wifiManager).registerTrafficStateCallback(any(), callbackCaptor.capture())