Fix race condition in mtu dialog

mullvad · Sep 7, 2023 · b4ed289 · b4ed289
1 parent 827a84c
commit b4ed289
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 37 deletions.
diff --git a/...app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreenTest.kt b/...app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreenTest.kt
@@ -13,7 +13,6 @@ import androidx.compose.ui.test.performTextInput
 import io.mockk.MockKAnnotations
 import io.mockk.mockk
 import io.mockk.verify
-import io.mockk.verifyAll
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.flow.asSharedFlow
 import net.mullvad.mullvadvpn.compose.state.VpnSettingsUiState
@@ -134,11 +133,9 @@ class VpnSettingsScreenTest {
     @Test
     fun testMtuDialogTextInput() {
         // Arrange
-        val mockedInputHandler: (String) -> Unit = mockk(relaxed = true)
         composeTestRule.setContent {
             VpnSettingsScreen(
                 uiState = VpnSettingsUiState.MtuDialogUiState(mtuEditValue = EMPTY_STRING),
-                onMtuInputChange = mockedInputHandler,
                 toastMessagesSharedFlow = MutableSharedFlow<String>().asSharedFlow()
             )
         }
@@ -147,13 +144,13 @@ class VpnSettingsScreenTest {
         composeTestRule.onNodeWithText(EMPTY_STRING).performTextInput(VALID_DUMMY_MTU_VALUE)
 
         // Assert
-        verifyAll { mockedInputHandler.invoke(VALID_DUMMY_MTU_VALUE) }
+        composeTestRule.onNodeWithText(VALID_DUMMY_MTU_VALUE).assertExists()
     }
 
     @Test
     fun testMtuDialogSubmitOfValidValue() {
         // Arrange
-        val mockedSubmitHandler: () -> Unit = mockk(relaxed = true)
+        val mockedSubmitHandler: (Int) -> Unit = mockk(relaxed = true)
         composeTestRule.setContent {
             VpnSettingsScreen(
                 uiState = VpnSettingsUiState.MtuDialogUiState(mtuEditValue = VALID_DUMMY_MTU_VALUE),
@@ -166,7 +163,7 @@ class VpnSettingsScreenTest {
         composeTestRule.onNodeWithText("Submit").assertIsEnabled().performClick()
 
         // Assert
-        verify { mockedSubmitHandler.invoke() }
+        verify { mockedSubmitHandler.invoke(VALID_DUMMY_MTU_VALUE.toInt()) }
     }
 
     @Test

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/MtuDialog.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/MtuDialog.kt
@@ -14,7 +14,8 @@ import androidx.compose.material3.ButtonDefaults
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
-import androidx.compose.ui.ExperimentalComposeUiApi
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.focus.FocusRequester
 import androidx.compose.ui.focus.focusRequester
@@ -36,12 +37,10 @@ import net.mullvad.mullvadvpn.lib.theme.MullvadWhite20
 import net.mullvad.mullvadvpn.lib.theme.MullvadWhite60
 import net.mullvad.mullvadvpn.util.isValidMtu
 
-@OptIn(ExperimentalComposeUiApi::class)
 @Composable
 fun MtuDialog(
-    mtuValue: String,
-    onMtuValueChanged: (String) -> Unit,
-    onSave: () -> Unit,
+    mtuInitial: Int?,
+    onSave: (Int) -> Unit,
     onRestoreDefaultValue: () -> Unit,
     onDismiss: () -> Unit,
 ) {
@@ -55,8 +54,10 @@ fun MtuDialog(
     val textMediumSize = dimensionResource(id = R.dimen.text_medium_plus).value.sp
     val textBigSize = dimensionResource(id = R.dimen.text_big).value.sp
 
+    val mtu = remember { mutableStateOf(mtuInitial?.toString() ?: "") }
+
     val textFieldFocusRequester = FocusRequester()
-    val isValidMtu = mtuValue.toIntOrNull()?.isValidMtu() == true
+    val isValidMtu = mtu.value.toIntOrNull()?.isValidMtu() == true
 
     Dialog(
         // Fix for https://issuetracker.google.com/issues/221643630
@@ -80,12 +81,13 @@ fun MtuDialog(
                     Modifier.wrapContentSize().clickable { textFieldFocusRequester.requestFocus() }
                 ) {
                     MtuTextField(
-                        value = mtuValue,
-                        onValueChanged = { newMtuValue -> onMtuValueChanged(newMtuValue) },
+                        value = mtu.value,
+                        onValueChanged = { newMtuValue -> mtu.value = newMtuValue },
                         onFocusChange = {},
                         onSubmit = { newMtuValue ->
-                            if (newMtuValue.toIntOrNull()?.isValidMtu() == true) {
-                                onSave()
+                            val mtuInt = newMtuValue.toIntOrNull()
+                            if (mtuInt?.isValidMtu() == true) {
+                                onSave(mtuInt)
                             }
                         },
                         isEnabled = true,
@@ -121,7 +123,12 @@ fun MtuDialog(
                             disabledContainerColor = MullvadWhite20
                         ),
                     enabled = isValidMtu,
-                    onClick = { onSave() },
+                    onClick = {
+                        val mtuInt = mtu.value.toIntOrNull()
+                        if (mtuInt?.isValidMtu() == true) {
+                            onSave(mtuInt)
+                        }
+                    },
                     shape = MaterialTheme.shapes.small
                 ) {
                     Text(text = stringResource(R.string.submit_button), fontSize = textMediumSize)
@@ -138,7 +145,7 @@ fun MtuDialog(
                             containerColor = MullvadBlue,
                             contentColor = MullvadWhite
                         ),
-                    onClick = { onRestoreDefaultValue() },
+                    onClick = onRestoreDefaultValue,
                     shape = MaterialTheme.shapes.small
                 ) {
                     Text(
@@ -158,7 +165,7 @@ fun MtuDialog(
                             containerColor = MullvadBlue,
                             contentColor = Color.White
                         ),
-                    onClick = { onDismiss() },
+                    onClick = onDismiss,
                     shape = MaterialTheme.shapes.small
                 ) {
                     Text(text = stringResource(R.string.cancel), fontSize = textMediumSize)

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt
@@ -102,7 +102,6 @@ private fun PreviewVpnSettings() {
                     customDnsItems = listOf(CustomDnsItem("0.0.0.0", false)),
                 ),
             onMtuCellClick = {},
-            onMtuInputChange = {},
             onSaveMtuClick = {},
             onRestoreMtuClick = {},
             onCancelMtuDialogClick = {},
@@ -146,8 +145,7 @@ fun VpnSettingsScreen(
     lifecycleOwner: LifecycleOwner = LocalLifecycleOwner.current,
     uiState: VpnSettingsUiState,
     onMtuCellClick: () -> Unit = {},
-    onMtuInputChange: (String) -> Unit = {},
-    onSaveMtuClick: () -> Unit = {},
+    onSaveMtuClick: (Int) -> Unit = {},
     onRestoreMtuClick: () -> Unit = {},
     onCancelMtuDialogClick: () -> Unit = {},
     onToggleAutoConnect: (Boolean) -> Unit = {},
@@ -186,9 +184,8 @@ fun VpnSettingsScreen(
     when (uiState) {
         is VpnSettingsUiState.MtuDialogUiState -> {
             MtuDialog(
-                mtuValue = uiState.mtuEditValue,
-                onMtuValueChanged = { onMtuInputChange(it) },
-                onSave = { onSaveMtuClick() },
+                mtuInitial = uiState.mtuEditValue.toIntOrNull(),
+                onSave = { onSaveMtuClick(it) },
                 onRestoreDefaultValue = { onRestoreMtuClick() },
                 onDismiss = { onCancelMtuDialogClick() }
             )

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/VpnSettingsFragment.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/fragment/VpnSettingsFragment.kt
@@ -27,7 +27,6 @@ class VpnSettingsFragment : BaseFragment() {
                     VpnSettingsScreen(
                         uiState = state,
                         onMtuCellClick = vm::onMtuCellClick,
-                        onMtuInputChange = vm::onMtuInputChange,
                         onSaveMtuClick = vm::onSaveMtuClick,
                         onRestoreMtuClick = vm::onRestoreMtuClick,
                         onCancelMtuDialogClick = vm::onCancelDialogClick,

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt
@@ -109,18 +109,11 @@ class VpnSettingsViewModel(
         dialogState.update { VpnSettingsDialogState.MtuDialog(vmState.value.mtuValue) }
     }
 
-    fun onMtuInputChange(value: String) {
-        dialogState.update { VpnSettingsDialogState.MtuDialog(value) }
-    }
-
-    fun onSaveMtuClick() =
+    fun onSaveMtuClick(mtuValue: Int) =
         viewModelScope.launch(dispatcher) {
-            val dialog = dialogState.value as? VpnSettingsDialogState.MtuDialog
-            dialog
-                ?.mtuEditValue
-                ?.toIntOrNull()
-                ?.takeIf { it.isValidMtu() }
-                ?.let { mtu -> repository.setWireguardMtu(mtu) }
+            if (mtuValue.isValidMtu()) {
+                repository.setWireguardMtu(mtuValue)
+            }
             hideDialog()
         }