Add server ip overrides feature

[Rawa]

This PR adds IP Overrides feature

---

This change is 

[linear]

DROID-709 Create Server IP Overrides composable

[Pururun]

Reviewed 57 of 57 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/InfoIconButton.kt line 16 at r1 (raw file):

        Icon(
            painter = painterResource(id = R.drawable.icon_info),
            contentDescription = null,

This should probably be a parameter

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/InfoIconButton.kt line 17 at r1 (raw file):

            painter = painterResource(id = R.drawable.icon_info),
            contentDescription = null,
            tint = MaterialTheme.colorScheme.onPrimary

Is this always used on primary? Otherwise this should be set as a parameter

---

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

    inactiveColor: Color = MaterialTheme.colorScheme.error,
) {
    Row(

Is there a reason this does not use BaseCell ?

---

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

    AlertDialog(
        containerColor = MaterialTheme.colorScheme.background,
        confirmButton = {

This should be a confirm button and a dismiss button

---

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

            Text(
                text = stringResource(id = R.string.server_ip_overrides_reset_title),
                color = MaterialTheme.colorScheme.onPrimary

This should be on background

---

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

    importByTextResult.OnNavResultValue(vm::importText)
    clearAllOverrides.OnNavResultValue { vm.clearAllOverrides() }

I am not sure how we want to do this, but in custom lists I put this call in the delete confirmation dialog.

---

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

        actions = {
            TopBarActions(
                state.overridesActive,

This could be a named argument as well?

---

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

        Column(
            modifier = modifier.animateContentSize(),

👏

---

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

import kotlinx.coroutines.launch

suspend fun SnackbarHostState.showSnackBarDirect(

The name of this function is a bit confusing, do we show it with a delay usually?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SettingsRepository.kt line 106 at r1 (raw file):

    suspend fun applySettingsPatch(json: String) =
        withContext(Dispatchers.IO) {
            val deferred = async { messageHandler.events<ApplyJsonSettingsResult>().first() }

Is deferred really necessary here? What would happen without it?

Also we are using Dispatchers.IO here, but there is a dispatcher in the constructor.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 99 at r1 (raw file):

        // Ensure we are responding to the correct request
        if (requestCode == 0) {

Magical number?

---

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

    private suspend fun applySettingsPatch(json: String) {
        // Wait for daemon to come online since we might be paused
        val connResult =

Is there a specific reason why we are waiting for connection result here? It is not required anywhere else.

---

android/lib/resource/src/main/res/values/strings.xml line 339 at r1 (raw file):

    <string name="import_overrides_bottom_sheet_override_warning">Importing new overrides might replace some previously imported overrides.</string>
    <string name="patch_not_matching_specification">Patch not matching specification</string>
    <string name="settings_patch_error_invalid_or_missing_value">Invalid or missing value: \'%1$s\'</string>

Maybe more of a design question, but in custom lists I used normal quotation marks like "".

[Rawa]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/InfoIconButton.kt line 16 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should probably be a parameter

Good idea! Fixed

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/InfoIconButton.kt line 17 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is this always used on primary? Otherwise this should be set as a parameter

True, fixing!

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Is there a reason this does not use BaseCell ?

Nope! Fixed!

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

This should be a confirm button and a dismiss button

Fixed

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

This should be on background

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

This could be a named argument as well?

Fixed!

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

The name of this function is a bit confusing, do we show it with a delay usually?

Discussed and fixed!

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/repository/SettingsRepository.kt line 106 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is deferred really necessary here? What would happen without it?

Also we are using Dispatchers.IO here, but there is a dispatcher in the constructor.

Probably wouldn't do much of a difference if we do not do deferred, but it feels more correct do listen before we execute the send. Otherwise if some one adds code in between at a later stage that takes time we might miss the message. E.g following scenario

• Send message
• Receive response
• Start listen, and wait forever.

Good catch! Thanks! 👏

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt line 99 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Magical number?

Yes! Reused from below, we should clearly define it. Fixed!

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Is there a specific reason why we are waiting for connection result here? It is not required anywhere else.

Yes! There is, because we launch intent to open the FilePicker, which is not part of our activity, it means we will get paused, and also kill our connection to the daemon. So sending the message right away would mean that the Daemon is not connected to the UI and message would get lost. I'll clarify it in the the comment.

---

android/lib/resource/src/main/res/values/strings.xml line 339 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe more of a design question, but in custom lists I used normal quotation marks like "".

I'll switch over to " for now. 👍

[Rawa]

Reviewable status: 46 of 60 files reviewed, 12 unresolved discussions (waiting on @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I am not sure how we want to do this, but in custom lists I put this call in the delete confirmation dialog.

Yeah, I think you are correct. If we ever want to introduce an error this would not be a nice experience. I've migrated it and created a new ViewModel for it.

[Pururun]

Reviewed 17 of 17 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/dialog/ResetServerIPOverridesConfirmationDialogTest.kt line 34 at r3 (raw file):

            // Assert
            onNodeWithText(DELETE_TITLE.format(name)).assertExists()

Nice

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/ServerIpOverridesScreenTest.kt line 54 at r3 (raw file):

    @Test
    fun testOverridesInactive() =

Why does all the tests start with test?

---

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

Previously, Rawa (David Göransson) wrote…

Yes! There is, because we launch intent to open the FilePicker, which is not part of our activity, it means we will get paused, and also kill our connection to the daemon. So sending the message right away would mean that the Daemon is not connected to the UI and message would get lost. I'll clarify it in the the comment.

Ah that's understandable. :)

[Rawa]

Reviewable status: 52 of 64 files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/ServerIpOverridesScreenTest.kt line 54 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Why does all the tests start with test?

Fixed!

[Pururun]

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

[MarkusPettersson98]

Reviewed 4 of 57 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 63 of 64 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)

---

mullvad-jni/src/daemon_interface.rs line 34 at r6 (raw file):

    #[error("Patch error")]
    Patch(#[source] patch::Error),

You could replace #[source] with #[from] to get the From<patch::Error for Error for free 😊

Code quote:

    #[error("Patch error")]
    Patch(#[source] patch::Error),

[Rawa]

Reviewable status: 62 of 64 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98 and @Pururun)

---

mullvad-jni/src/daemon_interface.rs line 34 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

You could replace #[source] with #[from] to get the From<patch::Error for Error for free 😊

🙌 thanks! Fixed now! :)

[MarkusPettersson98]

Reviewed 1 of 1 files at r7.
Reviewable status: 63 of 64 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)

---

mullvad-jni/src/daemon_interface.rs line 34 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

🙌 thanks! Fixed now! :)

🔥