From 957a878b44fb1fc3bad9a07edc83497ee75a90c8 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Mon, 30 Jan 2023 21:47:48 +0000 Subject: [PATCH 1/6] [SB Refactor] Add logs for the carrier merged connection. Bug: 264684296 Fixes: 267501739 Bug: 238425913 Test: Manual: Dumped MobileConnectionLog[3] and verified logs Test: atest FullMobileConnectionRepositoryTest CarrierMergedConnectionRepositoryTest Change-Id: Ife7d154e0bd07f5ede9cfd5404407e1863c78df6 --- .../prod/CarrierMergedConnectionRepository.kt | 3 - .../prod/FullMobileConnectionRepository.kt | 18 +- .../prod/MobileConnectionRepositoryImpl.kt | 23 +- .../FullMobileConnectionRepositoryTest.kt | 284 +++++++++++++++++- .../prod/MobileConnectionRepositoryTest.kt | 12 +- .../repository/prod/MobileTelephonyHelpers.kt | 39 +++ 6 files changed, 339 insertions(+), 40 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileTelephonyHelpers.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/CarrierMergedConnectionRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/CarrierMergedConnectionRepository.kt index c783b12e0c0b0..f5041d89c1d18 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/CarrierMergedConnectionRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/CarrierMergedConnectionRepository.kt @@ -91,9 +91,6 @@ class CarrierMergedConnectionRepository( .map { it.toMobileConnectionModel() } .stateIn(scope, SharingStarted.WhileSubscribed(), MobileConnectionModel()) - // TODO(b/238425913): Add logging to this class. - // TODO(b/238425913): Make sure SignalStrength.getEmptyState is used when appropriate. - // Carrier merged is never roaming. override val cdmaRoaming: StateFlow = MutableStateFlow(false).asStateFlow() diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepository.kt index dd2cc928f7ad1..f17791b655029 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepository.kt @@ -117,11 +117,22 @@ class FullMobileConnectionRepository( override val connectionInfo = activeRepo .flatMapLatest { it.connectionInfo } + .logDiffsForTable( + tableLogBuffer, + columnPrefix = "", + initialValue = activeRepo.value.connectionInfo.value, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), activeRepo.value.connectionInfo.value) override val dataEnabled = activeRepo .flatMapLatest { it.dataEnabled } + .logDiffsForTable( + tableLogBuffer, + columnPrefix = "", + columnName = "dataEnabled", + initialValue = activeRepo.value.dataEnabled.value, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), activeRepo.value.dataEnabled.value) override val numberOfLevels = @@ -132,6 +143,11 @@ class FullMobileConnectionRepository( override val networkName = activeRepo .flatMapLatest { it.networkName } + .logDiffsForTable( + tableLogBuffer, + columnPrefix = "", + initialValue = activeRepo.value.networkName.value, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), activeRepo.value.networkName.value) class Factory @@ -168,7 +184,7 @@ class FullMobileConnectionRepository( const val MOBILE_CONNECTION_BUFFER_SIZE = 100 /** Returns a log buffer name for a mobile connection with the given [subId]. */ - fun tableBufferLogName(subId: Int): String = "MobileConnectionLog [$subId]" + fun tableBufferLogName(subId: Int): String = "MobileConnectionLog[$subId]" } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt index 76fef358726d2..cfc4cc4ba9471 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryImpl.kt @@ -36,7 +36,6 @@ import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCall import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.dagger.qualifiers.Background import com.android.systemui.log.table.TableLogBuffer -import com.android.systemui.log.table.logDiffsForTable import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectionModel import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel import com.android.systemui.statusbar.pipeline.mobile.data.model.ResolvedNetworkType.DefaultNetworkType @@ -62,7 +61,6 @@ import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.mapNotNull @@ -87,7 +85,7 @@ class MobileConnectionRepositoryImpl( private val mobileMappingsProxy: MobileMappingsProxy, bgDispatcher: CoroutineDispatcher, logger: ConnectivityPipelineLogger, - mobileLogger: TableLogBuffer, + override val tableLogBuffer: TableLogBuffer, scope: CoroutineScope, ) : MobileConnectionRepository { init { @@ -101,8 +99,6 @@ class MobileConnectionRepositoryImpl( private val telephonyCallbackEvent = MutableSharedFlow(extraBufferCapacity = 1) - override val tableLogBuffer: TableLogBuffer = mobileLogger - /** * This flow defines the single shared connection to system_server via TelephonyCallback. Any * new callback should be added to this listener and funneled through callbackEvents via a data @@ -243,11 +239,6 @@ class MobileConnectionRepositoryImpl( val initial = MobileConnectionModel() callbackEvents .scan(initial, ::updateConnectionState) - .logDiffsForTable( - mobileLogger, - columnPrefix = "MobileConnection ($subId)", - initialValue = initial, - ) .stateIn(scope, SharingStarted.WhileSubscribed(), initial) } @@ -285,24 +276,12 @@ class MobileConnectionRepositoryImpl( intent.toNetworkNameModel(networkNameSeparator) ?: defaultNetworkName } } - .distinctUntilChanged() - .logDiffsForTable( - mobileLogger, - columnPrefix = "", - initialValue = defaultNetworkName, - ) .stateIn(scope, SharingStarted.WhileSubscribed(), defaultNetworkName) override val dataEnabled = run { val initial = telephonyManager.isDataConnectionAllowed callbackEvents .mapNotNull { (it as? CallbackEvent.OnDataEnabledChanged)?.enabled } - .logDiffsForTable( - mobileLogger, - columnPrefix = "", - columnName = "dataEnabled", - initialValue = initial - ) .stateIn(scope, SharingStarted.WhileSubscribed(), initial) } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepositoryTest.kt index 24b9f7d55da50..da208a7f08c9a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/FullMobileConnectionRepositoryTest.kt @@ -16,20 +16,32 @@ package com.android.systemui.statusbar.pipeline.mobile.data.repository.prod +import android.telephony.ServiceState +import android.telephony.SignalStrength +import android.telephony.TelephonyCallback +import android.telephony.TelephonyManager import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.log.table.TableLogBufferFactory import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectionModel +import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectionModel.Companion.COL_EMERGENCY +import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectionModel.Companion.COL_OPERATOR +import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectionModel.Companion.COL_PRIMARY_LEVEL import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.FakeMobileConnectionRepository import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionRepository +import com.android.systemui.statusbar.pipeline.mobile.data.repository.prod.MobileTelephonyHelpers.getTelephonyCallbackForType +import com.android.systemui.statusbar.pipeline.wifi.data.model.WifiNetworkModel +import com.android.systemui.statusbar.pipeline.wifi.data.repository.FakeWifiRepository import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.eq import com.android.systemui.util.mockito.mock import com.android.systemui.util.mockito.whenever import com.android.systemui.util.time.FakeSystemClock import com.google.common.truth.Truth.assertThat +import java.io.PrintWriter +import java.io.StringWriter import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach @@ -52,9 +64,10 @@ import org.mockito.Mockito.verify class FullMobileConnectionRepositoryTest : SysuiTestCase() { private lateinit var underTest: FullMobileConnectionRepository + private val systemClock = FakeSystemClock() private val testDispatcher = UnconfinedTestDispatcher() private val testScope = TestScope(testDispatcher) - private val tableLogBuffer = mock() + private val tableLogBuffer = TableLogBuffer(maxSize = 100, name = "TestName", systemClock) private val mobileFactory = mock() private val carrierMergedFactory = mock() @@ -347,8 +360,214 @@ class FullMobileConnectionRepositoryTest : SysuiTestCase() { .isSameInstanceAs(connection1Repeat.tableLogBuffer) } - // TODO(b/238425913): Verify that the logging switches correctly (once the carrier merged repo - // implements logging). + @Test + fun connectionInfo_logging_notCarrierMerged_getsUpdates() = + testScope.runTest { + // SETUP: Use real repositories to verify the diffing still works. (See b/267501739.) + val telephonyManager = mock() + createRealMobileRepo(telephonyManager) + createRealCarrierMergedRepo(FakeWifiRepository()) + + initializeRepo(startingIsCarrierMerged = false) + + val job = underTest.connectionInfo.launchIn(this) + + // WHEN we set up some mobile connection info + val serviceState = ServiceState() + serviceState.setOperatorName("longName", "OpTypical", "1") + serviceState.isEmergencyOnly = false + getTelephonyCallbackForType(telephonyManager) + .onServiceStateChanged(serviceState) + + // THEN it's logged to the buffer + assertThat(dumpBuffer()).contains("$COL_OPERATOR${BUFFER_SEPARATOR}OpTypical") + assertThat(dumpBuffer()).contains("$COL_EMERGENCY${BUFFER_SEPARATOR}false") + + // WHEN we update mobile connection info + val serviceState2 = ServiceState() + serviceState2.setOperatorName("longName", "OpDiff", "1") + serviceState2.isEmergencyOnly = true + getTelephonyCallbackForType(telephonyManager) + .onServiceStateChanged(serviceState2) + + // THEN the updates are logged + assertThat(dumpBuffer()).contains("$COL_OPERATOR${BUFFER_SEPARATOR}OpDiff") + assertThat(dumpBuffer()).contains("$COL_EMERGENCY${BUFFER_SEPARATOR}true") + + job.cancel() + } + + @Test + fun connectionInfo_logging_carrierMerged_getsUpdates() = + testScope.runTest { + // SETUP: Use real repositories to verify the diffing still works. (See b/267501739.) + createRealMobileRepo(mock()) + val wifiRepository = FakeWifiRepository() + createRealCarrierMergedRepo(wifiRepository) + + initializeRepo(startingIsCarrierMerged = true) + + val job = underTest.connectionInfo.launchIn(this) + + // WHEN we set up carrier merged info + val networkId = 2 + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 3, + ) + ) + + // THEN the carrier merged info is logged + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}3") + + // WHEN we update the info + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 1, + ) + ) + + // THEN the updates are logged + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}1") + + job.cancel() + } + + @Test + fun connectionInfo_logging_updatesWhenCarrierMergedUpdates() = + testScope.runTest { + // SETUP: Use real repositories to verify the diffing still works. (See b/267501739.) + val telephonyManager = mock() + createRealMobileRepo(telephonyManager) + + val wifiRepository = FakeWifiRepository() + createRealCarrierMergedRepo(wifiRepository) + + initializeRepo(startingIsCarrierMerged = false) + + val job = underTest.connectionInfo.launchIn(this) + + // WHEN we set up some mobile connection info + val signalStrength = mock() + whenever(signalStrength.level).thenReturn(1) + + getTelephonyCallbackForType(telephonyManager) + .onSignalStrengthsChanged(signalStrength) + + // THEN it's logged to the buffer + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}1") + + // WHEN isCarrierMerged is set to true + val networkId = 2 + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 3, + ) + ) + underTest.setIsCarrierMerged(true) + + // THEN the carrier merged info is logged + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}3") + + // WHEN the carrier merge network is updated + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 4, + ) + ) + + // THEN the new level is logged + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}4") + + // WHEN isCarrierMerged is set to false + underTest.setIsCarrierMerged(false) + + // THEN the typical info is logged + // Note: Since our first logs also had the typical info, we need to search the log + // contents for after our carrier merged level log. + val fullBuffer = dumpBuffer() + val carrierMergedContentIndex = fullBuffer.indexOf("${BUFFER_SEPARATOR}4") + val bufferAfterCarrierMerged = fullBuffer.substring(carrierMergedContentIndex) + assertThat(bufferAfterCarrierMerged).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}1") + + // WHEN the normal network is updated + val newMobileInfo = + MobileConnectionModel( + operatorAlphaShort = "Mobile Operator 2", + primaryLevel = 0, + ) + mobileRepo.setConnectionInfo(newMobileInfo) + + // THEN the new level is logged + assertThat(dumpBuffer()).contains("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}0") + + job.cancel() + } + + @Test + fun connectionInfo_logging_doesNotLogUpdatesForNotActiveRepo() = + testScope.runTest { + // SETUP: Use real repositories to verify the diffing still works. (See b/267501739.) + val telephonyManager = mock() + createRealMobileRepo(telephonyManager) + + val wifiRepository = FakeWifiRepository() + createRealCarrierMergedRepo(wifiRepository) + + // WHEN isCarrierMerged = false + initializeRepo(startingIsCarrierMerged = false) + + val job = underTest.connectionInfo.launchIn(this) + + val signalStrength = mock() + whenever(signalStrength.level).thenReturn(1) + getTelephonyCallbackForType(telephonyManager) + .onSignalStrengthsChanged(signalStrength) + + // THEN updates to the carrier merged level aren't logged + val networkId = 2 + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 4, + ) + ) + assertThat(dumpBuffer()).doesNotContain("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}4") + + wifiRepository.setWifiNetwork( + WifiNetworkModel.CarrierMerged( + networkId, + SUB_ID, + level = 3, + ) + ) + assertThat(dumpBuffer()).doesNotContain("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}3") + + // WHEN isCarrierMerged is set to true + underTest.setIsCarrierMerged(true) + + // THEN updates to the normal level aren't logged + whenever(signalStrength.level).thenReturn(5) + getTelephonyCallbackForType(telephonyManager) + .onSignalStrengthsChanged(signalStrength) + assertThat(dumpBuffer()).doesNotContain("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}5") + + whenever(signalStrength.level).thenReturn(6) + getTelephonyCallbackForType(telephonyManager) + .onSignalStrengthsChanged(signalStrength) + assertThat(dumpBuffer()).doesNotContain("$COL_PRIMARY_LEVEL${BUFFER_SEPARATOR}6") + + job.cancel() + } private fun initializeRepo(startingIsCarrierMerged: Boolean) { underTest = @@ -364,9 +583,68 @@ class FullMobileConnectionRepositoryTest : SysuiTestCase() { ) } + private fun createRealMobileRepo( + telephonyManager: TelephonyManager, + ): MobileConnectionRepositoryImpl { + whenever(telephonyManager.subscriptionId).thenReturn(SUB_ID) + + val realRepo = + MobileConnectionRepositoryImpl( + context, + SUB_ID, + defaultNetworkName = NetworkNameModel.Default("default"), + networkNameSeparator = SEP, + telephonyManager, + systemUiCarrierConfig = mock(), + fakeBroadcastDispatcher, + mobileMappingsProxy = mock(), + testDispatcher, + logger = mock(), + tableLogBuffer, + testScope.backgroundScope, + ) + whenever( + mobileFactory.build( + eq(SUB_ID), + any(), + eq(DEFAULT_NAME), + eq(SEP), + ) + ) + .thenReturn(realRepo) + + return realRepo + } + + private fun createRealCarrierMergedRepo( + wifiRepository: FakeWifiRepository, + ): CarrierMergedConnectionRepository { + wifiRepository.setIsWifiEnabled(true) + wifiRepository.setIsWifiDefault(true) + val realRepo = + CarrierMergedConnectionRepository( + SUB_ID, + tableLogBuffer, + defaultNetworkName = NetworkNameModel.Default("default"), + testScope.backgroundScope, + wifiRepository, + ) + whenever(carrierMergedFactory.build(eq(SUB_ID), any(), eq(DEFAULT_NAME))) + .thenReturn(realRepo) + + return realRepo + } + + private fun dumpBuffer(): String { + val outputWriter = StringWriter() + tableLogBuffer.dump(PrintWriter(outputWriter), arrayOf()) + return outputWriter.toString() + } + private companion object { const val SUB_ID = 42 private val DEFAULT_NAME = NetworkNameModel.Default("default name") private const val SEP = "-" + private const val BUFFER_SEPARATOR = "|" } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryTest.kt index 3f36bc18af08e..1a5cc9abd9b64 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionRepositoryTest.kt @@ -71,7 +71,6 @@ import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.shared.data.model.DataActivityModel import com.android.systemui.statusbar.pipeline.shared.data.model.toMobileDataActivityModel 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.whenever import com.google.common.truth.Truth.assertThat @@ -86,7 +85,6 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.mockito.Mock -import org.mockito.Mockito import org.mockito.MockitoAnnotations @Suppress("EXPERIMENTAL_IS_NOT_ENABLED") @@ -670,16 +668,8 @@ class MobileConnectionRepositoryTest : SysuiTestCase() { job.cancel() } - private fun getTelephonyCallbacks(): List { - val callbackCaptor = argumentCaptor() - Mockito.verify(telephonyManager).registerTelephonyCallback(any(), callbackCaptor.capture()) - return callbackCaptor.allValues - } - private inline fun getTelephonyCallbackForType(): T { - val cbs = getTelephonyCallbacks().filterIsInstance() - assertThat(cbs.size).isEqualTo(1) - return cbs[0] + return MobileTelephonyHelpers.getTelephonyCallbackForType(telephonyManager) } /** Convenience constructor for SignalStrength */ diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileTelephonyHelpers.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileTelephonyHelpers.kt new file mode 100644 index 0000000000000..621f79307e49f --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileTelephonyHelpers.kt @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.pipeline.mobile.data.repository.prod + +import android.telephony.TelephonyCallback +import android.telephony.TelephonyManager +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.argumentCaptor +import com.google.common.truth.Truth.assertThat +import org.mockito.Mockito.verify + +/** Helper methods for telephony-related callbacks for mobile tests. */ +object MobileTelephonyHelpers { + fun getTelephonyCallbacks(mockTelephonyManager: TelephonyManager): List { + val callbackCaptor = argumentCaptor() + verify(mockTelephonyManager).registerTelephonyCallback(any(), callbackCaptor.capture()) + return callbackCaptor.allValues + } + + inline fun getTelephonyCallbackForType(mockTelephonyManager: TelephonyManager): T { + val cbs = getTelephonyCallbacks(mockTelephonyManager).filterIsInstance() + assertThat(cbs.size).isEqualTo(1) + return cbs[0] + } +} From ed9266190b68a6fc5825ad6a4106c73a4645b502 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Fri, 3 Feb 2023 16:20:46 +0000 Subject: [PATCH 2/6] [Table logging] Support nullable ints and add more extension functions. Will be used in future status bar logging CLs. Bug: 238425913 Test: atest TableChangeTest LogDiffsForTableTest Change-Id: Iab75c65af1175521fa1721dbbf13c4e592ecbfb1 --- .../android/systemui/log/table/Diffable.kt | 58 +++- .../android/systemui/log/table/TableChange.kt | 4 +- .../systemui/log/table/TableLogBuffer.kt | 4 +- .../log/table/LogDiffsForTableTest.kt | 286 ++++++++++++++++++ .../systemui/log/table/TableChangeTest.kt | 22 ++ 5 files changed, 358 insertions(+), 16 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/log/table/Diffable.kt b/packages/SystemUI/src/com/android/systemui/log/table/Diffable.kt index 348d941d22cf4..ccd406001253a 100644 --- a/packages/SystemUI/src/com/android/systemui/log/table/Diffable.kt +++ b/packages/SystemUI/src/com/android/systemui/log/table/Diffable.kt @@ -79,10 +79,10 @@ fun > Flow.logDiffsForTable( } } -/** - * Each time the boolean flow is updated with a new value that's different from the previous value, - * logs the new value to the given [tableLogBuffer]. - */ +// Here and below: Various Flow extension functions that are effectively equivalent to the +// above [logDiffsForTable] method. + +/** See [logDiffsForTable(TableLogBuffer, String, T)]. */ fun Flow.logDiffsForTable( tableLogBuffer: TableLogBuffer, columnPrefix: String, @@ -100,10 +100,8 @@ fun Flow.logDiffsForTable( newVal } } -/** - * Each time the Int flow is updated with a new value that's different from the previous value, logs - * the new value to the given [tableLogBuffer]. - */ + +/** See [logDiffsForTable(TableLogBuffer, String, T)]. */ fun Flow.logDiffsForTable( tableLogBuffer: TableLogBuffer, columnPrefix: String, @@ -122,10 +120,26 @@ fun Flow.logDiffsForTable( } } -/** - * Each time the String? flow is updated with a new value that's different from the previous value, - * logs the new value to the given [tableLogBuffer]. - */ +/** See [logDiffsForTable(TableLogBuffer, String, T)]. */ +fun Flow.logDiffsForTable( + tableLogBuffer: TableLogBuffer, + columnPrefix: String, + columnName: String, + initialValue: Int?, +): Flow { + val initialValueFun = { + tableLogBuffer.logChange(columnPrefix, columnName, initialValue) + initialValue + } + return this.pairwiseBy(initialValueFun) { prevVal, newVal: Int? -> + if (prevVal != newVal) { + tableLogBuffer.logChange(columnPrefix, columnName, newVal) + } + newVal + } +} + +/** See [logDiffsForTable(TableLogBuffer, String, T)]. */ fun Flow.logDiffsForTable( tableLogBuffer: TableLogBuffer, columnPrefix: String, @@ -143,3 +157,23 @@ fun Flow.logDiffsForTable( newVal } } + +/** See [logDiffsForTable(TableLogBuffer, String, T)]. */ +fun Flow>.logDiffsForTable( + tableLogBuffer: TableLogBuffer, + columnPrefix: String, + columnName: String, + initialValue: List, +): Flow> { + val initialValueFun = { + tableLogBuffer.logChange(columnPrefix, columnName, initialValue.toString()) + initialValue + } + return this.pairwiseBy(initialValueFun) { prevVal, newVal: List -> + if (prevVal != newVal) { + // TODO(b/267761156): Can we log list changes without using toString? + tableLogBuffer.logChange(columnPrefix, columnName, newVal.toString()) + } + newVal + } +} diff --git a/packages/SystemUI/src/com/android/systemui/log/table/TableChange.kt b/packages/SystemUI/src/com/android/systemui/log/table/TableChange.kt index 68c297f76ab78..4880f80e77168 100644 --- a/packages/SystemUI/src/com/android/systemui/log/table/TableChange.kt +++ b/packages/SystemUI/src/com/android/systemui/log/table/TableChange.kt @@ -27,7 +27,7 @@ data class TableChange( var columnName: String = "", var type: DataType = DataType.EMPTY, var bool: Boolean = false, - var int: Int = 0, + var int: Int? = null, var str: String? = null, ) { /** Resets to default values so that the object can be recycled. */ @@ -54,7 +54,7 @@ data class TableChange( } /** Sets this to store an int change. */ - fun set(value: Int) { + fun set(value: Int?) { type = DataType.INT int = value } diff --git a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt index 2c299d67022d8..1712dab8aff9e 100644 --- a/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt +++ b/packages/SystemUI/src/com/android/systemui/log/table/TableLogBuffer.kt @@ -138,7 +138,7 @@ class TableLogBuffer( } /** Logs a Int change. */ - fun logChange(prefix: String, columnName: String, value: Int) { + fun logChange(prefix: String, columnName: String, value: Int?) { logChange(systemClock.currentTimeMillis(), prefix, columnName, value) } @@ -155,7 +155,7 @@ class TableLogBuffer( change.set(value) } - private fun logChange(timestamp: Long, prefix: String, columnName: String, value: Int) { + private fun logChange(timestamp: Long, prefix: String, columnName: String, value: Int?) { val change = obtain(timestamp, prefix, columnName) change.set(value) } diff --git a/packages/SystemUI/tests/src/com/android/systemui/log/table/LogDiffsForTableTest.kt b/packages/SystemUI/tests/src/com/android/systemui/log/table/LogDiffsForTableTest.kt index 3b5e6b9c145cb..d1744c61de587 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/log/table/LogDiffsForTableTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/log/table/LogDiffsForTableTest.kt @@ -275,6 +275,52 @@ class LogDiffsForTableTest : SysuiTestCase() { job.cancel() } + @Test + fun intNullable_logsNull() = + testScope.runTest { + systemClock.setCurrentTimeMillis(100L) + val flow = flow { + for (int in listOf(null, 6, null, 8)) { + systemClock.advanceTime(100L) + emit(int) + } + } + + val flowWithLogging = + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = 1234, + ) + + val job = launch { flowWithLogging.collect() } + + val logs = dumpLog() + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(100L) + SEPARATOR + FULL_NAME + SEPARATOR + "1234" + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(200L) + SEPARATOR + FULL_NAME + SEPARATOR + "null" + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(300L) + SEPARATOR + FULL_NAME + SEPARATOR + "6" + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(400L) + SEPARATOR + FULL_NAME + SEPARATOR + "null" + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(500L) + SEPARATOR + FULL_NAME + SEPARATOR + "8" + ) + + job.cancel() + } + @Test fun int_logsUpdates() = testScope.runTest { @@ -1030,6 +1076,246 @@ class LogDiffsForTableTest : SysuiTestCase() { job.cancel() } + // ---- Flow> tests ---- + + @Test + fun list_doesNotLogWhenNotCollected() { + val flow = flowOf(listOf(5), listOf(6), listOf(7)) + + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = listOf(1234), + ) + + val logs = dumpLog() + assertThat(logs).doesNotContain(COLUMN_PREFIX) + assertThat(logs).doesNotContain(COLUMN_NAME) + assertThat(logs).doesNotContain("1234") + } + + @Test + fun list_logsInitialWhenCollected() = + testScope.runTest { + val flow = flowOf(listOf(5), listOf(6), listOf(7)) + + val flowWithLogging = + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = listOf(1234), + ) + + systemClock.setCurrentTimeMillis(3000L) + val job = launch { flowWithLogging.collect() } + + val logs = dumpLog() + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(3000L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf(1234).toString() + ) + + job.cancel() + } + + @Test + fun list_logsUpdates() = + testScope.runTest { + systemClock.setCurrentTimeMillis(100L) + + val listItems = + listOf(listOf("val1", "val2"), listOf("val3"), listOf("val4", "val5", "val6")) + val flow = flow { + for (list in listItems) { + systemClock.advanceTime(100L) + emit(list) + } + } + + val flowWithLogging = + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = listOf("val0", "val00"), + ) + + val job = launch { flowWithLogging.collect() } + + val logs = dumpLog() + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(100L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val0", "val00").toString() + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(200L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val1", "val2").toString() + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(300L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val3").toString() + ) + assertThat(logs) + .contains( + TABLE_LOG_DATE_FORMAT.format(400L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val4", "val5", "val6").toString() + ) + + job.cancel() + } + + @Test + fun list_doesNotLogIfSameValue() = + testScope.runTest { + systemClock.setCurrentTimeMillis(100L) + + val listItems = + listOf( + listOf("val0", "val00"), + listOf("val1"), + listOf("val1"), + listOf("val1", "val2"), + ) + val flow = flow { + for (bool in listItems) { + systemClock.advanceTime(100L) + emit(bool) + } + } + + val flowWithLogging = + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = listOf("val0", "val00"), + ) + + val job = launch { flowWithLogging.collect() } + + val logs = dumpLog() + + val expected1 = + TABLE_LOG_DATE_FORMAT.format(100L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val0", "val00").toString() + val expected3 = + TABLE_LOG_DATE_FORMAT.format(300L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val1").toString() + val expected5 = + TABLE_LOG_DATE_FORMAT.format(500L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val1", "val2").toString() + assertThat(logs).contains(expected1) + assertThat(logs).contains(expected3) + assertThat(logs).contains(expected5) + + val unexpected2 = + TABLE_LOG_DATE_FORMAT.format(200L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val0", "val00") + val unexpected4 = + TABLE_LOG_DATE_FORMAT.format(400L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf("val1") + assertThat(logs).doesNotContain(unexpected2) + assertThat(logs).doesNotContain(unexpected4) + job.cancel() + } + + @Test + fun list_worksForStateFlows() = + testScope.runTest { + val flow = MutableStateFlow(listOf(1111)) + + val flowWithLogging = + flow.logDiffsForTable( + tableLogBuffer, + COLUMN_PREFIX, + COLUMN_NAME, + initialValue = listOf(1111), + ) + + systemClock.setCurrentTimeMillis(50L) + val job = launch { flowWithLogging.collect() } + assertThat(dumpLog()) + .contains( + TABLE_LOG_DATE_FORMAT.format(50L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf(1111).toString() + ) + + systemClock.setCurrentTimeMillis(100L) + flow.emit(listOf(2222, 3333)) + assertThat(dumpLog()) + .contains( + TABLE_LOG_DATE_FORMAT.format(100L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf(2222, 3333).toString() + ) + + systemClock.setCurrentTimeMillis(200L) + flow.emit(listOf(3333, 4444)) + assertThat(dumpLog()) + .contains( + TABLE_LOG_DATE_FORMAT.format(200L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf(3333, 4444).toString() + ) + + // Doesn't log duplicates + systemClock.setCurrentTimeMillis(300L) + flow.emit(listOf(3333, 4444)) + assertThat(dumpLog()) + .doesNotContain( + TABLE_LOG_DATE_FORMAT.format(300L) + + SEPARATOR + + FULL_NAME + + SEPARATOR + + listOf(3333, 4444).toString() + ) + + job.cancel() + } + private fun dumpLog(): String { val outputWriter = StringWriter() tableLogBuffer.dump(PrintWriter(outputWriter), arrayOf()) diff --git a/packages/SystemUI/tests/src/com/android/systemui/log/table/TableChangeTest.kt b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableChangeTest.kt index 432764a7de8fc..c7f3fa0830cd0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/log/table/TableChangeTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/log/table/TableChangeTest.kt @@ -35,6 +35,17 @@ class TableChangeTest : SysuiTestCase() { assertThat(underTest.getVal()).isEqualTo("fakeValue") } + @Test + fun setString_null() { + val underTest = TableChange() + + underTest.reset(timestamp = 100, columnPrefix = "", columnName = "fakeName") + underTest.set(null as String?) + + assertThat(underTest.hasData()).isTrue() + assertThat(underTest.getVal()).isEqualTo("null") + } + @Test fun setBoolean_isBoolean() { val underTest = TableChange() @@ -57,6 +68,17 @@ class TableChangeTest : SysuiTestCase() { assertThat(underTest.getVal()).isEqualTo("8900") } + @Test + fun setInt_null() { + val underTest = TableChange() + + underTest.reset(timestamp = 100, columnPrefix = "", columnName = "fakeName") + underTest.set(null as Int?) + + assertThat(underTest.hasData()).isTrue() + assertThat(underTest.getVal()).isEqualTo("null") + } + @Test fun setThenReset_isEmpty() { val underTest = TableChange() From b209bdeff03c3bf5a898cbec7b220acb1d4e6cc1 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Wed, 1 Feb 2023 18:53:31 +0000 Subject: [PATCH 3/6] [SB Refactor] Add a table logger for the parent mobile classes. This logs almost every flow in the parent repo and interactor. Once we've sufficiently dogfooded and tested the new pipeline, we can remove some of these logs. Bug: 267217719 Bug: 238425913 Test: manual: Dumped MobileSummaryLog Change-Id: I1f8b2b4ed7316b20eb994170578742f3dbc5dae4 --- .../pipeline/dagger/MobileSummaryLog.kt | 31 ++++++++++++++ .../dagger/StatusBarPipelineModule.kt | 7 ++++ .../data/model/MobileConnectivityModel.kt | 24 ++++++++++- .../prod/MobileConnectionsRepositoryImpl.kt | 42 +++++++++++++++++-- .../interactor/MobileIconsInteractor.kt | 34 ++++++++++++++- .../shared/ConnectivityPipelineLogger.kt | 9 ---- .../MobileRepositorySwitcherTest.kt | 3 ++ .../prod/MobileConnectionsRepositoryTest.kt | 3 ++ .../interactor/MobileIconsInteractorTest.kt | 1 + 9 files changed, 139 insertions(+), 15 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt new file mode 100644 index 0000000000000..2ac9ab3c25100 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/MobileSummaryLog.kt @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.pipeline.dagger + +import javax.inject.Qualifier + +/** + * Logs for mobile data that's **the same across all connections**. + * + * This buffer should only be used for the mobile parent classes like [MobileConnectionsRepository] + * and [MobileIconsInteractor]. It should *not* be used for classes that represent an individual + * connection, like [MobileConnectionRepository] or [MobileIconInteractor]. + */ +@Qualifier +@MustBeDocumented +@Retention(AnnotationRetention.RUNTIME) +annotation class MobileSummaryLog diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt index 5f3b0dcb1654d..60de1a38dd95d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/dagger/StatusBarPipelineModule.kt @@ -118,5 +118,12 @@ abstract class StatusBarPipelineModule { fun provideAirplaneTableLogBuffer(factory: TableLogBufferFactory): TableLogBuffer { return factory.create("AirplaneTableLog", 30) } + + @Provides + @SysUISingleton + @MobileSummaryLog + fun provideMobileSummaryLogBuffer(factory: TableLogBufferFactory): TableLogBuffer { + return factory.create("MobileSummaryLog", 100) + } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt index e61890523ebba..97a537ac0ce67 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/model/MobileConnectivityModel.kt @@ -17,6 +17,8 @@ package com.android.systemui.statusbar.pipeline.mobile.data.model import android.net.NetworkCapabilities +import com.android.systemui.log.table.Diffable +import com.android.systemui.log.table.TableRowLogger /** Provides information about a mobile network connection */ data class MobileConnectivityModel( @@ -24,4 +26,24 @@ data class MobileConnectivityModel( val isConnected: Boolean = false, /** Whether the mobile transport is validated [NetworkCapabilities.NET_CAPABILITY_VALIDATED] */ val isValidated: Boolean = false, -) +) : Diffable { + // TODO(b/267767715): Can we implement [logDiffs] and [logFull] generically for data classes? + override fun logDiffs(prevVal: MobileConnectivityModel, row: TableRowLogger) { + if (prevVal.isConnected != isConnected) { + row.logChange(COL_IS_CONNECTED, isConnected) + } + if (prevVal.isValidated != isValidated) { + row.logChange(COL_IS_VALIDATED, isValidated) + } + } + + override fun logFull(row: TableRowLogger) { + row.logChange(COL_IS_CONNECTED, isConnected) + row.logChange(COL_IS_VALIDATED, isValidated) + } + + companion object { + private const val COL_IS_CONNECTED = "isConnected" + private const val COL_IS_VALIDATED = "isValidated" + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt index 10f48a36e7f64..c660d311a9a18 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryImpl.kt @@ -42,6 +42,9 @@ import com.android.systemui.common.coroutine.ConflatedCallbackFlow.conflatedCall import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.dagger.qualifiers.Background +import com.android.systemui.log.table.TableLogBuffer +import com.android.systemui.log.table.logDiffsForTable +import com.android.systemui.statusbar.pipeline.dagger.MobileSummaryLog import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectivityModel import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel @@ -82,6 +85,7 @@ constructor( private val subscriptionManager: SubscriptionManager, private val telephonyManager: TelephonyManager, private val logger: ConnectivityPipelineLogger, + @MobileSummaryLog private val tableLogger: TableLogBuffer, mobileMappingsProxy: MobileMappingsProxy, broadcastDispatcher: BroadcastDispatcher, private val context: Context, @@ -114,6 +118,12 @@ constructor( } } .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "carrierMergedSubId", + initialValue = null, + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), null) private val mobileSubscriptionsChangeEvent: Flow = conflatedCallbackFlow { @@ -139,8 +149,14 @@ constructor( override val subscriptions: StateFlow> = merge(mobileSubscriptionsChangeEvent, carrierMergedSubId) .mapLatest { fetchSubscriptionsList().map { it.toSubscriptionModel() } } - .logInputChange(logger, "onSubscriptionsChanged") .onEach { infos -> updateRepos(infos) } + .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "subscriptions", + initialValue = listOf(), + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), listOf()) /** StateFlow that keeps track of the current active mobile data subscription */ @@ -157,7 +173,12 @@ constructor( awaitClose { telephonyManager.unregisterTelephonyCallback(callback) } } .distinctUntilChanged() - .logInputChange(logger, "onActiveDataSubscriptionIdChanged") + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "activeSubId", + initialValue = INVALID_SUBSCRIPTION_ID, + ) .stateIn(scope, started = SharingStarted.WhileSubscribed(), INVALID_SUBSCRIPTION_ID) private val defaultDataSubIdChangeEvent: MutableSharedFlow = @@ -171,7 +192,12 @@ constructor( intent.getIntExtra(PhoneConstants.SUBSCRIPTION_KEY, INVALID_SUBSCRIPTION_ID) } .distinctUntilChanged() - .logInputChange(logger, "ACTION_DEFAULT_DATA_SUBSCRIPTION_CHANGED") + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "defaultSubId", + initialValue = SubscriptionManager.getDefaultDataSubscriptionId(), + ) .onEach { defaultDataSubIdChangeEvent.tryEmit(Unit) } .stateIn( scope, @@ -247,7 +273,11 @@ constructor( awaitClose { connectivityManager.unregisterNetworkCallback(callback) } } .distinctUntilChanged() - .logInputChange(logger, "defaultMobileNetworkConnectivity") + .logDiffsForTable( + tableLogger, + columnPrefix = "$LOGGING_PREFIX.defaultConnection", + initialValue = MobileConnectivityModel(), + ) .stateIn(scope, SharingStarted.WhileSubscribed(), MobileConnectivityModel()) /** @@ -321,4 +351,8 @@ constructor( subscriptionId = subscriptionId, isOpportunistic = isOpportunistic, ) + + companion object { + private const val LOGGING_PREFIX = "Repo" + } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt index 0e4a4321bf263..bbb60f919730b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt @@ -23,6 +23,9 @@ import com.android.settingslib.SignalIcon.MobileIconGroup import com.android.settingslib.mobile.TelephonyIcons import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application +import com.android.systemui.log.table.TableLogBuffer +import com.android.systemui.log.table.logDiffsForTable +import com.android.systemui.statusbar.pipeline.dagger.MobileSummaryLog import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileConnectivityModel import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionRepository @@ -104,6 +107,7 @@ constructor( private val mobileConnectionsRepo: MobileConnectionsRepository, private val carrierConfigTracker: CarrierConfigTracker, private val logger: ConnectivityPipelineLogger, + @MobileSummaryLog private val tableLogger: TableLogBuffer, userSetupRepo: UserSetupRepository, @Application private val scope: CoroutineScope, ) : MobileIconsInteractor { @@ -173,7 +177,13 @@ constructor( } } .distinctUntilChanged() - .onEach { logger.logFilteredSubscriptionsChanged(it) } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "filteredSubscriptions", + initialValue = listOf(), + ) + .stateIn(scope, SharingStarted.WhileSubscribed(), listOf()) override val defaultDataSubId = mobileConnectionsRepo.defaultDataSubId @@ -195,6 +205,12 @@ constructor( delay(2000) emit(false) } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "forcingValidation", + initialValue = false, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val defaultMobileNetworkConnectivity: StateFlow = @@ -211,6 +227,12 @@ constructor( networkConnectivity } } + .distinctUntilChanged() + .logDiffsForTable( + tableLogger, + columnPrefix = "$LOGGING_PREFIX.defaultConnection", + initialValue = mobileConnectionsRepo.defaultMobileNetworkConnectivity.value, + ) .stateIn( scope, SharingStarted.WhileSubscribed(), @@ -259,6 +281,12 @@ constructor( !connectivityModel.isValidated } } + .logDiffsForTable( + tableLogger, + LOGGING_PREFIX, + columnName = "isDefaultConnectionFailed", + initialValue = false, + ) .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val isUserSetup: StateFlow = userSetupRepo.isUserSetupFlow @@ -277,4 +305,8 @@ constructor( isDefaultConnectionFailed, mobileConnectionsRepo.getRepoForSubId(subId), ) + + companion object { + private const val LOGGING_PREFIX = "Intr" + } } 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 6796a94cf6a92..45036969aefe9 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 @@ -204,15 +204,6 @@ constructor( // TODO(b/238425913): We should split this class into mobile-specific and wifi-specific loggers. - fun logFilteredSubscriptionsChanged(subs: List) { - buffer.log( - SB_LOGGING_TAG, - LogLevel.INFO, - { str1 = subs.toString() }, - { "Filtered subscriptions updated: $str1" }, - ) - } - fun logUiAdapterSubIdsUpdated(subs: List) { buffer.log( SB_LOGGING_TAG, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt index 96cca44407694..4da2104ca32e0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcherTest.kt @@ -25,6 +25,7 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.demomode.DemoMode import com.android.systemui.demomode.DemoModeController import com.android.systemui.dump.DumpManager +import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.log.table.TableLogBufferFactory import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.demo.DemoMobileConnectionsRepository @@ -81,6 +82,7 @@ class MobileRepositorySwitcherTest : SysuiTestCase() { @Mock private lateinit var subscriptionManager: SubscriptionManager @Mock private lateinit var telephonyManager: TelephonyManager @Mock private lateinit var logger: ConnectivityPipelineLogger + @Mock private lateinit var summaryLogger: TableLogBuffer @Mock private lateinit var demoModeController: DemoModeController @Mock private lateinit var dumpManager: DumpManager @@ -114,6 +116,7 @@ class MobileRepositorySwitcherTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt index b73348c185603..fef0981397561 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/prod/MobileConnectionsRepositoryTest.kt @@ -84,6 +84,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { @Mock private lateinit var subscriptionManager: SubscriptionManager @Mock private lateinit var telephonyManager: TelephonyManager @Mock private lateinit var logger: ConnectivityPipelineLogger + @Mock private lateinit var summaryLogger: TableLogBuffer @Mock private lateinit var logBufferFactory: TableLogBufferFactory private val mobileMappings = FakeMobileMappingsProxy() @@ -157,6 +158,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, @@ -616,6 +618,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { subscriptionManager, telephonyManager, logger, + summaryLogger, mobileMappings, fakeBroadcastDispatcher, context, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt index bd249221b6cae..23b634c7d2b8a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt @@ -79,6 +79,7 @@ class MobileIconsInteractorTest : SysuiTestCase() { connectionsRepository, carrierConfigTracker, logger = mock(), + tableLogger = mock(), userSetupRepository, testScope.backgroundScope, ) From a8cd6d353fdd1461cbfdf1874b3561a211e30849 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Thu, 2 Feb 2023 21:03:56 +0000 Subject: [PATCH 4/6] [SB Refactor] Add better logging for the network type icon. Instead of logging `networkTypeIcon|Resource(res=2131232263, contentDescription=Resource(res=2131952280))`, it'll now log: showNetworkTypeIcon|false networkTypeIcon|LTE Bug: 238425913 Test: manual: dumped logs for MobileConnectionLog[3] (my local sub ID) Change-Id: I385e205017dd46c9047f308f45e6ef45f46f0f26 --- .../domain/interactor/MobileIconInteractor.kt | 12 +++++++ .../ui/viewmodel/MobileIconViewModel.kt | 33 +++++++++---------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt index 9cdff96dc7d97..636d2cf91459e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt @@ -33,7 +33,9 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.mapLatest +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn interface MobileIconInteractor { @@ -181,6 +183,16 @@ class MobileIconInteractorImpl( else -> mapping[info.resolvedNetworkType.lookupKey] ?: defaultGroup } } + .distinctUntilChanged() + .onEach { + // Doesn't use [logDiffsForTable] because [MobileIconGroup] can't implement the + // [Diffable] interface. + tableLogBuffer.logChange( + prefix = "", + columnName = "networkTypeIcon", + value = it.name + ) + } .stateIn(scope, SharingStarted.WhileSubscribed(), defaultMobileIconGroup.value) override val isEmergencyOnly: StateFlow = diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt index 9e2024afda8f3..dc09fe45172c5 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt @@ -37,7 +37,6 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapLatest -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn /** Common interface for all of the location-based mobile icon view models. */ @@ -124,14 +123,22 @@ constructor( private val showNetworkTypeIcon: Flow = combine( - iconInteractor.isDataConnected, - iconInteractor.isDataEnabled, - iconInteractor.isDefaultConnectionFailed, - iconInteractor.alwaysShowDataRatIcon, - iconInteractor.isConnected, - ) { dataConnected, dataEnabled, failedConnection, alwaysShow, connected -> - alwaysShow || (dataConnected && dataEnabled && !failedConnection && connected) - } + iconInteractor.isDataConnected, + iconInteractor.isDataEnabled, + iconInteractor.isDefaultConnectionFailed, + iconInteractor.alwaysShowDataRatIcon, + iconInteractor.isConnected, + ) { dataConnected, dataEnabled, failedConnection, alwaysShow, connected -> + alwaysShow || (dataConnected && dataEnabled && !failedConnection && connected) + } + .distinctUntilChanged() + .logDiffsForTable( + iconInteractor.tableLogBuffer, + columnPrefix = "", + columnName = "showNetworkTypeIcon", + initialValue = false, + ) + .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val networkTypeIcon: Flow = combine( @@ -149,14 +156,6 @@ constructor( } } .distinctUntilChanged() - .onEach { - // This is done as an onEach side effect since Icon is not Diffable (yet) - iconInteractor.tableLogBuffer.logChange( - prefix = "", - columnName = "networkTypeIcon", - value = it.toString(), - ) - } .stateIn(scope, SharingStarted.WhileSubscribed(), null) override val roaming: StateFlow = From 83e7763535c136d2239b40b112e9b0fe63fe00f0 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Fri, 3 Feb 2023 19:09:35 +0000 Subject: [PATCH 5/6] [SB Refactor] Add better logging for the mobile icon ID. Previous logging had `iconId=1028` or `iconId=132096`, which is difficult to parse. Bug: 238425913 Test: Manual: Dumped DemoMobileConnectionLog [1] and verified output Test: atest MobileIconViewModelTest Change-Id: I8ddeafe4dfb579d9d05e5c23a62bf2e55e256998 --- .../mobile/ui/binder/MobileIconBinder.kt | 9 ++- .../mobile/ui/model/SignalIconModel.kt | 56 +++++++++++++++++++ .../ui/viewmodel/MobileIconViewModel.kt | 17 +++--- .../ui/viewmodel/MobileIconsViewModel.kt | 1 - .../ui/view/ModernStatusBarMobileViewTest.kt | 3 - .../LocationBasedMobileIconViewModelTest.kt | 18 +++--- .../ui/viewmodel/MobileIconViewModelTest.kt | 28 ++++------ 7 files changed, 90 insertions(+), 42 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/model/SignalIconModel.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt index a4b2abcf7385c..890de70f4959b 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt @@ -93,8 +93,13 @@ object MobileIconBinder { // Set the icon for the triangle launch { - viewModel.iconId.distinctUntilChanged().collect { iconId -> - mobileDrawable.level = iconId + viewModel.icon.distinctUntilChanged().collect { icon -> + mobileDrawable.level = + SignalDrawable.getState( + icon.level, + icon.numberOfLevels, + icon.showExclamationMark, + ) } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/model/SignalIconModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/model/SignalIconModel.kt new file mode 100644 index 0000000000000..16e176613ec94 --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/model/SignalIconModel.kt @@ -0,0 +1,56 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.pipeline.mobile.ui.model + +import com.android.systemui.log.table.Diffable +import com.android.systemui.log.table.TableRowLogger + +/** A model that will be consumed by [SignalDrawable] to show the mobile triangle icon. */ +data class SignalIconModel( + val level: Int, + val numberOfLevels: Int, + val showExclamationMark: Boolean, +) : Diffable { + // TODO(b/267767715): Can we implement [logDiffs] and [logFull] generically for data classes? + override fun logDiffs(prevVal: SignalIconModel, row: TableRowLogger) { + if (prevVal.level != level) { + row.logChange(COL_LEVEL, level) + } + if (prevVal.numberOfLevels != numberOfLevels) { + row.logChange(COL_NUM_LEVELS, numberOfLevels) + } + if (prevVal.showExclamationMark != showExclamationMark) { + row.logChange(COL_SHOW_EXCLAMATION, showExclamationMark) + } + } + + override fun logFull(row: TableRowLogger) { + row.logChange(COL_LEVEL, level) + row.logChange(COL_NUM_LEVELS, numberOfLevels) + row.logChange(COL_SHOW_EXCLAMATION, showExclamationMark) + } + + companion object { + /** Creates a [SignalIconModel] representing an empty and invalidated state. */ + fun createEmptyState(numberOfLevels: Int) = + SignalIconModel(level = 0, numberOfLevels, showExclamationMark = true) + + private const val COL_LEVEL = "level" + private const val COL_NUM_LEVELS = "numLevels" + private const val COL_SHOW_EXCLAMATION = "showExclamation" + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt index dc09fe45172c5..b584398b10dc5 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt @@ -24,8 +24,8 @@ import com.android.systemui.common.shared.model.Icon import com.android.systemui.log.table.logDiffsForTable import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.MobileIconInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.MobileIconsInteractor +import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants -import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.shared.data.model.DataActivityModel import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -42,8 +42,7 @@ import kotlinx.coroutines.flow.stateIn /** Common interface for all of the location-based mobile icon view models. */ interface MobileIconViewModelCommon { val subscriptionId: Int - /** An int consumable by [SignalDrawable] for display */ - val iconId: Flow + val icon: Flow val contentDescription: Flow val roaming: Flow /** The RAT icon (LTE, 3G, 5G, etc) to be displayed. Null if we shouldn't show anything */ @@ -72,7 +71,6 @@ class MobileIconViewModel constructor( override val subscriptionId: Int, iconInteractor: MobileIconInteractor, - logger: ConnectivityPipelineLogger, constants: ConnectivityConstants, scope: CoroutineScope, ) : MobileIconViewModelCommon { @@ -80,8 +78,8 @@ constructor( private val showExclamationMark: Flow = iconInteractor.isDefaultDataEnabled.mapLatest { !it } - override val iconId: Flow = run { - val initial = SignalDrawable.getEmptyState(iconInteractor.numberOfLevels.value) + override val icon: Flow = run { + val initial = SignalIconModel.createEmptyState(iconInteractor.numberOfLevels.value) combine( iconInteractor.level, iconInteractor.numberOfLevels, @@ -89,16 +87,15 @@ constructor( iconInteractor.isInService, ) { level, numberOfLevels, showExclamationMark, isInService -> if (!isInService) { - SignalDrawable.getEmptyState(numberOfLevels) + SignalIconModel.createEmptyState(numberOfLevels) } else { - SignalDrawable.getState(level, numberOfLevels, showExclamationMark) + SignalIconModel(level, numberOfLevels, showExclamationMark) } } .distinctUntilChanged() .logDiffsForTable( iconInteractor.tableLogBuffer, - columnPrefix = "", - columnName = "iconId", + columnPrefix = "icon", initialValue = initial, ) .stateIn(scope, SharingStarted.WhileSubscribed(), initial) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt index 24370d221ade7..fa95e1fb51810 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt @@ -56,7 +56,6 @@ constructor( ?: MobileIconViewModel( subId, interactor.createMobileConnectionInteractorForSubId(subId), - logger, constants, scope, ) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt index a2c1209f5a40b..39f77c9c75284 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt @@ -34,7 +34,6 @@ import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.LocationBased import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.MobileIconViewModel import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.QsMobileIconViewModel import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants -import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.util.mockito.whenever import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -58,7 +57,6 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { @Mock private lateinit var statusBarPipelineFlags: StatusBarPipelineFlags @Mock private lateinit var tableLogBuffer: TableLogBuffer - @Mock private lateinit var logger: ConnectivityPipelineLogger @Mock private lateinit var constants: ConnectivityConstants private lateinit var viewModel: LocationBasedMobileViewModel @@ -74,7 +72,6 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { MobileIconViewModel( subscriptionId = 1, interactor, - logger, constants, testScope.backgroundScope, ) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt index c960a06e6bb29..4f36d6396fe0b 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt @@ -22,9 +22,9 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.statusbar.pipeline.StatusBarPipelineFlags import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconInteractor +import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.MobileIconViewModelTest.Companion.defaultSignal import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants -import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.launchIn @@ -47,7 +47,6 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { private lateinit var keyguardIcon: KeyguardMobileIconViewModel private lateinit var interactor: FakeMobileIconInteractor @Mock private lateinit var statusBarPipelineFlags: StatusBarPipelineFlags - @Mock private lateinit var logger: ConnectivityPipelineLogger @Mock private lateinit var constants: ConnectivityConstants @Mock private lateinit var tableLogBuffer: TableLogBuffer @@ -67,8 +66,7 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { setNumberOfLevels(4) isDataConnected.value = true } - commonImpl = - MobileIconViewModel(SUB_1_ID, interactor, logger, constants, testScope.backgroundScope) + commonImpl = MobileIconViewModel(SUB_1_ID, interactor, constants, testScope.backgroundScope) homeIcon = HomeMobileIconViewModel(commonImpl, statusBarPipelineFlags) qsIcon = QsMobileIconViewModel(commonImpl, statusBarPipelineFlags) @@ -78,14 +76,14 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { @Test fun `location based view models receive same icon id when common impl updates`() = testScope.runTest { - var latestHome: Int? = null - val homeJob = homeIcon.iconId.onEach { latestHome = it }.launchIn(this) + var latestHome: SignalIconModel? = null + val homeJob = homeIcon.icon.onEach { latestHome = it }.launchIn(this) - var latestQs: Int? = null - val qsJob = qsIcon.iconId.onEach { latestQs = it }.launchIn(this) + var latestQs: SignalIconModel? = null + val qsJob = qsIcon.icon.onEach { latestQs = it }.launchIn(this) - var latestKeyguard: Int? = null - val keyguardJob = keyguardIcon.iconId.onEach { latestKeyguard = it }.launchIn(this) + var latestKeyguard: SignalIconModel? = null + val keyguardJob = keyguardIcon.icon.onEach { latestKeyguard = it }.launchIn(this) var expected = defaultSignal(level = 1) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt index b91a4df6ddda2..83c902f031ac6 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt @@ -19,15 +19,14 @@ package com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel import androidx.test.filters.SmallTest import com.android.settingslib.AccessibilityContentDescriptions.PHONE_SIGNAL_STRENGTH import com.android.settingslib.AccessibilityContentDescriptions.PHONE_SIGNAL_STRENGTH_NONE -import com.android.settingslib.graph.SignalDrawable import com.android.settingslib.mobile.TelephonyIcons.THREE_G import com.android.systemui.SysuiTestCase import com.android.systemui.common.shared.model.ContentDescription import com.android.systemui.common.shared.model.Icon import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconInteractor +import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants -import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.shared.data.model.DataActivityModel import com.android.systemui.util.mockito.whenever import com.google.common.truth.Truth.assertThat @@ -49,7 +48,6 @@ import org.mockito.MockitoAnnotations class MobileIconViewModelTest : SysuiTestCase() { private lateinit var underTest: MobileIconViewModel private lateinit var interactor: FakeMobileIconInteractor - @Mock private lateinit var logger: ConnectivityPipelineLogger @Mock private lateinit var constants: ConnectivityConstants @Mock private lateinit var tableLogBuffer: TableLogBuffer @@ -69,15 +67,14 @@ class MobileIconViewModelTest : SysuiTestCase() { setNumberOfLevels(4) isDataConnected.value = true } - underTest = - MobileIconViewModel(SUB_1_ID, interactor, logger, constants, testScope.backgroundScope) + underTest = MobileIconViewModel(SUB_1_ID, interactor, constants, testScope.backgroundScope) } @Test fun iconId_correctLevel_notCutout() = testScope.runTest { - var latest: Int? = null - val job = underTest.iconId.onEach { latest = it }.launchIn(this) + var latest: SignalIconModel? = null + val job = underTest.icon.onEach { latest = it }.launchIn(this) val expected = defaultSignal() assertThat(latest).isEqualTo(expected) @@ -90,8 +87,8 @@ class MobileIconViewModelTest : SysuiTestCase() { testScope.runTest { interactor.setIsDefaultDataEnabled(false) - var latest: Int? = null - val job = underTest.iconId.onEach { latest = it }.launchIn(this) + var latest: SignalIconModel? = null + val job = underTest.icon.onEach { latest = it }.launchIn(this) val expected = defaultSignal(level = 1, connected = false) assertThat(latest).isEqualTo(expected) @@ -102,8 +99,8 @@ class MobileIconViewModelTest : SysuiTestCase() { @Test fun `icon - uses empty state - when not in service`() = testScope.runTest { - var latest: Int? = null - val job = underTest.iconId.onEach { latest = it }.launchIn(this) + var latest: SignalIconModel? = null + val job = underTest.icon.onEach { latest = it }.launchIn(this) interactor.isInService.value = false @@ -368,7 +365,6 @@ class MobileIconViewModelTest : SysuiTestCase() { MobileIconViewModel( SUB_1_ID, interactor, - logger, constants, testScope.backgroundScope, ) @@ -407,7 +403,6 @@ class MobileIconViewModelTest : SysuiTestCase() { MobileIconViewModel( SUB_1_ID, interactor, - logger, constants, testScope.backgroundScope, ) @@ -466,10 +461,11 @@ class MobileIconViewModelTest : SysuiTestCase() { fun defaultSignal( level: Int = 1, connected: Boolean = true, - ): Int { - return SignalDrawable.getState(level, /* numLevels */ 4, !connected) + ): SignalIconModel { + return SignalIconModel(level, numberOfLevels = 4, showExclamationMark = !connected) } - fun emptySignal(): Int = SignalDrawable.getEmptyState(4) + fun emptySignal(): SignalIconModel = + SignalIconModel(level = 0, numberOfLevels = 4, showExclamationMark = true) } } From 2def6f9adf10b04e4e7216cf261d9b840658b68c Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Mon, 6 Feb 2023 20:42:11 +0000 Subject: [PATCH 6/6] [SB Refactor] Migrate mobile's `state.visible` to the new pipeline. StatusBarMobileView uses `MobileIconState.visible` to determine its visibility status. Following all the inputs to `visible`, then `visible = hasMobileData && !airplaneMode && !forceHiddenByTuning`. This CL implements that boolean logic. Bug: 238425913 Fixes: 267786582 Test: manual: turn on airplane mode and verify mobile triangle is hidden every where. Turn off airplane mode and verify triangle re-appears. Test: `adb shell pm enable com.android.systemui/.tuner.TunerActivity` then Settings > System > System UI Tuner > Status bar > Toggle the mobile on/off and see the mobile icon toggle as well Test: atest MobileIconViewModelTest Change-Id: I5986ba477ba5172480ab4a5f7b9900259a8466ea --- .../domain/interactor/MobileIconInteractor.kt | 4 + .../interactor/MobileIconsInteractor.kt | 15 ++- .../mobile/ui/binder/MobileIconBinder.kt | 5 +- .../ui/viewmodel/MobileIconViewModel.kt | 24 ++++ .../ui/viewmodel/MobileIconsViewModel.kt | 7 + .../interactor/FakeMobileIconInteractor.kt | 2 + .../interactor/FakeMobileIconsInteractor.kt | 2 + .../interactor/MobileIconInteractorTest.kt | 16 +++ .../interactor/MobileIconsInteractorTest.kt | 32 +++++ .../ui/view/ModernStatusBarMobileViewTest.kt | 91 +++++++++++-- .../LocationBasedMobileIconViewModelTest.kt | 18 ++- .../ui/viewmodel/MobileIconViewModelTest.kt | 122 +++++++++++++++--- .../ui/viewmodel/MobileIconsViewModelTest.kt | 11 ++ 13 files changed, 319 insertions(+), 30 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt index 636d2cf91459e..9b7614c2ad081 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractor.kt @@ -111,6 +111,9 @@ interface MobileIconInteractor { /** Based on [CarrierConfigManager.KEY_INFLATE_SIGNAL_STRENGTH_BOOL], either 4 or 5 */ val numberOfLevels: StateFlow + + /** See [MobileIconsInteractor.isForceHidden]. */ + val isForceHidden: Flow } /** Interactor for a single mobile connection. This connection _should_ have one subscription ID */ @@ -126,6 +129,7 @@ class MobileIconInteractorImpl( defaultMobileIconGroup: StateFlow, defaultDataSubId: StateFlow, override val isDefaultConnectionFailed: StateFlow, + override val isForceHidden: Flow, connectionRepository: MobileConnectionRepository, ) : MobileIconInteractor { private val connectionInfo = connectionRepository.connectionInfo diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt index bbb60f919730b..94f7af42b54ab 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt @@ -32,6 +32,8 @@ import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConn import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionsRepository import com.android.systemui.statusbar.pipeline.mobile.data.repository.UserSetupRepository import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger +import com.android.systemui.statusbar.pipeline.shared.data.model.ConnectivitySlot +import com.android.systemui.statusbar.pipeline.shared.data.repository.ConnectivityRepository import com.android.systemui.util.CarrierConfigTracker import javax.inject.Inject import kotlinx.coroutines.CoroutineScope @@ -45,8 +47,8 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapLatest -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.transformLatest @@ -91,6 +93,10 @@ interface MobileIconsInteractor { val isDefaultConnectionFailed: StateFlow /** True once the user has been set up */ val isUserSetup: StateFlow + + /** True if we're configured to force-hide the mobile icons and false otherwise. */ + val isForceHidden: Flow + /** * Vends out a [MobileIconInteractor] tracking the [MobileConnectionRepository] for the given * subId. Will throw if the ID is invalid @@ -108,6 +114,7 @@ constructor( private val carrierConfigTracker: CarrierConfigTracker, private val logger: ConnectivityPipelineLogger, @MobileSummaryLog private val tableLogger: TableLogBuffer, + connectivityRepository: ConnectivityRepository, userSetupRepo: UserSetupRepository, @Application private val scope: CoroutineScope, ) : MobileIconsInteractor { @@ -291,6 +298,11 @@ constructor( override val isUserSetup: StateFlow = userSetupRepo.isUserSetupFlow + override val isForceHidden: Flow = + connectivityRepository.forceHiddenSlots + .map { it.contains(ConnectivitySlot.MOBILE) } + .stateIn(scope, SharingStarted.WhileSubscribed(), false) + /** Vends out new [MobileIconInteractor] for a particular subId */ override fun createMobileConnectionInteractorForSubId(subId: Int): MobileIconInteractor = MobileIconInteractorImpl( @@ -303,6 +315,7 @@ constructor( defaultMobileIconGroup, defaultDataSubId, isDefaultConnectionFailed, + isForceHidden, mobileConnectionsRepo.getRepoForSubId(subId), ) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt index 890de70f4959b..db585e68d185c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/binder/MobileIconBinder.kt @@ -91,6 +91,8 @@ object MobileIconBinder { } } + launch { viewModel.isVisible.collect { isVisible -> view.isVisible = isVisible } } + // Set the icon for the triangle launch { viewModel.icon.distinctUntilChanged().collect { icon -> @@ -153,8 +155,7 @@ object MobileIconBinder { return object : ModernStatusBarViewBinding { override fun getShouldIconBeVisible(): Boolean { - // If this view model exists, then the icon should be visible. - return true + return viewModel.isVisible.value } override fun onVisibilityStateChanged(@StatusBarIconView.VisibleState state: Int) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt index b584398b10dc5..049627899eff5 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModel.kt @@ -22,6 +22,7 @@ import com.android.settingslib.graph.SignalDrawable import com.android.systemui.common.shared.model.ContentDescription import com.android.systemui.common.shared.model.Icon import com.android.systemui.log.table.logDiffsForTable +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.MobileIconInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.MobileIconsInteractor import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel @@ -42,6 +43,8 @@ import kotlinx.coroutines.flow.stateIn /** Common interface for all of the location-based mobile icon view models. */ interface MobileIconViewModelCommon { val subscriptionId: Int + /** True if this view should be visible at all. */ + val isVisible: StateFlow val icon: Flow val contentDescription: Flow val roaming: Flow @@ -71,6 +74,7 @@ class MobileIconViewModel constructor( override val subscriptionId: Int, iconInteractor: MobileIconInteractor, + airplaneModeInteractor: AirplaneModeInteractor, constants: ConnectivityConstants, scope: CoroutineScope, ) : MobileIconViewModelCommon { @@ -78,6 +82,26 @@ constructor( private val showExclamationMark: Flow = iconInteractor.isDefaultDataEnabled.mapLatest { !it } + override val isVisible: StateFlow = + if (!constants.hasDataCapabilities) { + flowOf(false) + } else { + combine( + airplaneModeInteractor.isAirplaneMode, + iconInteractor.isForceHidden, + ) { isAirplaneMode, isForceHidden -> + !isAirplaneMode && !isForceHidden + } + } + .distinctUntilChanged() + .logDiffsForTable( + iconInteractor.tableLogBuffer, + columnPrefix = "", + columnName = "visible", + initialValue = false, + ) + .stateIn(scope, SharingStarted.WhileSubscribed(), false) + override val icon: Flow = run { val initial = SignalIconModel.createEmptyState(iconInteractor.numberOfLevels.value) combine( diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt index fa95e1fb51810..185b6685ba0e1 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModel.kt @@ -17,9 +17,11 @@ package com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel import androidx.annotation.VisibleForTesting +import com.android.systemui.dagger.SysUISingleton import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.statusbar.phone.StatusBarLocation import com.android.systemui.statusbar.pipeline.StatusBarPipelineFlags +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.MobileIconsInteractor import com.android.systemui.statusbar.pipeline.mobile.ui.view.ModernStatusBarMobileView import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants @@ -39,6 +41,7 @@ class MobileIconsViewModel constructor( val subscriptionIdsFlow: StateFlow>, private val interactor: MobileIconsInteractor, + private val airplaneModeInteractor: AirplaneModeInteractor, private val logger: ConnectivityPipelineLogger, private val constants: ConnectivityConstants, @Application private val scope: CoroutineScope, @@ -56,6 +59,7 @@ constructor( ?: MobileIconViewModel( subId, interactor.createMobileConnectionInteractorForSubId(subId), + airplaneModeInteractor, constants, scope, ) @@ -73,10 +77,12 @@ constructor( subIdsToRemove.forEach { mobileIconSubIdCache.remove(it) } } + @SysUISingleton class Factory @Inject constructor( private val interactor: MobileIconsInteractor, + private val airplaneModeInteractor: AirplaneModeInteractor, private val logger: ConnectivityPipelineLogger, private val constants: ConnectivityConstants, @Application private val scope: CoroutineScope, @@ -86,6 +92,7 @@ constructor( return MobileIconsViewModel( subscriptionIdsFlow, interactor, + airplaneModeInteractor, logger, constants, scope, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconInteractor.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconInteractor.kt index 7aeaa48165aa6..b9eda717dc1a5 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconInteractor.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconInteractor.kt @@ -71,6 +71,8 @@ class FakeMobileIconInteractor( private val _numberOfLevels = MutableStateFlow(DEFAULT_NUM_LEVELS) override val numberOfLevels = _numberOfLevels + override val isForceHidden = MutableStateFlow(false) + fun setIconGroup(group: SignalIcon.MobileIconGroup) { _iconGroup.value = group } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconsInteractor.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconsInteractor.kt index 172755cb8d615..2699316d1b0bf 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconsInteractor.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/FakeMobileIconsInteractor.kt @@ -73,6 +73,8 @@ class FakeMobileIconsInteractor( private val _isUserSetup = MutableStateFlow(true) override val isUserSetup = _isUserSetup + override val isForceHidden = MutableStateFlow(false) + /** Always returns a new fake interactor */ override fun createMobileConnectionInteractorForSubId(subId: Int): MobileIconInteractor { return FakeMobileIconInteractor(tableLogBuffer) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractorTest.kt index c42aba5a7dd9d..f87f651a24802 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconInteractorTest.kt @@ -66,6 +66,7 @@ class MobileIconInteractorTest : SysuiTestCase() { mobileIconsInteractor.defaultMobileIconGroup, mobileIconsInteractor.defaultDataSubId, mobileIconsInteractor.isDefaultConnectionFailed, + mobileIconsInteractor.isForceHidden, connectionRepository, ) } @@ -550,6 +551,21 @@ class MobileIconInteractorTest : SysuiTestCase() { job.cancel() } + @Test + fun isForceHidden_matchesParent() = + runBlocking(IMMEDIATE) { + var latest: Boolean? = null + val job = underTest.isForceHidden.onEach { latest = it }.launchIn(this) + + mobileIconsInteractor.isForceHidden.value = true + assertThat(latest).isTrue() + + mobileIconsInteractor.isForceHidden.value = false + assertThat(latest).isFalse() + + job.cancel() + } + companion object { private val IMMEDIATE = Dispatchers.Main.immediate diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt index 23b634c7d2b8a..f8a978300dd37 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt @@ -27,6 +27,8 @@ import com.android.systemui.statusbar.pipeline.mobile.data.repository.FakeMobile import com.android.systemui.statusbar.pipeline.mobile.data.repository.FakeMobileConnectionsRepository import com.android.systemui.statusbar.pipeline.mobile.data.repository.FakeUserSetupRepository import com.android.systemui.statusbar.pipeline.mobile.util.FakeMobileMappingsProxy +import com.android.systemui.statusbar.pipeline.shared.data.model.ConnectivitySlot +import com.android.systemui.statusbar.pipeline.shared.data.repository.FakeConnectivityRepository import com.android.systemui.util.CarrierConfigTracker import com.android.systemui.util.mockito.mock import com.android.systemui.util.mockito.whenever @@ -50,6 +52,7 @@ import org.mockito.MockitoAnnotations @SmallTest class MobileIconsInteractorTest : SysuiTestCase() { private lateinit var underTest: MobileIconsInteractor + private lateinit var connectivityRepository: FakeConnectivityRepository private lateinit var connectionsRepository: FakeMobileConnectionsRepository private val userSetupRepository = FakeUserSetupRepository() private val mobileMappingsProxy = FakeMobileMappingsProxy() @@ -63,6 +66,8 @@ class MobileIconsInteractorTest : SysuiTestCase() { fun setUp() { MockitoAnnotations.initMocks(this) + connectivityRepository = FakeConnectivityRepository() + connectionsRepository = FakeMobileConnectionsRepository(mobileMappingsProxy, tableLogBuffer) connectionsRepository.setMobileConnectionRepositoryMap( mapOf( @@ -80,6 +85,7 @@ class MobileIconsInteractorTest : SysuiTestCase() { carrierConfigTracker, logger = mock(), tableLogger = mock(), + connectivityRepository, userSetupRepository, testScope.backgroundScope, ) @@ -610,6 +616,32 @@ class MobileIconsInteractorTest : SysuiTestCase() { job.cancel() } + @Test + fun isForceHidden_repoHasMobileHidden_true() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isForceHidden.onEach { latest = it }.launchIn(this) + + connectivityRepository.setForceHiddenIcons(setOf(ConnectivitySlot.MOBILE)) + + assertThat(latest).isTrue() + + job.cancel() + } + + @Test + fun isForceHidden_repoDoesNotHaveMobileHidden_false() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isForceHidden.onEach { latest = it }.launchIn(this) + + connectivityRepository.setForceHiddenIcons(setOf(ConnectivitySlot.WIFI)) + + assertThat(latest).isFalse() + + job.cancel() + } + companion object { private val tableLogBuffer = TableLogBuffer(8, "MobileIconsInteractorTest", FakeSystemClock()) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt index 39f77c9c75284..e68a3970ae93a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/view/ModernStatusBarMobileViewTest.kt @@ -29,11 +29,14 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.statusbar.StatusBarIconView import com.android.systemui.statusbar.pipeline.StatusBarPipelineFlags +import com.android.systemui.statusbar.pipeline.airplane.data.repository.FakeAirplaneModeRepository +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconInteractor import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.LocationBasedMobileViewModel import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.MobileIconViewModel import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.QsMobileIconViewModel import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants +import com.android.systemui.statusbar.pipeline.shared.data.repository.FakeConnectivityRepository import com.android.systemui.util.mockito.whenever import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -58,28 +61,36 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { @Mock private lateinit var statusBarPipelineFlags: StatusBarPipelineFlags @Mock private lateinit var tableLogBuffer: TableLogBuffer @Mock private lateinit var constants: ConnectivityConstants + private lateinit var interactor: FakeMobileIconInteractor + private lateinit var airplaneModeRepository: FakeAirplaneModeRepository + private lateinit var airplaneModeInteractor: AirplaneModeInteractor + private lateinit var viewModelCommon: MobileIconViewModel private lateinit var viewModel: LocationBasedMobileViewModel @Before fun setUp() { MockitoAnnotations.initMocks(this) + // This line was necessary to make the onDarkChanged and setStaticDrawableColor tests pass. + // But, it maybe *shouldn't* be necessary. + whenever(constants.hasDataCapabilities).thenReturn(true) + testableLooper = TestableLooper.get(this) - val interactor = FakeMobileIconInteractor(tableLogBuffer) - - val viewModelCommon = - MobileIconViewModel( - subscriptionId = 1, - interactor, - constants, - testScope.backgroundScope, + airplaneModeRepository = FakeAirplaneModeRepository() + airplaneModeInteractor = + AirplaneModeInteractor( + airplaneModeRepository, + FakeConnectivityRepository(), ) - viewModel = QsMobileIconViewModel(viewModelCommon, statusBarPipelineFlags) + + interactor = FakeMobileIconInteractor(tableLogBuffer) + createViewModel() } // Note: The following tests are more like integration tests, since they stand up a full - // [WifiViewModel] and test the interactions between the view, view-binder, and view-model. + // [MobileIconViewModel] and test the interactions between the view, view-binder, and + // view-model. @Test fun setVisibleState_icon_iconShownDotHidden() { @@ -127,7 +138,25 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { } @Test - fun isIconVisible_alwaysTrue() { + fun isIconVisible_noData_outputsFalse() { + whenever(constants.hasDataCapabilities).thenReturn(false) + createViewModel() + + val view = ModernStatusBarMobileView.constructAndBind(context, SLOT_NAME, viewModel) + + ViewUtils.attachView(view) + testableLooper.processAllMessages() + + assertThat(view.isIconVisible).isFalse() + + ViewUtils.detachView(view) + } + + @Test + fun isIconVisible_hasData_outputsTrue() { + whenever(constants.hasDataCapabilities).thenReturn(true) + createViewModel() + val view = ModernStatusBarMobileView.constructAndBind(context, SLOT_NAME, viewModel) ViewUtils.attachView(view) @@ -138,6 +167,34 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { ViewUtils.detachView(view) } + @Test + fun isIconVisible_notAirplaneMode_outputsTrue() { + airplaneModeRepository.setIsAirplaneMode(false) + + val view = ModernStatusBarMobileView.constructAndBind(context, SLOT_NAME, viewModel) + + ViewUtils.attachView(view) + testableLooper.processAllMessages() + + assertThat(view.isIconVisible).isTrue() + + ViewUtils.detachView(view) + } + + @Test + fun isIconVisible_airplaneMode_outputsTrue() { + airplaneModeRepository.setIsAirplaneMode(true) + + val view = ModernStatusBarMobileView.constructAndBind(context, SLOT_NAME, viewModel) + + ViewUtils.attachView(view) + testableLooper.processAllMessages() + + assertThat(view.isIconVisible).isFalse() + + ViewUtils.detachView(view) + } + @Test fun onDarkChanged_iconHasNewColor() { whenever(statusBarPipelineFlags.useDebugColoring()).thenReturn(false) @@ -181,6 +238,18 @@ class ModernStatusBarMobileViewTest : SysuiTestCase() { private fun View.getDotView(): View { return this.requireViewById(R.id.status_bar_dot) } + + private fun createViewModel() { + viewModelCommon = + MobileIconViewModel( + subscriptionId = 1, + interactor, + airplaneModeInteractor, + constants, + testScope.backgroundScope, + ) + viewModel = QsMobileIconViewModel(viewModelCommon, statusBarPipelineFlags) + } } private const val SLOT_NAME = "TestSlotName" diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt index 4f36d6396fe0b..f9830309252d2 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/LocationBasedMobileIconViewModelTest.kt @@ -21,10 +21,13 @@ import com.android.settingslib.mobile.TelephonyIcons import com.android.systemui.SysuiTestCase import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.statusbar.pipeline.StatusBarPipelineFlags +import com.android.systemui.statusbar.pipeline.airplane.data.repository.FakeAirplaneModeRepository +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconInteractor import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel import com.android.systemui.statusbar.pipeline.mobile.ui.viewmodel.MobileIconViewModelTest.Companion.defaultSignal import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants +import com.android.systemui.statusbar.pipeline.shared.data.repository.FakeConnectivityRepository import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.launchIn @@ -46,6 +49,7 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { private lateinit var qsIcon: QsMobileIconViewModel private lateinit var keyguardIcon: KeyguardMobileIconViewModel private lateinit var interactor: FakeMobileIconInteractor + private lateinit var airplaneModeInteractor: AirplaneModeInteractor @Mock private lateinit var statusBarPipelineFlags: StatusBarPipelineFlags @Mock private lateinit var constants: ConnectivityConstants @Mock private lateinit var tableLogBuffer: TableLogBuffer @@ -56,6 +60,11 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { @Before fun setUp() { MockitoAnnotations.initMocks(this) + airplaneModeInteractor = + AirplaneModeInteractor( + FakeAirplaneModeRepository(), + FakeConnectivityRepository(), + ) interactor = FakeMobileIconInteractor(tableLogBuffer) interactor.apply { setLevel(1) @@ -66,7 +75,14 @@ class LocationBasedMobileIconViewModelTest : SysuiTestCase() { setNumberOfLevels(4) isDataConnected.value = true } - commonImpl = MobileIconViewModel(SUB_1_ID, interactor, constants, testScope.backgroundScope) + commonImpl = + MobileIconViewModel( + SUB_1_ID, + interactor, + airplaneModeInteractor, + constants, + testScope.backgroundScope, + ) homeIcon = HomeMobileIconViewModel(commonImpl, statusBarPipelineFlags) qsIcon = QsMobileIconViewModel(commonImpl, statusBarPipelineFlags) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt index 83c902f031ac6..bec276a9c68fc 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconViewModelTest.kt @@ -24,10 +24,13 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.common.shared.model.ContentDescription import com.android.systemui.common.shared.model.Icon import com.android.systemui.log.table.TableLogBuffer +import com.android.systemui.statusbar.pipeline.airplane.data.repository.FakeAirplaneModeRepository +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconInteractor import com.android.systemui.statusbar.pipeline.mobile.ui.model.SignalIconModel import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants import com.android.systemui.statusbar.pipeline.shared.data.model.DataActivityModel +import com.android.systemui.statusbar.pipeline.shared.data.repository.FakeConnectivityRepository import com.android.systemui.util.mockito.whenever import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -48,6 +51,8 @@ import org.mockito.MockitoAnnotations class MobileIconViewModelTest : SysuiTestCase() { private lateinit var underTest: MobileIconViewModel private lateinit var interactor: FakeMobileIconInteractor + private lateinit var airplaneModeRepository: FakeAirplaneModeRepository + private lateinit var airplaneModeInteractor: AirplaneModeInteractor @Mock private lateinit var constants: ConnectivityConstants @Mock private lateinit var tableLogBuffer: TableLogBuffer @@ -57,6 +62,15 @@ class MobileIconViewModelTest : SysuiTestCase() { @Before fun setUp() { MockitoAnnotations.initMocks(this) + whenever(constants.hasDataCapabilities).thenReturn(true) + + airplaneModeRepository = FakeAirplaneModeRepository() + airplaneModeInteractor = + AirplaneModeInteractor( + airplaneModeRepository, + FakeConnectivityRepository(), + ) + interactor = FakeMobileIconInteractor(tableLogBuffer) interactor.apply { setLevel(1) @@ -67,9 +81,89 @@ class MobileIconViewModelTest : SysuiTestCase() { setNumberOfLevels(4) isDataConnected.value = true } - underTest = MobileIconViewModel(SUB_1_ID, interactor, constants, testScope.backgroundScope) + createAndSetViewModel() } + @Test + fun isVisible_notDataCapable_alwaysFalse() = + testScope.runTest { + // Create a new view model here so the constants are properly read + whenever(constants.hasDataCapabilities).thenReturn(false) + createAndSetViewModel() + + var latest: Boolean? = null + val job = underTest.isVisible.onEach { latest = it }.launchIn(this) + + assertThat(latest).isFalse() + + job.cancel() + } + + @Test + fun isVisible_notAirplane_notForceHidden_true() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isVisible.onEach { latest = it }.launchIn(this) + + airplaneModeRepository.setIsAirplaneMode(false) + interactor.isForceHidden.value = false + + assertThat(latest).isTrue() + + job.cancel() + } + + @Test + fun isVisible_airplane_false() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isVisible.onEach { latest = it }.launchIn(this) + + airplaneModeRepository.setIsAirplaneMode(true) + interactor.isForceHidden.value = false + + assertThat(latest).isFalse() + + job.cancel() + } + + @Test + fun isVisible_forceHidden_false() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isVisible.onEach { latest = it }.launchIn(this) + + airplaneModeRepository.setIsAirplaneMode(false) + interactor.isForceHidden.value = true + + assertThat(latest).isFalse() + + job.cancel() + } + + @Test + fun isVisible_respondsToUpdates() = + testScope.runTest { + var latest: Boolean? = null + val job = underTest.isVisible.onEach { latest = it }.launchIn(this) + + airplaneModeRepository.setIsAirplaneMode(false) + interactor.isForceHidden.value = false + + assertThat(latest).isTrue() + + airplaneModeRepository.setIsAirplaneMode(true) + assertThat(latest).isFalse() + + airplaneModeRepository.setIsAirplaneMode(false) + assertThat(latest).isTrue() + + interactor.isForceHidden.value = true + assertThat(latest).isFalse() + + job.cancel() + } + @Test fun iconId_correctLevel_notCutout() = testScope.runTest { @@ -361,13 +455,7 @@ class MobileIconViewModelTest : SysuiTestCase() { testScope.runTest { // Create a new view model here so the constants are properly read whenever(constants.shouldShowActivityConfig).thenReturn(false) - underTest = - MobileIconViewModel( - SUB_1_ID, - interactor, - constants, - testScope.backgroundScope, - ) + createAndSetViewModel() var inVisible: Boolean? = null val inJob = underTest.activityInVisible.onEach { inVisible = it }.launchIn(this) @@ -399,13 +487,7 @@ class MobileIconViewModelTest : SysuiTestCase() { testScope.runTest { // Create a new view model here so the constants are properly read whenever(constants.shouldShowActivityConfig).thenReturn(true) - underTest = - MobileIconViewModel( - SUB_1_ID, - interactor, - constants, - testScope.backgroundScope, - ) + createAndSetViewModel() var inVisible: Boolean? = null val inJob = underTest.activityInVisible.onEach { inVisible = it }.launchIn(this) @@ -454,6 +536,16 @@ class MobileIconViewModelTest : SysuiTestCase() { containerJob.cancel() } + private fun createAndSetViewModel() { + underTest = MobileIconViewModel( + SUB_1_ID, + interactor, + airplaneModeInteractor, + constants, + testScope.backgroundScope, + ) + } + companion object { private const val SUB_1_ID = 1 diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModelTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModelTest.kt index 58b50c7e7e6d7..d9268a2c3b94a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModelTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/ui/viewmodel/MobileIconsViewModelTest.kt @@ -20,11 +20,14 @@ import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.statusbar.phone.StatusBarLocation import com.android.systemui.statusbar.pipeline.StatusBarPipelineFlags +import com.android.systemui.statusbar.pipeline.airplane.data.repository.FakeAirplaneModeRepository +import com.android.systemui.statusbar.pipeline.airplane.domain.interactor.AirplaneModeInteractor import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.domain.interactor.FakeMobileIconsInteractor import com.android.systemui.statusbar.pipeline.mobile.util.FakeMobileMappingsProxy import com.android.systemui.statusbar.pipeline.shared.ConnectivityConstants import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger +import com.android.systemui.statusbar.pipeline.shared.data.repository.FakeConnectivityRepository import com.android.systemui.util.mockito.mock import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -46,6 +49,7 @@ class MobileIconsViewModelTest : SysuiTestCase() { private lateinit var underTest: MobileIconsViewModel private val interactor = FakeMobileIconsInteractor(FakeMobileMappingsProxy(), mock()) + private lateinit var airplaneModeInteractor: AirplaneModeInteractor @Mock private lateinit var statusBarPipelineFlags: StatusBarPipelineFlags @Mock private lateinit var logger: ConnectivityPipelineLogger @Mock private lateinit var constants: ConnectivityConstants @@ -57,6 +61,12 @@ class MobileIconsViewModelTest : SysuiTestCase() { fun setUp() { MockitoAnnotations.initMocks(this) + airplaneModeInteractor = + AirplaneModeInteractor( + FakeAirplaneModeRepository(), + FakeConnectivityRepository(), + ) + val subscriptionIdsFlow = interactor.filteredSubscriptions .map { subs -> subs.map { it.subscriptionId } } @@ -66,6 +76,7 @@ class MobileIconsViewModelTest : SysuiTestCase() { MobileIconsViewModel( subscriptionIdsFlow, interactor, + airplaneModeInteractor, logger, constants, testScope.backgroundScope,