Migrate to compose navigation

[Rawa]

---

This change is 

[linear]

DROID-173 Migrate to compose navigation

Some proposals for the NavGraph has been made in a previous issue: DROID-443

[Rawa]

Fixes:
#5350
#3395

[Pururun]

Reviewed 8 of 8 files at r33, 1 of 1 files at r35, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @sabercodic)

[albin-mullvad]

Reviewed 1 of 131 files at r1, 1 of 3 files at r11, 1 of 24 files at r13, 1 of 23 files at r30, 1 of 13 files at r31, 8 of 8 files at r33.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 19 at r34 (raw file):

import org.joda.time.DateTime

const val accountRefreshInterval = 1000L * 60L // 1 minute

I suggest renaming to accountRefreshIntervalMillis

Code quote:

accountRefreshInterval

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 20 at r34 (raw file):

const val accountRefreshInterval = 1000L * 60L // 1 minute
const val bufferTime = 1000L * 60L // 1 minute

I suggest renaming to bufferTimeMillis

Code quote:

bufferTime

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 1 at r34 (raw file):

package net.mullvad.mullvadvpn.viewmodel

It seems to me like it might be easier to minimize the logic in this vm and instead let the vpn settings vm interact with the repository by handling results from the dialog. Or are there clear benefits with this approach?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 59 at r34 (raw file):

    private val inetAddressValidator: InetAddressValidator,
    private val index: Int? = null,
    initialValue: String?,

I'm not sure these belong in the constructor. Seems like they should belong somewhere else, such as a setter or a regular function. Any thoughts about that?

Code quote:

    private val index: Int? = null,
    initialValue: String?,

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 42 at r34 (raw file):

) : ViewModel() {

    private val _uiSideEffect = MutableSharedFlow<UiSideEffect>(replay = 1)

As discussed, this isn't optimal, since this makes the side effect to more of a state, which wasn't the original intention. But perhaps an OK intermediate solution until we can handle it in a better way 🤔

Code quote:

replay = 1

[albin-mullvad]

Reviewed 1 of 131 files at r1, 1 of 48 files at r2, 1 of 17 files at r14, 1 of 23 files at r30, 1 of 1 files at r35.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ChangelogViewModelTest.kt line 27 at r35 (raw file):

    private lateinit var viewModel: ChangelogViewModel

    private val buildVersionCode = 10

nit: perhaps change to const

Code quote:

private val buildVersionCode = 10

[albin-mullvad]

Reviewed 1 of 24 files at r13, 1 of 15 files at r22, 2 of 11 files at r29, 1 of 23 files at r30, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCaseTest.kt line 46 at r35 (raw file):

    @Test
    fun `Tunnel is blocking because out of time should emit true`() = runTest {

Can we add another test to cover other tunnel states?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ChangelogViewModelTest.kt line 43 at r35 (raw file):

    @Test
    fun testUpToDateVersionCode() = runTest {

Can the name of this funciton be clarified a bit? It's not super clear what we intend to test when just reading the function name. Same applies for testNotUpToDateVersionCode.

Code quote:

testUpToDateVersionCode

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModelTest.kt line 55 at r35 (raw file):

    private val paymentAvailability = MutableStateFlow<PaymentAvailability?>(null)
    private val purchaseResult = MutableStateFlow<PurchaseResult?>(null)
    private val outOfTime = MutableStateFlow(true)

I suggest using the same name as in another test outOfTimeViewFlow

Code quote:

 outOfTime

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModelTest.kt line 54 at r35 (raw file):

    private val purchaseResult = MutableStateFlow<PurchaseResult?>(null)
    private val paymentAvailability = MutableStateFlow<PaymentAvailability?>(null)
    private val outOfTime = MutableStateFlow(true)

I suggest using the same name as in another test outOfTimeViewFlow

Code quote:

outOfTime 

[albin-mullvad]

Reviewed 2 of 48 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt line 23 at r35 (raw file):

import org.junit.Test

class ReportProblemModelTest {

Should probably be named ReportProblemViewModelTest? 🤔

Code quote:

ReportProblemModelTest

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt line 135 at r35 (raw file):

            {
                problemReportFlow.value = problemReportFlow.value.copy(description = arg(0))
            }

I'm a bit concered with introducing this type of logic using the mocks since the expected behavior isn't explicitly defined. This can potentially result in false-positives, especially when the code changes over time. Perhaps we can discuss it a bit?

Code quote:

        coEvery { mockProblemReportRepository.setEmail(any()) } answers
            {
                problemReportFlow.value = problemReportFlow.value.copy(email = arg(0))
            }
        coEvery { mockProblemReportRepository.setDescription(any()) } answers
            {
                problemReportFlow.value = problemReportFlow.value.copy(description = arg(0))
            }

[Rawa]

Reviewable status: 165 of 170 files reviewed, 12 unresolved discussions (waiting on @albin-mullvad, @Pururun, and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 19 at r34 (raw file):

Previously, albin-mullvad wrote…

I suggest renaming to accountRefreshIntervalMillis

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 20 at r34 (raw file):

Previously, albin-mullvad wrote…

I suggest renaming to bufferTimeMillis

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 1 at r34 (raw file):

Previously, albin-mullvad wrote…

It seems to me like it might be easier to minimize the logic in this vm and instead let the vpn settings vm interact with the repository by handling results from the dialog. Or are there clear benefits with this approach?

I see two major problems with letting the VPNSettings view model handling it.

1. It becomes a all knowing object doing more than it is suppose to.
2. It would not be able to handle input errors or logic when modifying values. E.g imagine, validation of input etc should not be the responsibility of the VpnSettingsViewModel, same goes actually for saving the actual value. If saving it would fail this dialog should convey that error message.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 59 at r34 (raw file):

Previously, albin-mullvad wrote…

I'm not sure these belong in the constructor. Seems like they should belong somewhere else, such as a setter or a regular function. Any thoughts about that?

Idea is that we create a view model that is dedicated to handling the dialog for a specific item, if we don't do it like this the view itself needs to send the correct index as part of it's starting setup or each modification. E.g with LaunchedEffect or through each modification etc.

Later on I'd also probably arguing at making two viewmodels or making the argument a bit more complex like this:

sealed interface DnsDialogNavArgs {
    data object NewItem: DnsDialogNavArgs
    data class Modify(index: Int, initialValue: String): DnsDialogNavArgs
}

Possibly initialValue might not be needed if we can consider the data from daemon to be instant or if we show a loading state before having received the data.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ChangelogViewModelTest.kt line 27 at r35 (raw file):

Previously, albin-mullvad wrote…

nit: perhaps change to const

Fixed and moved to Companion

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ChangelogViewModelTest.kt line 43 at r35 (raw file):

Previously, albin-mullvad wrote…

Can the name of this funciton be clarified a bit? It's not super clear what we intend to test when just reading the function name. Same applies for testNotUpToDateVersionCode.

I've clarified them a bit testUpToDateVersionCodeShouldNotEmitChangelog maybe we should consider other naming scheme Up to date VersionCode should not emit changelog would be easier to read. May another task when redoing tests?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModelTest.kt line 55 at r35 (raw file):

Previously, albin-mullvad wrote…

I suggest using the same name as in another test outOfTimeViewFlow

I was following the pattern of the other defined flows above, but I'll change them all. 👍

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModelTest.kt line 54 at r35 (raw file):

Previously, albin-mullvad wrote…

I suggest using the same name as in another test outOfTimeViewFlow

Same as above. Fixed.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ReportProblemModelTest.kt line 23 at r35 (raw file):

Previously, albin-mullvad wrote…

Should probably be named ReportProblemViewModelTest? 🤔

Done.

[albin-mullvad]

Reviewed 5 of 5 files at r36, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 59 at r34 (raw file):

Previously, Rawa (David Göransson) wrote…

Idea is that we create a view model that is dedicated to handling the dialog for a specific item, if we don't do it like this the view itself needs to send the correct index as part of it's starting setup or each modification. E.g with LaunchedEffect or through each modification etc.

Later on I'd also probably arguing at making two viewmodels or making the argument a bit more complex like this:

sealed interface DnsDialogNavArgs {
    data object NewItem: DnsDialogNavArgs
    data class Modify(index: Int, initialValue: String): DnsDialogNavArgs
}

Possibly initialValue might not be needed if we can consider the data from daemon to be instant or if we show a loading state before having received the data.

Yeah, I like the NavArgs approach better than setting it as part of the vm constructor. How much work would it be to do something like that now? We can perhaps plan a separate issue for it?

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/ChangelogViewModelTest.kt line 43 at r35 (raw file):

Previously, Rawa (David Göransson) wrote…

I've clarified them a bit testUpToDateVersionCodeShouldNotEmitChangelog maybe we should consider other naming scheme Up to date VersionCode should not emit changelog would be easier to read. May another task when redoing tests?

Sounds good 👍

[Rawa]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 59 at r34 (raw file):

Previously, albin-mullvad wrote…

Yeah, I like the NavArgs approach better than setting it as part of the vm constructor. How much work would it be to do something like that now? We can perhaps plan a separate issue for it?

I think we should keep this PR down, this is a bit more similar to how it worked before and then this PR starts with separating them a bit enabling for it to be improved in the future.
https://linear.app/mullvad/issue/DROID-579/improve-dnsdialogviewmodel-arguments

[albin-mullvad]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 1 at r34 (raw file):

Previously, Rawa (David Göransson) wrote…

I see two major problems with letting the VPNSettings view model handling it.

1. It becomes a all knowing object doing more than it is suppose to.
2. It would not be able to handle input errors or logic when modifying values. E.g imagine, validation of input etc should not be the responsibility of the VpnSettingsViewModel, same goes actually for saving the actual value. If saving it would fail this dialog should convey that error message.

Yeah, both have some pros and cons imo, but I'm fine keeping it as-is in this PR 👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 59 at r34 (raw file):

Previously, Rawa (David Göransson) wrote…

I think we should keep this PR down, this is a bit more similar to how it worked before and then this PR starts with separating them a bit enabling for it to be improved in the future.
https://linear.app/mullvad/issue/DROID-579/improve-dnsdialogviewmodel-arguments

👍

[albin-mullvad]

Reviewed 16 of 16 files at r37, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/dialog/CustomPortDialogTest.kt line 62 at r16 (raw file):

Previously, Rawa (David Göransson) wrote…

@sabercodic Could you create a linear issue with that you want achieved and mark this as not blocked? 🙏

@sabercodic ping! Please share a link here.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 42 at r34 (raw file):

Previously, albin-mullvad wrote…

As discussed, this isn't optimal, since this makes the side effect to more of a state, which wasn't the original intention. But perhaps an OK intermediate solution until we can handle it in a better way 🤔

No, Channel for this one?

[sabercodic]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Rawa)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/dialog/CustomPortDialogTest.kt line 62 at r16 (raw file):

Previously, albin-mullvad wrote…

@sabercodic ping! Please share a link here.

https://linear.app/mullvad/issue/DROID-581/mock-utils-classes-and-object-functions-to-remove-magic-entries-on

[sabercodic]

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

[albin-mullvad]

Looks good, just need to cleanup the commits a bit!

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

[albin-mullvad]

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