Reduce Mobile data switch flaky

Set initial value to null, so no animation when the actual value true is emitted. Bug: 329584989 Flag: EXEMPT bug fix Test: manual - on SIMs Test: unit test Change-Id: I3eea55115f02e65dcdcc44ccf917f9083622b723
2024-12-31 11:17:42 +08:00
parent 7938eeb0d4
commit 440c3c2779
4 changed files with 228 additions and 123 deletions
--- a/src/com/android/settings/spa/network/MobileDataSwitchPreference.kt
+++ b/src/com/android/settings/spa/network/MobileDataSwitchPreference.kt
@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.settings.spa.network
+
+import androidx.annotation.VisibleForTesting
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.rememberCoroutineScope
+import androidx.compose.ui.platform.LocalContext
+import androidx.compose.ui.res.stringResource
+import androidx.lifecycle.compose.collectAsStateWithLifecycle
+import com.android.settings.R
+import com.android.settings.network.telephony.MobileDataRepository
+import com.android.settings.network.telephony.subscriptionManager
+import com.android.settingslib.spa.framework.compose.rememberContext
+import com.android.settingslib.spa.widget.preference.SwitchPreference
+import com.android.settingslib.spa.widget.preference.SwitchPreferenceModel
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+
+@Composable
+fun MobileDataSwitchPreference(subId: Int) {
+    MobileDataSwitchPreference(
+        subId = subId,
+        mobileDataRepository = rememberContext(::MobileDataRepository),
+        setMobileData = setMobileDataImpl(subId),
+    )
+}
+
+@VisibleForTesting
+@Composable
+fun MobileDataSwitchPreference(
+    subId: Int,
+    mobileDataRepository: MobileDataRepository,
+    setMobileData: (newChecked: Boolean) -> Unit,
+) {
+    val mobileDataSummary = stringResource(id = R.string.mobile_data_settings_summary)
+    val isMobileDataEnabled by
+        remember(subId) { mobileDataRepository.isMobileDataEnabledFlow(subId) }
+            .collectAsStateWithLifecycle(initialValue = null)
+
+    SwitchPreference(
+        object : SwitchPreferenceModel {
+            override val title = stringResource(id = R.string.mobile_data_settings_title)
+            override val summary = { mobileDataSummary }
+            override val checked = { isMobileDataEnabled }
+            override val onCheckedChange = setMobileData
+        }
+    )
+}
+
+@Composable
+private fun setMobileDataImpl(subId: Int): (newChecked: Boolean) -> Unit {
+    val context = LocalContext.current
+    val coroutineScope = rememberCoroutineScope()
+    val wifiPickerTrackerHelper = rememberWifiPickerTrackerHelper()
+    return { newEnabled ->
+        coroutineScope.launch(Dispatchers.Default) {
+            setMobileData(
+                context = context,
+                subscriptionManager = context.subscriptionManager,
+                wifiPickerTrackerHelper = wifiPickerTrackerHelper,
+                subId = subId,
+                enabled = newEnabled,
+            )
+        }
+    }
+}
--- a/src/com/android/settings/spa/network/MobileDataSwitchingPreference.kt
+++ b/src/com/android/settings/spa/network/MobileDataSwitchingPreference.kt
@@ -1,48 +0,0 @@
-/*
- * Copyright (C) 2024 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.android.settings.spa.network
-
-import androidx.compose.runtime.Composable
-import androidx.compose.runtime.rememberCoroutineScope
-import androidx.compose.ui.res.stringResource
-import com.android.settings.R
-import com.android.settingslib.spa.widget.preference.SwitchPreference
-import com.android.settingslib.spa.widget.preference.SwitchPreferenceModel
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.launch
-
-@Composable
-fun MobileDataSwitchingPreference(
-    isMobileDataEnabled: () -> Boolean?,
-    setMobileDataEnabled: (newEnabled: Boolean) -> Unit,
-) {
-    val mobileDataSummary = stringResource(id = R.string.mobile_data_settings_summary)
-    val coroutineScope = rememberCoroutineScope()
-    SwitchPreference(
-        object : SwitchPreferenceModel {
-            override val title = stringResource(id = R.string.mobile_data_settings_title)
-            override val summary = { mobileDataSummary }
-            override val checked = { isMobileDataEnabled() }
-            override val onCheckedChange: (Boolean) -> Unit = { newEnabled ->
-                coroutineScope.launch(Dispatchers.Default) {
-                    setMobileDataEnabled(newEnabled)
-                }
-            }
-            override val changeable:() -> Boolean = {true}
-        }
-    )
-}
--- a/src/com/android/settings/spa/network/NetworkCellularGroupProvider.kt
+++ b/src/com/android/settings/spa/network/NetworkCellularGroupProvider.kt
@@ -29,6 +29,7 @@ import androidx.compose.material.icons.Icons
 import androidx.compose.material.icons.automirrored.outlined.Message
 import androidx.compose.material.icons.outlined.DataUsage
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.MutableIntState
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableIntStateOf
@@ -40,7 +41,6 @@ import androidx.compose.ui.graphics.vector.ImageVector
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.res.vectorResource
-import androidx.lifecycle.LifecycleOwner
 import androidx.lifecycle.LifecycleRegistry
 import androidx.lifecycle.compose.LocalLifecycleOwner
 import androidx.lifecycle.compose.collectAsStateWithLifecycle
@@ -60,7 +60,6 @@ import com.android.settingslib.spa.framework.common.SettingsPageProvider
 import com.android.settingslib.spa.framework.common.createSettingsPage
 import com.android.settingslib.spa.framework.compose.navigator
 import com.android.settingslib.spa.framework.compose.rememberContext
-import com.android.settingslib.spa.framework.util.collectLatestWithLifecycle
 import com.android.settingslib.spa.widget.preference.Preference
 import com.android.settingslib.spa.widget.preference.PreferenceModel
 import com.android.settingslib.spa.widget.scaffold.RegularScaffold
@@ -110,51 +109,48 @@ open class NetworkCellularGroupProvider : SettingsPageProvider, SearchablePage {
        var textsSelectedId = rememberSaveable {
            mutableIntStateOf(SubscriptionManager.INVALID_SUBSCRIPTION_ID)
        }
-        var mobileDataSelectedId = rememberSaveable {
-            mutableIntStateOf(SubscriptionManager.INVALID_SUBSCRIPTION_ID)
-        }
+        val mobileDataSelectedId = rememberSaveable { mutableStateOf<Int?>(null) }
        var nonDdsRemember = rememberSaveable {
            mutableIntStateOf(SubscriptionManager.INVALID_SUBSCRIPTION_ID)
        }
-        var showMobileDataSection = rememberSaveable {
-            mutableStateOf(false)
-        }
        val subscriptionViewModel = viewModel<SubscriptionInfoListViewModel>()

        CollectAirplaneModeAndFinishIfOn()

-        remember {
-            allOfFlows(context, subscriptionViewModel.selectableSubscriptionInfoListFlow)
-        }.collectLatestWithLifecycle(LocalLifecycleOwner.current) {
-            callsSelectedId.intValue = defaultVoiceSubId
-            textsSelectedId.intValue = defaultSmsSubId
-            mobileDataSelectedId.intValue = defaultDataSubId
-            nonDdsRemember.intValue = nonDds
+        LaunchedEffect(Unit) {
+            allOfFlows(context, subscriptionViewModel.selectableSubscriptionInfoListFlow).collect {
+                callsSelectedId.intValue = defaultVoiceSubId
+                textsSelectedId.intValue = defaultSmsSubId
+                mobileDataSelectedId.value = defaultDataSubId
+                nonDdsRemember.intValue = nonDds
+            }
        }

        val selectableSubscriptionInfoList by subscriptionViewModel
                .selectableSubscriptionInfoListFlow
                .collectAsStateWithLifecycle(initialValue = emptyList())
-        showMobileDataSection.value = selectableSubscriptionInfoList
-                .filter { subInfo -> subInfo.simSlotIndex > -1 }
-                .size > 0
-        val stringSims = stringResource(R.string.provider_network_settings_title)
-        RegularScaffold(title = stringSims) {
+
+        RegularScaffold(title = stringResource(R.string.provider_network_settings_title)) {
            SimsSection(selectableSubscriptionInfoList)
-            if(showMobileDataSection.value) {
-                MobileDataSectionImpl(
-                    mobileDataSelectedId,
-                    nonDdsRemember,
+            val mobileDataSelectedIdValue = mobileDataSelectedId.value
+            // Avoid draw mobile data UI before data ready to reduce flaky
+            if (mobileDataSelectedIdValue != null) {
+                val showMobileDataSection =
+                    selectableSubscriptionInfoList.any { subInfo -> subInfo.simSlotIndex > -1 }
+                if (showMobileDataSection) {
+                    MobileDataSectionImpl(mobileDataSelectedIdValue, nonDdsRemember.intValue)
+                }
+
+                PrimarySimSectionImpl(
+                    subscriptionViewModel.selectableSubscriptionInfoListFlow,
+                    callsSelectedId,
+                    textsSelectedId,
+                    remember(mobileDataSelectedIdValue) {
+                        mutableIntStateOf(mobileDataSelectedIdValue)
+                    },
                )
            }

-            PrimarySimSectionImpl(
-                subscriptionViewModel.selectableSubscriptionInfoListFlow,
-                callsSelectedId,
-                textsSelectedId,
-                mobileDataSelectedId,
-            )
-
            OtherSection()
        }
    }
@@ -217,46 +213,23 @@ open class NetworkCellularGroupProvider : SettingsPageProvider, SearchablePage {
 }

@Composable
-fun MobileDataSectionImpl(
-    mobileDataSelectedId: MutableIntState,
-    nonDds: MutableIntState,
-) {
-    val context = LocalContext.current
-    val localLifecycleOwner = LocalLifecycleOwner.current
+fun MobileDataSectionImpl(mobileDataSelectedId: Int, nonDds: Int) {
    val mobileDataRepository = rememberContext(::MobileDataRepository)

    Category(title = stringResource(id = R.string.mobile_data_settings_title)) {
-        val isAutoDataEnabled by remember(nonDds.intValue) {
+        MobileDataSwitchPreference(subId = mobileDataSelectedId)
+
+        val isAutoDataEnabled by remember(nonDds) {
            mobileDataRepository.isMobileDataPolicyEnabledFlow(
-                subId = nonDds.intValue,
+                subId = nonDds,
                policy = TelephonyManager.MOBILE_DATA_POLICY_AUTO_DATA_SWITCH
            )
        }.collectAsStateWithLifecycle(initialValue = null)
-
-        val mobileDataStateChanged by remember(mobileDataSelectedId.intValue) {
-            mobileDataRepository.isMobileDataEnabledFlow(mobileDataSelectedId.intValue)
-        }.collectAsStateWithLifecycle(initialValue = false)
-        val coroutineScope = rememberCoroutineScope()
-
-        MobileDataSwitchingPreference(
-            isMobileDataEnabled = { mobileDataStateChanged },
-            setMobileDataEnabled = { newEnabled ->
-                coroutineScope.launch {
-                    setMobileData(
-                        context,
-                        context.getSystemService(SubscriptionManager::class.java),
-                        getWifiPickerTrackerHelper(context, localLifecycleOwner),
-                        mobileDataSelectedId.intValue,
-                        newEnabled
-                    )
-                }
-           },
-        )
-        if (nonDds.intValue != SubscriptionManager.INVALID_SUBSCRIPTION_ID) {
+        if (SubscriptionManager.isValidSubscriptionId(nonDds)) {
            AutomaticDataSwitchingPreference(
                isAutoDataEnabled = { isAutoDataEnabled },
                setAutoDataEnabled = { newEnabled ->
-                    mobileDataRepository.setAutoDataSwitch(nonDds.intValue, newEnabled)
+                    mobileDataRepository.setAutoDataSwitch(nonDds, newEnabled)
                },
            )
        }
@@ -328,9 +301,6 @@ fun PrimarySimSectionImpl(
    mobileDataSelectedId: MutableIntState,
 ) {
    val context = LocalContext.current
-    val localLifecycleOwner = LocalLifecycleOwner.current
-    val wifiPickerTrackerHelper = getWifiPickerTrackerHelper(context, localLifecycleOwner)
-
    val primarySimInfo = remember(subscriptionInfoListFlow) {
        subscriptionInfoListFlow
            .map { subscriptionInfoList ->
@@ -346,7 +316,7 @@ fun PrimarySimSectionImpl(
            callsSelectedId,
            textsSelectedId,
            mobileDataSelectedId,
-            wifiPickerTrackerHelper
+            rememberWifiPickerTrackerHelper()
        )
    }
 }
@@ -354,22 +324,21 @@ fun PrimarySimSectionImpl(
@Composable
 fun CollectAirplaneModeAndFinishIfOn() {
    val context = LocalContext.current
-    context.settingsGlobalBooleanFlow(Settings.Global.AIRPLANE_MODE_ON)
-        .collectLatestWithLifecycle(LocalLifecycleOwner.current) { isAirplaneModeOn ->
+    LaunchedEffect(Unit) {
+        context.settingsGlobalBooleanFlow(Settings.Global.AIRPLANE_MODE_ON).collect {
+            isAirplaneModeOn ->
            if (isAirplaneModeOn) {
                context.getActivity()?.finish()
            }
        }
+    }
 }

-private fun getWifiPickerTrackerHelper(
-    context: Context,
-    lifecycleOwner: LifecycleOwner
-): WifiPickerTrackerHelper {
-    return WifiPickerTrackerHelper(
-        LifecycleRegistry(lifecycleOwner), context,
-        null /* WifiPickerTrackerCallback */
-    )
+@Composable
+fun rememberWifiPickerTrackerHelper(): WifiPickerTrackerHelper {
+    val context = LocalContext.current
+    val lifecycleOwner = LocalLifecycleOwner.current
+    return remember { WifiPickerTrackerHelper(LifecycleRegistry(lifecycleOwner), context, null) }
 }

 private fun Context.defaultVoiceSubscriptionFlow(): Flow<Int> =
