Implement multihop UI

[Pururun]

Implementation of multihop ui on android.

SelectLocation is split into two screens:
SelectLocation
SearchLocation

SelectLocation has two different lists, entry and exit. Both lists have their own viewModel to mostly keep track of expansions and selections. Entry lists is only available if multihop is enabled.

SearchLocation uses a sort of fake SearchBar from material 3. The input field is the same, but the lists is displayed in a normal LazyColumn

---

This change is 

[linear]

DROID-822 Implement multihop UI

[kl]

Reviewed 42 of 63 files at r1, 2 of 3 files at r2, 5 of 6 files at r3, all commit messages.
Reviewable status: 49 of 64 files reviewed, 3 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MultihopScreen.kt line 66 at r2 (raw file):

                contentScale = ContentScale.FillWidth,
                modifier =
                    Modifier.widthIn(min = Dp.Infinity, max = Dimens.settingsDetailsImageMaxWidth)

Should this not be min = Dp.Unspecified (the default value of min) instead of min = Dp.Infinity?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 88 at r3 (raw file):

}

private fun SelectLocationListUiState.indexOfSelectedRelayItem(): Int =

What about returning Int? here instead of -1? Also the false cases can be collapsed like this:

Code snippet:

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,
                is RelayListItem.CustomListFooter,
                RelayListItem.CustomListHeader,
                RelayListItem.LocationHeader,
                is RelayListItem.LocationsEmptyText -> false
            }
        }
    } else {
        null
    }

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt line 159 at r3 (raw file):

        onSelectRelay = vm::selectRelay,
        onSearchClick = { navigator.navigate(SearchLocationDestination(it)) },
        onBackClick = dropUnlessResumed { backNavigator.navigateBack() },

Do we need to wrap all these in dropUnlessResumed? If this callback is invoked when the user clicks a button the app has to be in the resumed state so dropUnlessResumed will never do anything.

[Pururun]

Reviewable status: 49 of 64 files reviewed, 3 unresolved discussions (waiting on @kl)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 88 at r3 (raw file):

Previously, kl (Kalle Lindström) wrote…

What about returning Int? here instead of -1? Also the false cases can be collapsed like this:

Sounds good!

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt line 159 at r3 (raw file):

Previously, kl (Kalle Lindström) wrote…

Do we need to wrap all these in dropUnlessResumed? If this callback is invoked when the user clicks a button the app has to be in the resumed state so dropUnlessResumed will never do anything.

It is mostly to protect against the user clicking the button multiple times.

[Rawa]

Reviewed 11 of 63 files at r1, 1 of 3 files at r2, 4 of 6 files at r3, all commit messages.
Reviewable status: 49 of 64 files reviewed, 10 unresolved discussions (waiting on @kl and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/MullvadSegmentedButton.kt line 57 at r3 (raw file):

}

sealed interface SegmentedButtonPosition {

Feels a bit unnecessary since it is only 3 options and it directly maps to one value. We could provide 3 different functions e.g
MullvadSegmentedStartButton, MullvadSegmentedMiddle, MullvadSegmentedEndButton, and have one where

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt line 49 at r3 (raw file):

