From 893fc8e84651a074d8235255c87c9295e873f3d2 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 | 92 +++++++++---------- .../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 - .../lib/daemon/grpc/mapper/ToDomain.kt | 4 +- 14 files changed, 75 insertions(+), 87 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, + ) + }, ) } @@ -183,23 +182,22 @@ private fun AppVersion(context: Context, state: SettingsUiState) { ) ) }, - bodyView = - @Composable { - if (!state.isPlayBuild) { - NavigationCellBody( - content = state.appVersion, - contentBodyDescription = stringResource(id = R.string.app_version), - textColor = MaterialTheme.colorScheme.onSurfaceVariant, - isExternalLink = true, - ) - } else { - Text( - text = state.appVersion, - style = MaterialTheme.typography.labelMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) - } - }, + bodyView = { + if (!state.isPlayBuild) { + NavigationCellBody( + content = state.appVersion, + contentBodyDescription = stringResource(id = R.string.app_version), + textColor = MaterialTheme.colorScheme.onSurfaceVariant, + isExternalLink = true, + ) + } else { + Text( + text = state.appVersion, + style = MaterialTheme.typography.labelMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + }, showWarning = !state.isSupportedVersion, isRowEnabled = !state.isPlayBuild, ) @@ -234,13 +232,12 @@ private fun FaqAndGuides(context: Context) { val faqGuideLabel = stringResource(id = R.string.faqs_and_guides) NavigationComposeCell( title = faqGuideLabel, - bodyView = - @Composable { - DefaultExternalLinkView( - chevronContentDescription = faqGuideLabel, - tint = MaterialTheme.colorScheme.onPrimary, - ) - }, + bodyView = { + DefaultExternalLinkView( + chevronContentDescription = faqGuideLabel, + tint = MaterialTheme.colorScheme.onPrimary, + ) + }, onClick = { context.openLink(Uri.parse(context.resources.getString(R.string.faqs_and_guides_url))) }, @@ -252,13 +249,12 @@ private fun PrivacyPolicy(context: Context, state: SettingsUiState) { val privacyPolicyLabel = stringResource(id = R.string.privacy_policy_label) NavigationComposeCell( title = privacyPolicyLabel, - bodyView = - @Composable { - DefaultExternalLinkView( - chevronContentDescription = privacyPolicyLabel, - tint = MaterialTheme.colorScheme.onPrimary, - ) - }, + bodyView = { + DefaultExternalLinkView( + chevronContentDescription = privacyPolicyLabel, + tint = MaterialTheme.colorScheme.onPrimary, + ) + }, onClick = { context.openLink( Uri.parse( 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 01c06273c2dc..3c6944aa62c9 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 @@ -73,6 +73,7 @@ fun LazyListScope.relayListContent( listItem, relayListSelection, { onSelectRelay(listItem.item) }, + // Only direct children can be removed if (listItem.depth == 1) { { onUpdateBottomSheetState( @@ -98,7 +99,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 370a81d3e6f6..10d45e6a0243 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 */ } } diff --git a/android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt b/android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt index c708a1166a8f..11bf762b4283 100644 --- a/android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt +++ b/android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt @@ -93,7 +93,7 @@ internal fun ManagementInterface.TunnelState.toDomain(): TunnelState = location.toDomain() } else null }, - featureIndicators = emptyList(), + featureIndicators = connecting.featureIndicators.toDomain(), ) ManagementInterface.TunnelState.StateCase.CONNECTED -> TunnelState.Connected( @@ -106,7 +106,7 @@ internal fun ManagementInterface.TunnelState.toDomain(): TunnelState = null } }, - featureIndicators = emptyList(), + featureIndicators = connected.featureIndicators.toDomain(), ) ManagementInterface.TunnelState.StateCase.DISCONNECTING -> TunnelState.Disconnecting(