From 7f894afe510888414cd45277ae7789ac2f7e6c17 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Tue, 10 Jan 2023 15:46:56 -0500 Subject: [PATCH] [Sb refactor] Implement 2s grace period for data switching The old mobile pipeline supported this particular use case. Given that there are 2 mobile subscriptions, A and B, then when data switches from A to B, we will consider B to be validated (see NetworkCapabilities) for up to 2 seconds under the conditions: 1. A was validated before the switch 2. A and B are both in the same group (i.e. they have the same group UUID) This CL implements the same exact criteria by monitoring for data switching in the same group and maintaining a validated state for 2 seconds for the default subscription. Test: MobileIconsInteractorTest Bug: 238425913 Change-Id: I8e798197beda562634c137377145dea510cbda4c --- .../repository/MobileConnectionsRepository.kt | 6 + .../repository/MobileRepositorySwitcher.kt | 3 + .../demo/DemoMobileConnectionsRepository.kt | 4 + .../prod/MobileConnectionsRepositoryImpl.kt | 32 +++ .../interactor/MobileIconsInteractor.kt | 42 ++- .../FakeMobileConnectionsRepository.kt | 2 + .../interactor/MobileIconsInteractorTest.kt | 254 ++++++++++++++++++ 7 files changed, 342 insertions(+), 1 deletion(-) 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 97b4c2cadbe58..828bb43bb05dd 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 @@ -37,6 +37,12 @@ interface MobileConnectionsRepository { /** Observable for the subscriptionId of the current mobile data connection */ val activeMobileDataSubscriptionId: StateFlow + /** + * Observable event for when the active data sim switches but the group stays the same. E.g., + * CBRS switching would trigger this + */ + val activeSubChangedInGroupEvent: Flow + /** The current connectivity status for the default mobile network connection */ val defaultMobileNetworkConnectivity: StateFlow diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcher.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcher.kt index 0c8593d60cf5a..164da3b390a72 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcher.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/MobileRepositorySwitcher.kt @@ -124,6 +124,9 @@ constructor( realRepository.activeMobileDataSubscriptionId.value ) + override val activeSubChangedInGroupEvent: Flow = + activeRepo.flatMapLatest { it.activeSubChangedInGroupEvent } + override val defaultDataSubRatConfig: StateFlow = activeRepo .flatMapLatest { it.defaultDataSubRatConfig } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt index 22aca0a8b0d7a..31a09c6449826 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/demo/DemoMobileConnectionsRepository.kt @@ -48,6 +48,7 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted @@ -120,6 +121,9 @@ constructor( subscriptions.value.firstOrNull()?.subscriptionId ?: INVALID_SUBSCRIPTION_ID ) + // TODO(b/261029387): consider adding a demo command for this + override val activeSubChangedInGroupEvent: Flow = flowOf() + /** Demo mode doesn't currently support modifications to the mobile mappings */ override val defaultDataSubRatConfig = MutableStateFlow(MobileMappings.Config.readConfig(context)) 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 4472e0972a0b0..29affcf381448 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 @@ -52,6 +52,7 @@ import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger.Companion.logInputChange import com.android.systemui.statusbar.pipeline.wifi.data.model.WifiNetworkModel import com.android.systemui.statusbar.pipeline.wifi.data.repository.WifiRepository +import com.android.systemui.util.kotlin.pairwiseBy import com.android.systemui.util.settings.GlobalSettings import javax.inject.Inject import kotlinx.coroutines.CoroutineDispatcher @@ -63,6 +64,8 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.merge @@ -258,6 +261,35 @@ constructor( .logInputChange(logger, "defaultMobileNetworkConnectivity") .stateIn(scope, SharingStarted.WhileSubscribed(), MobileConnectivityModel()) + /** + * Flow that tracks the active mobile data subscriptions. Emits `true` whenever the active data + * subscription Id changes but the subscription group remains the same. In these cases, we want + * to retain the previous subscription's validation status for up to 2s to avoid flickering the + * icon. + * + * TODO(b/265164432): we should probably expose all change events, not just same group + */ + @SuppressLint("MissingPermission") + override val activeSubChangedInGroupEvent = + flow { + activeMobileDataSubscriptionId.pairwiseBy { prevVal: Int, newVal: Int -> + if (!defaultMobileNetworkConnectivity.value.isValidated) { + return@pairwiseBy + } + val prevSub = subscriptionManager.getActiveSubscriptionInfo(prevVal) + val nextSub = subscriptionManager.getActiveSubscriptionInfo(newVal) + + if (prevSub == null || nextSub == null) { + return@pairwiseBy + } + + if (prevSub.groupUuid != null && prevSub.groupUuid == nextSub.groupUuid) { + emit(Unit) + } + } + } + .flowOn(bgDispatcher) + private fun isValidSubId(subId: Int): Boolean { subscriptions.value.forEach { if (it.subscriptionId == subId) { 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 96196e72b1031..b289315adb881 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,14 +32,17 @@ import com.android.systemui.util.CarrierConfigTracker import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.flow.transformLatest /** * Business layer logic for the set of mobile subscription icons. @@ -163,8 +166,45 @@ constructor( } } + /** + * Copied from the old pipeline. We maintain a 2s period of time where we will keep the + * validated bit from the old active network (A) while data is changing to the new one (B). + * + * This condition only applies if + * 1. A and B are in the same subscription group (e.c. for CBRS data switching) and + * 2. A was validated before the switch + * + * The goal of this is to minimize the flickering in the UI of the cellular indicator + */ + private val forcingCellularValidation = + mobileConnectionsRepo.activeSubChangedInGroupEvent + .filter { mobileConnectionsRepo.defaultMobileNetworkConnectivity.value.isValidated } + .transformLatest { + emit(true) + delay(2000) + emit(false) + } + .stateIn(scope, SharingStarted.WhileSubscribed(), false) + override val defaultMobileNetworkConnectivity: StateFlow = - mobileConnectionsRepo.defaultMobileNetworkConnectivity + combine( + mobileConnectionsRepo.defaultMobileNetworkConnectivity, + forcingCellularValidation, + ) { networkConnectivity, forceValidation -> + return@combine if (forceValidation) { + MobileConnectivityModel( + isValidated = true, + isConnected = networkConnectivity.isConnected + ) + } else { + networkConnectivity + } + } + .stateIn( + scope, + SharingStarted.WhileSubscribed(), + mobileConnectionsRepo.defaultMobileNetworkConnectivity.value + ) /** * Mapping from network type to [MobileIconGroup] using the config generated for the default diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionsRepository.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionsRepository.kt index 49d4bdc88c828..3acada492bf2e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionsRepository.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/data/repository/FakeMobileConnectionsRepository.kt @@ -26,6 +26,7 @@ import com.android.systemui.log.table.TableLogBuffer 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.util.MobileMappingsProxy +import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow // TODO(b/261632894): remove this in favor of the real impl or DemoMobileConnectionsRepository @@ -56,6 +57,7 @@ class FakeMobileConnectionsRepository( private val _activeMobileDataSubscriptionId = MutableStateFlow(INVALID_SUBSCRIPTION_ID) override val activeMobileDataSubscriptionId = _activeMobileDataSubscriptionId + override val activeSubChangedInGroupEvent: MutableSharedFlow = MutableSharedFlow() private val _mobileConnectivity = MutableStateFlow(MobileConnectivityModel()) override val defaultMobileNetworkConnectivity = _mobileConnectivity 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 9e8ae6db449a3..1b62d5cc15b5a 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 @@ -36,6 +36,7 @@ import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest import kotlinx.coroutines.yield import org.junit.After @@ -353,6 +354,259 @@ class MobileIconsInteractorTest : SysuiTestCase() { job.cancel() } + @Test + fun `data switch - in same group - validated matches previous value`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = true, + ) + ) + // Trigger a data change in the same subscription group + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = true, + ) + ) + + job.cancel() + } + + @Test + fun `data switch - in same group - validated matches previous value - expires after 2s`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = true, + ) + ) + // Trigger a data change in the same subscription group + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + // After 1s, the force validation bit is still present + advanceTimeBy(1000) + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = true, + ) + ) + + // After 2s, the force validation expires + advanceTimeBy(1001) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + job.cancel() + } + + @Test + fun `data switch - in same group - not validated - uses new value immediately`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = false, + ) + ) + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + job.cancel() + } + + @Test + fun `data switch - lose validation - then switch happens - clears forced bit`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + + // GIVEN the network starts validated + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = true, + ) + ) + + // WHEN a data change happens in the same group + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + + // WHEN the validation bit is lost + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + // WHEN another data change happens in the same group + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + + // THEN the forced validation bit is still removed after 2s + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = true, + ) + ) + + advanceTimeBy(1000) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = true, + ) + ) + + advanceTimeBy(1001) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + job.cancel() + } + + @Test + fun `data switch - while already forcing validation - resets clock`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = true, + ) + ) + + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + + advanceTimeBy(1000) + + // WHEN another change in same group event happens + connectionsRepository.activeSubChangedInGroupEvent.emit(Unit) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + // THEN the forced validation remains for exactly 2 more seconds from now + + // 1.500s from second event + advanceTimeBy(1500) + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = true, + ) + ) + + // 2.001s from the second event + advanceTimeBy(501) + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + job.cancel() + } + + @Test + fun `data switch - not in same group - uses new values`() = + testScope.runTest { + var latest: MobileConnectivityModel? = null + val job = + underTest.defaultMobileNetworkConnectivity.onEach { latest = it }.launchIn(this) + + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = true, + isValidated = true, + ) + ) + connectionsRepository.setMobileConnectivity( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + assertThat(latest) + .isEqualTo( + MobileConnectivityModel( + isConnected = false, + isValidated = false, + ) + ) + + job.cancel() + } + companion object { private val tableLogBuffer = TableLogBuffer(8, "MobileIconsInteractorTest", FakeSystemClock())