From 476bf5a5665b4f5c7ea4e0ad15d0d89bb4669767 Mon Sep 17 00:00:00 2001 From: Caitlin Shkuratov Date: Wed, 3 May 2023 19:29:00 +0000 Subject: [PATCH] [SB Refactor] Only use carrier merged sub ID if wifi or CM are default. (or the device is in airplane mode) The old pipeline's WifiSignalController only sent updates to MobileSignalController if the wifi info was carrier merged *and* wifi or carrier merged was the default connection or the device was in airplane mode. The new pipeline previously didn't check this default status, which caused the new pipeline to keep polling the carrier merged info instead of the normal cellular info. Also increases the size of the connectivity buffer, since investigating the BRs attached to this bug revealed that the buffer cut off too early. Bug: 278618530 Test: atest MobileConnectionsRepositoryTest Change-Id: Ib9dde0234c38921bb67179a138f9cc8c60e7d669 --- .../dagger/StatusBarPipelineModule.kt | 2 +- .../prod/MobileConnectionsRepositoryImpl.kt | 28 +++-- .../MobileRepositorySwitcherTest.kt | 2 + .../prod/MobileConnectionsRepositoryTest.kt | 106 +++++++++++++++--- 4 files changed, 111 insertions(+), 27 deletions(-) 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 7aa90336e2bfa..49de5a232f300 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 @@ -160,7 +160,7 @@ abstract class StatusBarPipelineModule { @SysUISingleton @SharedConnectivityInputLog fun provideSharedConnectivityTableLogBuffer(factory: LogBufferFactory): LogBuffer { - return factory.create("SharedConnectivityInputLog", 30) + return factory.create("SharedConnectivityInputLog", 60) } @Provides 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 0e9b6c56437ed..81a068d10a145 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 @@ -38,6 +38,7 @@ 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.airplane.data.repository.AirplaneModeRepository import com.android.systemui.statusbar.pipeline.dagger.MobileSummaryLog import com.android.systemui.statusbar.pipeline.mobile.data.MobileInputLogger import com.android.systemui.statusbar.pipeline.mobile.data.model.NetworkNameModel @@ -88,6 +89,7 @@ constructor( private val context: Context, @Background private val bgDispatcher: CoroutineDispatcher, @Application private val scope: CoroutineScope, + airplaneModeRepository: AirplaneModeRepository, // Some "wifi networks" should be rendered as a mobile connection, which is why the wifi // repository is an input to the mobile repository. // See [CarrierMergedConnectionRepository] for details. @@ -106,10 +108,20 @@ constructor( context.getString(R.string.status_bar_network_name_separator) private val carrierMergedSubId: StateFlow = - wifiRepository.wifiNetwork - .mapLatest { - if (it is WifiNetworkModel.CarrierMerged) { - it.subscriptionId + combine( + wifiRepository.wifiNetwork, + connectivityRepository.defaultConnections, + airplaneModeRepository.isAirplaneMode, + ) { wifiNetwork, defaultConnections, isAirplaneMode -> + // The carrier merged connection should only be used if it's also the default + // connection or mobile connections aren't available because of airplane mode. + val defaultConnectionIsNonMobile = + defaultConnections.carrierMerged.isDefault || + defaultConnections.wifi.isDefault || + isAirplaneMode + + if (wifiNetwork is WifiNetworkModel.CarrierMerged && defaultConnectionIsNonMobile) { + wifiNetwork.subscriptionId } else { null } @@ -269,12 +281,8 @@ constructor( .stateIn(scope, SharingStarted.WhileSubscribed(), false) override val hasCarrierMergedConnection: StateFlow = - combine( - connectivityRepository.defaultConnections, - carrierMergedSubId, - ) { defaultConnections, carrierMergedSubId -> - defaultConnections.carrierMerged.isDefault || carrierMergedSubId != null - } + carrierMergedSubId + .map { it != null } .distinctUntilChanged() .logDiffsForTable( tableLogger, 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 bde05b9f499ed..5a887ebcee800 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 @@ -26,6 +26,7 @@ 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.airplane.data.repository.FakeAirplaneModeRepository import com.android.systemui.statusbar.pipeline.mobile.data.MobileInputLogger import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.demo.DemoMobileConnectionsRepository @@ -131,6 +132,7 @@ class MobileRepositorySwitcherTest : SysuiTestCase() { context, IMMEDIATE, scope, + FakeAirplaneModeRepository(), wifiRepository, mock(), ) 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 dccbe915792a9..38c7432eb288a 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 @@ -43,6 +43,7 @@ import com.android.systemui.SysuiTestCase import com.android.systemui.coroutines.collectLastValue import com.android.systemui.log.table.TableLogBuffer import com.android.systemui.log.table.TableLogBufferFactory +import com.android.systemui.statusbar.pipeline.airplane.data.repository.FakeAirplaneModeRepository import com.android.systemui.statusbar.pipeline.mobile.data.MobileInputLogger import com.android.systemui.statusbar.pipeline.mobile.data.model.SubscriptionModel import com.android.systemui.statusbar.pipeline.mobile.data.repository.CarrierConfigRepository @@ -72,7 +73,6 @@ import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest -import kotlinx.coroutines.yield import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Before @@ -96,6 +96,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { private lateinit var carrierMergedFactory: CarrierMergedConnectionRepository.Factory private lateinit var fullConnectionFactory: FullMobileConnectionRepository.Factory private lateinit var connectivityRepository: ConnectivityRepository + private lateinit var airplaneModeRepository: FakeAirplaneModeRepository private lateinit var wifiRepository: WifiRepository private lateinit var carrierConfigRepository: CarrierConfigRepository @Mock private lateinit var connectivityManager: ConnectivityManager @@ -149,6 +150,8 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { mock(), ) + airplaneModeRepository = FakeAirplaneModeRepository() + wifiRepository = WifiRepositoryImpl( fakeBroadcastDispatcher, @@ -207,6 +210,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { context, dispatcher, testScope.backgroundScope, + airplaneModeRepository, wifiRepository, fullConnectionFactory, ) @@ -422,6 +426,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { testScope.runTest { collectLastValue(underTest.subscriptions) + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) whenever(subscriptionManager.completeActiveSubscriptionInfoList) .thenReturn(listOf(SUB_CM)) @@ -438,6 +443,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { testScope.runTest { collectLastValue(underTest.subscriptions) + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) whenever(subscriptionManager.completeActiveSubscriptionInfoList) .thenReturn(listOf(SUB_1, SUB_CM)) @@ -454,6 +460,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { testScope.runTest { collectLastValue(underTest.subscriptions) + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) whenever(subscriptionManager.completeActiveSubscriptionInfoList) .thenReturn(listOf(SUB_1, SUB_CM)) @@ -465,6 +472,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { assertThat(mobileRepo.getIsCarrierMerged()).isFalse() // WHEN the wifi network updates to be not carrier merged + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_ACTIVE) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_ACTIVE) runCurrent() @@ -480,6 +488,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { testScope.runTest { collectLastValue(underTest.subscriptions) + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_ACTIVE) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_ACTIVE) whenever(subscriptionManager.completeActiveSubscriptionInfoList) .thenReturn(listOf(SUB_1, SUB_CM)) @@ -492,6 +501,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { assertThat(mobileRepo.getIsCarrierMerged()).isFalse() // WHEN the wifi network updates to be carrier merged + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) runCurrent() @@ -531,6 +541,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { testScope.runTest { collectLastValue(underTest.subscriptions) + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) whenever(subscriptionManager.completeActiveSubscriptionInfoList) .thenReturn(listOf(SUB_1, SUB_2, SUB_CM)) @@ -759,7 +770,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { fun hasCarrierMergedConnection_carrierMergedViaWifi_isTrue() = testScope.runTest { val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val caps = mock().also { whenever(it.hasTransport(TRANSPORT_WIFI)).thenReturn(true) @@ -769,7 +783,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val latest by collectLastValue(underTest.hasCarrierMergedConnection) getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, caps) assertThat(latest).isTrue() } @@ -778,7 +792,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { fun hasCarrierMergedConnection_carrierMergedViaMobile_isTrue() = testScope.runTest { val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val caps = mock().also { whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) @@ -788,7 +805,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val latest by collectLastValue(underTest.hasCarrierMergedConnection) getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, caps) assertThat(latest).isTrue() } @@ -798,7 +815,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { fun hasCarrierMergedConnection_carrierMergedViaWifiWithVcnTransport_isTrue() = testScope.runTest { val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val caps = mock().also { whenever(it.hasTransport(TRANSPORT_WIFI)).thenReturn(true) @@ -808,7 +828,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val latest by collectLastValue(underTest.hasCarrierMergedConnection) getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, caps) assertThat(latest).isTrue() } @@ -817,7 +837,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { fun hasCarrierMergedConnection_carrierMergedViaMobileWithVcnTransport_isTrue() = testScope.runTest { val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val caps = mock().also { whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) @@ -827,7 +850,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val latest by collectLastValue(underTest.hasCarrierMergedConnection) getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, caps) assertThat(latest).isTrue() } @@ -839,7 +862,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val underlyingNetwork = mock() val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val underlyingWifiCapabilities = mock().also { whenever(it.hasTransport(TRANSPORT_WIFI)).thenReturn(true) @@ -858,7 +884,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { } getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, mainCapabilities) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, mainCapabilities) // THEN there's a carrier merged connection assertThat(latest).isTrue() @@ -871,7 +897,10 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { val underlyingCarrierMergedNetwork = mock() val carrierMergedInfo = - mock().apply { whenever(this.isCarrierMerged).thenReturn(true) } + mock().apply { + whenever(this.isCarrierMerged).thenReturn(true) + whenever(this.isPrimary).thenReturn(true) + } val underlyingCapabilities = mock().also { whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) @@ -891,7 +920,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { } getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, mainCapabilities) - yield() + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, mainCapabilities) // THEN there's a carrier merged connection assertThat(latest).isTrue() @@ -899,11 +928,11 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { /** Regression test for b/272586234. */ @Test - fun hasCarrierMergedConnection_defaultNotCarrierMerged_butWifiRepoHasCarrierMerged_isTrue() = + fun hasCarrierMergedConnection_defaultIsWifiNotCarrierMerged_wifiRepoIsCarrierMerged_isTrue() = testScope.runTest { val latest by collectLastValue(underTest.hasCarrierMergedConnection) - // WHEN the default callback isn't carrier merged + // WHEN the default callback is TRANSPORT_WIFI but not carrier merged val carrierMergedInfo = mock().apply { whenever(this.isCarrierMerged).thenReturn(false) } val caps = @@ -912,7 +941,6 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { whenever(it.transportInfo).thenReturn(carrierMergedInfo) } getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) - yield() // BUT the wifi repo has gotten updates that it *is* carrier merged getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) @@ -921,6 +949,51 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { assertThat(latest).isTrue() } + /** Regression test for b/278618530. */ + @Test + fun hasCarrierMergedConnection_defaultIsCellular_wifiRepoIsCarrierMerged_isFalse() = + testScope.runTest { + val latest by collectLastValue(underTest.hasCarrierMergedConnection) + + // WHEN the default callback is TRANSPORT_CELLULAR and not carrier merged + val caps = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(null) + } + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) + + // BUT the wifi repo has gotten updates that it *is* carrier merged + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) + + // THEN hasCarrierMergedConnection is **false** (The default network being CELLULAR + // takes precedence over the wifi network being carrier merged.) + assertThat(latest).isFalse() + } + + /** Regression test for b/278618530. */ + @Test + fun hasCarrierMergedConnection_defaultCellular_wifiIsCarrierMerged_airplaneMode_isTrue() = + testScope.runTest { + val latest by collectLastValue(underTest.hasCarrierMergedConnection) + + // WHEN the default callback is TRANSPORT_CELLULAR and not carrier merged + val caps = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(null) + } + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, caps) + + // BUT the wifi repo has gotten updates that it *is* carrier merged + getNormalNetworkCallback().onCapabilitiesChanged(NETWORK, WIFI_NETWORK_CAPS_CM) + // AND we're in airplane mode + airplaneModeRepository.setIsAirplaneMode(true) + + // THEN hasCarrierMergedConnection is true. + assertThat(latest).isTrue() + } + @Test fun defaultConnectionIsValidated_startsAsFalse() { assertThat(underTest.defaultConnectionIsValidated.value).isFalse() @@ -978,6 +1051,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() { context, dispatcher, testScope.backgroundScope, + airplaneModeRepository, wifiRepository, fullConnectionFactory, )