IOS-880 SwiftUI settings page for UDP/TCP obfuscation port #7143

acb-mv · 2024-11-07T14:14:07Z

This implements a SwiftUI-based settings page for selecting the port for UDP->TCP WireGuard obfuscation. The page's ViewModel couples its state directly into the TunnelManager's Settings, bypassing the UIKit-based structure of ViewModel/DataSource objects. Additionally, there is an abstraction for a styled choice list and a a helper class for stateful previews.

This change is

linear · 2024-11-07T14:14:11Z

IOS-880 Create new subsetting page for udp2tcp

buggmagnet

We should find a way to remove this gap

Reviewed all commit messages.
Reviewable status: 0 of 12 files reviewed, 11 unresolved discussions (waiting on @acb-mv)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

        case wireGuardObfuscation
        case wireGuardObfuscationOption
        case udpTcpObfuscationSettings

For the sake of consistency, let's keep the "udpOverTcp" name for definitions as well

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 323 at r2 (raw file):

        case .udpTcpObfuscationSettings:
            tableView.deselectRow(at: indexPath, animated: false)
            delegate?.showUDPTCPObfuscationSettings()

This should be instead called in the WireGuard obfuscation cell's ellipsis action handler

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsViewModel.swift line 34 at r2 (raw file):

            tunnelManager: tunnelManager,
            keyPath: \.udpOverTcpPort,
            .automatic

