Migrate redeem voucher dialog into compose

[sabercodic]

This change is 

[linear]

DROID-59 Migrate voucher redeem dialog to compose

Docs: https://mullvad.atlassian.net/wiki/spaces/UXUI/pages/2433810545/Voucher+confirmation+update

Zepline: https://zpl.io/KGm6qmX

[Pururun]

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

---

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

fun RedeemVoucherDialog(
    uiState: VoucherDialogUiState,
    viewActions: SharedFlow<Unit>,

It's a bit more readable and somewhat more scalable to have named actions, instead of just one action with unit. If it's not too much work.

---

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

    val voucher = remember { mutableStateOf("") }

    LaunchedEffect(Unit) { viewActions.collect { onDismiss() } }

See above, you could add an if statement here if we had named actions to make it easier to read.

---

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

                            contentColor = MaterialTheme.colorScheme.onSurface,
                            disabledContentColor =
                                MaterialTheme.colorScheme.onSurface.copy(alpha = AlphaInactive),

This is a new thing, but I think we should be adding compositeOver so we are not displaying alpha components.
So it would be MaterialTheme.colorScheme.onSurface.copy(alpha = AlphaInactive).compositeOver(MaterialTheme.colorScheme.surface)

---

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

                                MaterialTheme.colorScheme.onSurface.copy(alpha = AlphaInactive),
                            disabledContainerColor =
                                MaterialTheme.colorScheme.surface.copy(alpha = AlphaDisabled)

See above

---

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

                    placeholderText = stringResource(id = R.string.voucher_hint),
                    placeHolderColor =
                        MaterialTheme.colorScheme.onPrimary.copy(alpha = AlphaDisabled),

See comment about alpha above

---

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

}

private fun formatOtherVouchers(text: AnnotatedString): TransformedText {

This could be an extension function and maybe placed in a separate file?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 90 at r1 (raw file):

            MaterialTheme.colorScheme.onPrimary
        } else {
            MaterialTheme.colorScheme.onPrimary.copy(AlphaInactive)

See comment about alpha above

[Rawa]

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

---

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

    onDismiss: () -> Unit
) {
    val voucher = remember { mutableStateOf("") }

var voucher by remember { mutableStateof("") }
I think using the by delegate would make it more readable since we are activity changing and updating the voucher variable.

---

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

                    },
                    onValueChanged = { input ->
                        voucher.value = input.uppercase().format().replace(" ", "").replace("-", "")

Why do we have to remove "-" and " "? Can we convert it automatically instead?

---

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

                if (uiState.message != null) {
                    Text(
                        text = uiState.message!!,

Why do we need !!, if we had a null check above? Maybe

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

This could be an extension function and maybe placed in a separate file?

Also would be it possible to do in a simpler way? It looks a bit too complex, I did this one for AccountNumber, maybe it can be used as a reference:
https://github.com/mullvad/mullvadvpn-app/blob/b42e5e0262d021d955a9926ac18ea7971e0cd94b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/AccountTokenVisualTransformation.kt

---

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

    var message: String? = null,
    var isError: Boolean = false,
    var showLoading: Boolean = false

None of these should be varin my opinion.

---

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

    data object Verifying : VoucherDialogViewModelState()

    data class Success(var message: String?) : VoucherDialogViewModelState()

Should not be var

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 65 at r1 (raw file):

) {
    val shape = RoundedCornerShape(4.dp)
    val textFieldHeight = 44.dp

We should avoid having a fixed textFieldHeight.

---

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

                        viewActions = vm.viewActions,
                        onRedeem = { vm.onRedeem(it) },
                        onDismiss = { onDismiss(voucherDialog!!) }

Is !! necessary? Looks like it is already marked as lateinit var?

---

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

    val viewActions = _viewActions.asSharedFlow()

    private var voucherRedeemer: VoucherRedeemer? = null

Does this really have to be nullable? Can't we inject it?

---

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

    fun onRedeem(voucherCode: String) {
        serviceConnectionManager.serviceNotifier.subscribe(this) { connection ->
            voucherRedeemer = connection?.voucherRedeemer

Not sure if it makes sense that the voucherRedeemer can be null? If it is, then it should be an error to the user as well I believe. This function ends up becoming fail silent if voucherRedeemer is null. Probably better to !! it then if it really is necessary.

[Rawa]

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

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 71 at r3 (raw file):

    private fun setError(error: VoucherSubmissionError) {
        viewModelScope.launch {
            val message =

I think we should not call getString in view model

---

wireguard/libwg/go.mod line 13 at r3 (raw file):

	golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa // indirect
	golang.org/x/net v0.0.0-20220809184613-07c6da5e1ced // indirect
	golang.zx2c4.com/wintun v0.0.0-20211104114900-415007cec224 // indirect

Should probably not be in this commit.

---

wireguard/libwg/go.sum line 8 at r3 (raw file):

golang.org/x/sys v0.0.0-20220808155132-1c4a2a72c664/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.zx2c4.com/wintun v0.0.0-20211104114900-415007cec224 h1:Ug9qvr1myri/zFN6xL17LSCBGFDnphBBhzmILHsM5TY=
golang.zx2c4.com/wintun v0.0.0-20211104114900-415007cec224/go.mod h1:deeaetjYA+DHMHg+sMSMI58GrEteJUUzzw7en6TJQcI=

Should probably not be in this commit.

[sabercodic]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

It's a bit more readable and somewhat more scalable to have named actions, instead of just one action with unit. If it's not too much work.

Removed, its not needed anymore

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

This is a new thing, but I think we should be adding compositeOver so we are not displaying alpha components.
So it would be MaterialTheme.colorScheme.onSurface.copy(alpha = AlphaInactive).compositeOver(MaterialTheme.colorScheme.surface)

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

See above

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Why do we have to remove "-" and " "? Can we convert it automatically instead?

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

See comment about alpha above

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Why do we need !!, if we had a null check above? Maybe

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Also would be it possible to do in a simpler way? It looks a bit too complex, I did this one for AccountNumber, maybe it can be used as a reference:
https://github.com/mullvad/mullvadvpn-app/blob/b42e5e0262d021d955a9926ac18ea7971e0cd94b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/AccountTokenVisualTransformation.kt

Done.

---

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

Previously, Rawa (David Göransson) wrote…

None of these should be varin my opinion.

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Should not be var

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 65 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

We should avoid having a fixed textFieldHeight.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 90 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

See comment about alpha above

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Is !! necessary? Looks like it is already marked as lateinit var?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 71 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think we should not call getString in view model

Done.

---

wireguard/libwg/go.mod line 13 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should probably not be in this commit.

It was a mistake, reverted

---

wireguard/libwg/go.sum line 8 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should probably not be in this commit.

It was a mistake, reverted

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 218 at r4 (raw file):

                                text =
                                    if (uiState is VoucherDialogUiState.Error) uiState.errorMessage
                                    else "",

Is this an expected state? Will not look weird for a user or how will it look when this happens?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 66 at r4 (raw file):

    textAlign: TextAlign = TextAlign.Start
) {
    val shape = RoundedCornerShape(4.dp)

Could be replaced by MaterialTheme.shapes.small if we want to avoid magic numbers.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 27 at r4 (raw file):

    private var voucherRedeemer: VoucherRedeemer? =
        serviceConnectionManager.connectionState.value.readyContainer()?.voucherRedeemer

Probably better to do something like this:
(From ConnectionViewModel)

Code snippet:

private val _shared: SharedFlow<ServiceConnectionContainer> =
        serviceConnectionManager.connectionState
            .flatMapLatest { state ->
                if (state is ServiceConnectionState.ConnectedReady) {
                    flowOf(state.container)
                } else {
                    emptyFlow()
                }
            }
            .shareIn(viewModelScope, SharingStarted.WhileSubscribed())

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 53 at r4 (raw file):

                VoucherDialogUiState.Success(
                    if (days > 60) {
                        resources.getQuantityString(

Probably better to move this out of the view model

[Rawa]

Reviewable status: 7 of 36 files reviewed, 16 unresolved discussions (waiting on @Pururun and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 224 at r5 (raw file):

                }
            }
        },

There is a lot of depth to this function, is it possible to maybe refactor some out?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 233 at r5 (raw file):

const val ACCOUNT_TOKEN_CHUNK_SIZE = 4

private fun vouchersVisualTransformation(text: AnnotatedString, maxSize: Int): TransformedText {

It would be nicer to directly implement VisualTransformation interface:

VisualTransformation { annotatedString -> 
    // Implementation goes here
} ```

and move then logic of maxSize to the onValueChange handler.

---

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

Previously, sabercodic wrote…

Done.

Seems like data class Success(var message: String) : VoucherDialogUiState() still uses var, maybe latest has not been pushed?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VoucherDialogUiState.kt line 3 at r5 (raw file):

package net.mullvad.mullvadvpn.compose.state

sealed class VoucherDialogUiState {

sealed interface

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 65 at r1 (raw file):

Previously, sabercodic wrote…

Done.

If height is a fixed value, it will clip text when user has it set large fonts. We should avoid using height modifier on anything that contains text

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 146 at r5 (raw file):

                    onFocusChange(focusState.isFocused)
                }
                .height(textFieldHeight)

Here is where we should avoid using height, otherwise it might not render nicely when users have large fonts

[Pururun]

Reviewed 1 of 23 files at r5, 6 of 9 files at r6, 23 of 23 files at r7, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @Rawa and @sabercodic)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 42 at r7 (raw file):

import net.mullvad.mullvadvpn.lib.theme.MullvadWhite10

internal const val EMPTY_STRING = ""

Any specific reason this is internal?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/AccountExpiryConstant.kt line 6 at r7 (raw file):

const val SECONDS_PER_DAY: Long = 86400
const val SECONDS_PER_MONTH: Long = 2592000
const val SECONDS_PER_YEAR: Long = 31104000

Is there a better way to show this since it's just look like magic numbers?

---

gui/locales/messages.pot line 1605 at r7 (raw file):

msgstr ""

msgid "%d months was added to your account."

I guess all these strings were added by the script by mistake? This PR should not add any copy right?

[sabercodic]

Reviewable status: 30 of 37 files reviewed, 19 unresolved discussions (waiting on @Pururun and @Rawa)

---

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

Previously, Rawa (David Göransson) wrote…

var voucher by remember { mutableStateof("") }
I think using the by delegate would make it more readable since we are activity changing and updating the voucher variable.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 218 at r4 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is this an expected state? Will not look weird for a user or how will it look when this happens?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 66 at r4 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Could be replaced by MaterialTheme.shapes.small if we want to avoid magic numbers.

Done.

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VoucherDialogUiState.kt line 18 at r9 (raw file):

    data object Verifying : VoucherDialogState

    data class Success(var message: String) : VoucherDialogState

This should be a val

[sabercodic]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 224 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

There is a lot of depth to this function, is it possible to maybe refactor some out?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 233 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

It would be nicer to directly implement VisualTransformation interface:

VisualTransformation { annotatedString -> 
    // Implementation goes here
} ```

and move then logic of maxSize to the onValueChange handler.

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Seems like data class Success(var message: String) : VoucherDialogUiState() still uses var, maybe latest has not been pushed?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VoucherDialogUiState.kt line 3 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

sealed interface

please check again, implementation changed, so is this still required?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/VoucherDialogUiState.kt line 18 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should be a val

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 65 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

If height is a fixed value, it will clip text when user has it set large fonts. We should avoid using height modifier on anything that contains text

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/GroupedTextField.kt line 146 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Here is where we should avoid using height, otherwise it might not render nicely when users have large fonts

removed

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/AccountExpiryConstant.kt line 6 at r7 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is there a better way to show this since it's just look like magic numbers?

replaced by system constants

---

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

Previously, Rawa (David Göransson) wrote…

Does this really have to be nullable? Can't we inject it?

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Not sure if it makes sense that the voucherRedeemer can be null? If it is, then it should be an error to the user as well I believe. This function ends up becoming fail silent if voucherRedeemer is null. Probably better to !! it then if it really is necessary.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 27 at r4 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Probably better to do something like this:
(From ConnectionViewModel)

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VoucherDialogViewModel.kt line 53 at r4 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Probably better to move this out of the view model

Done.

---

gui/locales/messages.pot line 1605 at r7 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I guess all these strings were added by the script by mistake? This PR should not add any copy right?

these messages belong to redeem voucher success screen

[Pururun]

Reviewed 6 of 6 files at r10, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Rawa)

---

gui/locales/messages.pot line 1605 at r7 (raw file):

Previously, sabercodic wrote…

these messages belong to redeem voucher success screen

Ah nice. 👍

[Rawa]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/RedeemVoucherDialog.kt line 99 at r10 (raw file):

    onDismiss: () -> Unit
) {
    //    val voucher = remember { mutableStateOf("") }

Can we remove?

[albin-mullvad]

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

a discussion (no related file):

Previously, Pururun (Jonatan Rhodin) wrote…

@albin-mullvad In regards to the underline issue it seems to be related to the keyboard type that you set, so for example keyboard type number does not show it, but most other keyboard types do. The only keyboard type that does not show the underline is password and that one has a few other quirks. For example it will obfuscated any text in your clipboard.

Does that mean we can't use the password keyboard type? In what way does it obfuscate things? The documentation here doesn't say much: https://developer.android.com/reference/kotlin/androidx/compose/ui/text/input/KeyboardType#Password()

---

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

The top padding of the success dialog icon doesn't look correct according to other dialog icons. Perhaps also too much bottom padding. Please make sure all paddings are correct.

should I consider other dialogs? or Zepline?
https://zpl.io/29mWd8e

---

a discussion (no related file):

Previously, albin-mullvad wrote…

Does that mean we can't use the password keyboard type? In what way does it obfuscate things? The documentation here doesn't say much: https://developer.android.com/reference/kotlin/androidx/compose/ui/text/input/KeyboardType#Password()

Done.

---

a discussion (no related file):

Previously, albin-mullvad wrote…

There seem to be a few DPAD issues with this dialog:

• After opening the dialog, it's not possible to click down to enter the text field. Clicking down twice will focus the Cancel button.
• After opening the dialog, it's necessary to click up twice to reach the Cancel button.

The first one is probably specific to this PR, however the second one might be common with other dialogs and can perhaps we handled in a separate PR?

Done.

---

a discussion (no related file):

Previously, albin-mullvad wrote…

The input behavior is a bit different from desktop.

The current behavior in this PR is that if I enter "ABCD" and then try to enter the hyphen (-) according to the voucher code, I will simply not be allowed, which might be very confusing. If I enter another character it will however be automatically added before the new character. Is this the same as before the migration? To me it would make sense to not block the user from entering a hyphen on the correct place 🤔

As a comparison, on desktop the hyphen will be automatically added immediately after entering the "D" from the above example.

This visual transformation is a replica of login input, Should I change it?

---

[albin-mullvad]

Reviewable status: 55 of 57 files reviewed, 5 unresolved discussions (waiting on @Pururun and @sabercodic)

a discussion (no related file):

Previously, sabercodic wrote…

should I consider other dialogs? or Zepline?
https://zpl.io/29mWd8e

Can you check with Matilda whether that Zeplin design is correct since the icon padding is different from the other dialogs?

---

a discussion (no related file):
Please answer this question:

Is this the same as before the migration?

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

                            focusManager.moveFocus(FocusDirection.Up)
                            true
                        }

Will removing these not affect the DPAD navigation in other text fields in the settings view? I mean there was a reason we added these.

Code quote:

                        KeyEvent.KEYCODE_DPAD_DOWN -> {
                            focusManager.moveFocus(FocusDirection.Down)
                            true
                        }
                        KeyEvent.KEYCODE_DPAD_UP -> {
                            focusManager.moveFocus(FocusDirection.Up)
                            true
                        }

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

Please answer this question:

Is this the same as before the migration?

No, There was bugs in previous implementation in voucher input

WhatsApp Video 2023-10-10 at 10.39.11 AM.mp4

---

a discussion (no related file):

Previously, albin-mullvad wrote…

Can you check with Matilda whether that Zeplin design is correct since the icon padding is different from the other dialogs?

sure

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

Previously, albin-mullvad wrote…

Will removing these not affect the DPAD navigation in other text fields in the settings view? I mean there was a reason we added these.

We added this one when MTU input was in setting page(not inside the dialog) to not correct DPAD, it is not needed anymore

[Pururun]

Reviewable status: 55 of 57 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad)

a discussion (no related file):

Previously, sabercodic wrote…

Done.

@albin-mullvad To clarify.

1. No I don't think this means that we can not use password keyboard type.
2. What I'm referring to is past from clipboard action that can appear on some keyboards, see screenshot below.
   

---

[sabercodic]

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

a discussion (no related file):

Previously, sabercodic wrote…

sure

Matilda confirmed padding difference and asked for following Zeplin designs

---

a discussion (no related file):

Previously, Pururun (Jonatan Rhodin) wrote…

@albin-mullvad To clarify.

1. No I don't think this means that we can not use password keyboard type.
2. What I'm referring to is past from clipboard action that can appear on some keyboards, see screenshot below.
   

@albin-mullvad @Pururun which input type should we use here?

---

[albin-mullvad]

Reviewable status: 54 of 58 files reviewed, 3 unresolved discussions (waiting on @Pururun and @sabercodic)

a discussion (no related file):

Previously, sabercodic wrote…

Matilda confirmed padding difference and asked for following Zeplin designs

Alright, sounds good!

---

a discussion (no related file):

Previously, sabercodic wrote…

@albin-mullvad @Pururun which input type should we use here?

I believe we should use the password keyboard type unless it's causing any issues.

@Pururun do you agree?

---

a discussion (no related file):

Previously, sabercodic wrote…

No, There was bugs in previous implementation in voucher input

WhatsApp Video 2023-10-10 at 10.39.11 AM.mp4

Yes, there seem to be some various issue with the old voucher dialog, however in terms of separators/hyphens it would:

• Automatically add hyphens if not typed.
• Allow the user to add hyphens manually.

Can we support the same in this migration?

---

a discussion (no related file):

Previously, sabercodic wrote…

Done.

It does still not seem to fully work as expected. I'll do some more testing.

---

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

Yes, there seem to be some various issue with the old voucher dialog, however in terms of separators/hyphens it would:

• Automatically add hyphens if not typed.
• Allow the user to add hyphens manually.

Can we support the same in this migration?

It will be a little hard because we use VisualTransformation to show "-"

---

a discussion (no related file):

Previously, albin-mullvad wrote…

It does still not seem to fully work as expected. I'll do some more testing.

Sounds good, I already tested all usages

---

[Pururun]

Reviewable status: 54 of 58 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

a discussion (no related file):

Previously, albin-mullvad wrote…

I believe we should use the password keyboard type unless it's causing any issues.

@Pururun do you agree?

Yes agree with using password keyboard type.

---

[albin-mullvad]

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

a discussion (no related file):

Previously, sabercodic wrote…

It will be a little hard because we use VisualTransformation to show "-"

Perhaps it's easiest to change the logic to work similar to desktop? Basically to make the VisualTransformation show the hyphen before the user enters the 5th/9th/13th etc character. It should ofc skip showing the last one after the 16th character, which I assume we would have to hardcode.

---

a discussion (no related file):

Previously, sabercodic wrote…

Sounds good, I already tested all usages

I still seem to need double up/down DPAD clicks to reach stuff, but I'll try it somewhere else than my emulator.

---

a discussion (no related file):
When testing this on my emulator, clicking enter on my physical keyboard after filling in a voucher seem to just dismiss the dialog. I believe it's the case both for valid and invalid vouchers.

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

Previously, sabercodic wrote…

We added this one when MTU input was in setting page(not inside the dialog) to not correct DPAD, it is not needed anymore

I'm not sure that was the reason since I don't believe we merged the migration PR in a state where it was part directly of the settings view.

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

Perhaps it's easiest to change the logic to work similar to desktop? Basically to make the VisualTransformation show the hyphen before the user enters the 5th/9th/13th etc character. It should ofc skip showing the last one after the 16th character, which I assume we would have to hardcode.

Done.

---

a discussion (no related file):

Previously, albin-mullvad wrote…

I still seem to need double up/down DPAD clicks to reach stuff, but I'll try it somewhere else than my emulator.

This is an existing DPAD problem for Compose dialogs inside DialogFragment (such as MTU dialog)

---

a discussion (no related file):

Previously, albin-mullvad wrote…

When testing this on my emulator, clicking enter on my physical keyboard after filling in a voucher seem to just dismiss the dialog. I believe it's the case both for valid and invalid vouchers.

Fixed

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

Previously, albin-mullvad wrote…

I'm not sure that was the reason since I don't believe we merged the migration PR in a state where it was part directly of the settings view.

Reverted as discussed.

[albin-mullvad]

Reviewed 1 of 3 files at r30.
Reviewable status: 54 of 58 files reviewed, 1 unresolved discussion (waiting on @Pururun and @sabercodic)

a discussion (no related file):

Previously, sabercodic wrote…

This is an existing DPAD problem for Compose dialogs inside DialogFragment (such as MTU dialog)

I'm not sure it's specifically for DialogFragment since we only use that for the voucher, I believe it might be some issue with AlertDialog or how we use them, but let's work on a overall improvement for that in a separate PR. Can you create a Linear issue?

---

a discussion (no related file):

Previously, sabercodic wrote…

Done.

Great!

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

Previously, sabercodic wrote…

Reverted as discussed.

Why was the escape and up/down order changed?

[sabercodic]

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

a discussion (no related file):

Previously, albin-mullvad wrote…

I'm not sure it's specifically for DialogFragment since we only use that for the voucher, I believe it might be some issue with AlertDialog or how we use them, but let's work on a overall improvement for that in a separate PR. Can you create a Linear issue?

Done.
https://linear.app/mullvad/issue/DROID-401/investigate-and-fix-the-dpad-navigation-bug-on-alert-dialog

---

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/CustomTextField.kt line 161 at r26 (raw file):

Previously, albin-mullvad wrote…

Why was the escape and up/down order changed?

Done.

[albin-mullvad]

Reviewed 1 of 1 files at r31.
Reviewable status: 54 of 58 files reviewed, all discussions resolved (waiting on @Pururun)

a discussion (no related file):

Previously, sabercodic wrote…

Done.
https://linear.app/mullvad/issue/DROID-401/investigate-and-fix-the-dpad-navigation-bug-on-alert-dialog

Great, thanks!

---

[Pururun]

Reviewed 1 of 3 files at r28, 1 of 1 files at r29, 2 of 3 files at r30, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved