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

Add multi-hop toggle to settings view #6335

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Jun 10, 2024

This PR allows users to change the multi-hop state in the settings view.

Before After

This change is Reviewable

Copy link

linear bot commented Jun 10, 2024

@mojganii mojganii self-assigned this Jun 10, 2024
@mojganii mojganii added iOS Issues related to iOS feature request For issues asking for new features labels Jun 10, 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.

The cell stays selected if I tap on it, we should disable selection for this specific cell.
MultiSelect.mov

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


ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 56 at r1 (raw file):
nit:

if the multihop settings have changed


ios/MullvadVPN/View controllers/VPNSettings/CustomDNSDataSource.swift line 123 at r1 (raw file):

    private weak var tableView: UITableView?

    weak var delegate: DnsSettingsDataSourceDelegate?

nit
It should be DNSSettingsDataSourceDelegate?


ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSourceDelegate.swift line 23 at r1 (raw file):

    func showIPOverrides()
    func didSelectWireGuardPort(_ port: UInt16?)
    func showMultihopConfirmation(_ onSave: @escaping () -> Void, _ onDiscard: @escaping () -> Void)

We should leave the name arguments here, otherwise, it's really hard to tell which closure will do what on the call site.

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.

Reviewable status: 9 of 24 files reviewed, 4 unresolved discussions (waiting on @mojganii)


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 245 at r1 (raw file):

            for await newState in stateStream {
                // Pass relay constraints retrieved during the last read from setting into transport provider.
                if let relayConstraints = newState.relayConstraints {

Why was this line removed ?

Copy link
Collaborator Author

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


ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 56 at r1 (raw file):

Previously, buggmagnet wrote…

nit:

if the multihop settings have changed

done


ios/MullvadVPN/View controllers/VPNSettings/CustomDNSDataSource.swift line 123 at r1 (raw file):

Previously, buggmagnet wrote…

nit
It should be DNSSettingsDataSourceDelegate?

Right, it's acronym.


ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSourceDelegate.swift line 23 at r1 (raw file):

Previously, buggmagnet wrote…

We should leave the name arguments here, otherwise, it's really hard to tell which closure will do what on the call site.

indeed!


ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 245 at r1 (raw file):

Previously, buggmagnet wrote…

Why was this line removed ?

we are tracking the value or relay constraints above. we don't need fetch that from connection state.

@mojganii mojganii force-pushed the add-multihop-toggle-to-settings-view-ios-687 branch from 47c8e1d to bf291e1 Compare June 13, 2024 09:21
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 3 of 8 files at r2.
Reviewable status: 9 of 24 files reviewed, 2 unresolved discussions

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:

Reviewable status: 9 of 24 files reviewed, 1 unresolved discussion

@buggmagnet buggmagnet force-pushed the add-multihop-toggle-to-settings-view-ios-687 branch from bf291e1 to 1d03677 Compare June 13, 2024 12:06
@buggmagnet buggmagnet merged commit 57d678c into main Jun 13, 2024
6 of 7 checks passed
@buggmagnet buggmagnet deleted the add-multihop-toggle-to-settings-view-ios-687 branch June 13, 2024 12:08
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
feature request For issues asking for new features iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants