Fixed some more based on feedback

mullvad · Mar 12, 2024 · 8d23e02 · 8d23e02
1 parent 0169a3b
commit 8d23e02
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 73 deletions.
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt
@@ -43,7 +43,7 @@ import net.mullvad.mullvadvpn.lib.theme.color.AlphaInvisible
 import net.mullvad.mullvadvpn.lib.theme.color.AlphaVisible
 import net.mullvad.mullvadvpn.lib.theme.color.selected
 import net.mullvad.mullvadvpn.relaylist.RelayItem
-import net.mullvad.mullvadvpn.relaylist.allChildren
+import net.mullvad.mullvadvpn.relaylist.children
 
 @Composable
 @Preview
@@ -257,14 +257,17 @@ private fun RelayLocationCell(
                 ) {
                     leadingContent(relay)
                 }
-                Name(relay = relay)
+                Name(
+                    modifier = Modifier.weight(1f).align(Alignment.CenterVertically),
+                    relay = relay
+                )
             }
             if (relay.hasChildren) {
                 ExpandButton(isExpanded = expanded.value) { expand -> expanded.value = expand }
             }
         }
         if (expanded.value) {
-            relay.allChildren(false).forEach {
+            relay.children().forEach {
                 RelayLocationCell(
                     relay = it,
                     onClick = onClick,
@@ -280,13 +283,12 @@ private fun RelayLocationCell(
 }
 
 @Composable
-private fun RowScope.Name(relay: RelayItem) {
+private fun Name(modifier: Modifier = Modifier, relay: RelayItem) {
     Text(
         text = relay.name,
         color = MaterialTheme.colorScheme.onPrimary,
         modifier =
-            Modifier.weight(1f)
-                .align(Alignment.CenterVertically)
+            modifier
                 .alpha(
                     if (relay.active) {
                         AlphaVisible

diff --git a/...c/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt b/...c/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt
@@ -7,7 +7,6 @@ import androidx.compose.material3.Icon
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.focus.FocusRequester
 import androidx.compose.ui.focus.focusRequester
@@ -22,6 +21,7 @@ import net.mullvad.mullvadvpn.R
 import net.mullvad.mullvadvpn.compose.button.NegativeButton
 import net.mullvad.mullvadvpn.compose.button.PrimaryButton
 import net.mullvad.mullvadvpn.compose.communication.CustomListResult
+import net.mullvad.mullvadvpn.compose.util.LaunchedEffectCollect
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
 import net.mullvad.mullvadvpn.lib.theme.Dimens
 import net.mullvad.mullvadvpn.viewmodel.DeleteCustomListConfirmationSideEffect
@@ -45,12 +45,10 @@ fun DeleteCustomList(
     val viewModel: DeleteCustomListConfirmationViewModel =
         koinViewModel(parameters = { parametersOf(customListId) })
 
-    LaunchedEffect(Unit) {
-        viewModel.uiSideEffect.collect {
-            when (it) {
-                is DeleteCustomListConfirmationSideEffect.ReturnWithResult ->
-                    navigator.navigateBack(result = it.result)
-            }
+    LaunchedEffectCollect(viewModel.uiSideEffect) {
+        when (it) {
+            is DeleteCustomListConfirmationSideEffect.ReturnWithResult ->
+                navigator.navigateBack(result = it.result)
         }
     }
 

diff --git a/...oid/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt b/...oid/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt
@@ -4,7 +4,6 @@ import androidx.compose.material3.AlertDialog
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.derivedStateOf
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
@@ -23,6 +22,7 @@ import net.mullvad.mullvadvpn.compose.communication.CustomListResult
 import net.mullvad.mullvadvpn.compose.component.CustomListNameTextField
 import net.mullvad.mullvadvpn.compose.state.UpdateCustomListUiState
 import net.mullvad.mullvadvpn.compose.test.EDIT_CUSTOM_LIST_DIALOG_INPUT_TEST_TAG
+import net.mullvad.mullvadvpn.compose.util.LaunchedEffectCollect
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
 import net.mullvad.mullvadvpn.viewmodel.EditCustomListNameDialogSideEffect
 import net.mullvad.mullvadvpn.viewmodel.EditCustomListNameDialogViewModel
@@ -44,12 +44,10 @@ fun EditCustomListName(
 ) {
     val vm: EditCustomListNameDialogViewModel =
         koinViewModel(parameters = { parametersOf(customListId, initialName) })
-    LaunchedEffect(Unit) {
-        vm.uiSideEffect.collect { sideEffect ->
-            when (sideEffect) {
-                is EditCustomListNameDialogSideEffect.ReturnWithResult -> {
-                    backNavigator.navigateBack(result = sideEffect.result)
-                }
+    LaunchedEffectCollect(vm.uiSideEffect) { sideEffect ->
+        when (sideEffect) {
+            is EditCustomListNameDialogSideEffect.ReturnWithResult -> {
+                backNavigator.navigateBack(result = sideEffect.result)
             }
         }
     }

diff --git a/...id/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt b/...id/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt
@@ -15,7 +15,6 @@ import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Text
 import androidx.compose.material3.TextButton
 import androidx.compose.runtime.Composable
-import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
@@ -44,6 +43,7 @@ import net.mullvad.mullvadvpn.compose.test.CIRCULAR_PROGRESS_INDICATOR
 import net.mullvad.mullvadvpn.compose.test.SAVE_BUTTON_TEST_TAG
 import net.mullvad.mullvadvpn.compose.textfield.SearchTextField
 import net.mullvad.mullvadvpn.compose.transitions.SlideInFromRightTransition
+import net.mullvad.mullvadvpn.compose.util.LaunchedEffectCollect
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
 import net.mullvad.mullvadvpn.lib.theme.Dimens
 import net.mullvad.mullvadvpn.lib.theme.color.AlphaScrollbar
@@ -84,13 +84,11 @@ fun CustomListLocations(
         }
     }
 
-    LaunchedEffect(Unit) {
-        customListsViewModel.uiSideEffect.collect { sideEffect ->
-            when (sideEffect) {
-                is CustomListLocationsSideEffect.ReturnWithResult ->
-                    backNavigator.navigateBack(result = sideEffect.result)
-                CustomListLocationsSideEffect.CloseScreen -> backNavigator.navigateBack()
-            }
+    LaunchedEffectCollect(customListsViewModel.uiSideEffect) { sideEffect ->
+        when (sideEffect) {
+            is CustomListLocationsSideEffect.ReturnWithResult ->
+                backNavigator.navigateBack(result = sideEffect.result)
+            CustomListLocationsSideEffect.CloseScreen -> backNavigator.navigateBack()
         }
     }
 

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effect.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effect.kt
@@ -2,11 +2,14 @@ package net.mullvad.mullvadvpn.compose.util
 
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
+import androidx.compose.runtime.rememberCoroutineScope
+import androidx.compose.runtime.saveable.rememberSaveable
 import androidx.compose.ui.platform.LocalLifecycleOwner
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.flowWithLifecycle
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.launch
 
 @Composable
 inline fun <T> LaunchedEffectCollect(
@@ -34,3 +37,12 @@ inline fun <T> CollectSideEffectWithLifecycle(
         }
     }
 }
+
+@Composable
+fun RunOnKeyChange(key: Any, block: suspend CoroutineScope.() -> Unit) {
+    val scope = rememberCoroutineScope()
+    rememberSaveable(key) {
+        scope.launch { block() }
+        key
+    }
+}
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effects.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effects.kt
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/CustomListExtensions.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/CustomListExtensions.kt
@@ -28,6 +28,6 @@ fun List<RelayItem.CustomList>.filterOnSearchTerm(searchTerm: String) =
 
 fun RelayItem.CustomList.canAddLocation(location: RelayItem) =
     this.locations.none { it.code == location.code } &&
-        this.locations.flatMap { it.allChildren() }.none { it.code == location.code }
+        this.locations.flatMap { it.descendants() }.none { it.code == location.code }
 
 fun List<RelayItem.CustomList>.getById(id: String) = this.find { it.id == id }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt
@@ -1,6 +1,5 @@
 package net.mullvad.mullvadvpn.relaylist
 
-import net.mullvad.mullvadvpn.model.GeographicLocationConstraint
 import net.mullvad.mullvadvpn.model.LocationConstraint
 
 fun RelayItem.toLocationConstraint(): LocationConstraint {
@@ -12,31 +11,16 @@ fun RelayItem.toLocationConstraint(): LocationConstraint {
     }
 }
 
-private fun RelayItem.toGeographicLocationConstraint(): GeographicLocationConstraint =
-    when (this) {
-        is RelayItem.Country -> this.location
-        is RelayItem.City -> this.location
-        is RelayItem.Relay -> this.location
-        is RelayItem.CustomList ->
-            throw IllegalArgumentException("CustomList is not a geographic location")
-    }
-
-fun List<RelayItem>.toGeographicLocationConstraints(): ArrayList<GeographicLocationConstraint> =
-    ArrayList(
-        this.map { it.toGeographicLocationConstraint() },
-    )
-
-fun RelayItem.allChildren(includeChildrenOfChildren: Boolean = true): List<RelayItem> {
+fun RelayItem.children(): List<RelayItem> {
     return when (this) {
-        is RelayItem.Country ->
-            cities +
-                if (includeChildrenOfChildren) {
-                    cities.flatMap { it.relays }
-                } else {
-                    emptyList()
-                }
+        is RelayItem.Country -> cities
         is RelayItem.City -> relays
         is RelayItem.CustomList -> locations
         else -> emptyList()
     }
 }
+
+fun RelayItem.descendants(): List<RelayItem> {
+    val children = children()
+    return children + children.flatMap { it.descendants() }
+}
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt
@@ -264,4 +264,4 @@ fun RelayList.getGeographicLocationConstraintByCode(code: String): GeographicLoc
 
 fun List<RelayItem.Country>.getRelayItemsByCodes(codes: List<String>): List<RelayItem> =
     this.filter { codes.contains(it.code) } +
-        this.flatMap { it.allChildren() }.filter { codes.contains(it.code) }
+        this.flatMap { it.descendants() }.filter { codes.contains(it.code) }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt
@@ -15,7 +15,7 @@ import net.mullvad.mullvadvpn.compose.communication.CustomListAction
 import net.mullvad.mullvadvpn.compose.communication.CustomListResult
 import net.mullvad.mullvadvpn.compose.state.CustomListLocationsUiState
 import net.mullvad.mullvadvpn.relaylist.RelayItem
-import net.mullvad.mullvadvpn.relaylist.allChildren
+import net.mullvad.mullvadvpn.relaylist.descendants
 import net.mullvad.mullvadvpn.relaylist.filterOnSearchTerm
 import net.mullvad.mullvadvpn.relaylist.getById
 import net.mullvad.mullvadvpn.usecase.RelayListUseCase
@@ -120,7 +120,7 @@ class CustomListLocationsViewModel(
     private fun selectLocation(relayItem: RelayItem) {
         viewModelScope.launch {
             _selectedLocations.update {
-                it?.plus(relayItem)?.plus(relayItem.allChildren()) ?: setOf(relayItem)
+                it?.plus(relayItem)?.plus(relayItem.descendants()) ?: setOf(relayItem)
             }
         }
     }
@@ -130,7 +130,7 @@ class CustomListLocationsViewModel(
             _selectedLocations.update {
                 val newSelectedLocations = it?.toMutableSet() ?: mutableSetOf()
                 newSelectedLocations.remove(relayItem)
-                newSelectedLocations.removeAll(relayItem.allChildren().toSet())
+                newSelectedLocations.removeAll(relayItem.descendants().toSet())
                 // If a parent is selected, deselect it, since we only want to select a parent if
                 // all children are selected
                 newSelectedLocations.deselectParents(relayItem)
@@ -191,7 +191,7 @@ class CustomListLocationsViewModel(
     }
 
     private fun List<RelayItem>.selectChildren(): Set<RelayItem> =
-        (this + flatMap { it.allChildren() }).toSet()
+        (this + flatMap { it.descendants() }).toSet()
 
     private suspend fun fetchInitialSelectedLocations() {
         _selectedLocations.value =