Reviewable status: 0 of 21 files reviewed, 2 unresolved discussions

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 1 at r2 (raw file):

//

Something happened during rebase, likely because this PR was originally based on another non-main PR. In any case, this file was only moved and doesn't need to be reviewed (again).

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorContentView.swift line 1 at r2 (raw file):

//

Something happened during rebase, likely because this PR was originally based on another non-main PR. In any case, this file was only moved and doesn't need to be reviewed (again).

Reviewed 13 of 21 files at r1, 7 of 8 files at r3, all commit messages.
Reviewable status: 20 of 22 files reviewed, 3 unresolved discussions (waiting on @rablador)

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

    }

    static func == (lhs: LocationNode, rhs: LocationNode) -> Bool {

A minor nit, but shouldn't this be under an Equatable conformance rather than Hashable?

Reviewable status: 20 of 22 files reviewed, 10 unresolved discussions (waiting on @acb-mv and @rablador)

ios/MullvadVPN/UI appearance/UIColor+Palette.swift line 120 at r3 (raw file):

    }

    enum SubSubSubCell {

I would suggest you to refactor node background color in this way :

Code snippet:

  enum LocationNodeLevelCell {
        enum BackgroundColor {
            static let zero = UIColor(red: 0.15, green: 0.23, blue: 0.33, alpha: 1.0)
            static let one = UIColor(red: 0.15, green: 0.23, blue: 0.33, alpha: 1.0)
            static let two = UIColor(red: 0.13, green: 0.20, blue: 0.30, alpha: 1.0)
            static let three = UIColor(red: 0.11, green: 0.17, blue: 0.27, alpha: 1.0)
        }
    }
    
    or 
    
     enum LocationCell {
        enum NodeLevel {
            enum BackgroundColor {
                static let zero = UIColor(red: 0.15, green: 0.23, blue: 0.33, alpha: 1.0)
                static let one = UIColor(red: 0.15, green: 0.23, blue: 0.33, alpha: 1.0)
                static let two = UIColor(red: 0.13, green: 0.20, blue: 0.30, alpha: 1.0)
                static let three = UIColor(red: 0.11, green: 0.17, blue: 0.27, alpha: 1.0)
            }
        }
    }

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

class LocationNode {
    let nodeName: String

the class name contains node,having them in the properties are redundant

Code snippet:

class LocationNode {
    let name: String
    var code: String
    ...
     init(
        name: String,
        code: String,
        locations: [RelayLocation] = [],
        indentationLevel: Int = 0,
        parent: LocationNode? = nil,
        children: [LocationNode] = [],
        showsChildren: Bool = false
    ) {
        self.name = nodeName
        self.code = nodeCode
        self.locations = locations
        self.indentationLevel = indentationLevel
        self.parent = parent
        self.children = children
        self.showsChildren = showsChildren
    }
}

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

extension LocationNode {
    var topmostAncestor: LocationNode {

if I were you,I would rename that to root

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

    }

    func countryFor(countryCode: String) -> LocationNode? {

countryFor(countryCode: String)

Code snippet:

func country(for Code: String)

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

    func childNodeFor(nodeCode: String) -> LocationNode? {
        self.nodeCode == nodeCode ? self : children.compactMap { $0.childNodeFor(nodeCode: nodeCode) }.first

cannot we aggregate 3 functions in 1?

Code snippet:

   func find(by code : String) -> LocationNode? {
        nodeCode == code ? self : children.first(where: { $0.nodeCode == code })
    }

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

extension LocationNode {
    func copy(withParent parent: LocationNode? = nil) -> LocationNode {

LocationNode is a reference type.cannot we change just the properties we want instead of making another object?

Code snippet:

func deepCopy(with parent: LocationNode? = nil) -> LocationNode {
        self.parent = parent

        self.children = recursivelyCopyChildren(withParent: self)

        return self
    }

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

Hashable conforms Equatable. LocationNode should be Hashable for the essence of UITableViewDiffableDataSource

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

}

class ListLocationNode: LocationNode {

should it be CustomListLocationNode?

Reviewable status: 20 of 22 files reviewed, 10 unresolved discussions (waiting on @acb-mv and @mojganii)

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

Stylistically I agree, but from call site country(for: String) doesn't give any clue as to what for should be.

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

Yeah, perhaps we should. The reason I've kept them apart is that this operation is a lot more expensive that the others, since it recursively travels through the whole tree if needed.

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

That won't work, since the copy typically comes from the nodes in the other data source. If you change eg. showsChildren you'll end up expanding the referenced node as well. I'm not in love with the idea of copying like this, but we really do need separate objects here. An alternative could be to let CustomListDataSource construct its own complete list of nodes from the relay response, just like AllLocationsDataSource, but the operations is a little expensive and unnecessary to duplicate.

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

Correct

I've just realized that the search functionality isn't working as intended. Please disregard those parts from the PR until fixed.

Reviewable status: 20 of 22 files reviewed, 10 unresolved discussions (waiting on @acb-mv and @mojganii)

There, updated.

Reviewable status: 13 of 25 files reviewed, 10 unresolved discussions (waiting on @acb-mv and @mojganii)

Reviewed 12 of 21 files at r1, 1 of 8 files at r3, 2 of 7 files at r4, 2 of 5 files at r5, 7 of 8 files at r6, all commit messages.
Reviewable status: 24 of 26 files reviewed, 15 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

ios/MullvadVPN/UI appearance/UIColor+Palette.swift line 120 at r3 (raw file):

Wouldn't it be easier to just switch things around instead ?

    enum SubCell {
        static func backgroundColorForIdentationLevel(level: Int) -> UIColor {
            switch level {
            case 1:
                return UIColor(red: 0.15, green: 0.23, blue: 0.33, alpha: 1.0)
            case 2:
                return UIColor(red: 0.13, green: 0.20, blue: 0.30, alpha: 1.0)
            case 3:
                return UIColor(red: 0.11, green: 0.17, blue: 0.27, alpha: 1.0)
            default:
                return UIColor.Cell.backgroundColor

            }
        }
    }

and on the call site

    private func backgroundColorForIdentationLevel() -> UIColor {
        UIColor.SubCell.backgroundColorForIdentationLevel(level: indentationLevel)
    }

ios/MullvadVPN/View controllers/SelectLocation/AllLocationDataSource.swift line 20 at r6 (raw file):

    }

    func reload(_ response: REST.ServerRelaysResponse, relays: [REST.ServerRelay]) {

Could you add a brief comment of what the function does in broad terms ?
It doesn't have to be super deep, but just to help future readers get the general idea, for example why are cities special cased in this

ios/MullvadVPN/View controllers/SelectLocation/CustomListsDataSource.swift line 56 at r6 (raw file):

            return switch locations.first {
            case let .country(countryCode):
                customListNode.childNodeFor(nodeCode: "\(customListNode.nodeCode)-\(countryCode)")

IMHO we shouldn't pass a custom format here, it's very easy to get wrong.
What do you feel about passing an array of strings instead, which the childNodeFor function will know how to format properly ?

i.e. something along those lines

    func childNodeFor(nodes: [String]) -> LocationNode? {
        let nodeCode = nodes.joined(separator: "-")
        return childNodeFor(nodeCode: nodeCode)
    }

The callsite becomes customListNode.childNodeFor(nodes: [customListNode.nodeCode, countryCode]) which cannot have the wrong format now.

ios/MullvadVPN/View controllers/SelectLocation/LocationCellViewModel.swift line 23 at r6 (raw file):

    static func == (lhs: Self, rhs: Self) -> Bool {
        lhs.node == rhs.node &&
            lhs.section == rhs.section

nit
Do we want to check the indentation level here as well ?

ios/MullvadVPN/View controllers/SelectLocation/LocationDataSource.swift line 93 at r6 (raw file):

        updateDataSnapshot(with: list, reloadExisting: !searchString.isEmpty) {
            DispatchQueue.main.async {

Not sure why is this dispatch to main here, from what I can see, this filterRelays is always called on the main thread.
What problem did you have that required this workaround ?

ios/MullvadVPNTests/Location/AllLocationsDataSourceTests.swift line 42 at r6 (raw file):

        XCTAssertEqual(nodes, dataSource.nodes)
    }

For completion's sake, can we have 2 extra tests ?

• One test that calls node(by .country(...)
• One test that calls node(by: .city(...))

Reviewable status: 24 of 26 files reviewed, 15 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)

ios/MullvadVPN/UI appearance/UIColor+Palette.swift line 120 at r3 (raw file):

I made changes to the Cell enum already present there.

ios/MullvadVPN/View controllers/SelectLocation/AllLocationDataSource.swift line 20 at r6 (raw file):

Done.

ios/MullvadVPN/View controllers/SelectLocation/CustomListsDataSource.swift line 56 at r6 (raw file):

It's not a custom format, and I can see how it easily could be interpreted as such. customListNode.nodeCode as a prefix is a necessary evil to differentiate nodes and scope them under a list. Without this the diffable data source will complain about identical item identifiers. We could add something like a custom list id to LocationNode and include it in the hash. That would remove the need for this jank, but at the same time add further bloat to each node.

ios/MullvadVPN/View controllers/SelectLocation/LocationCellViewModel.swift line 23 at r6 (raw file):

Originally no, since selecting an item while searching could yeild a different indentation level than when all nodes are visible (custom list top node is removed from search results, meaning one less level of indentation). This would make comparisons of selectedItem with any other item unreliable. However, I could change the one place where we compare this way (in tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPat)) to compare nodes instead. This works, but we may have future side effects this way.

ios/MullvadVPN/View controllers/SelectLocation/LocationDataSource.swift line 93 at r6 (raw file):

If updateDataSnapshot is called while the snapshot is being applied the tableView will throw:

Deadlock detected: calling this method on the main queue with outstanding async updates is not permitted and will deadlock. Please always submit updates either always on the main queue or always off the main queue

The call from LocationViewController.setUpDataSource is what's causing this.

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

Done.

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

Done.

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

Done.

ios/MullvadVPNTests/Location/AllLocationsDataSourceTests.swift line 42 at r6 (raw file):

Done.

Reviewed 1 of 7 files at r4, 1 of 8 files at r6, 17 of 18 files at r7, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPN/View controllers/SelectLocation/CustomListsDataSource.swift line 56 at r6 (raw file):

nit
I think we misunderstood each other, but it's not a big deal. Marking this as resolved

Reviewable status: 29 of 32 files reviewed, 8 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

ios/MullvadVPN/View controllers/SelectLocation/LocationCellViewModel.swift line 16 at r8 (raw file):

    var indentationLevel = 0

    func hash(into hasher: inout Hasher) {

do we need to implement hasher?

ios/MullvadVPN/View controllers/SelectLocation/LocationDataSource.swift line 22 at r8 (raw file):

    private var selectedItem: LocationCellViewModel?

    var didSelectRelayLocations: (([RelayLocation], UUID?) -> Void)?

cannot we use RelayLocations model instead of tuple value?

Code snippet:

var didSelectRelays: ((RelayLocations) -> Void)?

Reviewable status: 29 of 32 files reviewed, 8 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

Reviewed 3 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

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

ios/MullvadVPN/View controllers/SelectLocation/LocationCellViewModel.swift line 16 at r8 (raw file):

Nope, will remove.

ios/MullvadVPN/View controllers/SelectLocation/LocationDataSource.swift line 22 at r8 (raw file):

Yeah, that's better!

Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii)