Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wireguard mtu selection showing again after closing it #5102

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Sep 5, 2023

Also made some improvements to the mtu dialog.


This change is Reviewable

@linear
Copy link

linear bot commented Sep 5, 2023

DROID-303 WireGuard MTU selection pops up twice

Steps to reproduce

  • Press the "Default" text to make the WireGuard MTU alert show up
  • Tap on "Enter MTU"
  • Write any number
  • Press "Reset to default" or "Cancel"
  • Another pop up shows with the same content, but the keyboard is gone
  • You have to press again "Reset to Default" or "Cancel" to make the popup disappear

Note: The bug does not trigger if the text field is empty

@Pururun Pururun added the Android Issues related to Android label Sep 5, 2023
Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the wireguard-mtu-selection-pops-up-twice-droid-303 branch from 2d8b59c to a9b8125 Compare September 6, 2023 07:13
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

@Composable
fun MtuDialog(
    mtuValueInitial: String,

Can we change this to be Int or Int? easily?


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

    val smallPadding = 5.dp

    val mtuValue = remember { mutableStateOf(mtuValueInitial) }

Maybe to avoid writing mtuValue.value
var mtuValue by remember { mutableStateOf(mtuValueInitial) }


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

                        ),
                    text = stringResource(R.string.reset_to_default_button),
                    onClick = { onRestoreDefaultValue() }

Would it be possible to send the function reference directly?
onClick = onRestoreDefaultValue


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

                        ),
                    text = stringResource(R.string.cancel),
                    onClick = { onDismiss() }

Same as a above previous

@Pururun Pururun force-pushed the wireguard-mtu-selection-pops-up-twice-droid-303 branch 2 times, most recently from 5b7520e to c823ee0 Compare September 6, 2023 12:43
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


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

Previously, Rawa (David Göransson) wrote…

Can we change this to be Int or Int? easily?

Done


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

Previously, Rawa (David Göransson) wrote…

Maybe to avoid writing mtuValue.value
var mtuValue by remember { mutableStateOf(mtuValueInitial) }

Done


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

Previously, Rawa (David Göransson) wrote…

Would it be possible to send the function reference directly?
onClick = onRestoreDefaultValue

Done


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

Previously, Rawa (David Göransson) wrote…

Same as a above previous

Done

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @sabercodic)

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the wireguard-mtu-selection-pops-up-twice-droid-303 branch from c823ee0 to 5fd84a8 Compare September 7, 2023 11:57
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/MtuDialog.kt line 120 at r2 (raw file):

                    isEnabled = isValidMtu,
                    text = stringResource(R.string.submit_button),
                    onClick = { onSave(mtu.value.toIntOrNull() ?: 0) }

Shouldn't this do the same as onSubmit above? In that case, it would be nice to unify it.

Also, I'm not sure it makes sense to send 0 if we fail to parse.

Code quote:

onClick = { onSave(mtu.value.toIntOrNull() ?: 0) }

@Pururun Pururun force-pushed the wireguard-mtu-selection-pops-up-twice-droid-303 branch from 5fd84a8 to 0c75ee6 Compare September 7, 2023 14:38
Copy link
Contributor Author

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/MtuDialog.kt line 120 at r2 (raw file):

Previously, albin-mullvad wrote…

Shouldn't this do the same as onSubmit above? In that case, it would be nice to unify it.

Also, I'm not sure it makes sense to send 0 if we fail to parse.

Yeah stopped it from saving in both places.

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Pururun Pururun force-pushed the wireguard-mtu-selection-pops-up-twice-droid-303 branch from 0c75ee6 to 4bdf3a5 Compare September 8, 2023 08:04
@Pururun Pururun merged commit dcd42b2 into main Sep 8, 2023
@Pururun Pururun deleted the wireguard-mtu-selection-pops-up-twice-droid-303 branch September 8, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants