Ios 881 shadowsocks obfuscation settings

[acb-mv]

Create the SwiftUI-based Shadowsocks port configuration page.

This uses the same SingleChoiceList element created for IOS-880. However, since the UI is more complex, as custom user-entered ports are allowed, this element has been extended to implement this functionality (In fact, most of the changes in this PR are to SingleChoiceList). Documentation and examples (in SwiftUI Previews) have been added for this element as well, as it should prove reusable for other screens.

A screen recording of the UI is attached:

RPReplay_Final1732184675.MP4

---

This change is 

[linear]

IOS-881 Create new subsetting page for shadowsocks selection

[buggmagnet]

It looks like one cannot delete the first character in the custom field for shadowsocks obfuscation
RPReplay_Final1732191538.mov

Reviewable status: 0 of 5 files reviewed, all discussions resolved

[buggmagnet]

Right now, any custom port selection for Shadowsocks obfuscation does not work, and enters the blocked state immediately.

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

---

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

                tableName: "Shadowsocks",
                // currently padded with spaces to make space
                value: "Port        ",

That really seems like a hack, isn't there a better way to do this ?

---

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

            ),
            customFieldMode: .numericText
        )

The valid range indicator text field is missing

---

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

        VStack(spacing: UIMetrics.TableView.separatorHeight) {
            HStack {
                Text(title).fontWeight(.semibold)

This title is a different color than the rows in the spec

[acb-mv]

Re: the first character not being deletable, this is because the value the form sets would not be valid if the field is empty. If the text field changes to an invalid value, it is corrected to the value of the current custom value's parameter.
If this is unacceptable, I could add a fallback value to choose if it is invalid, though that would feel like overcomplicating things.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

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

Previously, buggmagnet wrote…

That really seems like a hack, isn't there a better way to do this ?

I agree that it looks horrible. I could set the width of the TextField's frame in points, but not in characters. Given that the text size could change, padding the string with spaces is the least-worst option.

[acb-mv]

The first character issue is now fixed.

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

---

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

Previously, buggmagnet wrote…

The valid range indicator text field is missing

Done.

[acb-mv]

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

---

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

Previously, buggmagnet wrote…

This title is a different color than the rows in the spec

Is it? It looks white (#ffffff) on screen, as it is on the Figma board.

[acb-mv]

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

Previously, acb-mv wrote…

I agree that it looks horrible. I could set the width of the TextField's frame in points, but not in characters. Given that the text size could change, padding the string with spaces is the least-worst option.

The other option would be to wrap the TextField in a ZStack, placing a Text("00000") or similar beneath it for sizing. Which we can do, though it doesn't quite sound like less of a hack.

[rablador]

Reviewed 2 of 5 files at r1, 5 of 6 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

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

                .cornerRadius(4.0)
                .focused($customValueIsFocused)
                .onChange(of: customValue) { newValue in

1. I think we should spell out abbreviations to make it clearer what's returned from parsing.
2. Can we somehow make use of a view model to do the custom logic? Maybe it's ok for the view to validate its values, but I feel it would be nice to hide away as much as possible.

[acb-mv]

Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

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

Previously, rablador (Jon Petersson) wrote…

1. I think we should spell out abbreviations to make it clearer what's returned from parsing.
2. Can we somehow make use of a view model to do the custom logic? Maybe it's ok for the view to validate its values, but I feel it would be nice to hide away as much as possible.

I've renamed the two single-letter variables in the method. As for view models, I think that would be overkill for SingleChoiceList, as it's purely a UI component, and the only piece of @State it keeps is its initial value in case it needs to backtrack from an invalid custom value.

[rablador]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

[rablador]

I just remembered, please remove the Todos in VPNSettingsDataSource -> didSelectRowAt. They're not needed anymore.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

[rablador]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

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

Previously, acb-mv wrote…

The other option would be to wrap the TextField in a ZStack, placing a Text("00000") or similar beneath it for sizing. Which we can do, though it doesn't quite sound like less of a hack.

We currently hard code 100 px for SettingsInputCell (used in wireguard port). I think we should use that for now here too.

[buggmagnet]

The port text is not readable when the phone is in dark mode.
It should be mode agnostic here since the UI is dark itself.

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

---

a discussion (no related file):
We should add some input validation to make sure users can't break the UI

I also managed to select port 0, which shouldn't be possible.
Invalid ports should appear in red, we should reuse the same logic as we used in WireGuard Ports, for the "Custom" option.

RPReplay_Final1732546765.mov

[acb-mv]

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

Previously, rablador (Jon Petersson) wrote…

We currently hard code 100 px for SettingsInputCell (used in wireguard port). I think we should use that for now here too.

Given that SwiftUI automatically scales text with the user's preferences (yes, we get Dynamic Text for free), this will break horribly the first time a person with poor eyesight and extra-large phone text uses our app. We could nerf Dynamic Text and hardwire everything to fixed sizes, but that would be a retrograde step.

[acb-mv]

Works fine for me /shrug

Are you sure the code you're running sets the text field's foreground colour to UIColor.TextField.textColor (line 212)?

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

a discussion (no related file):

Previously, buggmagnet wrote…

We should add some input validation to make sure users can't break the UI

I also managed to select port 0, which shouldn't be possible.
Invalid ports should appear in red, we should reuse the same logic as we used in WireGuard Ports, for the "Custom" option.

RPReplay_Final1732546765.mov

Done; it now rejects port 0, and displays any invalid input as entered, in red (with a red rounded border as well)

[acb-mv]

Reviewable status: 6 of 9 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)

---

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

Previously, acb-mv wrote…

Given that SwiftUI automatically scales text with the user's preferences (yes, we get Dynamic Text for free), this will break horribly the first time a person with poor eyesight and extra-large phone text uses our app. We could nerf Dynamic Text and hardwire everything to fixed sizes, but that would be a retrograde step.

After spending half an hour or so fruitlessly trying to find a way of specifying SwiftUI frames in character-width-relative units, I have changed this to set the minimum width of the TextField to 100px and removed the trailing spaces in "Port ". It will have to do for now. Hopefully, at some future time. something like TextField(...).frame(minWidth: .em(5)) will exist.

[rablador]

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

---

a discussion (no related file):

Previously, acb-mv wrote…

Done; it now rejects port 0, and displays any invalid input as entered, in red (with a red rounded border as well)

Validation seems to work fine now, but it's still possible to break the UI like in the clip above.

---

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

Previously, acb-mv wrote…

After spending half an hour or so fruitlessly trying to find a way of specifying SwiftUI frames in character-width-relative units, I have changed this to set the minimum width of the TextField to 100px and removed the trailing spaces in "Port ". It will have to do for now. Hopefully, at some future time. something like TextField(...).frame(minWidth: .em(5)) will exist.

Aha, it's built in. Then a fixed size isn't great here... I can see that you added a minumum size instead, which I think is fine. Could you rename it to reflect that it's a min size?

---

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

            Text(label)
            Spacer()
            TextField("value", text: $customValueInput, prompt: Text(prompt))

Text should be right-aligned.

---

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

                        : Color(UIColor.TextField.textColor)
                )
                .background(Color(UIColor.TextField.backgroundColor))

The default background color should be dark, focused white. Check out SettingsInputCell for the correct colors.

[acb-mv]

Reviewable status: 7 of 9 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

a discussion (no related file):

Previously, rablador (Jon Petersson) wrote…

Validation seems to work fine now, but it's still possible to break the UI like in the clip above.

Done.

[acb-mv]

Reviewable status: 7 of 9 files reviewed, 6 unresolved discussions (waiting on @buggmagnet and @rablador)

---

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

Previously, rablador (Jon Petersson) wrote…

Aha, it's built in. Then a fixed size isn't great here... I can see that you added a minumum size instead, which I think is fine. Could you rename it to reflect that it's a min size?

Done

[rablador]

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

[buggmagnet]

Reviewed 2 of 5 files at r1, 4 of 6 files at r4, 1 of 3 files at r8, 1 of 2 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv)

---

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

Previously, acb-mv wrote…

Is it? It looks white (#ffffff) on screen, as it is on the Figma board.

I meant the header bar color, my bad, for some reason Reviewable doesn't let me upload a screenshot in this comment, so I will upload it somewhere else.

---

ios/MullvadVPN/View controllers/Settings/Obfuscation/ShadowsocksObfuscationSettingsView.swift line 70 at r14 (raw file):

#Preview {
    var model = MockShadowsocksObfuscationSettingsViewModel(shadowsocksPort: .automatic)

This line generates a swiftlint warning because model is a var, we should change that into a let declaration instead

---

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

import MullvadSettings

protocol UDPTCPObfuscationSettingsViewModel: ObservableObject {

nit
looks like we missed a lot of renaming of UDPTCP to UDPOverTCP

---

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

  A component presenting a vertical list in the Mullvad style for selecting a single item from a list.
  This is parametrised over a value type known as `Value`, which can be any Equatable type. One would typically use an `enum` for this. As the name suggests, this allows one value to be chosen, which it sets a provided binding to.

This is great, I love this !

---

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

    // Construct the one row with a custom input field for a custom value
    private func customRow(

We have 2 swiftlint warnings that we should fix here:

• function_parameter_count
• function_body_length

Given that declarative UI tends to be verbose in terms of space, it's okay to silence them here IMHO

---

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

                customFieldMode == .numericText
                    ? .trailing
                    : .leading/*@END_MENU_TOKEN@*/

What is this *@END_MENU_TOKEN@*/ for ?

---

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

            )
            .focused($customValueIsFocused)
            .onChange(of: customValueInput) { newValue in

👮 ⚠️ Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)

[acb-mv]

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

---

ios/MullvadVPN/View controllers/Settings/Obfuscation/ShadowsocksObfuscationSettingsView.swift line 70 at r14 (raw file):

Previously, buggmagnet wrote…

This line generates a swiftlint warning because model is a var, we should change that into a let declaration instead

Done.

---

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

Previously, buggmagnet wrote…

I meant the header bar color, my bad, for some reason Reviewable doesn't let me upload a screenshot in this comment, so I will upload it somewhere else.

I compared the colours in screenshots of the SwiftUI and UIKit settings in the running app and they're identical.

---

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

Previously, buggmagnet wrote…

What is this *@END_MENU_TOKEN@*/ for ?

On line 232? No idea, but it's gone now.

---

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

Previously, buggmagnet wrote…

nit
looks like we missed a lot of renaming of UDPTCP to UDPOverTCP

OK, fixed.

[acb-mv]

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

---

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

Previously, buggmagnet wrote…

This is great, I love this !

Thanks; as SingleChoiceList is a versatile component I expect to be reused, I feel it deserved documentation to facilitate this.

---

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

Previously, buggmagnet wrote…

We have 2 swiftlint warnings that we should fix here:

• function_parameter_count
• function_body_length

Given that declarative UI tends to be verbose in terms of space, it's okay to silence them here IMHO

Done.

---

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

Previously, buggmagnet wrote…

👮 ⚠️ Unused Closure Parameter Violation: Unused parameter in a closure should be replaced with _ (unused_closure_parameter)

Done.

[rablador]

Reviewed 5 of 6 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Let's fix those 2 swiftlint warnings and we can merge this

Reviewed 5 of 6 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

---

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

Previously, acb-mv wrote…

I compared the colours in screenshots of the SwiftUI and UIKit settings in the running app and they're identical.

This was discussed offline, and we decided to not change it for consistency reasons within the app.

[rablador]

Reviewable status: all files reviewed, 1 unresolved discussion

[rablador]

@acb-mv Ping me when done, I'll merge after that.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[acb-mv]

Done

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)

[rablador]

Reviewed 1 of 1 files at r18, 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[github-actions]

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