Skip to content

Commit

Permalink
Fix review issues
Browse files Browse the repository at this point in the history
  • Loading branch information
Pururun committed Oct 28, 2024
1 parent 5c8f420 commit 893fc8e
Show file tree
Hide file tree
Showing 14 changed files with 75 additions and 87 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ class SettingsUiStatePreviewParameterProvider : PreviewParameterProvider<Setting
isLoggedIn = true,
isSupportedVersion = true,
isPlayBuild = true,
useMultihop = false,
multihopEnabled = false,
),
SettingsUiState(
appVersion = "9000.1",
isLoggedIn = false,
isSupportedVersion = false,
isPlayBuild = false,
useMultihop = false,
multihopEnabled = false,
),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ fun SettingsScreen(
if (state.isLoggedIn) {
itemWithDivider {
Multihop(
isMultihopEnabled = state.useMultihop,
isMultihopEnabled = state.multihopEnabled,
onMultihopClick = onMultihopClick,
)
}
Expand Down Expand Up @@ -151,22 +151,21 @@ private fun Multihop(isMultihopEnabled: Boolean, onMultihopClick: () -> 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,
)
},
)
}

Expand All @@ -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,
)
Expand Down Expand Up @@ -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)))
},
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ fun LazyListScope.relayListContent(
listItem,
relayListSelection,
{ onSelectRelay(listItem.item) },
// Only direct children can be removed
if (listItem.depth == 1) {
{
onUpdateBottomSheetState(
Expand All @@ -98,7 +99,6 @@ fun LazyListScope.relayListContent(
relayListSelection = relayListSelection,
{ onSelectRelay(listItem.item) },
{
// Only direct children can be removed
onUpdateBottomSheetState(
ShowLocationBottomSheet(customLists, listItem.item)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ data class SettingsUiState(
val isLoggedIn: Boolean,
val isSupportedVersion: Boolean,
val isPlayBuild: Boolean,
val useMultihop: Boolean,
val multihopEnabled: Boolean,
)
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class SettingsViewModel(
appVersion = versionInfo.currentVersion,
isSupportedVersion = versionInfo.isSupported,
isPlayBuild = isPlayBuild,
useMultihop = wireguardConstraints?.useMultihop ?: false,
multihopEnabled = wireguardConstraints?.useMultihop ?: false,
)
}
.stateIn(
Expand All @@ -41,7 +41,7 @@ class SettingsViewModel(
isLoggedIn = false,
isSupportedVersion = true,
isPlayBuild = isPlayBuild,
useMultihop = false,
multihopEnabled = false,
),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,32 @@ 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<RelayItem.Location.Country>,
customLists: List<RelayItem.CustomList>,
selectedItem: RelayItemId?,
disabledItem: RelayItemId?,
expandedItems: Set<String>,
): List<RelayListItem> {
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,
countries = relayCountries,
) {
it in expandedItems
}
if (relayItems.isEmpty() && searchTerm != null) {
if (relayItems.isEmpty()) {
add(RelayListItem.LocationsEmptyText(searchTerm))
} else {
addAll(relayItems)
Expand Down Expand Up @@ -266,3 +266,5 @@ private fun RelayItemId?.singleRelayId(customLists: List<RelayItem.CustomList>):
?.id as? GeoLocationId.Hostname
else -> null
}

private fun String.isSearching() = length >= MIN_SEARCH_LENGTH
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,12 @@ class SearchLocationViewModel(
.map { it.second }

private fun filterChips() =
filterChipUseCase().map { filterChips: List<FilterChip> ->
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
Expand All @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -65,18 +64,15 @@ class SelectLocationListViewModel(
private fun initialExpand(item: RelayItemId?): Set<String> = 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 */
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -106,7 +106,7 @@ internal fun ManagementInterface.TunnelState.toDomain(): TunnelState =
null
}
},
featureIndicators = emptyList(),
featureIndicators = connected.featureIndicators.toDomain(),
)
ManagementInterface.TunnelState.StateCase.DISCONNECTING ->
TunnelState.Disconnecting(
Expand Down

0 comments on commit 893fc8e

Please sign in to comment.