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

Refactor TunnelManager account setting methods replacing callbacks with async #5748

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented Feb 1, 2024

This PR replaces public methods in TunnelManager, and their downstream consumers, which have used callbacks with versions using async instead, in the interest of modernising the codebase to use modern Swift concurrency.

The affected code deals with logging in, creating new accounts, deleting accounts, logging out and being bumped out due to device deletion.


This change is Reviewable

@acb-mv acb-mv added the iOS Issues related to iOS label Feb 1, 2024
@acb-mv acb-mv self-assigned this Feb 1, 2024
Copy link

linear bot commented Feb 1, 2024

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 10 of 10 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)


ios/MullvadVPN/Coordinators/AccountCoordinator.swift line 144 at r1 (raw file):

        let alertPresenter = AlertPresenter(context: self)
        

Please make sure to run swiftformat . in the ios folder before pushing changes


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 374 at r1 (raw file):

        try await withCheckedThrowingContinuation { continuation in
            setAccount(action: action) { result in
                continuation.resume(with: result)

It always irked me that we have a Result type with an Optional type defined as its success.
I wonder if we could make the success type non optional here

Or at least we could make the callsite nicer by unwrapping the optional here

What do you think ?

@buggmagnet buggmagnet force-pushed the asynchronize-tunnelmanagersetaccount-ios-474 branch from 4ee4251 to 83347b3 Compare February 6, 2024 08:09
@buggmagnet buggmagnet merged commit f42dcb3 into main Feb 6, 2024
4 of 5 checks passed
@buggmagnet buggmagnet deleted the asynchronize-tunnelmanagersetaccount-ios-474 branch February 6, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants