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

Make the 'Clear all overrides' button only active when there are overrides #5912

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Mar 7, 2024

Currently, the button is always active. According to the specs, we should make sure it is not active if there are no overrides being enabled.


This change is Reviewable

@rablador rablador added bug iOS Issues related to iOS labels Mar 7, 2024
@rablador rablador requested review from buggmagnet and acb-mv March 7, 2024 08:55
Copy link

linear bot commented Mar 7, 2024

Copy link
Contributor Author

@rablador rablador 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: 0 of 3 files reviewed, 1 unresolved discussion


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 90 at r1 (raw file):

    }

    private func resetToDefaultStatus(delay: Duration = .zero) {

Added semi-related improvement here as well. Currently there can be weird timings for the status messages depending on user behavior (and patience).

@rablador rablador force-pushed the make-the-clear-all-overrides-button-only-active-when-there-ios-547 branch from ee5a803 to 5c2b495 Compare March 7, 2024 11:57
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 94 at r1 (raw file):

        let statusWorkItem = DispatchWorkItem { [weak self] in
            guard let self else { return }

Calling cancel on dispatchWorkItems doesn't actually cancel the execution if the item has already scheduled.
We should check here as well that the item itself is not cancelled

guard let self, self.statusWorkItem?.isCancelled == false else { return }

Copy link
Contributor Author

@rablador rablador 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideInteractor.swift line 94 at r1 (raw file):

Previously, buggmagnet wrote…

Calling cancel on dispatchWorkItems doesn't actually cancel the execution if the item has already scheduled.
We should check here as well that the item itself is not cancelled

guard let self, self.statusWorkItem?.isCancelled == false else { return }

Aha! Didn't know that. My intention was to cancel it altogether.

@rablador rablador force-pushed the make-the-clear-all-overrides-button-only-active-when-there-ios-547 branch from 5c2b495 to 7e0d268 Compare March 7, 2024 14:01
Copy link
Contributor

@buggmagnet buggmagnet 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 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

@buggmagnet buggmagnet force-pushed the make-the-clear-all-overrides-button-only-active-when-there-ios-547 branch from 7e0d268 to a067675 Compare March 11, 2024 07:58
@rablador rablador force-pushed the make-the-clear-all-overrides-button-only-active-when-there-ios-547 branch from a067675 to 8dbe9cb Compare March 11, 2024 08:01
@buggmagnet buggmagnet merged commit f89c0e8 into main Mar 11, 2024
5 of 6 checks passed
@buggmagnet buggmagnet deleted the make-the-clear-all-overrides-button-only-active-when-there-ios-547 branch March 11, 2024 08:05
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants