Migrate login screen to compose

[Rawa]

Convert login screen to compose

Also fixes theme colors, adds AccountToken to Model and account visual transformation.

---

This change is 

[linear]

DROID-53 Migrate the login view to compose

[Pururun]

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Rawa)

---

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

        val scrollState = rememberScrollState()

        Surface(modifier = Modifier.fillMaxSize(), color = MaterialTheme.colorScheme.background) {

Just curious, why was this changed?

---

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

        statusBarColor = MaterialTheme.colorScheme.primary,
        navigationBarColor = MaterialTheme.colorScheme.background,
        iconTintColor = MaterialTheme.colorScheme.onPrimary.copy(alpha = AlphaTopBar),

Should we user overlay here as you proposed somewhere else?

---

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

        onAccountClicked = null
    ) {
        Column(modifier = Modifier.fillMaxSize().background(MaterialTheme.colorScheme.primary)) {

Have you tested this on very small screens, the xml file had a scrollview and since this is not scrollable, it might break?

---

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

                AppTheme {
                    val loginUiState by vm.uiState.collectAsState()
                    LaunchedEffect(Unit) {

I don't think we have decided on a standard in regards to actions, but fwiw how it has been done in other screens is that the collect is inside the screen and are referred to as view actions and not side effects, which I guess might be confusing since it's the same name as the function in compose.

---

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

private const val MINIMUM_LOADING_SPINNER_TIME_MILLIS = 500L

sealed interface LoginSideEffect {

Just a personal preference but I don't think this should be in the ViewModel file but its own separate file.
Also see previous comment in regards to naming.

---

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

    @Test
    fun testDefaultState() =
        runTest(UnconfinedTestDispatcher()) {

I think if you set @get:Rule val testCoroutineRule \= TestCoroutineRule() at the top of the test class you don't need to specify UnconfinedTestDispatcher() for every test.

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 60 at r1 (raw file):

    @Test
    fun testCreateAccount() =
        runTest(UnconfinedTestDispatcher()) {

Now this was not in the previous file, but it would be nice to split it up in steps of "Arrange", "Act" and "Assert". See other view model test files for an example.

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/Theme.kt line 27 at r1 (raw file):

        headlineSmall =
            TextStyle(
                color = MullvadWhite,

Really nice that you removed the colors here.
I did that in some other more experimental PR and as far as I remember this hade some unintended side effects in VpnSettingsScreen or somewhere. Have you checked for this?

[Pururun]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Just a personal preference but I don't think this should be in the ViewModel file but its own separate file.
Also see previous comment in regards to naming.

My mistake, actions were inside the view model file in other cases so ignore that.

[Rawa]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Just curious, why was this changed?

Understandable :D I discussed it with @albin-mullvad. Normally a scaffold would setup a surface so onSurface color is used on for the texts. Our third party component for top app bar does not do this, thus the wrong colors was being used.

(Reason for originally removing removing colors from typography, is that they should not contain that information per default, and it caused problems with Material3 TextInputField where we could not set custom colors because some of our fonts used at colors in them. This would cause us to have to wrap text fields with new fonts just because or fonts originally contains colors.)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Should we user overlay here as you proposed somewhere else?

For now I think it is fine since we set the navigation bar color the line above. But long-term I think we should completely remove the current top bar and also then this alpha.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Have you tested this on very small screens, the xml file had a scrollview and since this is not scrollable, it might break?

Good catch, I'll try it and update.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I don't think we have decided on a standard in regards to actions, but fwiw how it has been done in other screens is that the collect is inside the screen and are referred to as view actions and not side effects, which I guess might be confusing since it's the same name as the function in compose.

I saw that when reviewing another PR, I think the name viewActions is a bit confusing but it is better to align it with the rest than making up something different here. I'll update it. 👍

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

My mistake, actions were inside the view model file in other cases so ignore that.

I do agree with you, I think they belong closer to where the ViewState is, they are sort of the contracts between the view and the view model. Maybe further down the road we can group it more nicely by Screen.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I think if you set @get:Rule val testCoroutineRule \= TestCoroutineRule() at the top of the test class you don't need to specify UnconfinedTestDispatcher() for every test.

Thanks, I'll take a look at it! 🥇

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 60 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Now this was not in the previous file, but it would be nice to split it up in steps of "Arrange", "Act" and "Assert". See other view model test files for an example.

Good idea, I'll take a look. 👍

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/Theme.kt line 27 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Really nice that you removed the colors here.
I did that in some other more experimental PR and as far as I remember this hade some unintended side effects in VpnSettingsScreen or somewhere. Have you checked for this?

I did some fix (Surface issue above), I looked around in the app but could not find any more glaring issues. But there could definitely be placed that I've missed or that slightly changed in color now when they use the default colors from theme.

[Rawa]

Reviewable status: 20 of 24 files reviewed, 8 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Rawa (David Göransson) wrote…

Good catch, I'll try it and update.

Fixed

---

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

Previously, Rawa (David Göransson) wrote…

I saw that when reviewing another PR, I think the name viewActions is a bit confusing but it is better to align it with the rest than making up something different here. I'll update it. 👍

Fixed

---

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

Previously, Rawa (David Göransson) wrote…

Thanks, I'll take a look at it! 🥇

Fixed, thanks for the tip!

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 60 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Good idea, I'll take a look. 👍

Does it look better now?

[Pururun]

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

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 60 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Does it look better now?

I guess it could be clarified even more with comments like // Arrange but not super important.

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/Theme.kt line 27 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I did some fix (Surface issue above), I looked around in the app but could not find any more glaring issues. But there could definitely be placed that I've missed or that slightly changed in color now when they use the default colors from theme.

Just a reminder to fix the issues we discovered in the design meeting.

[Rawa]

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

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModelTest.kt line 60 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I guess it could be clarified even more with comments like // Arrange but not super important.

Fixed

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/Theme.kt line 27 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Just a reminder to fix the issues we discovered in the design meeting.

Fixed the ones we saw, settings and split tunneling 👍 c4a6e91#diff-783f2fb929c3c90fc8a4580133499504be9229defc67d9de43127b60110de78b

[Pururun]

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

---

android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/Theme.kt line 27 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Fixed the ones we saw, settings and split tunneling 👍 c4a6e91#diff-783f2fb929c3c90fc8a4580133499504be9229defc67d9de43127b60110de78b

Nice

[sabercodic]

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

---

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

                                top = Dimens.mediumPadding
                            )
                            .size(Dimens.progressIndicatorSize)

Is it related to this task? it is better to do it in different PR if it is unrelated

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/LoginScreen.kt line 96 at r4 (raw file):

    onDeleteHistoryClick: () -> Unit = {},
    onAccountNumberChange: (String) -> Unit = {},
    onSettingsClick: () -> Unit = {},

App crashes when I click on Settings (if you click on text field and then click on settings)

---

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

                            modifier =
                                Modifier.size(Dimens.progressIndicatorSize)
                                    .testTag(CIRCULAR_PROGRESS_INDICATOR),

same as before

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/AccountTokenVisualTransformation.kt line 20 at r4 (raw file):

        object : OffsetMapping {
            override fun originalToTransformed(offset: Int): Int =
                offset + (offset - 1) / ACCOUNT_TOKEN_CHUNK_SIZE

this will prevent user from change cursor to the end of input after adding 2 space(10 character)

[Rawa]

Reviewable status: 20 of 27 files reviewed, 4 unresolved discussions (waiting on @Pururun and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/LoginScreen.kt line 96 at r4 (raw file):

Previously, sabercodic wrote…

App crashes when I click on Settings (if you click on text field and then click on settings)

Fixed with menuAchor being conditionally added! 🙏

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/AccountTokenVisualTransformation.kt line 20 at r4 (raw file):

Previously, sabercodic wrote…

this will prevent user from change cursor to the end of input after adding 2 space(10 character)

Good catch! Should be working now :)

---

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

Previously, sabercodic wrote…

Is it related to this task? it is better to do it in different PR if it is unrelated

Done.

---

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

Previously, sabercodic wrote…

same as before

Done.

[sabercodic]

Reviewed 4 of 7 files at r5, all commit messages.
Reviewable status: 24 of 27 files reviewed, 6 unresolved discussions (waiting on @Pururun and @Rawa)

---

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

                    Image(
                        painter = painterResource(id = R.drawable.icon_fail),
                        contentDescription = null,

It is better to add content description for icons

---

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

                        contentScale = ContentScale.Inside
                    )
                }

just suggestion, it will be good to add some comment here to explain why there is no icon (just empty big box) sometimes

[Rawa]

Reviewable status: 24 of 27 files reviewed, 2 unresolved discussions (waiting on @Pururun and @sabercodic)

---

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

Previously, sabercodic wrote…

It is better to add content description for icons

I didn't feel that it added anything for the user in this case since the text below will say, "Login failed" or "Logged in", but I can add them if you feel they make sense.

---

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

Previously, sabercodic wrote…

just suggestion, it will be good to add some comment here to explain why there is no icon (just empty big box) sometimes

Done.

[Rawa]

Reviewable status: 24 of 27 files reviewed, 2 unresolved discussions (waiting on @Pururun and @sabercodic)

---

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

Previously, Rawa (David Göransson) wrote…

I didn't feel that it added anything for the user in this case since the text below will say, "Login failed" or "Logged in", but I can add them if you feel they make sense.

Done

[sabercodic]

Reviewed 2 of 7 files at r5, 2 of 3 files at r6, all commit messages.
Reviewable status: 26 of 27 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)

---

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

@OptIn(ExperimentalMaterial3Api::class)
private fun LoginContent(
    state: LoginUiState,

Just suggestion, we use uiState instead of state in most of screens, it will be better if we do same here

[Rawa]

Reviewable status: 25 of 27 files reviewed, 1 unresolved discussion (waiting on @Pururun and @sabercodic)

---

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

Previously, sabercodic wrote…

Just suggestion, we use uiState instead of state in most of screens, it will be better if we do same here

Done.

[sabercodic]

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

[Pururun]

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