Add info dialog for device name

[sabercodic]

This change is 

[linear]

DROID-94 Add info dialog for device name

To help users further to understand the device concept, an info button next to the device name in the account view is added that opens an info dialog that explains the device concept.

Note: The Confluence and Zeplin links references other work related to the device concept as well

---

Confluence: https://mullvad.atlassian.net/l/cp/1tRhWft7

Zeplin: https://zpl.io/XYon3P8

[Pururun]

Reviewed 8 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 38 at r1 (raw file):

) {
    return if (content.isNotEmpty()) {

This is probably unnecessary to have in this commit.

---

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

                appendLine(stringResource(id = R.string.device_name_info_part1))
                appendLine(stringResource(id = R.string.device_name_info_part2))
                appendLine(stringResource(id = R.string.device_name_info_part3))

Any specific reason on why this needed to be split up in three parts?

---

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

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

User material colors.

---

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

    val accountExpiry: DateTime?

    data class DefaultUiState(

Is there a better way to do this?
I know that we use it in VpnSettings, but it feels like it just creates a lot of duplicate code.
Maybe adding a parameter that is called dialogState?

---

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

import net.mullvad.mullvadvpn.model.DeviceState

data class AccountViewModelState(

Do we need a ViewModel state? Seems like it is adding unneeded complexity.

[albin-mullvad]

Reviewed 3 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Pururun and @sabercodic)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

User material colors.

MullvadWhite and other colors have been marked as Deprecated or internal as of #5034 and should therefore not be used. Instead material colors should be used as mentioned by @Pururun . If not possible we'll discuss it on a case-to-case basis.

---

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

                )
                val horizontalPadding = Dimens.mediumPadding
                val verticalPadding = Dimens.infoButtonVerticalPadding

Are these vals necessary?

Code quote:

                val horizontalPadding = Dimens.mediumPadding
                val verticalPadding = Dimens.infoButtonVerticalPadding

[sabercodic]

Reviewable status: 6 of 9 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Any specific reason on why this needed to be split up in three parts?

to be same as desktop

---

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

Previously, albin-mullvad wrote…

MullvadWhite and other colors have been marked as Deprecated or internal as of #5034 and should therefore not be used. Instead material colors should be used as mentioned by @Pururun . If not possible we'll discuss it on a case-to-case basis.

Done.

---

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

Previously, albin-mullvad wrote…

Are these vals necessary?

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Is there a better way to do this?
I know that we use it in VpnSettings, but it feels like it just creates a lot of duplicate code.
Maybe adding a parameter that is called dialogState?

I did it to keep integrity on our architecture, should I change it?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/InformationView.kt line 38 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This is probably unnecessary to have in this commit.

Done.

[albin-mullvad]

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

---

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

                modifier = Modifier.padding(start = Dimens.sideMargin, end = Dimens.sideMargin)
            )
            Row {

Out of curiosity, why did you have to add this Row?

Code quote:

Row {

[sabercodic]

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

Previously, albin-mullvad wrote…

Out of curiosity, why did you have to add this Row?

to place info icon next to the device name

[sabercodic]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Do we need a ViewModel state? Seems like it is adding unneeded complexity.

I did it to keep integrity on our architecture, should I change it?

[Pururun]

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

---

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

Previously, sabercodic wrote…

I did it to keep integrity on our architecture, should I change it?

I was thinking we should use something else, since when we have used it in VpnSettings it has caused a lot of easy to miss bugs. More or less states are not kept when the dialog is showing.

---

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

Previously, sabercodic wrote…

I did it to keep integrity on our architecture, should I change it?

See comment above.

[sabercodic]

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

I was thinking we should use something else, since when we have used it in VpnSettings it has caused a lot of easy to miss bugs. More or less states are not kept when the dialog is showing.

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

See comment above.

Done.

[Pururun]

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

---

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

Previously, sabercodic wrote…

Done.

Nice, I think it's an improvement. I guess from a future-proof perspective I think it could be even better if instead of a boolean for every dialog we could have some kind of dialogState.
So something like this:

sealed interface AccountDialogState
data object None: AccountDialogState
data object DeviceInfoDialog: AccountDialogState

And then we can add
val dialogState: AccountDialogState to AccountUiState

---

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

Previously, sabercodic wrote…

Done.

Still not sure why we need a ui state and a view model state when they are the same. I guess we could move the dialogState idea from the view model state here?

[sabercodic]

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Nice, I think it's an improvement. I guess from a future-proof perspective I think it could be even better if instead of a boolean for every dialog we could have some kind of dialogState.
So something like this:

sealed interface AccountDialogState
data object None: AccountDialogState
data object DeviceInfoDialog: AccountDialogState

And then we can add
val dialogState: AccountDialogState to AccountUiState

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Still not sure why we need a ui state and a view model state when they are the same. I guess we could move the dialogState idea from the view model state here?

Done.

[Pururun]

Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @sabercodic)

---

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

Previously, sabercodic wrote…

Done.

Nice work. I think it would be better to call the single state UiState, that is what it is called before your PR.

[sabercodic]

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Nice work. I think it would be better to call the single state UiState, that is what it is called before your PR.

Done.

[Pururun]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

[albin-mullvad]

Reviewed 1 of 8 files at r1, 2 of 6 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

---

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

                modifier = Modifier.padding(start = Dimens.sideMargin, end = Dimens.sideMargin)
            )
            uiState.deviceName?.let {

Why was this changed? Seems unrelated to adding the dialog. It seems like this would result in not showing the spinner if the device name is null.

This conditional check is also different from below if (uiState.accountNumber != null) {

Code quote:

uiState.deviceName?.let {

[sabercodic]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

---

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

Previously, albin-mullvad wrote…

Why was this changed? Seems unrelated to adding the dialog. It seems like this would result in not showing the spinner if the device name is null.

This conditional check is also different from below if (uiState.accountNumber != null) {

Done.

[sabercodic]

Reviewable status: 4 of 29 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)

[albin-mullvad]

Reviewed 6 of 6 files at r8, 20 of 20 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

---

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

            if (uiState.accountNumber != null) {
                CopyableObfuscationView(content = uiState.accountNumber)
            }

Why this if-statement? The CopyableObfuscationView has a built in spinner which is supposed to be showen the the content is empty.

Code quote:

            if (uiState.accountNumber != null) {
                CopyableObfuscationView(content = uiState.accountNumber)
            }

---

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

    val deviceName: String,
    val accountNumber: String?,
    val accountExpiry: DateTime?,

Is there a reason for not unifying these properties? Since they are very similar I suggest that either all or none are nullable.

Code quote:

    val deviceName: String,
    val accountNumber: String?,
    val accountExpiry: DateTime?,

[sabercodic]

Reviewable status: 26 of 29 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

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

Previously, albin-mullvad wrote…

Why this if-statement? The CopyableObfuscationView has a built in spinner which is supposed to be showen the the content is empty.

Done.

---

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

Previously, albin-mullvad wrote…

Is there a reason for not unifying these properties? Since they are very similar I suggest that either all or none are nullable.

Done.

[albin-mullvad]

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

---

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

Previously, sabercodic wrote…

Done.

Since it's now nullable, callers should also be updated

[sabercodic]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

---

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

Previously, albin-mullvad wrote…

Since it's now nullable, callers should also be updated

Done.

[albin-mullvad]

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