This shouldn't have default values, but instead they should be consumed from the TunnelSettings

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

    var value: Binding<T>

    func row(_ v: T) -> some View {

Let's avoid single letter names unless it's an index in a for loop.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

        return HStack {
            Image("IconTick").opacity(isSelected ? 1.0 : 0.0)
            Text(verbatim: "\(v)")

Should this be displayed without localization ?

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 39 at r2 (raw file):

        VStack(spacing: 0) {
            HStack {
                Text(title)

The title should be bold according to Figma designs

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 42 at r2 (raw file):

                Spacer()
            }
            .padding(16)

nit
Let's make sure we reuse constants we define in UIMetrics instead of using magic numbers as much as we can.
We had a discussion in the past about how often did we want to have constants in lieu, and we came to the conclusion that if it's used once, it can be left as is, otherwise we should reuse paddings and offsets.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 75 at r2 (raw file):

                "UDP_TCP_OBFUSCATION_CELL_LABEL",
                tableName: "VPNSettings",
                value: "UDP/TCP Obfuscation",

The title should be "UDP-over-TCP", why was it changed ?

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewController.swift line 133 at r2 (raw file):

        let view = UDPTCPObfuscationSettingsView(viewModel: viewModel)
        let vc = UIHostingController(rootView: view)
        vc.title = "UDP TCP Obfuscation"

Let's make sure to reuse the "UDP-over-TCP" localized title here

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 17 at r2 (raw file):

    var body: some View {
        SingleChoiceList(
            title: "Port",

Shouldn't that be a localised string ?

ios/MullvadVPN/View controllers/Settings/Obfuscation/TunnelObfuscationSettingsWatchingObservableObject.swift line 42 at r2 (raw file):

    private func updateValueFromSettings(_ settings: WireGuardObfuscationSettings) {
        let newValue = settings

It looks like this function doesn't do anything, is it leftover code, or did we plan to add something later here ?

rablador

You need to add the new .udpTcpObfuscationSettings to either heightForHeaderInSection or heightForFooterInSection (or both) in VPNSettingsDataSource in order to fix the gap.

Reviewable status: 0 of 12 files reviewed, 11 unresolved discussions (waiting on @acb-mv)

rablador

The list content should be aligned left, so that title, section header and checkmarks all match. In the designs the title is too much on the left side though (we should keep Apple's default margins there, as we do in all other views).

Reviewed 1 of 12 files at r1, all commit messages.
Reviewable status: 1 of 12 files reviewed, 11 unresolved discussions (waiting on @acb-mv)

rablador

I just realised that we should not fix the gap, but rather there shouldn't be a new section for this feature. If you unfold Wireguard Obfuscation there are already rows with ellipsis buttons prepared for navigating to port selection.

Reviewable status: 1 of 12 files reviewed, 15 unresolved discussions (waiting on @acb-mv)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewController.swift line 115 at r2 (raw file):

    }

    func showDetails(for: VPNSettingsDetailsButtonItem) {

We should remove this if it's not going to be used.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 331 at r2 (raw file):

            selectObfuscationState(.udpOverTcp)
            delegate?.didUpdateTunnelSettings(TunnelSettingsUpdate.obfuscation(obfuscationSettings))
        // TODO: When ready, add implementation for selected obfuscation (navigate to new view etc).

Not sure When selecting

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

    var value: Binding<T>

    func row(_ v: T) -> some View {

I think this function should instead be a view that we can reuse.

ios/MullvadVPN.xcodeproj/project.pbxproj line 2597 at r2 (raw file):

			isa = PBXGroup;
			children = (
				4422C0702CCFF6790001A385 /* UDPTCPObfuscationSettingsView.swift */,

Files should be ordered by name.

rablador

Reviewed 10 of 12 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 323 at r2 (raw file):

Previously, buggmagnet wrote…

This should be instead called in the WireGuard obfuscation cell's ellipsis action handler

Yes, see comment here: https://reviewable.io/reviews/mullvad/mullvadvpn-app/7143#-OBQiIww24ow8do-nojJ

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 331 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Not sure When selecting

Sorry, nevermind this, I accidentally left an unfinished comment here!

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewController.swift line 115 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

We should remove this if it's not going to be used.

My bad, we already have code that calls this function when the ellipsis buttons are clicked in the Wireguard Obfuscation cells. The code in showUDPTCPObfuscationSettings() below should be moved up here.

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

    func showDNSSettings()
    func showIPOverrides()
    func showUDPTCPObfuscationSettings()

Not necessary, func showDetails(for: VPNSettingsDetailsButtonItem) already handles this.

ios/MullvadVPN/View controllers/Settings/Obfuscation/TunnelObfuscationSettingsWatchingObservableObject.swift line 26 at r2 (raw file):

            objectWillChange.send()
            var obfuscationSettings = tunnelManager.settings.wireGuardObfuscation
            obfuscationSettings[keyPath: keyPath] = newValue

When port is changed the tunnel should reconnect. Will this trigger a reconnection?

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 372 at r2 (raw file):

            configureObfuscationHeader(view)
            return view
        case .wireGuardObfuscationPort:

This is the header of the foldable obfuscation section and should not be removed.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

        case wireGuardObfuscation
        case wireGuardObfuscationOption
        case udpTcpObfuscationSettings

No new identifier should be added here. case wireGuardObfuscationOption already handles this.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 71 at r2 (raw file):

        case wireGuardPorts
        case wireGuardObfuscation
        case udpTcpObfuscationSettings

No new section should be added here. case wireGuardObfuscation already handles this. However, the gaps need to be adjusted below that section.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 81 at r2 (raw file):

        case wireGuardPort(_ port: UInt16?)
        case wireGuardCustomPort
        case udpTcpObfuscationSettings

No new item should be added here. case wireGuardObfuscation already handles this.

acb-mv

Reviewable status: 2 of 12 files reviewed, 18 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/Settings/Obfuscation/TunnelObfuscationSettingsWatchingObservableObject.swift line 42 at r2 (raw file):

Previously, buggmagnet wrote…

It looks like this function doesn't do anything, is it leftover code, or did we plan to add something later here ?

Good catch. The logic had been implemented in the non-generalised version of the view model, though somehow didn't end up being copied over.

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 17 at r2 (raw file):

Previously, buggmagnet wrote…

Shouldn't that be a localised string ?

Done.

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsViewModel.swift line 34 at r2 (raw file):

Previously, buggmagnet wrote…

This shouldn't have default values, but instead they should be consumed from the TunnelSettings

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think this function should instead be a view that we can reuse.

I will generalise this when I've written the Shadowsocks options (which require a more bespoke kind of list, having custom port options)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, buggmagnet wrote…

Let's avoid single letter names unless it's an index in a for loop.

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, buggmagnet wrote…

Should this be displayed without localization ?

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 39 at r2 (raw file):

Previously, buggmagnet wrote…

The title should be bold according to Figma designs

Done.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 75 at r2 (raw file):

Previously, buggmagnet wrote…

The title should be "UDP-over-TCP", why was it changed ?

Done.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

Previously, buggmagnet wrote…

For the sake of consistency, let's keep the "udpOverTcp" name for definitions as well

Done.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

No new identifier should be added here. case wireGuardObfuscationOption already handles this.

The WireGuard Obfuscation menu is still used for selecting the obfuscation method, with the new view being just settings for the UDP->TCP method, so we can't recycle the old item just yet.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 71 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

No new section should be added here. case wireGuardObfuscation already handles this. However, the gaps need to be adjusted below that section.

The wireGuardObfuscation section is still used for selecting the obfuscation method.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 81 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

No new item should be added here. case wireGuardObfuscation already handles this.

The WireGuard Obfuscation menu is still used for selecting the obfuscation method, with the new view being just settings for the UDP->TCP method, so we can't recycle the old item just yet.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 323 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Yes, see comment here: https://reviewable.io/reviews/mullvad/mullvadvpn-app/7143#-OBQiIww24ow8do-nojJ

Done

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsViewController.swift line 133 at r2 (raw file):

Previously, buggmagnet wrote…

Let's make sure to reuse the "UDP-over-TCP" localized title here

Done.

ios/MullvadVPN.xcodeproj/project.pbxproj line 2597 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Files should be ordered by name.

Done.

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

Previously, rablador (Jon Petersson) wrote…

Not necessary, func showDetails(for: VPNSettingsDetailsButtonItem) already handles this.

Done.

acb-mv · 2024-11-13T11:01:51Z

ios/MullvadVPN/View controllers/Settings/Obfuscation/TunnelObfuscationSettingsWatchingObservableObject.swift line 26 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

When port is changed the tunnel should reconnect. Will this trigger a reconnection?

It does now...

acb-mv

Reviewable status: 2 of 12 files reviewed, 18 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 372 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

This is the header of the foldable obfuscation section and should not be removed.

Done.

rablador

Reviewable status: 2 of 12 files reviewed, 18 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

Previously, acb-mv wrote…

The WireGuard Obfuscation menu is still used for selecting the obfuscation method, with the new view being just settings for the UDP->TCP method, so we can't recycle the old item just yet.

The new items in the old menu have new ellipsis buttons that navigate to port settings.

rablador

Reviewable status: 2 of 12 files reviewed, 18 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

The new items in the old menu have new ellipsis buttons that navigate to port settings.

The "UDP-over-TCP" port above will be removed once this feature is done (it's only there for legacy purposes of being able to select a port until your work with the new views is done).

rablador

Reviewed 8 of 10 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, acb-mv wrote…

Done.

Nit: We should use the tablename params and such like in other places with localizations.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 39 at r2 (raw file):

Previously, acb-mv wrote…

Done.

It's should be semi-bold 🙂

acb-mv

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: We should use the tablename params and such like in other places with localizations.

Which tablename should be used?

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 39 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

It's should be semi-bold 🙂

Done.

rablador

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, acb-mv wrote…

Which tablename should be used?

Something like this?

Code snippet:

NSLocalizedString(
    "SINGLE_CHOICE_LIST_TITLE",
    tableName: "SingleChoiceList",
    value: "Text",
    comment: ""
)

acb-mv · 2024-11-14T13:35:47Z

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Something like this?

Though SingleChoiceList is not a screen in the app but a component which is composed into screens. It having a table of its own in localisation data doesn't make a lot of sense to me. In this case, shouldn't we move localisation upstream to the view that uses it?

rablador

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, acb-mv wrote…

Though SingleChoiceList is not a screen in the app but a component which is composed into screens. It having a table of its own in localisation data doesn't make a lot of sense to me. In this case, shouldn't we move localisation upstream to the view that uses it?

Yeah, perhaps that's a better idea.

acb-mv · 2024-11-14T14:26:12Z

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Yeah, perhaps that's a better idea.

Done.

rablador

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @acb-mv and @buggmagnet)

rablador

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @acb-mv and @buggmagnet)

acb-mv

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDataSource.swift line 25 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

The "UDP-over-TCP" port above will be removed once this feature is done (it's only there for legacy purposes of being able to select a port until your work with the new views is done).

Done.

rablador

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, acb-mv wrote…

Done.

It's still abbreviated.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 32 at r7 (raw file):

        let isSelected = value.wrappedValue == v
        return HStack {
            Image("IconTick").opacity(isSelected ? 1.0 : 0.0)

We should use the .iconTick image resource here instead.
Also, there should be some spacing between tick and text, defined in UIMetrics.SettingsCell.selectableSettingsCellLeftViewSpacing.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 36 at r7 (raw file):

            Spacer()
        }
        .padding(16)

Default padding is specified in UIMetrics.SettingsCell.layoutMargins.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 37 at r7 (raw file):

        }
        .padding(16)
        .background(isSelected ? Color(UIColor.Cell.Background.selected) : Color(UIColor.Cell.Background.normal))

Unselected color should be UIColor.Cell.Background.indentationLevelOne.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 45 at r7 (raw file):

    var body: some View {
        VStack(spacing: 0) {

There should be a separator between each row. It's defined in UIMetrics.TableView.separatorHeight.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 46 at r7 (raw file):

    var body: some View {
        VStack(spacing: 0) {
            HStack {

This header should have bg color UIColor.Cell.Background.normal.

rablador

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 22 at r7 (raw file):

            comment: ""
        )
        SingleChoiceList(

Nit: Should we wrap this whole view into a scrollview to make it general for future use cases too?

buggmagnet

It looks like the selection in the subview isn't being honored in the UI.
Is there a way for the VPNSettingsViewController to be informed that it needs to reload its data source when things change under the hood ? Or should we just make it reload its data source on re-appears ?
RPReplay_Final1731673220.mov

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @acb-mv)

rablador

There are two things going on with the cells not updating text:

The tunnel observer was never added to tunnel manager, so no updates were coming through. It's been like that forever... 🙈
The data source should not be rebuilt on every settings update, since it would mean losing eg. expanded state. Instead we can just reload the contents.

I have made the changes in a branch from your branch to make it easy to see the changes: IOS-880-obfuscation-settings-page-jon

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @acb-mv and @buggmagnet)

acb-mv · 2024-11-15T13:51:07Z

Merged into this branch; thanks

rablador

👍

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @acb-mv and @buggmagnet)

acb-mv

Dismissed @buggmagnet from a discussion.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 22 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Should we wrap this whole view into a scrollview to make it general for future use cases too?

Depends on how many items we're expecting. I don't think this use case needs it, and we could do so on the calling side to keep concerns separated.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

It's still abbreviated.

Not in the most recent version in the branch (where this line is also at line 29)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 36 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

Default padding is specified in UIMetrics.SettingsCell.layoutMargins.

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 37 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

Unselected color should be UIColor.Cell.Background.indentationLevelOne.

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 45 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

There should be a separator between each row. It's defined in UIMetrics.TableView.separatorHeight.

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 46 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

This header should have bg color UIColor.Cell.Background.normal.

Done.

acb-mv

Reviewable status: 12 of 13 files reviewed, 14 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 32 at r7 (raw file):

Previously, rablador (Jon Petersson) wrote…

We should use the .iconTick image resource here instead.
Also, there should be some spacing between tick and text, defined in UIMetrics.SettingsCell.selectableSettingsCellLeftViewSpacing.

Done.

rablador

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, acb-mv wrote…

Not in the most recent version in the branch (where this line is also at line 29)

I checked out the branch, but I still only see v. 🤷‍♂️

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 22 at r9 (raw file):

            comment: ""
        )
        SingleChoiceList(

This whole view should have a padding of 24 to the large page title so that the content is consistent with other pages/views with large titles.

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 37 at r9 (raw file):

#Preview {
    var model = MockUDPTCPObfuscationSettingsViewModel(udpTcpPort: .port5001)

Should be let.

rablador

Reviewed all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv and @buggmagnet)

pinkisemils · 2024-11-18T10:17:03Z

This needs a rebase on top of the latest main.

acb-mv

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @buggmagnet and @rablador)

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 22 at r9 (raw file):

Previously, rablador (Jon Petersson) wrote…

This whole view should have a padding of 24 to the large page title so that the content is consistent with other pages/views with large titles.

Done.

ios/MullvadVPN/View controllers/Settings/Obfuscation/UDPTCPObfuscationSettingsView.swift line 37 at r9 (raw file):

Previously, rablador (Jon Petersson) wrote…

Should be let.

Done.

ios/MullvadVPN/View controllers/Settings/SwiftUI components/SingleChoiceList.swift line 21 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I checked out the branch, but I still only see v. 🤷‍♂️

Oh, you mean the variable name, not the type name? Fixed.

rablador

Reviewable status: 11 of 13 files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

rablador

Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

rablador

Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

github-actions · 2024-11-18T10:54:00Z

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

acb-mv requested review from rablador, buggmagnet and mojganii November 7, 2024 14:14

acb-mv force-pushed the IOS-880-obfuscation-settings-page branch 2 times, most recently from 02843c8 to d1d07ad Compare November 7, 2024 15:54

buggmagnet assigned acb-mv Nov 8, 2024

buggmagnet added the iOS Issues related to iOS label Nov 8, 2024

acb-mv force-pushed the IOS-880-obfuscation-settings-page branch 2 times, most recently from fd40edb to e5a8213 Compare November 8, 2024 18:23

buggmagnet requested changes Nov 11, 2024

View reviewed changes

rablador reviewed Nov 11, 2024

View reviewed changes

rablador requested changes Nov 11, 2024

View reviewed changes

acb-mv commented Nov 12, 2024

View reviewed changes

acb-mv commented Nov 13, 2024

View reviewed changes

rablador requested changes Nov 13, 2024

View reviewed changes

rablador reviewed Nov 14, 2024

View reviewed changes

acb-mv commented Nov 14, 2024

View reviewed changes

rablador reviewed Nov 14, 2024

View reviewed changes

acb-mv force-pushed the IOS-880-obfuscation-settings-page branch from a311888 to 19c2b8f Compare November 14, 2024 15:15

rablador reviewed Nov 15, 2024

View reviewed changes

acb-mv commented Nov 15, 2024

View reviewed changes

rablador requested changes Nov 15, 2024

View reviewed changes

rablador reviewed Nov 15, 2024

View reviewed changes

buggmagnet reviewed Nov 15, 2024

View reviewed changes

rablador reviewed Nov 15, 2024

View reviewed changes

acb-mv commented Nov 15, 2024

View reviewed changes

acb-mv force-pushed the IOS-880-obfuscation-settings-page branch 3 times, most recently from 3b33c1d to 2a9e656 Compare November 18, 2024 09:57

rablador requested changes Nov 18, 2024

View reviewed changes

rablador reviewed Nov 18, 2024

View reviewed changes

acb-mv commented Nov 18, 2024

View reviewed changes

acb-mv force-pushed the IOS-880-obfuscation-settings-page branch from 22921e2 to a96f105 Compare November 18, 2024 10:34

rablador approved these changes Nov 18, 2024

View reviewed changes

pinkisemils force-pushed the IOS-880-obfuscation-settings-page branch from a96f105 to 18dd3e1 Compare November 18, 2024 10:48

rablador approved these changes Nov 18, 2024

View reviewed changes

acb-mv and others added 2 commits November 18, 2024 11:49

Implement SwiftUI UI for UDP TCP Obfuscation port selector view

61e5a95

Fix reloading of data in settings cells

b087eb7

pinkisemils force-pushed the IOS-880-obfuscation-settings-page branch from 18dd3e1 to b087eb7 Compare November 18, 2024 10:50

pinkisemils merged commit c7f429a into main Nov 18, 2024
9 of 10 checks passed

pinkisemils deleted the IOS-880-obfuscation-settings-page branch November 18, 2024 10:50

IOS-880 SwiftUI settings page for UDP/TCP obfuscation port #7143

IOS-880 SwiftUI settings page for UDP/TCP obfuscation port #7143

Conversation

acb-mv commented Nov 7, 2024 • edited by faern Loading

linear bot commented Nov 7, 2024

buggmagnet left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

acb-mv commented Nov 13, 2024

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

acb-mv commented Nov 14, 2024

rablador left a comment

Choose a reason for hiding this comment

acb-mv commented Nov 14, 2024

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

acb-mv commented Nov 15, 2024

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

pinkisemils commented Nov 18, 2024

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2024

acb-mv commented Nov 7, 2024 •

edited by faern

Loading