Fixes based on feedback

mullvad · Aug 30, 2024 · cbe21e7 · cbe21e7
1 parent ad2b330
commit cbe21e7
Show file tree

Hide file tree

Showing 15 changed files with 124 additions and 132 deletions.
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt
@@ -33,7 +33,7 @@ import net.mullvad.mullvadvpn.lib.theme.typeface.listItemText
 @Composable
 private fun PreviewSelectObfuscationCellCell(
     @PreviewParameter(SelectObfuscationCellPreviewParameterProvider::class)
-    selectedObfuscationCellData: Triple<SelectedObfuscation, Constraint<Port>?, Boolean>
+    selectedObfuscationCellData: Triple<SelectedObfuscation, Constraint<Port>, Boolean>
 ) {
     AppTheme {
         SelectObfuscationCell(
@@ -49,7 +49,7 @@ private fun PreviewSelectObfuscationCellCell(
 @Composable
 fun SelectObfuscationCell(
     selectedObfuscation: SelectedObfuscation,
-    port: Constraint<Port>? = null,
+    port: Constraint<Port>,
     isSelected: Boolean,
     onSelected: (SelectedObfuscation) -> Unit,
     onNavigate: () -> Unit = {},
@@ -67,7 +67,7 @@ fun SelectObfuscationCell(
             subtitleStyle = MaterialTheme.typography.listItemSubText,
             subtitleColor = MaterialTheme.colorScheme.onSurface,
             titleText = selectedObfuscation.toTitle(),
-            subtitleText = port.toSubTitle()?.let { stringResource(id = R.string.port_x, it) },
+            subtitleText = stringResource(id = R.string.port_x, port.toSubTitle()),
             onCellClicked = { onSelected(selectedObfuscation) },
             minHeight = Dimens.cellHeight,
             background =
@@ -84,22 +84,19 @@ fun SelectObfuscationCell(
                 )
             },
         )
-        if (port != null) {
-            VerticalDivider(
-                color = Color.Transparent,
-                modifier =
-                    Modifier.fillMaxHeight().padding(vertical = Dimens.verticalDividerPadding),
-            )
-            Icon(
-                painterResource(id = R.drawable.icon_chevron),
-                contentDescription = null,
-                tint = MaterialTheme.colorScheme.onPrimary,
-                modifier =
-                    Modifier.fillMaxHeight()
-                        .clickable { onNavigate() }
-                        .padding(horizontal = Dimens.obfuscationNavigationPadding),
-            )
-        }
+        VerticalDivider(
+            color = MaterialTheme.colorScheme.surface,
+            modifier = Modifier.fillMaxHeight().padding(vertical = Dimens.verticalDividerPadding),
+        )
+        Icon(
+            painterResource(id = R.drawable.icon_chevron),
+            contentDescription = null,
+            tint = MaterialTheme.colorScheme.onPrimary,
+            modifier =
+                Modifier.fillMaxHeight()
+                    .clickable { onNavigate() }
+                    .padding(horizontal = Dimens.obfuscationNavigationPadding),
+        )
     }
 }
 
@@ -113,9 +110,8 @@ private fun SelectedObfuscation.toTitle() =
     }
 
 @Composable
-private fun Constraint<Port>?.toSubTitle() =
+private fun Constraint<Port>.toSubTitle() =
     when (this) {
         Constraint.Any -> stringResource(id = R.string.automatic)
         is Constraint.Only -> this.value.toString()
-        null -> null
     }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectableCell.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectableCell.kt
@@ -106,8 +106,11 @@ fun RowScope.SelectableIcon(
         modifier =
             Modifier.padding(end = Dimens.selectableCellTextMargin)
                 .alpha(
-                    if (isSelected && !isEnabled) AlphaDisabled
-                    else if (isSelected) AlphaVisible else AlphaInvisible
+                    when {
+                        isSelected && !isEnabled -> AlphaDisabled
+                        isSelected -> AlphaVisible
+                        else -> AlphaInvisible
+                    }
                 ),
     )
 }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt
