[Sb refactor] Move active data repo to connections repo

Since MobileIconsInteractor was always eagerly creating a mobile
connection repository tracking the active data subscripton, this CL
moves that to the MobileConnectionsRepository. This allows us to make th
following changes:

- Change activeMobileDataSubscriptionId to return null if the id is
  invalid
- Expose the active mobile data repo as a nullable flow
- Rework the isValidSubId() method to take this active subid into
  account
- Same for the cached repos

This change is specifically written to better handle the case where
TelephonyCallback#onActiveDataSubscriptionIdChanged is called before
SubscriptionManager#onSubscriptionsChanged is called. In that case, the
old implementation would throw an exception for an invalid subId,
despite having been given a valid subId from the system.

Test: MobileConnectionsRepositoryTest
Bug: 268146648
Change-Id: I39698d4d516a783fe17ae661fdcb220407d89486
This commit is contained in:
Evan Laird
2023-02-08 15:37:25 -05:00
parent c5a9ef5573
commit b83e1afc9d
7 changed files with 257 additions and 72 deletions

View File

@@ -34,8 +34,14 @@ interface MobileConnectionsRepository {
/** Observable list of current mobile subscriptions */
val subscriptions: StateFlow<List<SubscriptionModel>>
/** Observable for the subscriptionId of the current mobile data connection */
val activeMobileDataSubscriptionId: StateFlow<Int>
/**
* Observable for the subscriptionId of the current mobile data connection. Null if we don't
* have a valid subscription id
*/
val activeMobileDataSubscriptionId: StateFlow<Int?>
/** Repo that tracks the current [activeMobileDataSubscriptionId] */
val activeMobileDataRepository: StateFlow<MobileConnectionRepository?>
/**
* Observable event for when the active data sim switches but the group stays the same. E.g.,

View File

@@ -47,7 +47,6 @@ import kotlinx.coroutines.flow.stateIn
* interface in its own repository, completely separate from the real version, while still using all
* of the prod implementations for the rest of the pipeline (interactors and onward). Looks
* something like this:
*
* ```
* RealRepository
* │
@@ -115,7 +114,7 @@ constructor(
.flatMapLatest { it.subscriptions }
.stateIn(scope, SharingStarted.WhileSubscribed(), realRepository.subscriptions.value)
override val activeMobileDataSubscriptionId: StateFlow<Int> =
override val activeMobileDataSubscriptionId: StateFlow<Int?> =
activeRepo
.flatMapLatest { it.activeMobileDataSubscriptionId }
.stateIn(
@@ -124,6 +123,15 @@ constructor(
realRepository.activeMobileDataSubscriptionId.value
)
override val activeMobileDataRepository: StateFlow<MobileConnectionRepository?> =
activeRepo
.flatMapLatest { it.activeMobileDataRepository }
.stateIn(
scope,
SharingStarted.WhileSubscribed(),
realRepository.activeMobileDataRepository.value
)
override val activeSubChangedInGroupEvent: Flow<Unit> =
activeRepo.flatMapLatest { it.activeSubChangedInGroupEvent }

View File

@@ -55,6 +55,7 @@ import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.filterNotNull
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
@@ -121,6 +122,15 @@ constructor(
subscriptions.value.firstOrNull()?.subscriptionId ?: INVALID_SUBSCRIPTION_ID
)
override val activeMobileDataRepository: StateFlow<MobileConnectionRepository?> =
activeMobileDataSubscriptionId
.map { getRepoForSubId(it) }
.stateIn(
scope,
SharingStarted.WhileSubscribed(),
getRepoForSubId(activeMobileDataSubscriptionId.value)
)
// TODO(b/261029387): consider adding a demo command for this
override val activeSubChangedInGroupEvent: Flow<Unit> = flowOf()

View File

@@ -159,13 +159,16 @@ constructor(
)
.stateIn(scope, started = SharingStarted.WhileSubscribed(), listOf())
/** StateFlow that keeps track of the current active mobile data subscription */
override val activeMobileDataSubscriptionId: StateFlow<Int> =
override val activeMobileDataSubscriptionId: StateFlow<Int?> =
conflatedCallbackFlow {
val callback =
object : TelephonyCallback(), ActiveDataSubscriptionIdListener {
override fun onActiveDataSubscriptionIdChanged(subId: Int) {
trySend(subId)
if (subId != INVALID_SUBSCRIPTION_ID) {
trySend(subId)
} else {
trySend(null)
}
}
}
@@ -179,7 +182,18 @@ constructor(
columnName = "activeSubId",
initialValue = INVALID_SUBSCRIPTION_ID,
)
.stateIn(scope, started = SharingStarted.WhileSubscribed(), INVALID_SUBSCRIPTION_ID)
.stateIn(scope, started = SharingStarted.WhileSubscribed(), null)
override val activeMobileDataRepository =
activeMobileDataSubscriptionId
.map { activeSubId ->
if (activeSubId == null) {
null
} else {
getOrCreateRepoForSubId(activeSubId)
}
}
.stateIn(scope, SharingStarted.WhileSubscribed(), null)
private val defaultDataSubIdChangeEvent: MutableSharedFlow<Unit> =
MutableSharedFlow(extraBufferCapacity = 1)
@@ -240,10 +254,13 @@ constructor(
)
}
return subIdRepositoryCache[subId]
?: createRepositoryForSubId(subId).also { subIdRepositoryCache[subId] = it }
return getOrCreateRepoForSubId(subId)
}
private fun getOrCreateRepoForSubId(subId: Int) =
subIdRepositoryCache[subId]
?: createRepositoryForSubId(subId).also { subIdRepositoryCache[subId] = it }
@SuppressLint("MissingPermission")
override val defaultMobileNetworkConnectivity: StateFlow<MobileConnectivityModel> =
conflatedCallbackFlow {
@@ -292,7 +309,9 @@ constructor(
override val activeSubChangedInGroupEvent =
activeMobileDataSubscriptionId
.pairwise()
.mapNotNull { (prevVal: Int, newVal: Int) ->
.mapNotNull { (prevVal: Int?, newVal: Int?) ->
if (prevVal == null || newVal == null) return@mapNotNull null
val prevSub = subscriptionManager.getActiveSubscriptionInfo(prevVal)?.groupUuid
val nextSub = subscriptionManager.getActiveSubscriptionInfo(newVal)?.groupUuid
@@ -300,15 +319,7 @@ constructor(
}
.flowOn(bgDispatcher)
private fun isValidSubId(subId: Int): Boolean {
subscriptions.value.forEach {
if (it.subscriptionId == subId) {
return true
}
}
return false
}
private fun isValidSubId(subId: Int): Boolean = checkSub(subId, subscriptions.value)
@VisibleForTesting fun getSubIdRepoCache() = subIdRepositoryCache
@@ -335,12 +346,27 @@ constructor(
private fun dropUnusedReposFromCache(newInfos: List<SubscriptionModel>) {
// Remove any connection repository from the cache that isn't in the new set of IDs. They
// will get garbage collected once their subscribers go away
val currentValidSubscriptionIds = newInfos.map { it.subscriptionId }
subIdRepositoryCache =
subIdRepositoryCache
.filter { currentValidSubscriptionIds.contains(it.key) }
.toMutableMap()
subIdRepositoryCache.filter { checkSub(it.key, newInfos) }.toMutableMap()
}
/**
* True if the checked subId is in the list of current subs or the active mobile data subId
*
* @param checkedSubs the list to validate [subId] against. To invalidate the cache, pass in the
* new subscription list. Otherwise use [subscriptions.value] to validate a subId against the
* current known subscriptions
*/
private fun checkSub(subId: Int, checkedSubs: List<SubscriptionModel>): Boolean {
if (activeMobileDataSubscriptionId.value == subId) return true
checkedSubs.forEach {
if (it.subscriptionId == subId) {
return true
}
}
return false
}
private suspend fun fetchSubscriptionsList(): List<SubscriptionInfo> =

View File

@@ -18,7 +18,6 @@ package com.android.systemui.statusbar.pipeline.mobile.domain.interactor
import android.telephony.CarrierConfigManager
import android.telephony.SubscriptionManager
import android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID
import com.android.settingslib.SignalIcon.MobileIconGroup
import com.android.settingslib.mobile.TelephonyIcons
import com.android.systemui.dagger.SysUISingleton
@@ -118,22 +117,8 @@ constructor(
userSetupRepo: UserSetupRepository,
@Application private val scope: CoroutineScope,
) : MobileIconsInteractor {
private val activeMobileDataSubscriptionId =
mobileConnectionsRepo.activeMobileDataSubscriptionId
private val activeMobileDataConnectionRepo: StateFlow<MobileConnectionRepository?> =
activeMobileDataSubscriptionId
.mapLatest { activeId ->
if (activeId == INVALID_SUBSCRIPTION_ID) {
null
} else {
mobileConnectionsRepo.getRepoForSubId(activeId)
}
}
.stateIn(scope, SharingStarted.WhileSubscribed(), null)
override val activeDataConnectionHasDataEnabled: StateFlow<Boolean> =
activeMobileDataConnectionRepo
mobileConnectionsRepo.activeMobileDataRepository
.flatMapLatest { it?.dataEnabled ?: flowOf(false) }
.stateIn(scope, SharingStarted.WhileSubscribed(), false)
@@ -154,35 +139,37 @@ constructor(
* and by checking which subscription is opportunistic, or which one is active.
*/
override val filteredSubscriptions: Flow<List<SubscriptionModel>> =
combine(unfilteredSubscriptions, activeMobileDataSubscriptionId) { unfilteredSubs, activeId
->
// Based on the old logic,
if (unfilteredSubs.size != 2) {
return@combine unfilteredSubs
}
combine(
unfilteredSubscriptions,
mobileConnectionsRepo.activeMobileDataSubscriptionId,
) { unfilteredSubs, activeId ->
// Based on the old logic,
if (unfilteredSubs.size != 2) {
return@combine unfilteredSubs
}
val info1 = unfilteredSubs[0]
val info2 = unfilteredSubs[1]
// If both subscriptions are primary, show both
if (!info1.isOpportunistic && !info2.isOpportunistic) {
return@combine unfilteredSubs
}
val info1 = unfilteredSubs[0]
val info2 = unfilteredSubs[1]
// If both subscriptions are primary, show both
if (!info1.isOpportunistic && !info2.isOpportunistic) {
return@combine unfilteredSubs
}
// NOTE: at this point, we are now returning a single SubscriptionInfo
// NOTE: at this point, we are now returning a single SubscriptionInfo
// If carrier required, always show the icon of the primary subscription.
// Otherwise, show whichever subscription is currently active for internet.
if (carrierConfigTracker.alwaysShowPrimarySignalBarInOpportunisticNetworkDefault) {
// return the non-opportunistic info
return@combine if (info1.isOpportunistic) listOf(info2) else listOf(info1)
} else {
return@combine if (info1.subscriptionId == activeId) {
listOf(info1)
// If carrier required, always show the icon of the primary subscription.
// Otherwise, show whichever subscription is currently active for internet.
if (carrierConfigTracker.alwaysShowPrimarySignalBarInOpportunisticNetworkDefault) {
// return the non-opportunistic info
return@combine if (info1.isOpportunistic) listOf(info2) else listOf(info1)
} else {
listOf(info2)
return@combine if (info1.subscriptionId == activeId) {
listOf(info1)
} else {
listOf(info2)
}
}
}
}
.distinctUntilChanged()
.logDiffsForTable(
tableLogger,

View File

@@ -55,8 +55,12 @@ class FakeMobileConnectionsRepository(
private val _subscriptions = MutableStateFlow<List<SubscriptionModel>>(listOf())
override val subscriptions = _subscriptions
private val _activeMobileDataSubscriptionId = MutableStateFlow(INVALID_SUBSCRIPTION_ID)
private val _activeMobileDataSubscriptionId = MutableStateFlow<Int?>(null)
override val activeMobileDataSubscriptionId = _activeMobileDataSubscriptionId
private val _activeMobileRepository = MutableStateFlow<MobileConnectionRepository?>(null)
override val activeMobileDataRepository = _activeMobileRepository
override val activeSubChangedInGroupEvent: MutableSharedFlow<Unit> = MutableSharedFlow()
private val _defaultDataSubId = MutableStateFlow(INVALID_SUBSCRIPTION_ID)
@@ -66,6 +70,7 @@ class FakeMobileConnectionsRepository(
override val defaultMobileNetworkConnectivity = _mobileConnectivity
private val subIdRepos = mutableMapOf<Int, MobileConnectionRepository>()
override fun getRepoForSubId(subId: Int): MobileConnectionRepository {
return subIdRepos[subId]
?: FakeMobileConnectionRepository(subId, tableLogBuffer).also { subIdRepos[subId] = it }
@@ -92,7 +97,14 @@ class FakeMobileConnectionsRepository(
}
fun setActiveMobileDataSubscriptionId(subId: Int) {
_activeMobileDataSubscriptionId.value = subId
// Simulate the filtering that the repo does
if (subId == INVALID_SUBSCRIPTION_ID) {
_activeMobileDataSubscriptionId.value = null
_activeMobileRepository.value = null
} else {
_activeMobileDataSubscriptionId.value = subId
_activeMobileRepository.value = getRepoForSubId(subId)
}
}
fun setMobileConnectionRepositoryMap(connections: Map<Int, MobileConnectionRepository>) {

View File

@@ -26,6 +26,7 @@ import android.os.ParcelUuid
import android.telephony.CarrierConfigManager
import android.telephony.SubscriptionInfo
import android.telephony.SubscriptionManager
import android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID
import android.telephony.TelephonyCallback
import android.telephony.TelephonyCallback.ActiveDataSubscriptionIdListener
import android.telephony.TelephonyManager
@@ -39,6 +40,7 @@ import com.android.systemui.log.table.TableLogBufferFactory
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.CarrierConfigRepository
import com.android.systemui.statusbar.pipeline.mobile.data.repository.MobileConnectionRepository
import com.android.systemui.statusbar.pipeline.mobile.data.repository.prod.FullMobileConnectionRepository.Factory.Companion.tableBufferLogName
import com.android.systemui.statusbar.pipeline.mobile.util.FakeMobileMappingsProxy
import com.android.systemui.statusbar.pipeline.shared.ConnectivityPipelineLogger
@@ -55,6 +57,7 @@ import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.runBlocking
@@ -254,10 +257,9 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() {
}
@Test
fun testActiveDataSubscriptionId_initialValueIsInvalidId() =
fun testActiveDataSubscriptionId_initialValueIsNull() =
runBlocking(IMMEDIATE) {
assertThat(underTest.activeMobileDataSubscriptionId.value)
.isEqualTo(SubscriptionManager.INVALID_SUBSCRIPTION_ID)
assertThat(underTest.activeMobileDataSubscriptionId.value).isEqualTo(null)
}
@Test
@@ -275,6 +277,140 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() {
job.cancel()
}
@Test
fun activeSubId_nullIfInvalidSubIdIsReceived() =
runBlocking(IMMEDIATE) {
var latest: Int? = null
val job = underTest.activeMobileDataSubscriptionId.onEach { latest = it }.launchIn(this)
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
assertThat(latest).isNotNull()
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(INVALID_SUBSCRIPTION_ID)
assertThat(latest).isNull()
job.cancel()
}
@Test
fun activeRepo_initiallyNull() {
assertThat(underTest.activeMobileDataRepository.value).isNull()
}
@Test
fun activeRepo_updatesWithActiveDataId() =
runBlocking(IMMEDIATE) {
var latest: MobileConnectionRepository? = null
val job = underTest.activeMobileDataRepository.onEach { latest = it }.launchIn(this)
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
assertThat(latest?.subId).isEqualTo(SUB_2_ID)
job.cancel()
}
@Test
fun activeRepo_nullIfActiveDataSubIdBecomesInvalid() =
runBlocking(IMMEDIATE) {
var latest: MobileConnectionRepository? = null
val job = underTest.activeMobileDataRepository.onEach { latest = it }.launchIn(this)
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
assertThat(latest).isNotNull()
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(INVALID_SUBSCRIPTION_ID)
assertThat(latest).isNull()
job.cancel()
}
@Test
/** Regression test for b/268146648. */
fun activeSubIdIsSetBeforeSubscriptionsAreUpdated_doesNotThrow() =
runBlocking(IMMEDIATE) {
var activeRepo: MobileConnectionRepository? = null
var subscriptions: List<SubscriptionModel>? = null
val activeRepoJob =
underTest.activeMobileDataRepository.onEach { activeRepo = it }.launchIn(this)
val subscriptionsJob =
underTest.subscriptions.onEach { subscriptions = it }.launchIn(this)
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
assertThat(subscriptions).isEmpty()
assertThat(activeRepo).isNotNull()
activeRepoJob.cancel()
subscriptionsJob.cancel()
}
@Test
fun getRepoForSubId_activeDataSubIdIsRequestedBeforeSubscriptionsUpdate() =
runBlocking(IMMEDIATE) {
var latest: MobileConnectionRepository? = null
var subscriptions: List<SubscriptionModel>? = null
val activeSubIdJob =
underTest.activeMobileDataSubscriptionId
.filterNotNull()
.onEach { latest = underTest.getRepoForSubId(it) }
.launchIn(this)
val subscriptionsJob =
underTest.subscriptions.onEach { subscriptions = it }.launchIn(this)
// Active data subscription id is sent, but no subscription change has been posted yet
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
// Subscriptions list is empty
assertThat(subscriptions).isEmpty()
// getRepoForSubId does not throw
assertThat(latest).isNotNull()
activeSubIdJob.cancel()
subscriptionsJob.cancel()
}
@Test
fun activeDataSentBeforeSubscriptionList_subscriptionReusesActiveDataRepo() =
runBlocking(IMMEDIATE) {
var activeRepo: MobileConnectionRepository? = null
val job = underTest.activeMobileDataRepository.onEach { activeRepo = it }.launchIn(this)
val subscriptionsJob = underTest.subscriptions.launchIn(this)
// GIVEN active repo is updated before the subscription list updates
getTelephonyCallbackForType<ActiveDataSubscriptionIdListener>()
.onActiveDataSubscriptionIdChanged(SUB_2_ID)
assertThat(activeRepo).isNotNull()
// GIVEN the subscription list is then updated which includes the active data sub id
whenever(subscriptionManager.completeActiveSubscriptionInfoList)
.thenReturn(listOf(SUB_2))
getSubscriptionCallback().onSubscriptionsChanged()
// WHEN requesting a connection repository for the subscription
val newRepo = underTest.getRepoForSubId(SUB_2_ID)
// THEN the newly request repo has been cached and reused
assertThat(activeRepo).isSameInstanceAs(newRepo)
job.cancel()
subscriptionsJob.cancel()
}
@Test
fun testConnectionRepository_validSubId_isCached() =
runBlocking(IMMEDIATE) {
@@ -475,7 +611,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() {
}
@Test
fun `connection repository - log buffer contains sub id in its name`() =
fun connectionRepository_logBufferContainsSubIdInItsName() =
runBlocking(IMMEDIATE) {
val job = underTest.subscriptions.launchIn(this)
@@ -691,7 +827,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() {
}
@Test
fun `active data change - in same group - emits unit`() =
fun activeDataChange_inSameGroup_emitsUnit() =
runBlocking(IMMEDIATE) {
var latest: Unit? = null
val job = underTest.activeSubChangedInGroupEvent.onEach { latest = it }.launchIn(this)
@@ -707,7 +843,7 @@ class MobileConnectionsRepositoryTest : SysuiTestCase() {
}
@Test
fun `active data change - not in same group - does not emit`() =
fun activeDataChange_notInSameGroup_doesNotEmit() =
runBlocking(IMMEDIATE) {
var latest: Unit? = null
val job = underTest.activeSubChangedInGroupEvent.onEach { latest = it }.launchIn(this)