Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 1e79aacc authored by Evan Laird's avatar Evan Laird
Browse files

[Sb refactor] Remove the concept of `isDefault`

I feel like this CL deserves some introspection as to what is going on.

INTRO

The old pipeline has two main concepts that it tracks when it comes to
knowing which subscription is special: `active` and `default`. They come
from static methods on `SubscriptionManager`: getActiveDataSubscriptionId
and getDefaultDataSubscriptionId.

The old pipeline will map from `networkType` to icon group except for
the following cases. For each subscription:

  1. When !telephonyManager.isDataConnectionAllowed (!dataEnabled) AND
     the defaultDataSubId is not the current subscription, set the
     MobileIconGroup to NOT_DEFAULT_DATA.
  2. When !dataEnabled AND this is the defaultDataSubId, set the
     MobileIconGroup to DATA_DISABLED.

Note that neither of these icon groups represent icons, they have a
resId of 0, meaning that later on when it comes time to use them, they
will _not_ show any UI on screen, nor will their content descriptions be
used. Also note that this is just system state stored in an UI view
model-like class.

THE PROBLEM

So, the question becomes: what do we do with these two mobile icon
groups in the new pipeline? Do we use the same input logic as the old
pipeline and thread these two special-case MobileIconGroups through the
MobileIconInteractor and then filter them out at the View Model? Or can
we rework the system to be smarter?

THIS CHANGE

Note that from the intro we stated that both of these cases only apply
when telephony reports `false` for `isDataConnectionAllowed`. Only
_then_ does the old pipeline do anything with these icon groups. This
means that as far as the UI is concerned, NOT_DEFAULT_DATA and
DATA_DISABLED are equivalent (showing no UI). The only time that
NOT_DEFAULT_DATA comes into play is when determining when to show the
exclamation mark.

Side note, there is a related bug in the old pipeline about checking
NOT_DEFAULT_DATA. When NetworkControllerIpml is attempting to find out
which controller is the data controller, it checks against the _active_
data subscriptio ID vs the _default_. This probably doesn't account for
a huge amount of bugs, but it's worth mentioning this problem.

With all of that out of the way, this change is rather simple: *remove
the concept of `isDefault` from the pipeline*. The only thing we need to
track is whether or not to show the data icon. The only remaining
relevant piece to track was the exclamation mark on the triangle, but
that field is covered elsewhere by a more appropriately-named field
tracking the active data.

Test: MobileIconInteractorTest
Test: MobileIconViewModelTest
Bug: 274724674
Bug: 270621876

Change-Id: I508a87e597cda5f9351053266701ebec84a0bcc3
parent 21a25876
Loading
Loading
Loading
Loading
+1 −18
Original line number Diff line number Diff line
@@ -21,7 +21,6 @@ import android.telephony.CarrierConfigManager
import com.android.settingslib.SignalIcon.MobileIconGroup
import com.android.settingslib.mobile.MobileIconCarrierIdOverrides
import com.android.settingslib.mobile.MobileIconCarrierIdOverridesImpl
import com.android.settingslib.mobile.TelephonyIcons.NOT_DEFAULT_DATA
import com.android.systemui.dagger.qualifiers.Application
import com.android.systemui.log.table.TableLogBuffer
import com.android.systemui.log.table.logDiffsForTable
@@ -41,7 +40,6 @@ import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.mapLatest
import kotlinx.coroutines.flow.stateIn

interface MobileIconInteractor {
@@ -125,7 +123,6 @@ class MobileIconInteractorImpl(
    override val mobileIsDefault: StateFlow<Boolean>,
    defaultMobileIconMapping: StateFlow<Map<String, MobileIconGroup>>,
    defaultMobileIconGroup: StateFlow<MobileIconGroup>,
    defaultDataSubId: StateFlow<Int>,
    override val isDefaultConnectionFailed: StateFlow<Boolean>,
    override val isForceHidden: Flow<Boolean>,
    connectionRepository: MobileConnectionRepository,
@@ -138,15 +135,6 @@ class MobileIconInteractorImpl(

    override val isDataEnabled: StateFlow<Boolean> = connectionRepository.dataEnabled

    private val isDefault =
        defaultDataSubId
            .mapLatest { connectionRepository.subId == it }
            .stateIn(
                scope,
                SharingStarted.WhileSubscribed(),
                connectionRepository.subId == defaultDataSubId.value
            )

    // True if there exists _any_ icon override for this carrierId. Note that overrides can include
    // any or none of the icon groups defined in MobileMappings, so we still need to check on a
    // per-network-type basis whether or not the given icon group is overridden
@@ -180,12 +168,7 @@ class MobileIconInteractorImpl(
                connectionRepository.resolvedNetworkType,
                defaultMobileIconMapping,
                defaultMobileIconGroup,
                isDefault,
            ) { resolvedNetworkType, mapping, defaultGroup, isDefault ->
                if (!isDefault) {
                    return@combine NOT_DEFAULT_DATA
                }

            ) { resolvedNetworkType, mapping, defaultGroup ->
                when (resolvedNetworkType) {
                    is ResolvedNetworkType.CarrierMergedNetworkType ->
                        resolvedNetworkType.iconGroupOverride
+0 −6
Original line number Diff line number Diff line
@@ -76,9 +76,6 @@ interface MobileIconsInteractor {
    /** True if the CDMA level should be preferred over the primary level. */
    val alwaysUseCdmaLevel: StateFlow<Boolean>

    /** Tracks the subscriptionId set as the default for data connections */
    val defaultDataSubId: StateFlow<Int>

    /** The icon mapping from network type to [MobileIconGroup] for the default subscription */
    val defaultMobileIconMapping: StateFlow<Map<String, MobileIconGroup>>

@@ -186,8 +183,6 @@ constructor(
            )
            .stateIn(scope, SharingStarted.WhileSubscribed(), listOf())

    override val defaultDataSubId = mobileConnectionsRepo.defaultDataSubId

    /**
     * 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).
@@ -284,7 +279,6 @@ constructor(
            mobileIsDefault,
            defaultMobileIconMapping,
            defaultMobileIconGroup,
            defaultDataSubId,
            isDefaultConnectionFailed,
            isForceHidden,
            mobileConnectionsRepo.getRepoForSubId(subId),
+2 −3
Original line number Diff line number Diff line
@@ -146,11 +146,10 @@ constructor(
        combine(
                iconInteractor.isDataConnected,
                iconInteractor.isDataEnabled,
                iconInteractor.isDefaultConnectionFailed,
                iconInteractor.alwaysShowDataRatIcon,
                iconInteractor.mobileIsDefault,
            ) { dataConnected, dataEnabled, failedConnection, alwaysShow, mobileIsDefault ->
                alwaysShow || (dataConnected && dataEnabled && !failedConnection && mobileIsDefault)
            ) { dataConnected, dataEnabled, alwaysShow, mobileIsDefault ->
                alwaysShow || (dataEnabled && dataConnected && mobileIsDefault)
            }
            .distinctUntilChanged()
            .logDiffsForTable(
+0 −1
Original line number Diff line number Diff line
@@ -59,7 +59,6 @@ class FakeMobileIconsInteractor(
    override val alwaysShowDataRatIcon = MutableStateFlow(false)

    override val alwaysUseCdmaLevel = MutableStateFlow(false)
    override val defaultDataSubId = MutableStateFlow(DEFAULT_DATA_SUB_ID)

    override val mobileIsDefault = MutableStateFlow(false)

+0 −23
Original line number Diff line number Diff line
@@ -244,27 +244,6 @@ class MobileIconInteractorTest : SysuiTestCase() {
            job.cancel()
        }

    @Test
    fun `icon group - checks default data`() =
        testScope.runTest {
            mobileIconsInteractor.defaultDataSubId.value = SUB_1_ID
            connectionRepository.resolvedNetworkType.value =
                DefaultNetworkType(mobileMappingsProxy.toIconKey(THREE_G))

            var latest: NetworkTypeIconModel? = null
            val job = underTest.networkTypeIconGroup.onEach { latest = it }.launchIn(this)

            assertThat(latest).isEqualTo(NetworkTypeIconModel.DefaultIcon(TelephonyIcons.THREE_G))

            // Default data sub id changes to something else
            mobileIconsInteractor.defaultDataSubId.value = 123

            assertThat(latest)
                .isEqualTo(NetworkTypeIconModel.DefaultIcon(TelephonyIcons.NOT_DEFAULT_DATA))

            job.cancel()
        }

    @Test
    fun overrideIcon_usesCarrierIdOverride() =
        testScope.runTest {
@@ -276,7 +255,6 @@ class MobileIconInteractorTest : SysuiTestCase() {

            underTest = createInteractor(overrides)

            mobileIconsInteractor.defaultDataSubId.value = SUB_1_ID
            connectionRepository.resolvedNetworkType.value =
                DefaultNetworkType(mobileMappingsProxy.toIconKey(THREE_G))

@@ -506,7 +484,6 @@ class MobileIconInteractorTest : SysuiTestCase() {
            mobileIconsInteractor.mobileIsDefault,
            mobileIconsInteractor.defaultMobileIconMapping,
            mobileIconsInteractor.defaultMobileIconGroup,
            mobileIconsInteractor.defaultDataSubId,
            mobileIconsInteractor.isDefaultConnectionFailed,
            mobileIconsInteractor.isForceHidden,
            connectionRepository,
Loading