@@ -26,17 +26,16 @@ private fun PreviewTwoRowCell() {
 @Composable
 fun TwoRowCell(
     titleText: String,
-    subtitleText: String?,
+    subtitleText: String,
     modifier: Modifier = Modifier,
     bodyView: @Composable ColumnScope.() -> Unit = {},
     iconView: @Composable RowScope.() -> Unit = {},
-    onCellClicked: () -> Unit = {},
+    onCellClicked: (() -> Unit)? = null,
     titleColor: Color = MaterialTheme.colorScheme.onPrimary,
     subtitleColor: Color = MaterialTheme.colorScheme.onPrimary,
     titleStyle: TextStyle = MaterialTheme.typography.labelLarge,
     subtitleStyle: TextStyle = MaterialTheme.typography.labelLarge,
     background: Color = MaterialTheme.colorScheme.primary,
-    isRowEnabled: Boolean = true,
     endPadding: Dp = Dimens.cellEndPadding,
     minHeight: Dp = Dimens.cellHeightTwoRows,
 ) {
@@ -52,23 +51,21 @@ fun TwoRowCell(
                     maxLines = 1,
                     overflow = TextOverflow.Ellipsis,
                 )
-                subtitleText?.let {
-                    Text(
-                        modifier = Modifier.fillMaxWidth(),
-                        text = subtitleText,
-                        style = subtitleStyle,
-                        color = subtitleColor,
-                        maxLines = 1,
-                        overflow = TextOverflow.Ellipsis,
-                    )
-                }
+                Text(
+                    modifier = Modifier.fillMaxWidth(),
+                    text = subtitleText,
+                    style = subtitleStyle,
+                    color = subtitleColor,
+                    maxLines = 1,
+                    overflow = TextOverflow.Ellipsis,
+                )
             }
         },
         bodyView = bodyView,
         iconView = iconView,
-        onCellClicked = onCellClicked,
+        onCellClicked = onCellClicked ?: {},
         background = background,
-        isRowEnabled = isRowEnabled,
+        isRowEnabled = onCellClicked != null,
         minHeight = minHeight,
         endPadding = endPadding,
     )

diff --git a/...n/net/mullvad/mullvadvpn/compose/preview/SelectObfuscationCellPreviewParameterProvider.kt b/...n/net/mullvad/mullvadvpn/compose/preview/SelectObfuscationCellPreviewParameterProvider.kt
@@ -6,11 +6,9 @@ import net.mullvad.mullvadvpn.lib.model.Port
 import net.mullvad.mullvadvpn.lib.model.SelectedObfuscation
 
 class SelectObfuscationCellPreviewParameterProvider :
-    PreviewParameterProvider<Triple<SelectedObfuscation, Constraint<Port>?, Boolean>> {
-    override val values: Sequence<Triple<SelectedObfuscation, Constraint<Port>?, Boolean>> =
+    PreviewParameterProvider<Triple<SelectedObfuscation, Constraint<Port>, Boolean>> {
+    override val values: Sequence<Triple<SelectedObfuscation, Constraint<Port>, Boolean>> =
         sequenceOf(
-            Triple(SelectedObfuscation.Auto, null, false),
-            Triple(SelectedObfuscation.Auto, null, true),
             Triple(SelectedObfuscation.Shadowsocks, Constraint.Any, false),
             Triple(SelectedObfuscation.Shadowsocks, Constraint.Any, true),
             Triple(SelectedObfuscation.Shadowsocks, Constraint.Only(Port(PORT)), false),
@@ -19,8 +17,6 @@ class SelectObfuscationCellPreviewParameterProvider :
             Triple(SelectedObfuscation.Udp2Tcp, Constraint.Any, true),
             Triple(SelectedObfuscation.Udp2Tcp, Constraint.Only(Port(PORT)), false),
             Triple(SelectedObfuscation.Udp2Tcp, Constraint.Only(Port(PORT)), true),
-            Triple(SelectedObfuscation.Off, null, false),
-            Triple(SelectedObfuscation.Off, null, true),
         )
 }
 

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DeviceListScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/DeviceListScreen.kt
@@ -345,7 +345,7 @@ private fun DeviceListItem(device: Device, isLoading: Boolean, onDeviceRemovalCl
                 }
             }
         },
-        isRowEnabled = false,
+        onCellClicked = null,
         endPadding = Dimens.smallPadding,
         minHeight = Dimens.cellHeight,
     )

diff --git a/...id/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt b/...id/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt
@@ -31,8 +31,6 @@ import net.mullvad.mullvadvpn.constant.SHADOWSOCKS_PRESET_PORTS
 import net.mullvad.mullvadvpn.lib.model.Constraint
 import net.mullvad.mullvadvpn.lib.model.Port
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
-import net.mullvad.mullvadvpn.util.hasValue
-import net.mullvad.mullvadvpn.util.isCustom
 import net.mullvad.mullvadvpn.viewmodel.ShadowsocksSettingsViewModel
 import org.koin.androidx.compose.koinViewModel
 
@@ -68,17 +66,19 @@ fun ShadowsocksSettings(
         state = state,
         navigateToCustomPortDialog =
             dropUnlessResumed {
-                val args =
-                    CustomPortNavArgs(
-                        title =
-                            context.getString(
-                                R.string.custom_port_dialog_title,
-                                context.getString(R.string.shadowsocks),
-                            ),
-                        customPort = state.customPort,
-                        allowedPortRanges = state.validPortRanges,
+                navigator.navigate(
+                    CustomPortDestination(
+                        CustomPortNavArgs(
+                            title =
+                                context.getString(
+                                    R.string.custom_port_dialog_title,
+                                    context.getString(R.string.shadowsocks),
+                                ),
+                            customPort = state.customPort,
+                            allowedPortRanges = state.validPortRanges,
+                        )
                     )
-                navigator.navigate(CustomPortDestination(args))
+                )
             },
         onObfuscationPortSelected = viewModel::onObfuscationPortSelected,
         onBackClick = dropUnlessResumed { navigator.navigateUp() },
@@ -110,16 +110,16 @@ fun ShadowsocksSettingsScreen(
                 SHADOWSOCKS_PRESET_PORTS.forEach { port ->
                     SelectableCell(
                         title = port.toString(),
-                        isSelected = state.port.hasValue(port),
-                        onCellClicked = { onObfuscationPortSelected(Constraint.Only(Port(port))) },
-                        testTag = String.format(null, SHADOWSOCKS_PORT_ITEM_X_TEST_TAG, port),
+                        isSelected = state.port.getOrNull() == port,
+                        onCellClicked = { onObfuscationPortSelected(Constraint.Only(port)) },
+                        testTag = String.format(null, SHADOWSOCKS_PORT_ITEM_X_TEST_TAG, port.value),
                     )
                 }
             }
             itemWithDivider {
                 CustomPortCell(
                     title = stringResource(id = R.string.wireguard_custon_port_title),
-                    isSelected = state.port.isCustom(SHADOWSOCKS_PRESET_PORTS),
+                    isSelected = state.isCustom,
                     port = state.customPort,
                     onMainCellClicked = {
                         if (state.customPort != null) {

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/Udp2TcpSettingsScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/Udp2TcpSettingsScreen.kt
@@ -25,7 +25,6 @@ import net.mullvad.mullvadvpn.constant.UDP2TCP_PRESET_PORTS
 import net.mullvad.mullvadvpn.lib.model.Constraint
 import net.mullvad.mullvadvpn.lib.model.Port
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
-import net.mullvad.mullvadvpn.util.hasValue
 import net.mullvad.mullvadvpn.viewmodel.Udp2TcpSettingsViewModel
 import org.koin.androidx.compose.koinViewModel
 
@@ -79,9 +78,9 @@ fun Udp2TcpSettingsScreen(
                 UDP2TCP_PRESET_PORTS.forEach { port ->
                     SelectableCell(
                         title = port.toString(),
-                        isSelected = state.port.hasValue(port),
-                        onCellClicked = { onObfuscationPortSelected(Constraint.Only(Port(port))) },
-                        testTag = String.format(null, UDP_OVER_TCP_PORT_ITEM_X_TEST_TAG, port),
+                        isSelected = state.port.getOrNull() == port,
+                        onCellClicked = { onObfuscationPortSelected(Constraint.Only(port)) },
+                        testTag = String.format(null, UDP_OVER_TCP_PORT_ITEM_X_TEST_TAG, port.value),
                     )
                 }
             }

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
@@ -97,9 +97,6 @@ import net.mullvad.mullvadvpn.lib.model.QuantumResistantState
 import net.mullvad.mullvadvpn.lib.model.SelectedObfuscation
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
 import net.mullvad.mullvadvpn.lib.theme.Dimens
-import net.mullvad.mullvadvpn.util.hasValue
-import net.mullvad.mullvadvpn.util.isCustom
-import net.mullvad.mullvadvpn.util.toPortOrNull
 import net.mullvad.mullvadvpn.viewmodel.CustomDnsItem
 import net.mullvad.mullvadvpn.viewmodel.VpnSettingsSideEffect
 import net.mullvad.mullvadvpn.viewmodel.VpnSettingsViewModel
@@ -238,17 +235,19 @@ fun VpnSettings(
             },
         navigateToWireguardPortDialog =
             dropUnlessResumed {
-                val args =
-                    CustomPortNavArgs(
-                        title =
-                            context.getString(
-                                R.string.custom_port_dialog_title,
-                                context.getString(R.string.wireguard),
-                            ),
-                        customPort = state.customWireguardPort?.toPortOrNull(),
-                        allowedPortRanges = state.availablePortRanges,
+                navigator.navigate(
+                    CustomPortDestination(
+                        CustomPortNavArgs(
+                            title =
+                                context.getString(
+                                    R.string.custom_port_dialog_title,
+                                    context.getString(R.string.wireguard),
+                                ),
+                            customPort = state.customWireguardPort,
+                            allowedPortRanges = state.availablePortRanges,
+                        )
                     )
-                navigator.navigate(CustomPortDestination(args))
+                )
             },
         onToggleDnsClick = vm::onToggleCustomDns,
         onBackClick = dropUnlessResumed { navigator.navigateUp() },
@@ -522,21 +521,25 @@ fun VpnSettingsScreen(
                     SelectableCell(
                         title = port.toString(),
                         testTag =
-                            String.format(null, LAZY_LIST_WIREGUARD_PORT_ITEM_X_TEST_TAG, port),
-                        isSelected = state.selectedWireguardPort.hasValue(port),
-                        onCellClicked = { onWireguardPortSelected(Constraint.Only(Port(port))) },
+                            String.format(
+                                null,
+                                LAZY_LIST_WIREGUARD_PORT_ITEM_X_TEST_TAG,
+                                port.value,
+                            ),
+                        isSelected = state.selectedWireguardPort.getOrNull() == port,
+                        onCellClicked = { onWireguardPortSelected(Constraint.Only(port)) },
                     )
                 }
             }
 
             itemWithDivider {
                 CustomPortCell(
                     title = stringResource(id = R.string.wireguard_custon_port_title),
-                    isSelected = state.selectedWireguardPort.isCustom(WIREGUARD_PRESET_PORTS),
-                    port = state.customWireguardPort?.toPortOrNull(),
+                    isSelected = state.isCustomWireguardPort,
+                    port = state.customWireguardPort,
                     onMainCellClicked = {
                         if (state.customWireguardPort != null) {
-                            onWireguardPortSelected(state.customWireguardPort)
+                            onWireguardPortSelected(Constraint.Only(state.customWireguardPort))
                         } else {
                             navigateToWireguardPortDialog()
                         }
@@ -556,10 +559,10 @@ fun VpnSettingsScreen(
                 )
             }
             itemWithDivider {
-                SelectObfuscationCell(
-                    selectedObfuscation = SelectedObfuscation.Auto,
+                SelectableCell(
+                    title = stringResource(id = R.string.automatic),
                     isSelected = state.selectedObfuscation == SelectedObfuscation.Auto,
-                    onSelected = onSelectObfuscationSetting,
+                    onCellClicked = { onSelectObfuscationSetting(SelectedObfuscation.Auto) },
                 )
             }
             itemWithDivider {
@@ -581,10 +584,10 @@ fun VpnSettingsScreen(
                 )
             }
             itemWithDivider {
-                SelectObfuscationCell(
-                    selectedObfuscation = SelectedObfuscation.Off,
+                SelectableCell(
+                    title = stringResource(id = R.string.off),
                     isSelected = state.selectedObfuscation == SelectedObfuscation.Off,
-                    onSelected = onSelectObfuscationSetting,
+                    onCellClicked = { onSelectObfuscationSetting(SelectedObfuscation.Off) },
                 )
             }
 

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/ShadowsocksSettingsState.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/ShadowsocksSettingsState.kt
@@ -8,4 +8,6 @@ data class ShadowsocksSettingsState(
     val port: Constraint<Port> = Constraint.Any,
     val customPort: Port? = null,
     val validPortRanges: List<PortRange> = emptyList(),
-)
+) {
+    val isCustom = port is Constraint.Only && port.value == customPort
+}
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VpnSettingsUiState.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VpnSettingsUiState.kt
@@ -21,11 +21,13 @@ data class VpnSettingsUiState(
     val selectedShadowsSocksObfuscationPort: Constraint<Port>,
     val quantumResistant: QuantumResistantState,
     val selectedWireguardPort: Constraint<Port>,
-    val customWireguardPort: Constraint<Port>?,
+    val customWireguardPort: Port?,
     val availablePortRanges: List<PortRange>,
     val systemVpnSettingsAvailable: Boolean,
 ) {
-    val selectObfuscationPortEnabled = selectedObfuscation != SelectedObfuscation.Off
+    val isCustomWireguardPort =
+        selectedWireguardPort is Constraint.Only &&
+            selectedWireguardPort.value == customWireguardPort
 
     companion object {
         fun createDefault(
@@ -40,7 +42,7 @@ data class VpnSettingsUiState(
             selectedShadowsSocksObfuscationPort: Constraint<Port> = Constraint.Any,
             quantumResistant: QuantumResistantState = QuantumResistantState.Off,
             selectedWireguardPort: Constraint<Port> = Constraint.Any,
-            customWireguardPort: Constraint.Only<Port>? = null,
+            customWireguardPort: Port? = null,
             availablePortRanges: List<PortRange> = emptyList(),
             systemVpnSettingsAvailable: Boolean = false,
         ) =

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt
@@ -1,5 +1,7 @@
 package net.mullvad.mullvadvpn.constant
 
-val WIREGUARD_PRESET_PORTS = listOf(51820, 53)
-val UDP2TCP_PRESET_PORTS = listOf(80, 5001)
-val SHADOWSOCKS_PRESET_PORTS = listOf(80, 443)
+import net.mullvad.mullvadvpn.lib.model.Port
+
+val WIREGUARD_PRESET_PORTS = listOf(Port(51820), Port(53))
+val UDP2TCP_PRESET_PORTS = listOf(Port(80), Port(5001))
+val SHADOWSOCKS_PRESET_PORTS = listOf(Port(80), Port(443))