Make AccessibilityIdentifier no longer a String enum

[acb-mv]

This removes the backing String value type from AccessibilityIdentifier. As of this change,

• AccessibilityIdentifier no longer has a .rawValue. For producing the string-based identifier, there is a .asString value which does the same thing
• UIAccessibilityIdentification no longer has an accessibilityIdentifier variable typed to AccessibilityIdentifier, but in turn has a setAccessibilityIdentifier method. The handful of use cases that involved matching a string to an AccessibilityIdentifier value are done by comparing the resulting strings.

Why have we done this, you may be wondering? The main thing this unblocks is more sophisticated AccessibilityIdentifier types and less boilerplate. Whereas until now, each value that had its own cell/button would have had its own AccessibilityIdentifier value, with a case statement to convert the underlying values to them, with this change it will be possible to have tagged values of AccessibilityIdentifier, with all options having the same underlying value and each differing only in the data value it encapsulates.

---

This change is 

[linear]

IOS-962 AccessibilityIdentifier enhancements to add tagged values

[rablador]

Reviewed 66 of 68 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv)

---

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

            cell.titleLabel.text = portString
            // TODO: replace this with a tagged AccessibilityIdentifier

Is this planned work in an upcoming pr? Otherwise we should either do it here or remove the todo.

---

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

            // location here is fine.
            if let location = item.node.locations.first {
                // we can probably replace this with a tagged AccessibilityIdentifier and cut this case statement

If you have an idea, go for it :)

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

        // Ensure changelog is no longer shown
        _ = app
            .otherElements[AccessibilityIdentifier.changeLogAlert.asString]

Thanks to the subscript function in XCUIElementQuery we can simplify this with .otherElements[.changeLogAlert]. This apply to all similar cases below.

[buggmagnet]

Please make sure to also run the UITests on the CI to guarantee we don't break anything (or at least not make it worse given its recent issues)

Reviewed 66 of 68 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv)

---

ios/MullvadVPN/View controllers/CreationAccount/Welcome/WelcomeContentView.swift line 254 at r3 (raw file):

    @objc private func tapped(button: AppButton) {
        switch button.accessibilityIdentifier {
        case AccessibilityIdentifier.purchaseButton.asString:

Do we need to specify the whole type name here ? It feels like a downgrade in readability

---

ios/MullvadVPN/Classes/AccessbilityIdentifier.swift line 218 at r3 (raw file):

extension AccessibilityIdentifier {
    public var asString: String {

Let's just make it compliant to CustomStringConvertible instead of doing this, we don't need to reinvent the wheel

[acb-mv]

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Thanks to the subscript function in XCUIElementQuery we can simplify this with .otherElements[.changeLogAlert]. This apply to all similar cases below.

Do we need to configure it to handle our custom AccessibilityIdentifier type?

[acb-mv]

Reviewable status: 66 of 69 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Classes/AccessbilityIdentifier.swift line 218 at r3 (raw file):

Previously, buggmagnet wrote…

Let's just make it compliant to CustomStringConvertible instead of doing this, we don't need to reinvent the wheel

That's what I did initially. The problem is, if you do that, you enter a loop, with "\(self)" calling the newly available self.description method until the stack overflows. There does not appear to be a user-accessible tagged-enum-formatting mechanism lower level than string interpolation (which calls description).

For what it's worth, the string we're getting is more specific than a catch-all description: it is a unique value generated from the enum and used for comparison, so it is not inappropriate for it to have a different computed variable.

---

ios/MullvadVPN/View controllers/CreationAccount/Welcome/WelcomeContentView.swift line 254 at r3 (raw file):

Previously, buggmagnet wrote…

Do we need to specify the whole type name here ? It feels like a downgrade in readability

Unfortunately, we do, as we're doing string comparisons. Unless we somehow hack storage onto arbitrary UIKit objects to store our structured AccessibilityIdentifier type, or else implement a parser to convert arbitrary strings from UIKit to whatever they plausibly came from, we're working in the string domain. Though there isn't a huge amount of this code, and perhaps a future solution could be a Swift macro to hide the boilerplate.

---

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

If you have an idea, go for it :)

I tried replacing that one with a AccessibilityIdentifier.locationCell(RelayLocation), though bringing MullvadTypes into the dependencies caused building UI tests to fail with Missing required module "WireGuardKitC". Is there an easy fix for this?

---

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

Previously, rablador (Jon Petersson) wrote…

Is this planned work in an upcoming pr? Otherwise we should either do it here or remove the todo.

Done.

[rablador]

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

---

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

Previously, acb-mv wrote…

I tried replacing that one with a AccessibilityIdentifier.locationCell(RelayLocation), though bringing MullvadTypes into the dependencies caused building UI tests to fail with Missing required module "WireGuardKitC". Is there an easy fix for this?

I don't think so, although I'm not terribly confident in evaluating the dependency problems we have sometimes.

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

Previously, acb-mv wrote…

Do we need to configure it to handle our custom AccessibilityIdentifier type?

Not sure I understand what you mean. We don't need any extra handling for most (all?) of them, right? And if we do, perhaps we could just have make and exception for outliers.

[acb-mv]

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

---

ios/MullvadVPNUITests/Base/BaseUITestCase.swift line 251 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Not sure I understand what you mean. We don't need any extra handling for most (all?) of them, right? And if we do, perhaps we could just have make and exception for outliers.

Ah yes, I see. Done.

[acb-mv]

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

---

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 330 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

I don't think so, although I'm not terribly confident in evaluating the dependency problems we have sometimes.

Fair enough. I'll consign this particular case statement to the too-hard basket for now, removing this comment, and stick to attaching types that don't come from dependency hell.

[acb-mv]

Done. Nothing new appears to be broken.

[rablador]

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

[buggmagnet]

Reviewed 3 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

[github-actions]

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