From c1ea62c1cb78b9943226eee792ab6de2ee33b822 Mon Sep 17 00:00:00 2001 From: Jonatan Rhodin Date: Mon, 28 Oct 2024 09:30:20 +0100 Subject: [PATCH] Fix review issues --- .../mullvadvpn/compose/cell/FilterRow.kt | 2 +- ...SettingsUiStatePreviewParameterProvider.kt | 4 +- .../compose/screen/SettingsScreen.kt | 37 +++++++++---------- .../screen/location/RelayListContent.kt | 2 +- .../screen/location/SearchLocationScreen.kt | 4 +- .../screen/location/SelectLocationList.kt | 15 ++++---- .../screen/location/SelectLocationScreen.kt | 4 +- .../mullvadvpn/compose/state/RelayListItem.kt | 4 +- .../compose/state/SettingsUiState.kt | 2 +- .../mullvadvpn/viewmodel/SettingsViewModel.kt | 4 +- .../location/RelayItemListCreator.kt | 12 +++--- .../location/SearchLocationViewModel.kt | 9 ++--- .../location/SelectLocationListViewModel.kt | 4 -- 13 files changed, 47 insertions(+), 56 deletions(-) diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt index e3118042e140..c744beae564b 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt @@ -46,7 +46,7 @@ fun FilterRow( Modifier.padding(horizontal = Dimens.searchFieldHorizontalPadding) .fillMaxWidth() .horizontalScroll(scrollState), - horizontalArrangement = Arrangement.spacedBy(Dimens.chipSpace, alignment = Alignment.Start), + horizontalArrangement = Arrangement.spacedBy(Dimens.chipSpace), ) { if (showTitle) { Text( diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/SettingsUiStatePreviewParameterProvider.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/SettingsUiStatePreviewParameterProvider.kt index c51c6bae437a..18f422a988af 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/SettingsUiStatePreviewParameterProvider.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/SettingsUiStatePreviewParameterProvider.kt @@ -11,14 +11,14 @@ class SettingsUiStatePreviewParameterProvider : PreviewParameterProvider Unit) { NavigationComposeCell( title = title, onClick = onMultihopClick, - bodyView = - @Composable { - NavigationCellBody( - content = - stringResource( - if (isMultihopEnabled) { - R.string.on - } else { - R.string.off - } - ), - contentBodyDescription = title, - textColor = MaterialTheme.colorScheme.onSurfaceVariant, - isExternalLink = false, - ) - }, + bodyView = { + NavigationCellBody( + content = + stringResource( + if (isMultihopEnabled) { + R.string.on + } else { + R.string.off + } + ), + contentBodyDescription = title, + textColor = MaterialTheme.colorScheme.onSurfaceVariant, + isExternalLink = false, + ) + }, ) } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt index 04806517ef1e..34580181058f 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt @@ -74,6 +74,7 @@ fun LazyListScope.relayListContent( listItem, relayListSelection, { onSelectRelay(listItem.item) }, + // Only direct children can be removed if (listItem.depth == 1) { { onUpdateBottomSheetState( @@ -99,7 +100,6 @@ fun LazyListScope.relayListContent( relayListSelection = relayListSelection, { onSelectRelay(listItem.item) }, { - // Only direct children can be removed onUpdateBottomSheetState( ShowLocationBottomSheet(customLists, listItem.item) ) diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt index 556ecb3f018e..69be6bd197a7 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt @@ -19,7 +19,6 @@ import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Scaffold import androidx.compose.material3.SearchBarDefaults -import androidx.compose.material3.SnackbarDuration import androidx.compose.material3.SnackbarHost import androidx.compose.material3.SnackbarHostState import androidx.compose.material3.Text @@ -119,8 +118,7 @@ fun SearchLocation( SearchLocationSideEffect.GenericError -> launch { snackbarHostState.showSnackbarImmediately( - message = context.getString(R.string.error_occurred), - duration = SnackbarDuration.Short, + message = context.getString(R.string.error_occurred) ) } } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt index 8182a0862f07..a8064f4a61ed 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt @@ -44,8 +44,7 @@ fun SelectLocationList( val lazyListState = rememberLazyListState() val stateActual = state RunOnKeyChange(stateActual is SelectLocationListUiState.Content) { - val index = stateActual.indexOfSelectedRelayItem() - if (index != -1) { + stateActual.indexOfSelectedRelayItem()?.let { index -> lazyListState.scrollToItem(index) lazyListState.animateScrollAndCentralizeItem(index) } @@ -85,21 +84,21 @@ private fun LazyListScope.loading() { } } -private fun SelectLocationListUiState.indexOfSelectedRelayItem(): Int = +private fun SelectLocationListUiState.indexOfSelectedRelayItem(): Int? = if (this is SelectLocationListUiState.Content) { relayListItems.indexOfFirst { when (it) { is RelayListItem.CustomListItem -> it.isSelected is RelayListItem.GeoLocationItem -> it.isSelected - is RelayListItem.CustomListEntryItem -> false - is RelayListItem.CustomListFooter -> false - RelayListItem.CustomListHeader -> false - RelayListItem.LocationHeader -> false + is RelayListItem.CustomListEntryItem, + is RelayListItem.CustomListFooter, + RelayListItem.CustomListHeader, + RelayListItem.LocationHeader, is RelayListItem.LocationsEmptyText -> false } } } else { - -1 + null } private suspend fun LazyListState.animateScrollAndCentralizeItem(index: Int) { diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt index 97df71110e97..c2b536eca47e 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt @@ -18,7 +18,6 @@ import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.SingleChoiceSegmentedButtonRow -import androidx.compose.material3.SnackbarDuration import androidx.compose.material3.SnackbarHostState import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect @@ -118,8 +117,7 @@ fun SelectLocation( SelectLocationSideEffect.GenericError -> launch { snackbarHostState.showSnackbarImmediately( - message = context.getString(R.string.error_occurred), - duration = SnackbarDuration.Short, + message = context.getString(R.string.error_occurred) ) } } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt index e3161a221f78..c88bdf414d3c 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt @@ -62,7 +62,7 @@ sealed interface RelayListItem { } data object LocationHeader : RelayListItem { - override val key: Any = "location_header" + override val key = "location_header" override val contentType = RelayListItemContentType.LOCATION_HEADER } @@ -78,7 +78,7 @@ sealed interface RelayListItem { } data class LocationsEmptyText(val searchTerm: String) : RelayListItem { - override val key: Any = "locations_empty_text" + override val key = "locations_empty_text" override val contentType = RelayListItemContentType.LOCATIONS_EMPTY_TEXT } } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt index 35aff800590c..4ebbf9ad23fe 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt @@ -5,5 +5,5 @@ data class SettingsUiState( val isLoggedIn: Boolean, val isSupportedVersion: Boolean, val isPlayBuild: Boolean, - val useMultihop: Boolean, + val multihopEnabled: Boolean, ) diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SettingsViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SettingsViewModel.kt index 6d038f786013..2c0a051009d1 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SettingsViewModel.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/SettingsViewModel.kt @@ -30,7 +30,7 @@ class SettingsViewModel( appVersion = versionInfo.currentVersion, isSupportedVersion = versionInfo.isSupported, isPlayBuild = isPlayBuild, - useMultihop = wireguardConstraints?.useMultihop ?: false, + multihopEnabled = wireguardConstraints?.useMultihop ?: false, ) } .stateIn( @@ -41,7 +41,7 @@ class SettingsViewModel( isLoggedIn = false, isSupportedVersion = true, isPlayBuild = isPlayBuild, - useMultihop = false, + multihopEnabled = false, ), ) } diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt index 243a1ffa0385..9d8ccbc80ee2 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt @@ -7,24 +7,24 @@ import net.mullvad.mullvadvpn.lib.model.GeoLocationId import net.mullvad.mullvadvpn.lib.model.RelayItem import net.mullvad.mullvadvpn.lib.model.RelayItemId import net.mullvad.mullvadvpn.lib.model.SelectedLocation +import net.mullvad.mullvadvpn.relaylist.MIN_SEARCH_LENGTH import net.mullvad.mullvadvpn.relaylist.filterOnSearchTerm // Creates a relay list to be displayed by RelayListContent -// Search input as null is defined as not searching internal fun relayListItems( - searchTerm: String? = null, + searchTerm: String = "", relayCountries: List, customLists: List, selectedItem: RelayItemId?, disabledItem: RelayItemId?, expandedItems: Set, ): List { - val filteredCustomLists = customLists.filterOnSearchTerm(searchTerm ?: "") + val filteredCustomLists = customLists.filterOnSearchTerm(searchTerm) return buildList { val relayItems = createRelayListItems( - isSearching = searchTerm != null, + isSearching = searchTerm.isSearching(), selectedItem = selectedItem, disabledItem = disabledItem, customLists = filteredCustomLists, @@ -32,7 +32,7 @@ internal fun relayListItems( ) { it in expandedItems } - if (relayItems.isEmpty() && searchTerm != null) { + if (relayItems.isEmpty()) { add(RelayListItem.LocationsEmptyText(searchTerm)) } else { addAll(relayItems) @@ -266,3 +266,5 @@ private fun RelayItemId?.singleRelayId(customLists: List): ?.id as? GeoLocationId.Hostname else -> null } + +private fun String.isSearching() = length >= MIN_SEARCH_LENGTH diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt index fb40db7a7bd6..fcaa3f0cbbbe 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt @@ -140,10 +140,12 @@ class SearchLocationViewModel( .map { it.second } private fun filterChips() = - filterChipUseCase().map { filterChips: List -> + combine(filterChipUseCase(), wireguardConstraintsRepository.wireguardConstraints) { + filterChips, + constraints -> filterChips.toMutableList().apply { // Do not show entry and exit filter chips if multihop is disabled - if (multihopEnabled()) { + if (constraints?.useMultihop == true) { add( when (relayListSelection) { RelayListSelection.Entry -> FilterChip.Entry @@ -154,9 +156,6 @@ class SearchLocationViewModel( } } - private fun multihopEnabled() = - wireguardConstraintsRepository.wireguardConstraints.value?.useMultihop == true - fun addLocationToList(item: RelayItem.Location, customList: RelayItem.CustomList) { viewModelScope.launch { val result = diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt index a4d529266fc5..6f852538409d 100644 --- a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt +++ b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt @@ -2,7 +2,6 @@ package net.mullvad.mullvadvpn.viewmodel.location import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope -import co.touchlab.kermit.Logger import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow @@ -65,18 +64,15 @@ class SelectLocationListViewModel( private fun initialExpand(item: RelayItemId?): Set = buildSet { when (item) { is GeoLocationId.City -> { - Logger.d("GC item: $item") add(item.country.code) } is GeoLocationId.Hostname -> { - Logger.d("GH item: $item") add(item.country.code) add(item.city.code) } is CustomListId, is GeoLocationId.Country, null -> { - Logger.d("NO item: $item") /* No expands */ } }