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 Multihop feature chip appear if settings or current state say so #7815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented Mar 13, 2025

The Multihop feature chip has been modified to appear if the settings enable multihop, or if the current tunnel state is multihop (i.e., if this has been enabled due to DAITA)


This change is Reviewable

@acb-mv acb-mv added the iOS Issues related to iOS label Mar 13, 2025
@acb-mv acb-mv requested review from rablador and SteffenErn March 13, 2025 13:45
@acb-mv acb-mv self-assigned this Mar 13, 2025
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 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@mojganii mojganii 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


ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ChipView/ChipFeature.swift line 57 at r1 (raw file):

    let state: TunnelState
    var isEnabled: Bool {
        settings.tunnelMultihopState.isEnabled || state.isMultihop

Since either of these conditions (settings.tunnelMultihopState.isEnabled || settings.daita.isAutomaticRouting) implies multihop, there is no need for a computed variable for isMultihop or for State as a dependency injection here.

Code snippet:

   var isEnabled: Bool {
        settings.tunnelMultihopState.isEnabled || settings.daita.isAutomaticRouting
    }

@acb-mv
Copy link
Contributor Author

acb-mv commented Mar 13, 2025

ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ChipView/ChipFeature.swift line 57 at r1 (raw file):

Previously, mojganii wrote…

Since either of these conditions (settings.tunnelMultihopState.isEnabled || settings.daita.isAutomaticRouting) implies multihop, there is no need for a computed variable for isMultihop or for State as a dependency injection here.

Though if you have DAITA automatic routing enabled but select a relay that has DAITA, will that still enable multihop? I thought that in that case, your traffic will go through just that single relay, in which case a "Multihop" feature chip would be misleading.

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.

3 participants