From 83e7763535c136d2239b40b112e9b0fe63fe00f0 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Fri, 3 Feb 2023 19:09:35 +0000 Subject: [PATCH] [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) } }