From 8d23e02dd25c8cf4e47f00c409241548503bd11d Mon Sep 17 00:00:00 2001 From: Jonatan Rhodin Date: Tue, 12 Mar 2024 16:32:09 +0100 Subject: [PATCH] Fixed some more based on feedback --- .../compose/cell/RelayLocationCell.kt | 14 +++++---- .../DeleteCustomListConfirmationDialog.kt | 12 ++++---- .../dialog/EditCustomListNameDialog.kt | 12 ++++---- .../screen/CustomListLocationsScreen.kt | 14 ++++----- .../mullvad/mullvadvpn/compose/util/Effect.kt | 12 ++++++++ .../mullvadvpn/compose/util/Effects.kt | 16 ---------- .../relaylist/CustomListExtensions.kt | 2 +- .../relaylist/RelayItemExtensions.kt | 30 +++++-------------- .../relaylist/RelayListExtensions.kt | 2 +- .../viewmodel/CustomListLocationsViewModel.kt | 8 ++--- 10 files changed, 49 insertions(+), 73 deletions(-) delete mode 100644 android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effects.kt 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 index f14b0f2cc720..032695be8895 100644 --- 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/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt index 8d74ae58f042..236dedec6a36 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt +++ b/android/app/src/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/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt index 14f3edea668b..9f46ee1d5aef 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt +++ b/android/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/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt index 1da2ccc33031..3bd924a18990 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt +++ b/android/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 index 42d5f6caa090..c8a0847e89c4 100644 --- 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 LaunchedEffectCollect( @@ -34,3 +37,12 @@ inline fun 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 deleted file mode 100644 index 80e7d7c9ca6b..000000000000 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effects.kt +++ /dev/null @@ -1,16 +0,0 @@ -package net.mullvad.mullvadvpn.compose.util - -import androidx.compose.runtime.Composable -import androidx.compose.runtime.rememberCoroutineScope -import androidx.compose.runtime.saveable.rememberSaveable -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.launch - -@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/relaylist/CustomListExtensions.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/CustomListExtensions.kt index 2fcab76572b1..6fb87a6af523 100644 --- 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.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.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 index 1a99995410bd..a71005a78a57 100644 --- 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.toGeographicLocationConstraints(): ArrayList = - ArrayList( - this.map { it.toGeographicLocationConstraint() }, - ) - -fun RelayItem.allChildren(includeChildrenOfChildren: Boolean = true): List { +fun RelayItem.children(): List { 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 { + 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 index 0bebe7730da7..882a3e42a4de 100644 --- 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.getRelayItemsByCodes(codes: List): List = 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 index 2434dcf331ed..5fa99306a35f 100644 --- 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.selectChildren(): Set = - (this + flatMap { it.allChildren() }).toSet() + (this + flatMap { it.descendants() }).toSet() private suspend fun fetchInitialSelectedLocations() { _selectedLocations.value =