Add support for custom lists

[Pururun]

Add support for custom lists and connected actions to the android app.

THIS DOES NOT INCLUDE ANY UI CHANGES. This will come in the next PR.

---

This change is 

[linear]

DROID-653 Add support for Custom List in the app layer

[dlon]

Reviewed 5 of 32 files at r1.
Reviewable status: 5 of 32 files reviewed, all discussions resolved

[dlon]

Reviewed 2 of 4 files at r2.
Reviewable status: 5 of 34 files reviewed, all discussions resolved

[Rawa]

Reviewed 27 of 32 files at r1, 2 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/CustomListExtensions.kt line 6 at r2 (raw file):

fun CustomList.toRelayItemList(relayCountries: List<RelayCountry>): CustomRelayItemList =
    // return this.items.map { RelayItem(it) }

Old comment?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt line 73 at r2 (raw file):

fun List<RelayCountry>.findItemForGeographicLocationConstraint(
    location: GeographicLocationConstraint

constraint sounds like a better variable name than location, it feels more accurate to the type. What do you think?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt line 233 at r2 (raw file):

                }
            }
            else -> this

Can we be explicit and enter the other alternatives?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/SelectedLocation.kt line 8 at r2 (raw file):

    val id: String,
    val name: String,
    val geographicLocationConstraint: GeographicLocationConstraint?

I'm a bit confused by the naming here. SelectedLocation, why does it need a location constraint? Is it the constraint that was used to select this location?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/CustomListsUseCase.kt line 6 at r2 (raw file):

import net.mullvad.mullvadvpn.repository.CustomListsRepository

class CustomListsUseCase(private val customListsRepository: CustomListsRepository) {

I'm not sure if this UseCase add any value, seems like it just wraps the repository, if this is the case and no other logic is needed we might as well just use the Repository right away.

---

android/lib/ipc/src/main/kotlin/net/mullvad/mullvadvpn/lib/ipc/Event.kt line 68 at r2 (raw file):

    @Parcelize object VpnPermissionRequest : Event()

    @Parcelize data class CreateCustomListResultEvent(val result: String) : Event()

CreateCustomListResultwork be enough, also what does result contain? Can we make it more clear? How do we interpret it?

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/CustomListsSettings.kt line 6 at r2 (raw file):

import kotlinx.parcelize.Parcelize

@Parcelize data class CustomListsSettings(val customLists: ArrayList<CustomList>) : Parcelable

Is ArrayList because it is from the daemon? Would be nice if it was List otherwise.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Settings.kt line 10 at r2 (raw file):

    val relaySettings: RelaySettings,
    val obfuscationSettings: ObfuscationSettings,
    val customLists: CustomListsSettings,

I guess we don't have full control of this structure since it comes from the daemon? It would make more sense otherwise if it just was:

data class Settings( ... val customLists: List<CustomList> ... )

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/CustomListExtensions.kt line 6 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Old comment?

Probably. Fixed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt line 73 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

constraint sounds like a better variable name than location, it feels more accurate to the type. What do you think?

Sure. Fixed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt line 233 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Can we be explicit and enter the other alternatives?

Added null instead.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/SelectedLocation.kt line 8 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I'm a bit confused by the naming here. SelectedLocation, why does it need a location constraint? Is it the constraint that was used to select this location?

I think we should discuss this in person.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/CustomListsUseCase.kt line 6 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I'm not sure if this UseCase add any value, seems like it just wraps the repository, if this is the case and no other logic is needed we might as well just use the Repository right away.

Yeah it was added when I thought it would be more complex. Will remove.

---

android/lib/ipc/src/main/kotlin/net/mullvad/mullvadvpn/lib/ipc/Event.kt line 68 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

CreateCustomListResultwork be enough, also what does result contain? Can we make it more clear? How do we interpret it?

Yeah agree it is quite redundant to add Event at the end. Will remove. Updated the val to call it listId as it is the value returned.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/CustomListsSettings.kt line 6 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Is ArrayList because it is from the daemon? Would be nice if it was List otherwise.

Yeah daemon dependency. :/

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Settings.kt line 10 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I guess we don't have full control of this structure since it comes from the daemon? It would make more sense otherwise if it just was:

data class Settings( ... val customLists: List<CustomList> ... )

Yeah would just add a lot of unnecessary code to do it like that.

[Pururun]

Reviewable status: 11 of 43 files reviewed, 6 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/SelectedLocation.kt line 8 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think we should discuss this in person.

Implemented what we discussed, making CustomList implement RelayItem. Please check. :)

[Rawa]

Reviewed 32 of 32 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

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

                    }
                }
                else -> {}

Preferably we should be explicit here:

RelayItem.CustomList, RelayItem.Relay -> {}

---

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

                is RelayItem.City -> relayListState.selectedItem.location.countryCode
                is RelayItem.Relay -> relayListState.selectedItem.location.countryCode
                else -> null

Same here, lets avoid else.

We could also make it an extension function or property:

fun RelayListState.RelayList.indexOfSelectedRelayItem(): Int {
    if(selectedItem == null) {
        return -1
    }
    countries.indexOfFirst { relayCountry ->
        relayCountry.location.location.country ==
            when (relayListState.selectedItem) {
                is RelayItem.CustomList -> null
                is RelayItem.Country -> relayListState.selectedItem.code
                is RelayItem.City -> relayListState.selectedItem.location.countryCode
                is RelayItem.Relay -> relayListState.selectedItem.location.countryCode
                null -> null
    }
}

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayItem.kt line 79 at r3 (raw file):

        }
    }
}

Looks really good! Nice work! 👏 I'm a little bit against having expanded here but since it does not introduce it as a new concept I think we can consider that in another PR.

---

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

        is RelayItem.CustomList -> LocationConstraint.CustomList(id)
    }
}

Nice 🤩

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/relaylist/RelayListExtensions.kt line 233 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Added null instead.

Nice 👍

---

android/lib/ipc/src/main/kotlin/net/mullvad/mullvadvpn/lib/ipc/Event.kt line 68 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Yeah agree it is quite redundant to add Event at the end. Will remove. Updated the val to call it listId as it is the value returned.

Great 👍 much more clear!

[Pururun]

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

---

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

Previously, Rawa (David Göransson) wrote…

Preferably we should be explicit here:

RelayItem.CustomList, RelayItem.Relay -> {}

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Same here, lets avoid else.

We could also make it an extension function or property:

fun RelayListState.RelayList.indexOfSelectedRelayItem(): Int {
    if(selectedItem == null) {
        return -1
    }
    countries.indexOfFirst { relayCountry ->
        relayCountry.location.location.country ==
            when (relayListState.selectedItem) {
                is RelayItem.CustomList -> null
                is RelayItem.Country -> relayListState.selectedItem.code
                is RelayItem.City -> relayListState.selectedItem.location.countryCode
                is RelayItem.Relay -> relayListState.selectedItem.location.countryCode
                null -> null
    }
}

Done.

[Rawa]

Reviewed 4 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

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

}

private fun RelayListState.RelayList.indexOfSelectedRelayItem(selectedItem: RelayItem?): Int =

Nitpicking, indexOfRelayItem(item: RelayItem?) selection really has nothing to do with it at this level. Soooorrrii 🗡️

[Rawa]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

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

Previously, Rawa (David Göransson) wrote…

Nitpicking, indexOfRelayItem(item: RelayItem?) selection really has nothing to do with it at this level. Soooorrrii 🗡️

or rather just indexOf() relayItem is sort of already described in argument

[Rawa]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

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

Previously, Rawa (David Göransson) wrote…

or rather just indexOf() relayItem is sort of already described in argument

Clarification, we can remove selectedItem since it already exists in RelayList

[Pururun]

Reviewable status: 42 of 43 files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

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

Previously, Rawa (David Göransson) wrote…

Clarification, we can remove selectedItem since it already exists in RelayList

Done.

[dlon]

Reviewed 1 of 4 files at r4.
Reviewable status: 42 of 43 files reviewed, 1 unresolved discussion (waiting on @Rawa)

[Rawa]

Great work! 👏

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