--- a/tests/spa_unit/src/com/android/settings/spa/network/MobileDataSwitchPreferenceTest.kt
+++ b/tests/spa_unit/src/com/android/settings/spa/network/MobileDataSwitchPreferenceTest.kt
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.settings.spa.network
+
+import android.content.Context
+import androidx.compose.runtime.CompositionLocalProvider
+import androidx.compose.ui.platform.LocalContext
+import androidx.compose.ui.test.assertIsDisplayed
+import androidx.compose.ui.test.junit4.createComposeRule
+import androidx.compose.ui.test.onNodeWithText
+import androidx.compose.ui.test.performClick
+import androidx.test.core.app.ApplicationProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.android.settings.R
+import com.android.settings.network.telephony.MobileDataRepository
+import com.google.common.truth.Truth.assertThat
+import kotlinx.coroutines.flow.emptyFlow
+import kotlinx.coroutines.flow.flowOf
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.kotlin.any
+import org.mockito.kotlin.doReturn
+import org.mockito.kotlin.mock
+import org.mockito.kotlin.spy
+import org.mockito.kotlin.stub
+
+@RunWith(AndroidJUnit4::class)
+class MobileDataSwitchPreferenceTest {
+    @get:Rule val composeTestRule = createComposeRule()
+
+    private val context: Context = spy(ApplicationProvider.getApplicationContext()) {}
+
+    private val mockMobileDataRepository =
+        mock<MobileDataRepository> { on { isMobileDataEnabledFlow(any()) } doReturn emptyFlow() }
+
+    @Test
+    fun title_displayed() {
+        composeTestRule.setContent {
+            CompositionLocalProvider(LocalContext provides context) {
+                MobileDataSwitchPreference(SUB_ID, mockMobileDataRepository) {}
+            }
+        }
+
+        composeTestRule
+            .onNodeWithText(context.getString(R.string.mobile_data_settings_title))
+            .assertIsDisplayed()
+    }
+
+    @Test
+    fun summary_displayed() {
+        composeTestRule.setContent {
+            CompositionLocalProvider(LocalContext provides context) {
+                MobileDataSwitchPreference(SUB_ID, mockMobileDataRepository) {}
+            }
+        }
+
+        composeTestRule
+            .onNodeWithText(context.getString(R.string.mobile_data_settings_summary))
+            .assertIsDisplayed()
+    }
+
+    @Test
+    fun onClick_whenOff_turnedOn() {
+        mockMobileDataRepository.stub {
+            on { isMobileDataEnabledFlow(SUB_ID) } doReturn flowOf(false)
+        }
+        var newCheckedCalled: Boolean? = null
+        composeTestRule.setContent {
+            CompositionLocalProvider(LocalContext provides context) {
+                MobileDataSwitchPreference(SUB_ID, mockMobileDataRepository) {
+                    newCheckedCalled = it
+                }
+            }
+        }
+
+        composeTestRule
+            .onNodeWithText(context.getString(R.string.mobile_data_settings_title))
+            .performClick()
+
+        assertThat(newCheckedCalled).isTrue()
+    }
+
+    private companion object {
+        const val SUB_ID = 12
+    }
+}