From 93b945ce54f1e61d75e3cf9cef4a4083f03c5e24 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Wed, 26 Oct 2022 16:04:08 -0400 Subject: [PATCH] [SB refactor] Upgrade most data sources to `StateFlow` Most of the data model for mobile is more correctly represented as a StateFlow, since the state from callbacks needs to persist even when there is no corrent collector on the mobile icon. This change also exposes a current issue: toggling mobile data on/off multiple times can introduce an inconsistency in the UI. This is approximately the same as the old pipeline, which also experiences the same problem. Test: manual Test: atest packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/* Bug: 240492102 Change-Id: Idd5fa0205380b4ba15f7d723b32b08e114456f7f --- .../repository/MobileConnectionRepository.kt | 27 ++++-- .../repository/MobileConnectionsRepository.kt | 2 +- .../data/repository/UserSetupRepository.kt | 3 +- .../domain/interactor/MobileIconInteractor.kt | 88 +++++++++++-------- .../interactor/MobileIconsInteractor.kt | 39 ++++---- .../FakeMobileConnectionRepository.kt | 3 +- .../repository/FakeUserSetupRepository.kt | 3 +- .../MobileConnectionRepositoryTest.kt | 20 +++-- .../interactor/MobileIconInteractorTest.kt | 4 + 9 files changed, 112 insertions(+), 77 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepository.kt index 41bcab1be2d59..581842bc2f579 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepository.kt @@ -47,10 +47,12 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.asExecutor import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow -import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.merge +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn /** @@ -71,12 +73,12 @@ interface MobileConnectionRepository { */ val subscriptionModelFlow: Flow /** Observable tracking [TelephonyManager.isDataConnectionAllowed] */ - val dataEnabled: Flow + val dataEnabled: StateFlow /** * True if this connection represents the default subscription per * [SubscriptionManager.getDefaultDataSubscriptionId] */ - val isDefaultDataSubscription: Flow + val isDefaultDataSubscription: StateFlow } @Suppress("EXPERIMENTAL_IS_NOT_ENABLED") @@ -86,7 +88,7 @@ class MobileConnectionRepositoryImpl( private val subId: Int, private val telephonyManager: TelephonyManager, private val globalSettings: GlobalSettings, - defaultDataSubId: Flow, + defaultDataSubId: StateFlow, globalMobileDataSettingChangedEvent: Flow, bgDispatcher: CoroutineDispatcher, logger: ConnectivityPipelineLogger, @@ -101,6 +103,8 @@ class MobileConnectionRepositoryImpl( } } + private val telephonyCallbackEvent = MutableSharedFlow(extraBufferCapacity = 1) + override val subscriptionModelFlow: StateFlow = run { var state = MobileSubscriptionModel() conflatedCallbackFlow { @@ -180,12 +184,11 @@ class MobileConnectionRepositoryImpl( telephonyManager.registerTelephonyCallback(bgDispatcher.asExecutor(), callback) awaitClose { telephonyManager.unregisterTelephonyCallback(callback) } } + .onEach { telephonyCallbackEvent.tryEmit(Unit) } .logOutputChange(logger, "MobileSubscriptionModel") .stateIn(scope, SharingStarted.WhileSubscribed(), state) } - private val telephonyCallbackEvent = subscriptionModelFlow.map {} - /** Produces whenever the mobile data setting changes for this subId */ private val localMobileDataSettingChangedEvent: Flow = conflatedCallbackFlow { val observer = @@ -216,11 +219,17 @@ class MobileConnectionRepositoryImpl( globalMobileDataSettingChangedEvent, ) - override val dataEnabled: Flow = telephonyPollingEvent.map { dataConnectionAllowed() } + override val dataEnabled: StateFlow = + telephonyPollingEvent + .mapLatest { dataConnectionAllowed() } + .stateIn(scope, SharingStarted.WhileSubscribed(), dataConnectionAllowed()) private fun dataConnectionAllowed(): Boolean = telephonyManager.isDataConnectionAllowed - override val isDefaultDataSubscription: Flow = defaultDataSubId.map { it == subId } + override val isDefaultDataSubscription: StateFlow = + defaultDataSubId + .mapLatest { it == subId } + .stateIn(scope, SharingStarted.WhileSubscribed(), defaultDataSubId.value == subId) class Factory @Inject @@ -234,7 +243,7 @@ class MobileConnectionRepositoryImpl( ) { fun build( subId: Int, - defaultDataSubId: Flow, + defaultDataSubId: StateFlow, globalMobileDataSettingChangedEvent: Flow, ): MobileConnectionRepository { return MobileConnectionRepositoryImpl( diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionsRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionsRepository.kt index bd9f85ee9dcbb..89edc8a873b96 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionsRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionsRepository.kt @@ -65,7 +65,7 @@ interface MobileConnectionsRepository { val subscriptionsFlow: Flow> /** Observable for the subscriptionId of the current mobile data connection */ - val activeMobileDataSubscriptionId: Flow + val activeMobileDataSubscriptionId: StateFlow /** Observable for [MobileMappings.Config] tracking the defaults */ val defaultDataSubRatConfig: StateFlow diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/UserSetupRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/UserSetupRepository.kt index 77de849691db3..91886bb121d5f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/UserSetupRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/UserSetupRepository.kt @@ -26,7 +26,6 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.awaitClose -import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.mapLatest @@ -40,7 +39,7 @@ import kotlinx.coroutines.withContext */ interface UserSetupRepository { /** Observable tracking [DeviceProvisionedController.isUserSetup] */ - val isUserSetupFlow: Flow + val isUserSetupFlow: StateFlow } @Suppress("EXPERIMENTAL_IS_NOT_ENABLED") 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 cdd4982f0a791..dd8a91fe58ed4 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 @@ -18,81 +18,97 @@ package com.android.systemui.statusbar.pipeline.mobile.domain.interactor import android.telephony.CarrierConfigManager import com.android.settingslib.SignalIcon.MobileIconGroup +import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.statusbar.pipeline.mobile.data.model.DefaultNetworkType import com.android.systemui.statusbar.pipeline.mobile.data.model.OverrideNetworkType import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionRepository import com.android.systemui.statusbar.pipeline.mobile.util.MobileMappingsProxy import com.android.systemui.util.CarrierConfigTracker +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.flowOf -import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.mapLatest +import kotlinx.coroutines.flow.stateIn interface MobileIconInteractor { // TODO(b/256839546): clarify naming of default vs active /** True if we want to consider the data connection enabled */ - val isDefaultDataEnabled: Flow + val isDefaultDataEnabled: StateFlow /** Observable for the data enabled state of this connection */ - val isDataEnabled: Flow + val isDataEnabled: StateFlow /** Observable for RAT type (network type) indicator */ - val networkTypeIconGroup: Flow + val networkTypeIconGroup: StateFlow /** True if this line of service is emergency-only */ - val isEmergencyOnly: Flow + val isEmergencyOnly: StateFlow /** Int describing the connection strength. 0-4 OR 1-5. See [numberOfLevels] */ - val level: Flow + val level: StateFlow /** Based on [CarrierConfigManager.KEY_INFLATE_SIGNAL_STRENGTH_BOOL], either 4 or 5 */ - val numberOfLevels: Flow + val numberOfLevels: StateFlow } /** Interactor for a single mobile connection. This connection _should_ have one subscription ID */ +@Suppress("EXPERIMENTAL_IS_NOT_ENABLED") +@OptIn(ExperimentalCoroutinesApi::class) class MobileIconInteractorImpl( - defaultSubscriptionHasDataEnabled: Flow, - defaultMobileIconMapping: Flow>, - defaultMobileIconGroup: Flow, + @Application scope: CoroutineScope, + defaultSubscriptionHasDataEnabled: StateFlow, + defaultMobileIconMapping: StateFlow>, + defaultMobileIconGroup: StateFlow, mobileMappingsProxy: MobileMappingsProxy, connectionRepository: MobileConnectionRepository, ) : MobileIconInteractor { private val mobileStatusInfo = connectionRepository.subscriptionModelFlow - override val isDataEnabled: Flow = connectionRepository.dataEnabled + override val isDataEnabled: StateFlow = connectionRepository.dataEnabled override val isDefaultDataEnabled = defaultSubscriptionHasDataEnabled /** Observable for the current RAT indicator icon ([MobileIconGroup]) */ - override val networkTypeIconGroup: Flow = + override val networkTypeIconGroup: StateFlow = combine( - mobileStatusInfo, - defaultMobileIconMapping, - defaultMobileIconGroup, - ) { info, mapping, defaultGroup -> - val lookupKey = - when (val resolved = info.resolvedNetworkType) { - is DefaultNetworkType -> mobileMappingsProxy.toIconKey(resolved.type) - is OverrideNetworkType -> mobileMappingsProxy.toIconKeyOverride(resolved.type) - } - mapping[lookupKey] ?: defaultGroup - } - - override val isEmergencyOnly: Flow = mobileStatusInfo.map { it.isEmergencyOnly } - - override val level: Flow = - mobileStatusInfo.map { mobileModel -> - // TODO: incorporate [MobileMappings.Config.alwaysShowCdmaRssi] - if (mobileModel.isGsm) { - mobileModel.primaryLevel - } else { - mobileModel.cdmaLevel + mobileStatusInfo, + defaultMobileIconMapping, + defaultMobileIconGroup, + ) { info, mapping, defaultGroup -> + val lookupKey = + when (val resolved = info.resolvedNetworkType) { + is DefaultNetworkType -> mobileMappingsProxy.toIconKey(resolved.type) + is OverrideNetworkType -> + mobileMappingsProxy.toIconKeyOverride(resolved.type) + } + mapping[lookupKey] ?: defaultGroup } - } + .stateIn(scope, SharingStarted.WhileSubscribed(), defaultMobileIconGroup.value) + + override val isEmergencyOnly: StateFlow = + mobileStatusInfo + .mapLatest { it.isEmergencyOnly } + .stateIn(scope, SharingStarted.WhileSubscribed(), false) + + override val level: StateFlow = + mobileStatusInfo + .mapLatest { mobileModel -> + // TODO: incorporate [MobileMappings.Config.alwaysShowCdmaRssi] + if (mobileModel.isGsm) { + mobileModel.primaryLevel + } else { + mobileModel.cdmaLevel + } + } + .stateIn(scope, SharingStarted.WhileSubscribed(), 0) /** * This will become variable based on [CarrierConfigManager.KEY_INFLATE_SIGNAL_STRENGTH_BOOL] * once it's wired up inside of [CarrierConfigTracker] */ - override val numberOfLevels: Flow = flowOf(4) + override val numberOfLevels: StateFlow = MutableStateFlow(4) } 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 31cb21f7720f7..9d30ec05aad31 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 @@ -38,7 +38,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf -import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.stateIn /** @@ -55,13 +55,13 @@ interface MobileIconsInteractor { /** List of subscriptions, potentially filtered for CBRS */ val filteredSubscriptions: Flow> /** True if the active mobile data subscription has data enabled */ - val activeDataConnectionHasDataEnabled: Flow + val activeDataConnectionHasDataEnabled: StateFlow /** The icon mapping from network type to [MobileIconGroup] for the default subscription */ - val defaultMobileIconMapping: Flow> + val defaultMobileIconMapping: StateFlow> /** Fallback [MobileIconGroup] in the case where there is no icon in the mapping */ - val defaultMobileIconGroup: Flow + val defaultMobileIconGroup: StateFlow /** True once the user has been set up */ - val isUserSetup: Flow + val isUserSetup: StateFlow /** * Vends out a [MobileIconInteractor] tracking the [MobileConnectionRepository] for the given * subId. Will throw if the ID is invalid @@ -84,17 +84,21 @@ constructor( private val activeMobileDataSubscriptionId = mobileConnectionsRepo.activeMobileDataSubscriptionId - private val activeMobileDataConnectionRepo: Flow = - activeMobileDataSubscriptionId.map { activeId -> - if (activeId == INVALID_SUBSCRIPTION_ID) { - null - } else { - mobileConnectionsRepo.getRepoForSubId(activeId) + private val activeMobileDataConnectionRepo: StateFlow = + activeMobileDataSubscriptionId + .mapLatest { activeId -> + if (activeId == INVALID_SUBSCRIPTION_ID) { + null + } else { + mobileConnectionsRepo.getRepoForSubId(activeId) + } } - } + .stateIn(scope, SharingStarted.WhileSubscribed(), null) - override val activeDataConnectionHasDataEnabled: Flow = - activeMobileDataConnectionRepo.flatMapLatest { it?.dataEnabled ?: flowOf(false) } + override val activeDataConnectionHasDataEnabled: StateFlow = + activeMobileDataConnectionRepo + .flatMapLatest { it?.dataEnabled ?: flowOf(false) } + .stateIn(scope, SharingStarted.WhileSubscribed(), false) private val unfilteredSubscriptions: Flow> = mobileConnectionsRepo.subscriptionsFlow @@ -149,20 +153,21 @@ constructor( */ override val defaultMobileIconMapping: StateFlow> = mobileConnectionsRepo.defaultDataSubRatConfig - .map { mobileMappingsProxy.mapIconSets(it) } + .mapLatest { mobileMappingsProxy.mapIconSets(it) } .stateIn(scope, SharingStarted.WhileSubscribed(), initialValue = mapOf()) /** If there is no mapping in [defaultMobileIconMapping], then use this default icon group */ override val defaultMobileIconGroup: StateFlow = mobileConnectionsRepo.defaultDataSubRatConfig - .map { mobileMappingsProxy.getDefaultIcons(it) } + .mapLatest { mobileMappingsProxy.getDefaultIcons(it) } .stateIn(scope, SharingStarted.WhileSubscribed(), initialValue = TelephonyIcons.G) - override val isUserSetup: Flow = userSetupRepo.isUserSetupFlow + override val isUserSetup: StateFlow = userSetupRepo.isUserSetupFlow /** Vends out new [MobileIconInteractor] for a particular subId */ override fun createMobileConnectionInteractorForSubId(subId: Int): MobileIconInteractor = MobileIconInteractorImpl( + scope, activeDataConnectionHasDataEnabled, defaultMobileIconMapping, defaultMobileIconGroup, diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionRepository.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionRepository.kt index 4c7f7eab981d3..288f54c7d03c0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionRepository.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionRepository.kt @@ -17,12 +17,11 @@ package com.android.systemui.statusbar.pipeline.mobile.data.repository import com.android.systemui.statusbar.pipeline.mobile.data.model.MobileSubscriptionModel -import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow class FakeMobileConnectionRepository : MobileConnectionRepository { private val _subscriptionsModelFlow = MutableStateFlow(MobileSubscriptionModel()) - override val subscriptionModelFlow: Flow = _subscriptionsModelFlow + override val subscriptionModelFlow = _subscriptionsModelFlow private val _dataEnabled = MutableStateFlow(true) override val dataEnabled = _dataEnabled diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeUserSetupRepository.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeUserSetupRepository.kt index 6c495c5c705af..141b50c017e11 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeUserSetupRepository.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeUserSetupRepository.kt @@ -16,13 +16,12 @@ package com.android.systemui.statusbar.pipeline.mobile.data.repository -import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow /** Defaults to `true` */ class FakeUserSetupRepository : UserSetupRepository { private val _isUserSetup: MutableStateFlow = MutableStateFlow(true) - override val isUserSetupFlow: Flow = _isUserSetup + override val isUserSetupFlow = _isUserSetup fun setUserSetup(setup: Boolean) { _isUserSetup.value = setup diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepositoryTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepositoryTest.kt index ed3eda5beaf35..5ce51bb62c78e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepositoryTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileConnectionRepositoryTest.kt @@ -300,14 +300,20 @@ class MobileConnectionRepositoryTest : SysuiTestCase() { } @Test - fun dataEnabled_isEnabled() = + fun dataEnabled_initial_false() = runBlocking(IMMEDIATE) { whenever(telephonyManager.isDataConnectionAllowed).thenReturn(true) - var latest: Boolean? = null - val job = underTest.dataEnabled.onEach { latest = it }.launchIn(this) + assertThat(underTest.dataEnabled.value).isFalse() + } - assertThat(latest).isTrue() + @Test + fun dataEnabled_isEnabled_true() = + runBlocking(IMMEDIATE) { + whenever(telephonyManager.isDataConnectionAllowed).thenReturn(true) + val job = underTest.dataEnabled.launchIn(this) + + assertThat(underTest.dataEnabled.value).isTrue() job.cancel() } @@ -316,11 +322,9 @@ class MobileConnectionRepositoryTest : SysuiTestCase() { fun dataEnabled_isDisabled() = runBlocking(IMMEDIATE) { whenever(telephonyManager.isDataConnectionAllowed).thenReturn(false) + val job = underTest.dataEnabled.launchIn(this) - var latest: Boolean? = null - val job = underTest.dataEnabled.onEach { latest = it }.launchIn(this) - - assertThat(latest).isFalse() + assertThat(underTest.dataEnabled.value).isFalse() job.cancel() } 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 4d6ed2fc7493b..8dd7ef88a0fb6 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 @@ -34,6 +34,7 @@ import com.android.systemui.statusbar.pipeline.mobile.util.FakeMobileMappingsPro import com.android.systemui.util.mockito.mock import com.android.systemui.util.mockito.whenever import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach @@ -49,10 +50,13 @@ class MobileIconInteractorTest : SysuiTestCase() { private val mobileIconsInteractor = FakeMobileIconsInteractor(mobileMappingsProxy) private val connectionRepository = FakeMobileConnectionRepository() + private val scope = CoroutineScope(IMMEDIATE) + @Before fun setUp() { underTest = MobileIconInteractorImpl( + scope, mobileIconsInteractor.activeDataConnectionHasDataEnabled, mobileIconsInteractor.defaultMobileIconMapping, mobileIconsInteractor.defaultMobileIconGroup,