                .fillMaxWidth()
                .horizontalScroll(scrollState),
        horizontalArrangement = Arrangement.spacedBy(Dimens.chipSpace, alignment = Alignment.Start),

Isn't Alignment.Start default?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 68 at r3 (raw file):

fun StatusRelayItemCell(
    item: RelayItem,
    name: String,

Isn't name the name off the RelayItem? Why do we need it on the side? If this is the case we maybe should consider making a RelayItem agnostic component on the side?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 124 at r3 (raw file):

    name: String,
    hasChildren: Boolean,
    isEnabled: Boolean,

Why did we replace RelayItem? isEnabled is different from before?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/LocationsEmptyText.kt line 17 at r3 (raw file):

@Composable
fun LocationsEmptyText(searchTerm: String) {
    if (searchTerm.length >= MIN_SEARCH_LENGTH) {

Nice clean up ⭐ 🧹

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MultihopScreen.kt line 66 at r2 (raw file):

Previously, kl (Kalle Lindström) wrote…

Should this not be min = Dp.Unspecified (the default value of min) instead of min = Dp.Infinity?

Is min an optional parameter?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SettingsScreen.kt line 155 at r3 (raw file):

        onClick = onMultihopClick,
        bodyView =
            @Composable {

@Composable shouldn't be necessary right?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/LocationBottomSheet.kt line 49 at r3 (raw file):

@OptIn(ExperimentalMaterial3Api::class)
@Composable
internal fun LocationBottomSheets(

Nice compromise, too bad BottomSheet destinations doesn't exist in Material3 yet 😢

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt line 101 at r3 (raw file):

                            { onSelectRelay(listItem.item) },
                            {
                                // Only direct children can be removed

Not introduced by you, but shouldn't this comment be few lines up?

           if (listItem.depth == 1) {
                                {
                                    onUpdateBottomSheetState(
                                        ShowCustomListsEntryBottomSheet(
                                            listItem.parentId,
                                            listItem.parentName,
                                            listItem.item,
                                        )
                                    )
                                }

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt line 123 at r3 (raw file):

                    snackbarHostState.showSnackbarImmediately(
                        message = context.getString(R.string.error_occurred),
                        duration = SnackbarDuration.Short,

This should already be default right?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 88 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Sounds good!

👍

[Rawa]

Reviewable status: 49 of 64 files reviewed, 10 unresolved discussions (waiting on @kl and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt line 159 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It is mostly to protect against the user clicking the button multiple times.

Unfortunately a button can be clicked in a not resumed state, so you can actually get multiple clicks while animating the view away.

[Rawa]

Reviewed 7 of 63 files at r1, 1 of 6 files at r3.
Reviewable status: 52 of 64 files reviewed, 10 unresolved discussions (waiting on @kl and @Pururun)

[Rawa]

Reviewed 32 of 63 files at r1, 2 of 3 files at r2, 1 of 6 files at r3.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @kl and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt line 65 at r3 (raw file):

    data object LocationHeader : RelayListItem {
        override val key: Any = "location_header"

We should be able to remove : Any from the subtypes. Top level should still be Any

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SearchSelectLocationUiState.kt line 20 at r3 (raw file):

        val relayListItems: List<RelayListItem>,
        val customLists: List<RelayItem.CustomList>,
        val relayListSelection: RelayListSelection,

I think the name of this type can be a bit more clear, was confused to it's purpose and had to go into the type so find out.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt line 8 at r3 (raw file):

    val isSupportedVersion: Boolean,
    val isPlayBuild: Boolean,
    val useMultihop: Boolean,

nit: multihopEnabled or isMultihopEnabled would be more clear.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 13 at r3 (raw file):

// Creates a relay list to be displayed by RelayListContent
// Search input as null is defined as not searching

Is empty string searching?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 255 at r3 (raw file):

// city it is fine to have same entry and exit
// For custom lists we will block if the custom lists only contains one relay and
// nothing else

Could this cause problems if I add 2 items to the custom lists, use multihop and them remove the item that was not selected as my entry?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt line 146 at r3 (raw file):

            filterChips.toMutableList().apply {
                // Do not show entry and exit filter chips if multihop is disabled
                if (multihopEnabled()) {

This is a bit dangerous, since if multihop gets disabled/enabled, we don't get a new list of filterChips. We should user combine inorder to always emit the correct value.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt line 68 at r3 (raw file):

        when (item) {
            is GeoLocationId.City -> {
                Logger.d("GC item: $item")

Reminder to remove these logs.

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt line 96 at r3 (raw file):

                        } else null
                    },
                featureIndicators = emptyList(),

What happened here? 🙃

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt line 109 at r3 (raw file):

                        }
                    },
                featureIndicators = emptyList(),

and here.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/SelectedLocation.kt line 3 at r3 (raw file):

package net.mullvad.mullvadvpn.lib.model

sealed interface SelectedLocation {

I think we should try and come up with a better name. SelectedLocation sounds like there is only one location you select. Let's discuss within the team, some quick proposals:

RelayItemSelection
TunnelPathSelection
HopConfiguration

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/SelectedLocation.kt line 11 at r3 (raw file):

        val entryLocation: Constraint<RelayItemId>,
        override val exitLocation: Constraint<RelayItemId>,
    ) : SelectedLocation

The structure of the type looks good 💯

[Pururun]

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @kl and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/MullvadSegmentedButton.kt line 57 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels a bit unnecessary since it is only 3 options and it directly maps to one value. We could provide 3 different functions e.g
MullvadSegmentedStartButton, MullvadSegmentedMiddle, MullvadSegmentedEndButton, and have one where

Done

[Rawa]

Reviewed 1 of 3 files at r4, all commit messages.
Reviewable status: 62 of 64 files reviewed, 19 unresolved discussions (waiting on @kl and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/MullvadSegmentedButton.kt line 57 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Done

Nice ⭐

[Rawa]

Reviewed 5 of 63 files at r1, 2 of 3 files at r4.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @kl and @Pururun)

[Pururun]

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @kl and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterRow.kt line 49 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Isn't Alignment.Start default?

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 68 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Isn't name the name off the RelayItem? Why do we need it on the side? If this is the case we maybe should consider making a RelayItem agnostic component on the side?

It is used for blocked text, but maybe we should move that to the viewModel, let's discuss this.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 124 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Why did we replace RelayItem? isEnabled is different from before?

See above

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/LocationsEmptyText.kt line 17 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice clean up ⭐ 🧹

:)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MultihopScreen.kt line 66 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Is min an optional parameter?

The idea is that the image should scale up to fill the width of the screen until a certain width. Setting min to Dp.Unspecified makes the image not scale up and setting fillMaxWidth makes it ignore the max width. If there is a better way to do this I am all ears.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SettingsScreen.kt line 155 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

@Composable shouldn't be necessary right?

Seems to have been a copy-paste artifact from the other cells in this screen, removed all of then.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/LocationBottomSheet.kt line 49 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice compromise, too bad BottomSheet destinations doesn't exist in Material3 yet 😢

😢

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt line 101 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Not introduced by you, but shouldn't this comment be few lines up?

           if (listItem.depth == 1) {
                                {
                                    onUpdateBottomSheetState(
                                        ShowCustomListsEntryBottomSheet(
                                            listItem.parentId,
                                            listItem.parentName,
                                            listItem.item,
                                        )
                                    )
                                }

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SearchLocationScreen.kt line 123 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

This should already be default right?

That is true, removed here and in select location

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 88 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

👍

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt line 65 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

We should be able to remove : Any from the subtypes. Top level should still be Any

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SettingsUiState.kt line 8 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

nit: multihopEnabled or isMultihopEnabled would be more clear.

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 13 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Is empty string searching?

Empty string should not be able to happen because of checks outside of the code, but I will tidy up the code to make it more clear

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 255 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Could this cause problems if I add 2 items to the custom lists, use multihop and them remove the item that was not selected as my entry?

I am not sure I follow, but it should not matter since the list should update?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt line 146 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

This is a bit dangerous, since if multihop gets disabled/enabled, we don't get a new list of filterChips. We should user combine inorder to always emit the correct value.

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SelectLocationListViewModel.kt line 68 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Reminder to remove these logs.

Done

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt line 96 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

What happened here? 🙃

Not sure... Fixed

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/mapper/ToDomain.kt line 109 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

and here.

Not sure.. fixed

[kl]

Reviewable status: 50 of 64 files reviewed, 17 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt line 159 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Unfortunately a button can be clicked in a not resumed state, so you can actually get multiple clicks while animating the view away.

I wasn't aware of that. Seems like a bug in Android for a button to be clicked in a state other than Resumed.

[Pururun]

Reviewable status: 35 of 66 files reviewed, 17 unresolved discussions (waiting on @kl and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 68 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It is used for blocked text, but maybe we should move that to the viewModel, let's discuss this.

Fixed

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 124 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

See above

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MultihopScreen.kt line 66 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

The idea is that the image should scale up to fill the width of the screen until a certain width. Setting min to Dp.Unspecified makes the image not scale up and setting fillMaxWidth makes it ignore the max width. If there is a better way to do this I am all ears.

Fixed by setting fillMaxWidth() after widthIn

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SearchSelectLocationUiState.kt line 20 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

I think the name of this type can be a bit more clear, was confused to it's purpose and had to go into the type so find out.

Changed to RelayListType

[Pururun]

Reviewable status: 35 of 66 files reviewed, 17 unresolved discussions (waiting on @kl and @Rawa)

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/SelectedLocation.kt line 3 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

I think we should try and come up with a better name. SelectedLocation sounds like there is only one location you select. Let's discuss within the team, some quick proposals:

RelayItemSelection
TunnelPathSelection
HopConfiguration

Renamed to RelayItemSelection

[Rawa]

Reviewed 3 of 14 files at r5, 27 of 28 files at r6, 23 of 23 files at r7, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kl and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 213 at r7 (raw file):

private fun Name(modifier: Modifier = Modifier, name: String, state: RelayListItemState?) {
    Text(
        text = name.appendName(state),

Nit: I think it is better to be explicit.

text = if(state == null) {
    name
} else {
    name.appendName(state)
}

Then we can avoid handling null in appendName

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 264 at r7 (raw file):

@Composable
private fun String.appendName(state: RelayListItemState?) =

Function name does not really match what it does, appendName sounds like we are appending a name. Maybe withSuffix(state)?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/MultihopScreen.kt line 66 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Fixed by setting fillMaxWidth() after widthIn

👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SettingsScreen.kt line 155 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Seems to have been a copy-paste artifact from the other cells in this screen, removed all of then.

⭐

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SettingsScreen.kt line 225 at r7 (raw file):

@Composable
private fun Multihop(isMultihopEnabled: Boolean, onMultihopClick: () -> Unit) {

This is a very generic name. Should be something more specific. MultihopCell or MultihopRow

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/RelayListContent.kt line 101 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Done

🦸

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationList.kt line 88 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Done

Nice improvement.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/location/SelectLocationScreen.kt line 159 at r3 (raw file):

Previously, kl (Kalle Lindström) wrote…

I wasn't aware of that. Seems like a bug in Android for a button to be clicked in a state other than Resumed.

Yeah, there is nothing protecting against a user clicking on a UI element while a screen is in "CREATED" state, thus causing multiple navigations up. @kl Please mark if you are happy with solution.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt line 18 at r7 (raw file):

enum class RelayListItemState {
    BLOCKED_BY_ENTRY,

BLOCKED_BY_ENTRY or would USED_AS_ENTRY be more correct?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 255 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I am not sure I follow, but it should not matter since the list should update?

But it would then force the daemon to use the same Entry and Exit? Would we enter blocking?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 21 at r7 (raw file):

    customLists: List<RelayItem.CustomList>,
    selectedItem: RelayItemId?,
    disabledItem: RelayItemId?,

If I understand it correctly this is either a Entry or a Exit RelayItemId and thus can't be used and should be disabled? Can we make this clear with a better name?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 117 at r7 (raw file):

                        if (
                            disabledItem == customList.id ||
                                customList.locations.all { it.id == disabledItem }

We should break out this logic to another function.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 188 at r7 (raw file):

            item = item,
            state =
                if (disabledItem == item.id) {

Break out as function as we implement this code twice.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt line 144 at r7 (raw file):

            filterChips.toMutableList().apply {
                // Do not show entry and exit filter chips if multihop is disabled
                if (constraints?.useMultihop == true) {

useMultihop should probably be renamed to isMultihopEnabled

[Pururun]

Reviewable status: 74 of 88 files reviewed, 11 unresolved discussions (waiting on @kl and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 213 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

Nit: I think it is better to be explicit.

text = if(state == null) {
    name
} else {
    name.appendName(state)
}

Then we can avoid handling null in appendName

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/RelayLocationCell.kt line 264 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

Function name does not really match what it does, appendName sounds like we are appending a name. Maybe withSuffix(state)?

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SettingsScreen.kt line 225 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

This is a very generic name. Should be something more specific. MultihopCell or MultihopRow

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayListItem.kt line 18 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

BLOCKED_BY_ENTRY or would USED_AS_ENTRY be more correct?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 255 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

But it would then force the daemon to use the same Entry and Exit? Would we enter blocking?

Yes we would. :)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 21 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

If I understand it correctly this is either a Entry or a Exit RelayItemId and thus can't be used and should be disabled? Can we make this clear with a better name?

Changed to selectedByThisEntryExitList and selectedByOtherEntryExitList

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 117 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

We should break out this logic to another function.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 188 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

Break out as function as we implement this code twice.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/SearchLocationViewModel.kt line 144 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

useMultihop should probably be renamed to isMultihopEnabled

Done.

[Rawa]

Reviewed 14 of 14 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/transitions/SlideInFromBottomTransition.kt line 77 at r9 (raw file):

            when (initialState.destination()) {
                NoDaemonDestination -> fadeIn(snap(0))
                SearchLocationDestination -> fadeIn()

✨

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/location/RelayItemListCreator.kt line 255 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Yes we would. :)

💯

[Rawa]

Reviewed 12 of 13 files at r10, 29 of 43 files at r11, 26 of 32 files at r12, 41 of 41 files at r14, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/Daita.kt line 5 at r12 (raw file):

import net.mullvad.mullvadvpn.compose.state.RelayListType

fun showOnlyRelaysWithDaita(

We should change this function name. Sounds like we are setting some property rather get deciding if we should show / filter on Daita relays.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/FilterCustomListsRelayItemUseCase.kt line 34 at r12 (raw file):

                ownership = selectedOwnership,
                providers = selectedProviders,
                shouldFilterByDaita =

We should improve on argument name.

shouldFilterByDaita is a parameter into filter. So the addition of FilterByDaita makes it confusing.

[Rawa]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 414 at r14 (raw file):

        val hostname =
            when {

nit: Refactor into to another composable

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/dimensions/Dimensions.kt line 59 at r14 (raw file):

    val screenVerticalMargin: Dp = 22.dp,
    val searchFieldHeight: Dp = 42.dp,
    val searchFieldHeightExpanded: Dp = 85.dp,

This is a really odd height? Why exactly 85.dp?

[Pururun]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 414 at r14 (raw file):

Previously, Rawa (David Göransson) wrote…

nit: Refactor into to another composable

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/FilterCustomListsRelayItemUseCase.kt line 34 at r12 (raw file):

Previously, Rawa (David Göransson) wrote…

We should improve on argument name.

shouldFilterByDaita is a parameter into filter. So the addition of FilterByDaita makes it confusing.

Changed to daita after discussion

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/Daita.kt line 5 at r12 (raw file):

Previously, Rawa (David Göransson) wrote…

We should change this function name. Sounds like we are setting some property rather get deciding if we should show / filter on Daita relays.

Changed to shouldFilterByDaita after discussion

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/dimensions/Dimensions.kt line 59 at r14 (raw file):

Previously, Rawa (David Göransson) wrote…

This is a really odd height? Why exactly 85.dp?

Changed it to what it says in the material design guidelines

[Rawa]

Reviewed 21 of 22 files at r15, all commit messages.
Reviewable status: 116 of 117 files reviewed, all discussions resolved

[Rawa]

Reviewed 1 of 1 files at r16.
Reviewable status: complete! all files reviewed, all discussions resolved

[Rawa]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved