Add UI for custom lists feature

[Pururun]

Add the custom list feature to the app ui.

---

This change is 

[linear]

DROID-654 Create ui for custom list

[Rawa]

Reviewed 27 of 33 files at r1, 1 of 5 files at r2.
Reviewable status: 26 of 35 files reviewed, 46 unresolved discussions (waiting on @Pururun)

---

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

@Composable
fun NormalRelayLocationCell(

What is the difference between a NormalRelayLocationCell vs RelayLocationCell? I find the naming confusing

---

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

                        .size(Dimens.relayCircleSize)
                        .background(
                            color =

General remark for this file: indentation is really deep, almost a point where it becomes hard to read, maybe we should evaluate the background color first to avoid if, when etc inside the modifiers. also if necessary break out layout composables (Box/Column/Row) to new composables.

---

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

                            color =
                                when {
                                    selected -> Color.Transparent

Can we avoid using Transparent?

---

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

    modifier: Modifier = Modifier,
    onRelaySelected: (item: RelayItem) -> Unit = {},
    onRelayDeselected: (item: RelayItem) -> Unit = {},

Do we need two lambas? Would also make it simpler with the onCheckedChange on the checkbox in this cell.

onRelayCheckedChange: (item: RelayItem, checked: Boolean) -> Unit

---

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

                modifier =
                    Modifier.size(Dimens.checkBoxSize)
                        .background(Color.White, MaterialTheme.shapes.small)

A bit weird, doesn't checkbox have a background too it? Why do we not use the Checkbox defaults to show it's background? If we need this work around to make it look as we want we should break it out to a separate component, e.g MullvadCheckbox or similar.

---

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

    leadingContent: @Composable BoxScope.(relay: RelayItem) -> Unit,
    modifier: Modifier = Modifier,
    specialBackgroundColor: @Composable (relayItem: RelayItem) -> Color? = { null },

I don't think this function is needed at all? We evaluate it with the relay provide it above, so all the arguments are already fulfilled by the caller, we should be able to make this a color.

---

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

    specialBackgroundColor: @Composable (relayItem: RelayItem) -> Color? = { null },
    onClick: (item: RelayItem) -> Unit,
    depth: Int

Another alternative than depth would be to pass in some startPadding that would get accumulated up.

---

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

                        .then(
                            if (relay.active) {
                                Modifier.clickable { onClick(relay) }

Modifier.clickable also have a enabled argument, seems like that is what we want to use in this case?

Modifier.weight(1f).clickable(enabled = relay.active, onClick = { onClick(relay) })

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 30 at r1 (raw file):

) {
    BaseCell(
        title = {

Looks kind of weird that we enter a title & subtitle into a title. Maybe BaseCell needs to be more generic or rethought.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 48 at r1 (raw file):

        onCellClicked = onCellClicked,
        background = background,
        minHeight = 72.dp

Hard-coded value

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/constant/CommonContentKey.kt line 8 at r1 (raw file):

    const val PROGRESS = "progress"
    const val HEADER = "header"
    const val EMPTY = "empty"

What is difference between EMPTY and SPACER?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 47 at r1 (raw file):

                }
                CreateCustomListDialogSideEffect.CreateCustomListError -> {
                    showError.value = true

This is not really a side effect anymore when we persist it in the state, we should use uiState to provide this value.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 85 at r1 (raw file):

                    keyboardType = KeyboardType.Text,
                    placeholderText = "",
                    isValidValue = name.value.isNotBlank(),

When I open the dialog for the first time the cursor is red, so feel like I have an error by default. I believe this line might be the culprit?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 88 at r1 (raw file):

                    isDigitsOnlyAllowed = false
                )
                if (showError) {

TextField have the ability to show errors, maybe it worth bringing up with the design to align it closer to material, it would make this composable more clear.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 91 at r1 (raw file):

                    Spacer(modifier = Modifier.height(Dimens.smallPadding))
                    Text(
                        text = stringResource(id = R.string.error_occurred),

Not sure if this according to design, but "An error occurred" doesn't really help the user in any way, we should define all the types of errors that can happen in a sealed interface:

sealed interface CreateCustomListError {
    data class DuplicateListName(val name: String): CreateCustomListError
    data object EmptyCustomListName: CreateCustomList
    ...
}

then we can have more helpful error messages like: "A custom list with name 'x' already exists" or "Custom list name cannot be empty"

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 104 at r1 (raw file):

            PrimaryButton(
                text = stringResource(id = R.string.create),
                onClick = { createCustomList(name.value) }

I tried this out a bit, seems like we can create a list with a empty name and with just spaces, maybe we should do some validation or limits on what type of list names are possible?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 41 at r2 (raw file):

fun DeleteCustomListConfirmationDialog(navigator: ResultBackNavigator<Boolean>, name: String) {
    AlertDialog(
        onDismissRequest = { navigator.navigateBack() },

navigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 40 at r1 (raw file):

fun EditCustomListName(navigator: DestinationsNavigator, customListId: String, name: String) {
    val vm: EditCustomListNameDialogViewModel =
        koinViewModel(parameters = { parametersOf(customListId) })

Nice passing it to the viewModel right away and letting it handle it.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 49 at r1 (raw file):

                }
                EditCustomListNameDialogSideEffect.UpdateNameError -> {
                    showError.value = true

Same as above, we should provide uiState from viewmodel.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 80 at r1 (raw file):

        text = {
            Column {
                CustomTextField(

Same as above, Use TextField for showing error if possible

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 89 at r1 (raw file):

                    keyboardType = KeyboardType.Text,
                    placeholderText = "",
                    isValidValue = input.value.isNotBlank(),

Same as above, show error right away.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RemoveDeviceConfirmationDialog.kt line 45 at r2 (raw file):

fun RemoveDeviceConfirmationDialog(navigator: ResultBackNavigator<String>, device: Device) {
    AlertDialog(
        onDismissRequest = { navigator.navigateBack() },

navigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 74 at r1 (raw file):

    val state by customListsViewModel.uiState.collectAsState()
    CustomListLocationsScreen(
        newList = newList,

We should pass this the viewModel and integrate it into the uiState, so we have one uiState.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 153 at r1 (raw file):

            color = MaterialTheme.colorScheme.onPrimary
        )
        IconButton(onClick = {}) {

Empty onClick lambda, this was the search part that still wasn't completed right?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 181 at r1 (raw file):

    uiState: CustomListLocationsUiState.Content.Data,
    onRelaySelected: (RelayItem) -> Unit,
    onRelayDeselected: (RelayItem) -> Unit

Merge onRelaySelected & Deselected

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 69 at r1 (raw file):

    EditCustomListScreen(
        uiState = uiState,
        onDeleteList = { viewModel.deleteList() },

viewModel::deleteList

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 74 at r1 (raw file):

            navigator.navigate(CustomListLocationsDestination(customListKey = it, newList = false))
        },
        onBackClick = { navigator.navigateUp() }

navigator::navigateUp

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 150 at r1 (raw file):

                            ),
                    onClick = {
                        onDeleteList()

Maybe more towards UX, but we should have some confirmation before deleting a list. E.g "Are you sure you want to delete custom list 'x'?

---

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

fun SelectLocation(
    navigator: DestinationsNavigator,
    createCustomListResultRecipient: ResultRecipient<CreateCustomListDestination, String>

👏

---

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

            navigator.navigate(CreateCustomListDestination) { launchSingleTop = true }
        },
        onEditCustomList = { navigator.navigate(EditCustomListDestination(it.id)) },

We should rework scroll to current location. Currently if E.g Singapore is selected, I scroll to top to edit a custom list and then go back, it would scroll the user back to Singapore. We need to rework the scroll to work with the custom lists.

---

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

                onCustomListClicked = onEditCustomList,
                closeBottomSheet = { showBottomSheet = false }
            )

Nice and simple 💯

---

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

        )
    }
    items(

The items extension function would make more sense in this case, then we don't have to bother with index:

    items(
        relayListState.customLists,
        key = { it.hashCode() },
        contentType = { ContentType.ITEM }
    ) {}

---

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

    closeBottomSheet: () -> Unit
) {
    val sheetState = rememberModalBottomSheetState()

Should we enable skipPartiallyExpanded?

---

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

        showBottomSheet &&
            uiState is SelectLocationUiState.Data &&
            uiState.relayListState is RelayListState.RelayList

Is it possible to break this out? Or do we rely on the smart cast?

---

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

                background = MaterialTheme.colorScheme.background
            )
            uiState.relayListState.customLists.forEach { customList ->

Swiping this list up from bottom, can cause you to see the content behind, not sure if it is a bug or limitation of sheet?

---

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

                            .invokeOnCompletion {
                                if (!sheetState.isVisible) {
                                    closeBottomSheet()

This looks a bit like a hack, by do we have to close it twice?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/CustomListLocationsUiState.kt line 19 at r1 (raw file):

        ) : Content
    }
}

🥇

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/CustomListUseCase.kt line 10 at r1 (raw file):

import net.mullvad.mullvadvpn.repository.SettingsRepository

class CustomListUseCase(

Generally I don't think a use case should be having multiple functions unless there is a really good reason for it. Almost feels like we let the UseCase become another version of the Repository, maybe it is worth discussing internally to have a more clear vision of how we want to do it.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 69 at r1 (raw file):

                    ?.apply { customListName = name }
                    ?.locations
                    ?.selectChildren()

This looks a bit complex, maybe we can simplify it or break it out to a extension function that describes what it does.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 78 at r1 (raw file):

                val newSelectedLocations = it?.toMutableSet() ?: mutableSetOf()
                newSelectedLocations.add(relayItem)
                when (relayItem) {

Maybe it could be more clear if we make an extension that returns are the relevant relays for e.g a RelayItem and then removes / adds them all in one swoop. The same function get all relevant relays can be used when adding and removing relays.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 127 at r1 (raw file):

        viewModelScope.launch {
            _selectedLocations.value?.let { selectedLocations ->
                customListUseCase.updateCustomListLocations(

Can this fail? Is it something we care about?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 140 at r1 (raw file):

            ?: emptyList()

    private fun Set<RelayItem>.selectParents(relayItem: RelayItem): Set<RelayItem> {

I believe this one selects a parent if all children are checked but the naming of it is not really clear, sounds like we always selects the parents

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 155 at r1 (raw file):

                        .flatMap { country -> country.cities }
                        .find { it.code == relayItem.location.cityCode }
                Log.d("LOLZ", "city: $city code=${relayItem.location.cityCode}")

Remove

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 159 at r1 (raw file):

                    updateSelectionList.add(city)
                    val country = availableLocations.find { it.code == city.location.countryCode }
                    if (country != null && updateSelectionList.containsAll(country.cities)) {

This looks like the code above, shouldn't it be possible to do a recursive call to selectParent when we filled a parent?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListNameDialogViewModel.kt line 23 at r1 (raw file):

        viewModelScope.launch {
            if (customListUseCase.updateCustomListName(id, name)) {
                _uiSideEffect.send(EditCustomListNameDialogSideEffect.CloseScreen)

This seems to trigger no matter if the dialog was successful or not. E.g If i create custom list with name "1" and "2" then try and rename "2" to "1" I do not get any error and the dialog just closes.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListViewModel.kt line 35 at r1 (raw file):

                            id = it?.id ?: "",
                            name = it?.name ?: "",
                            numberOfLocations = it?.locations?.size ?: 0

Feels like we should be able to avoid this case some how with all these defaults. Maybe we should just crash otherwise?

[Pururun]

Reviewable status: 21 of 45 files reviewed, 42 unresolved discussions (waiting on @Rawa)

---

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

Previously, Rawa (David Göransson) wrote…

What is the difference between a NormalRelayLocationCell vs RelayLocationCell? I find the naming confusing

Normal is the one that we have used before without the Checkbox, I am not sure what a good name could be here, StatusRelayLocationCell?

---

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

Previously, Rawa (David Göransson) wrote…

Do we need two lambas? Would also make it simpler with the onCheckedChange on the checkbox in this cell.

onRelayCheckedChange: (item: RelayItem, checked: Boolean) -> Unit

Done

---

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

Previously, Rawa (David Göransson) wrote…

A bit weird, doesn't checkbox have a background too it? Why do we not use the Checkbox defaults to show it's background? If we need this work around to make it look as we want we should break it out to a separate component, e.g MullvadCheckbox or similar.

I just copied the same Checkbox we had in the FilterScreen, but I agree that I should make component out of it. Done.

---

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

Previously, Rawa (David Göransson) wrote…

Modifier.clickable also have a enabled argument, seems like that is what we want to use in this case?

Modifier.weight(1f).clickable(enabled = relay.active, onClick = { onClick(relay) })

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 48 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Hard-coded value

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/constant/CommonContentKey.kt line 8 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

What is difference between EMPTY and SPACER?

Spacer is for space between items and empty is for the empty state of a screen.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 47 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

This is not really a side effect anymore when we persist it in the state, we should use uiState to provide this value.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 88 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

TextField have the ability to show errors, maybe it worth bringing up with the design to align it closer to material, it would make this composable more clear.

Added error to the text field instead.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 91 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Not sure if this according to design, but "An error occurred" doesn't really help the user in any way, we should define all the types of errors that can happen in a sealed interface:

sealed interface CreateCustomListError {
    data class DuplicateListName(val name: String): CreateCustomListError
    data object EmptyCustomListName: CreateCustomList
    ...
}

then we can have more helpful error messages like: "A custom list with name 'x' already exists" or "Custom list name cannot be empty"

This has been improved.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 104 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I tried this out a bit, seems like we can create a list with a empty name and with just spaces, maybe we should do some validation or limits on what type of list names are possible?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 41 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

navigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 49 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Same as above, we should provide uiState from viewmodel.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 80 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Same as above, Use TextField for showing error if possible

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RemoveDeviceConfirmationDialog.kt line 45 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

navigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 74 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

We should pass this the viewModel and integrate it into the uiState, so we have one uiState.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 153 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Empty onClick lambda, this was the search part that still wasn't completed right?

Yes, but let us keep this comment so that we don't forget about it.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 181 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Merge onRelaySelected & Deselected

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 74 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

navigator::navigateUp

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 150 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe more towards UX, but we should have some confirmation before deleting a list. E.g "Are you sure you want to delete custom list 'x'?

Added

---

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

Previously, Rawa (David Göransson) wrote…

The items extension function would make more sense in this case, then we don't have to bother with index:

    items(
        relayListState.customLists,
        key = { it.hashCode() },
        contentType = { ContentType.ITEM }
    ) {}

Done

---

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

Previously, Rawa (David Göransson) wrote…

Should we enable skipPartiallyExpanded?

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 127 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Can this fail? Is it something we care about?

It can fail, I'm not sure if it is a case that we should bother with.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListNameDialogViewModel.kt line 23 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

This seems to trigger no matter if the dialog was successful or not. E.g If i create custom list with name "1" and "2" then try and rename "2" to "1" I do not get any error and the dialog just closes.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListViewModel.kt line 35 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels like we should be able to avoid this case some how with all these defaults. Maybe we should just crash otherwise?

Since we are deleting the list and then moving away, I think it is reasonable that we should handle some kind of case when we can't find the list.

[Pururun]

Reviewable status: 12 of 59 files reviewed, 41 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 30 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Looks kind of weird that we enter a title & subtitle into a title. Maybe BaseCell needs to be more generic or rethought.

Changed title in BaceCell to headline content

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/constant/CommonContentKey.kt line 8 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Spacer is for space between items and empty is for the empty state of a screen.

I will remove this

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 69 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

viewModel::deleteList

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Swiping this list up from bottom, can cause you to see the content behind, not sure if it is a bug or limitation of sheet?

Probably bug.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 155 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Remove

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/CustomListUseCase.kt line 10 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Generally I don't think a use case should be having multiple functions unless there is a really good reason for it. Almost feels like we let the UseCase become another version of the Repository, maybe it is worth discussing internally to have a more clear vision of how we want to do it.

Moved to repository

[Rawa]

Reviewable status: 12 of 59 files reviewed, 16 unresolved discussions (waiting on @A and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Normal is the one that we have used before without the Checkbox, I am not sure what a good name could be here, StatusRelayLocationCell?

Sounds better 👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 153 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Yes, but let us keep this comment so that we don't forget about it.

👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 127 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It can fail, I'm not sure if it is a case that we should bother with.

Would it throw and crash or be fail silent?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListViewModel.kt line 35 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Since we are deleting the list and then moving away, I think it is reasonable that we should handle some kind of case when we can't find the list.

I agree. Maybe our ViewState should be able to handle when it is not found? Then we can avoid these defaults. If we get a UI update before user is navigated away it would also say, couldn't find the item rather then empty name and 0 in size.

[Pururun]

This is being paused until the updated UX has been updated.

[Rawa]

Reviewed 9 of 18 files at r3, 16 of 33 files at r4, 7 of 28 files at r5, 60 of 60 files at r6, 9 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 44 unresolved discussions (waiting on @Pururun)

---

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

        modifier = modifier,
        onClick = { onRelayCheckedChange(it, !selectedRelays.contains(it)) },
        onLongClick = {},

We should make onLongClick nullable and null by default.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 10 at r6 (raw file):

    @Parcelize
    data class Rename(val customListId: String, val name: String) : CustomListAction {
        fun not(): CustomListAction = this

This doesn't make sense to me, not operation would not undo the Rename action, should we provide a argument with previousName?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 15 at r6 (raw file):

    @Parcelize
    data class Delete(val customListId: String, val name: String) : CustomListAction {
        fun not(locations: List<String>): CustomListAction = Create(name, locations)

Nice 🤩

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 24 at r6 (raw file):

        val locationNames: List<String> = emptyList()
    ) : CustomListAction, Parcelable {
        fun not(customListId: String) = Delete(customListId, name)

Really clean ⭐

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 30 at r6 (raw file):

    data class UpdateLocations(
        val customListId: String,
        val newList: Boolean = false,

Is this parameter needed? Just looks weird to have it there.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/MullvadModalBottomSheet.kt line 51 at r6 (raw file):

    onBackgroundColor: Color = MaterialTheme.colorScheme.onSurface,
    closeBottomSheet: () -> Unit,
    sheetContent: @Composable ColumnScope.(onBackgroundColor: Color, onClose: () -> Unit) -> Unit

Is it possible to simplify this? Seems quite complex, to have to pass a closeBottomSheet lambda and then also passing a lambda that gets me an onClose? I remember that the example was kind of weird from google, but looking at the signature it is not really clear to me how this works.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 72 at r6 (raw file):

                is CreateCustomListDialogSideEffect.NavigateToCustomListLocationsScreen -> {
                    navigator.popBackStack()
                    navigator.navigate(

There is a option you can pass called popUpTo, I believe this is what you want to achieve?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 128 at r6 (raw file):

                    },
                    onSubmit = {
                        if (it.isNotBlank()) {

Would be nice if logic like this is moved to ViewModel, should we show an error if they press submit with no input? Or maybe just add to this Logic to ViewState? then we can reuse it further down to for the confirm button:

isEnabled = name.value.isNotBlank()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 143 at r6 (raw file):

                                        id =
                                            if (
                                                uiState.error == CustomListsError.CustomListExists

Very deep indentation here, maybe make a

@Composable
fun CustomListsError.toString() = stringResource(
    when(this) {
        ...
    }
... 

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 43 at r6 (raw file):

@Composable
@Destination(style = DestinationStyle.Dialog::class)
fun DeleteCustomList(

I used Dialog or Screen suffix to make it clear that it is a destination. Maybe we should also make a Konsist rule for this 🤔

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 44 at r6 (raw file):

@Destination(style = DestinationStyle.Dialog::class)
fun DeleteCustomList(
    navigator: ResultBackNavigator<CustomListResult.ListDeleted>,

Shouldn't we be able to reuse the same action as created above instead of ListDeleted? 🤔

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 62 at r6 (raw file):

        name = request.parsedAction<CustomListAction.Delete>().name,
        onDelete = viewModel::deleteCustomList,
        onBack = { navigator.navigateBack() }

navigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DiscardChangesDialog.kt line 21 at r6 (raw file):

fun DiscardChangesDialog(resultBackNavigator: ResultBackNavigator<Boolean>) {
    AlertDialog(
        onDismissRequest = { resultBackNavigator.navigateBack() },

resultBackNavigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DiscardChangesDialog.kt line 26 at r6 (raw file):

            PrimaryButton(
                modifier = Modifier.focusRequester(FocusRequester()),
                onClick = { resultBackNavigator.navigateBack() },

resultBackNavigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 61 at r6 (raw file):

    }

    val uiState = vm.uiState.collectAsState().value

NIT: val uiState by vm.uiState.collectAsState()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 95 at r6 (raw file):

                    },
                    onSubmit = {
                        if (it.isNotBlank()) {

Same as above, we can move this logic to UiState or ViewModel

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 72 at r6 (raw file):

    discardChangesResultRecipient.onNavResult(
        listener = {

Remove named argument and open lambda

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 100 at r6 (raw file):

        onRelaySelectionClick = customListsViewModel::onRelaySelectionClick,
        onBackClick = {
            if (state.willDiscardChanges) {

maybe something hasStagedChanges or hasUnsavedChanges?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 62 at r6 (raw file):

@Composable
@Destination(style = SlideInFromRightTransition::class)
fun CustomLists(

Add a Screensuffix since it is a destination.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 74 at r6 (raw file):

    editCustomListResultRecipient.onNavResult(
        listener = { result ->

Removed named argument and open lambda

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 92 at r6 (raw file):

                            onAction = {
                                viewModel.undoDeleteCustomList(
                                    result.value.reverseAction as CustomListAction.Create

Feels like we should be able to avoid this by using the right types. Let's discuss and have a pair programming session about it.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 122 at r6 (raw file):

@Composable
fun CustomListsScreen(

Rename this to something else or merge with the function above.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 77 at r6 (raw file):

@Composable
@Destination(style = SlideInFromRightTransition::class)
fun EditCustomList(

Add Screen suffix

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 130 at r6 (raw file):

            }
        },
        onBackClick = { backNavigator.navigateBack() }

backNavigator::navigateBack

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 135 at r6 (raw file):

@Composable
fun EditCustomListScreen(

Remove Screen suffix

---

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

                            CustomListAction.Create(
                                locations = relayItem?.code?.let { listOf(it) } ?: emptyList(),
                                locationNames = relayItem?.name?.let { listOf(it) } ?: emptyList()

listOfNotNull(relayItem?.code)
listOfNotNull(relayItem?.name)

---

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

}

private suspend fun LazyListState.animateScrollAndCentralizeItem(index: Int) {

Nice catch 👍

---

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

private const val EXTRA_ITEM_CUSTOM_LIST = 1

sealed interface BottomSheetState {

Do we need hidden or can that just be null? Otherwise really nice 👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/CustomListsRepository.kt line 19 at r6 (raw file):

import net.mullvad.mullvadvpn.ui.serviceconnection.RelayListListener

class CustomListsRepository(

General remark for this class, lets replace:

{ 
     return X
}

with

= X

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 196 at r6 (raw file):

    private fun List<RelayItem>.selectChildren(): Set<RelayItem> =
        (this + flatMap { it.allChildren() }).toSet()

⭐

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 196 at r6 (raw file):

    fun createCustomList(name: String): CreateCustomListResult {
        return createCustomList(daemonInterfaceAddress, name)

replace return with =

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 204 at r6 (raw file):

    fun updateCustomList(customList: CustomList): UpdateCustomListResult {
        return updateCustomList(daemonInterfaceAddress, customList)

replace return with =

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 85 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

When I open the dialog for the first time the cursor is red, so feel like I have an error by default. I believe this line might be the culprit?

This should be fixed now

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 89 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Same as above, show error right away.

This should be fixed.

---

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

Previously, Rawa (David Göransson) wrote…

We should rework scroll to current location. Currently if E.g Singapore is selected, I scroll to top to edit a custom list and then go back, it would scroll the user back to Singapore. We need to rework the scroll to work with the custom lists.

The list scrolls to the correct location now, but it still scrolls to often.

---

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

Previously, Rawa (David Göransson) wrote…

Is it possible to break this out? Or do we rely on the smart cast?

I think this comment is not longer relevant?

---

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

Previously, Rawa (David Göransson) wrote…

This looks a bit like a hack, by do we have to close it twice?

I think this comment is not longer relevant?

---

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

Previously, Rawa (David Göransson) wrote…

Do we need hidden or can that just be null? Otherwise really nice 👍

I guess it could just be null

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 127 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Would it throw and crash or be fail silent?

It would fail silent, but I'm not sure what this is referring to now.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 140 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I believe this one selects a parent if all children are checked but the naming of it is not really clear, sounds like we always selects the parents

Should be fixed now.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListViewModel.kt line 35 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I agree. Maybe our ViewState should be able to handle when it is not found? Then we can avoid these defaults. If we get a UI update before user is navigated away it would also say, couldn't find the item rather then empty name and 0 in size.

Will add.

[Pururun]

Reviewable status: 74 of 104 files reviewed, 38 unresolved discussions (waiting on @Rawa)

---

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

Previously, Rawa (David Göransson) wrote…

Sounds better 👍

Done

---

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

Previously, Rawa (David Göransson) wrote…

Can we avoid using Transparent?

Done.

---

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

Previously, Rawa (David Göransson) wrote…

We should make onLongClick nullable and null by default.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 10 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

This doesn't make sense to me, not operation would not undo the Rename action, should we provide a argument with previousName?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 15 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice 🤩

Thanks

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListAction.kt line 30 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Is this parameter needed? Just looks weird to have it there.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 143 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Very deep indentation here, maybe make a

@Composable
fun CustomListsError.toString() = stringResource(
    when(this) {
        ...
    }
... 

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 43 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

I used Dialog or Screen suffix to make it clear that it is a destination. Maybe we should also make a Konsist rule for this 🤔

We decided to use this format after a brief discussion.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 44 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Shouldn't we be able to reuse the same action as created above instead of ListDeleted? 🤔

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DeleteCustomListConfirmationDialog.kt line 62 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

navigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DiscardChangesDialog.kt line 21 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

resultBackNavigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DiscardChangesDialog.kt line 26 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

resultBackNavigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/EditCustomListNameDialog.kt line 61 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

NIT: val uiState by vm.uiState.collectAsState()

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 72 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Remove named argument and open lambda

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListLocationsScreen.kt line 100 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

maybe something hasStagedChanges or hasUnsavedChanges?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 62 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Add a Screensuffix since it is a destination.

We decided not to do this.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 74 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Removed named argument and open lambda

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 92 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels like we should be able to avoid this by using the right types. Let's discuss and have a pair programming session about it.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 122 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Rename this to something else or merge with the function above.

We decided not to do this.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 77 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Add Screen suffix

We decided not to do this.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 130 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

backNavigator::navigateBack

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/EditCustomListScreen.kt line 135 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Remove Screen suffix

We decided not to do this.

---

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

Previously, Rawa (David Göransson) wrote…

listOfNotNull(relayItem?.code)
listOfNotNull(relayItem?.name)

This is not longer relevant.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I guess it could just be null

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/CustomListsRepository.kt line 19 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

General remark for this class, lets replace:

{ 
     return X
}

with

= X

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 69 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

This looks a bit complex, maybe we can simplify it or break it out to a extension function that describes what it does.

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 78 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe it could be more clear if we make an extension that returns are the relevant relays for e.g a RelayItem and then removes / adds them all in one swoop. The same function get all relevant relays can be used when adding and removing relays.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 159 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

This looks like the code above, shouldn't it be possible to do a recursive call to selectParent when we filled a parent?

We do no longer select parents

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/EditCustomListViewModel.kt line 35 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Will add.

Done

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 196 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

replace return with =

Done.

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadDaemon.kt line 204 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

replace return with =

Done.

[Rawa]

Reviewed 11 of 21 files at r8, 19 of 19 files at r9, 2 of 7 files at r10, all commit messages.
Reviewable status: 99 of 104 files reviewed, 9 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 138 at r9 (raw file):

                            elevation = 8.dp,
                            spotColor = Color(0x26000000),
                            ambientColor = Color(0x26000000)

Hard-coded values, maybe even should define a MullvadFab if needed? Also why do we have duplicate usage of shadow?

---

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

                is SelectLocationSideEffect.LocationAddedToCustomList -> {
                    launch {
                        snackbarHostState.showResultSnackbar(

👏 Nice! This became much, much more clean and readable.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListLocationsViewModel.kt line 127 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It would fail silent, but I'm not sure what this is referring to now.

Not sure either 🙈

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeleteCustomListConfirmationViewModel.kt line 24 at r9 (raw file):

                customListActionUseCase
                    .performAction(CustomListAction.Delete(customListId))
                    .getOrThrow()

This would crash the app? Should we be able to handle failure of deletions?

[Rawa]

Reviewable status: 99 of 104 files reviewed, 8 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/CustomListsScreen.kt line 138 at r9 (raw file):

Previously, Rawa (David Göransson) wrote…

Hard-coded values, maybe even should define a MullvadFab if needed? Also why do we have duplicate usage of shadow?

Seems like this jsut got fixed

[Rawa]

Reviewed 1 of 7 files at r10.
Reviewable status: 100 of 104 files reviewed, 8 unresolved discussions (waiting on @Pururun)

[Rawa]

Reviewable status: 100 of 104 files reviewed, 9 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SelectLocationUiState.kt line 25 at r10 (raw file):

        // Show empty state if we don't have any relays or if we are searching and no custom list or
        // relay is found
        val showEmpty = countries.isEmpty() && (inSearch.not() || filteredCustomLists.isEmpty())

Nice addition 👍

[Rawa]

Reviewed 3 of 7 files at r10.
Reviewable status: 103 of 104 files reviewed, 9 unresolved discussions (waiting on @Pururun)

[Rawa]

Reviewed 1 of 7 files at r10.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Pururun)

[Rawa]

Reviewed 14 of 29 files at r11, 4 of 12 files at r12, 20 of 25 files at r13, 10 of 10 files at r14, 4 of 4 files at r15, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DropdownMenuCell.kt line 21 at r15 (raw file):

@Preview
@Composable
private fun PreviewThreeDotCell() {

Did you get caught by Konsist test? 😄

---

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

@Composable
@Preview
private fun PreviewStatusRelayLocationCell() {

This became much more readable 👍

---

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

@Composable
private fun RowScope.Name(relay: RelayItem) {

We only need RowScope because of Alignment.CenterVertically right? We should be able to replace RowScope. with modifier: Modifier = Modifier instead in this case.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/MullvadModalBottomSheet.kt line 51 at r6 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Made some improvements, please check.

🥇 much better!

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 72 at r6 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This seems to work now, please check.

This still looks weird to me. How is this destination removed? Does it rely on that the user of it calls launchSingleTop true?

I was thinking something like this:

                    navigator.navigate(
                        CustomListLocationsDestination(
                            customListId = sideEffect.customListId,
                            newList = true
                        )
                    ) {
                        popUpTo(CreateCustomListDestination) {
                            inclusive = true
                        }
                    }

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Made a fix for this, please check.

Works great now! 👏

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 6 at r15 (raw file):

import net.mullvad.mullvadvpn.relaylist.RelayItem

fun generateRelayItemCountry(

Nice additions 💯

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 29 at r15 (raw file):

    )

fun RelayItem.allChildren(includeChildrenOfChildren: Boolean = true): List<RelayItem> {

I think naming could be better here. Generally children of a node in a tree is the direct children, I believe for all child and grand-child etc. a better name is descendants.
fun RelayItem.children()

fun RelayItem.descendants()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/CustomListsViewModel.kt line 30 at r15 (raw file):

    fun undoDeleteCustomList(action: CustomListAction.Create) {
        viewModelScope.launch { customListActionUseCase.performAction(action) }

😍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeleteCustomListConfirmationViewModel.kt line 24 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This is a little bit obfuscated but delete action can not really return an exception and will always return success. Not sure what we want to do here.

Then I think it is fine if we expect that, then it should crash the app.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt line 41 at r15 (raw file):

    @Test
    fun `give action create when successful should return created result`() = runTest {

On all below given?

A general remark, for this class but also some of the other ones. Mostly I think the tests names are good, but in some cases should be a bit more relaxed, some examples:

give action create when successful should return created result
->
create action should return OK on success (Maybe on success suffix is too much as well?

give action rename when list name already exists should return error
->
rename action should return error when name already exists

give action update locations when successful should return locations changed result
->
update locations action should return OK with changed locations

Maybe @albin-mullvad has an opinion?

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DropdownMenuCell.kt line 21 at r15 (raw file):

Previously, Rawa (David Göransson) wrote…

Did you get caught by Konsist test? 😄

Yeah quite useful. :)

---

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

Previously, Rawa (David Göransson) wrote…

This became much more readable 👍

:)

---

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

Previously, Rawa (David Göransson) wrote…

We only need RowScope because of Alignment.CenterVertically right? We should be able to replace RowScope. with modifier: Modifier = Modifier instead in this case.

I could try

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/MullvadModalBottomSheet.kt line 51 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

🥇 much better!

Thanks. :)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 72 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

This still looks weird to me. How is this destination removed? Does it rely on that the user of it calls launchSingleTop true?

I was thinking something like this:

                    navigator.navigate(
                        CustomListLocationsDestination(
                            customListId = sideEffect.customListId,
                            newList = true
                        )
                    ) {
                        popUpTo(CreateCustomListDestination) {
                            inclusive = true
                        }
                    }

It seems to work fine, but I'm not sure why.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 6 at r15 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice additions 💯

Thanks. If we want to use it in more places I guess we could move it to somewhere else later.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 29 at r15 (raw file):

Previously, Rawa (David Göransson) wrote…

I think naming could be better here. Generally children of a node in a tree is the direct children, I believe for all child and grand-child etc. a better name is descendants.
fun RelayItem.children()

fun RelayItem.descendants()

Can check.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DeleteCustomListConfirmationViewModel.kt line 24 at r9 (raw file):

Previously, Rawa (David Göransson) wrote…

Then I think it is fine if we expect that, then it should crash the app.

👍

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt line 41 at r15 (raw file):

Previously, Rawa (David Göransson) wrote…

On all below given?

A general remark, for this class but also some of the other ones. Mostly I think the tests names are good, but in some cases should be a bit more relaxed, some examples:

give action create when successful should return created result
->
create action should return OK on success (Maybe on success suffix is too much as well?

give action rename when list name already exists should return error
->
rename action should return error when name already exists

give action update locations when successful should return locations changed result
->
update locations action should return OK with changed locations

Maybe @albin-mullvad has an opinion?

I think my idea was to have some kind of pattern of:
given [action] when [internal result] should return [external result] but I think your way of writing is a bit clearer.

[Rawa]

Reviewed 10 of 10 files at r16, 9 of 10 files at r17, all commit messages.
Reviewable status: 127 of 128 files reviewed, 3 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/CreateCustomListDialog.kt line 72 at r6 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It seems to work fine, but I'm not sure why.

As I understand we navigate as follows right now:

Select Location
Select Location -> Create Custom List Dialog ->
Select Location -> Create Custom List Dialog -> Update Locations
Select Location -> ??? Not sure what happens here, because we do up from Update Locations

My proposal above would do the following:

Select Location
Select Location -> Create Custom List Dialog ->
Select Location -> Update Locations
Select Location

We should investigate the back stack and see what it contains.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/PreviewData.kt line 6 at r15 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Thanks. If we want to use it in more places I guess we could move it to somewhere else later.

Yeah, sounds good. That is for another PR.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 23 at r17 (raw file):

}

fun RelayItem.descendants(): List<RelayItem> {

Nice 🌟

[Rawa]

Reviewable status: 127 of 128 files reviewed, 2 unresolved discussions (waiting on @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I could try

looks like this should work fine right?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 29 at r15 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Can check.

Looks great!

[Pururun]

Reviewable status: 125 of 128 files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I could try

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItemExtensions.kt line 29 at r15 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Can check.

Done

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt line 41 at r15 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think my idea was to have some kind of pattern of:
given [action] when [internal result] should return [external result] but I think your way of writing is a bit clearer.

Updated the names of tests somewhat.

[Rawa]

Reviewed 1 of 10 files at r17, 2 of 2 files at r18, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt line 41 at r15 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Updated the names of tests somewhat.

Nice, I think the new names are better 💯

[Rawa]

Reviewed 21 of 21 files at r19, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved