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

Do not show a toast after dismissing the dns dialog #5986

Conversation

Pururun
Copy link
Contributor

@Pururun Pururun commented Mar 20, 2024


This change is Reviewable

@Pururun Pururun requested a review from Rawa March 20, 2024 10:08
@Pururun Pururun added the Android Issues related to Android label Mar 20, 2024
@Pururun Pururun self-assigned this Mar 20, 2024
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.

Logic does unfortunately not work coherently.

E.g

  • Add the 1.1.1.1 (see the toast)
  • Toggle off ( = will show toast)
  • Toggle on ( = will show toast)
  • Remove 1.1.1.1, will toggle off but not show toast

What if we instead use the NavResult of dns dialog, and we can use value to indicate that the value has changed. If it has changed we can show the snackbar.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@Pururun Pururun force-pushed the dns-settings-might-not-go-into-effect-immediately-snackbar-droid-743 branch from 4bea895 to e5061c4 Compare March 22, 2024 07:51
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.

Fixed

Reviewable status: 0 of 4 files reviewed, all discussions resolved

@Pururun Pururun requested a review from Rawa March 22, 2024 08:19
@Pururun Pururun force-pushed the dns-settings-might-not-go-into-effect-immediately-snackbar-droid-743 branch from e5061c4 to ef7561e Compare March 22, 2024 10:47
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 4 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt line 131 at r2 (raw file):

        if (enable && vmState.value.customDnsList.isEmpty()) {
            viewModelScope.launch { _uiSideEffect.send(VpnSettingsSideEffect.NavigateToDnsDialog) }
        } else if (showToast) {

Shouldn't this suffice? I don't think showToast parameter is needed.

        } else if (vmState.value.customDnsList.isNotEmpty()) {
            showApplySettingChangesWarningToast()
        }

@Pururun Pururun force-pushed the dns-settings-might-not-go-into-effect-immediately-snackbar-droid-743 branch from ef7561e to 6d10536 Compare March 22, 2024 14:11
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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt line 131 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Shouldn't this suffice? I don't think showToast parameter is needed.

        } else if (vmState.value.customDnsList.isNotEmpty()) {
            showApplySettingChangesWarningToast()
        }

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.

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

@Pururun Pururun merged commit 570d731 into main Mar 25, 2024
26 checks passed
@Pururun Pururun deleted the dns-settings-might-not-go-into-effect-immediately-snackbar-droid-743 branch March 25, 2024 08:50
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.

2 participants