Refactor location data type and dependencies #5872

rablador · 2024-02-28T08:09:30Z

We need to split the location view into two sections, one for custom lists and one for all locations. Due to the current data structure being pretty much locked into a certain hierarchy we decided to create a new flexible data type and refactor the main data source and its dependencies.

Includes:

To test, add the following snippet somewhere before the call to show locations:

try? CustomListRepository().create("Netflix", locations: [
    .hostname("al", "tia", "ca-tor-wg-001"),
    .country("se"),
    .city("ca", "tor"),
])

try? CustomListRepository().create("Youtube", locations: [
    .hostname("ca", "tor", "ca-tor-wg-101"),
    .city("de", "ber"),
])

This change is

linear · 2024-02-28T08:09:32Z

IOS-514 Split select location view into two sections

rablador

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).

acb-mv

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?

mojganii

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):

Previously, acb-mv wrote…

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

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?

rablador

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):

Previously, mojganii wrote…

countryFor(countryCode: String)

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):

Previously, mojganii wrote…

cannot we aggregate 3 functions in 1?

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):

Previously, mojganii wrote…

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

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):

Previously, mojganii wrote…

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

Correct

rablador

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)

rablador

There, updated.

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

buggmagnet

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):

Previously, mojganii wrote…

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

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(...))

rablador

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):

Previously, buggmagnet wrote…

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)
    }

I made changes to the Cell enum already present there.

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

Previously, buggmagnet wrote…

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

Done.

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

Previously, buggmagnet wrote…

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.

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):

Previously, buggmagnet wrote…

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

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):

Previously, buggmagnet wrote…

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 ?

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):

Previously, mojganii wrote…

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

Done.

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

Previously, mojganii wrote…

if I were you,I would rename that to root

Done.

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

Previously, mojganii wrote…

should it be CustomListLocationNode?

Done.

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

Previously, buggmagnet wrote…

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

One test that calls node(by .country(...)

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

Done.

buggmagnet

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):

Previously, rablador (Jon Petersson) wrote…

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.

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

mojganii

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)?

mojganii

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

buggmagnet

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

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):

Previously, mojganii wrote…

do we need to implement hasher?

Nope, will remove.

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

Previously, mojganii wrote…

cannot we use RelayLocations model instead of tuple value?

Yeah, that's better!

buggmagnet

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

rablador added the iOS Issues related to iOS label Feb 28, 2024

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch 4 times, most recently from 1a5b939 to f73eadb Compare February 28, 2024 08:47

rablador commented Feb 28, 2024

View reviewed changes

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch from f73eadb to d2d5ce2 Compare February 28, 2024 09:04

rablador changed the title ~~Split select location view into two sections~~ Refactor location data type and dependencies Feb 28, 2024

rablador requested review from buggmagnet, mojganii and acb-mv February 28, 2024 09:06

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch 8 times, most recently from a1982d9 to 72b58a4 Compare February 28, 2024 16:22

acb-mv requested changes Feb 29, 2024

View reviewed changes

mojganii requested changes Feb 29, 2024

View reviewed changes

rablador commented Feb 29, 2024

View reviewed changes

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch 4 times, most recently from 8c2109a to ba8146d Compare February 29, 2024 15:29

rablador commented Feb 29, 2024

View reviewed changes

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch from ba8146d to b21fbed Compare February 29, 2024 15:49

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch 2 times, most recently from 3b5a629 to 9b1fbc5 Compare February 29, 2024 16:02

buggmagnet requested changes Mar 1, 2024

View reviewed changes

rablador commented Mar 1, 2024

View reviewed changes

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch from 9b1fbc5 to 2bda283 Compare March 1, 2024 13:06

buggmagnet approved these changes Mar 4, 2024

View reviewed changes

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch 3 times, most recently from 634bab1 to d715098 Compare March 4, 2024 13:53

mojganii requested changes Mar 4, 2024

View reviewed changes

mojganii approved these changes Mar 4, 2024

View reviewed changes

buggmagnet approved these changes Mar 4, 2024

View reviewed changes

rablador commented Mar 4, 2024

View reviewed changes

Split select location view into two sections

4107302

rablador force-pushed the split-select-location-view-into-two-sections-ios-514 branch from d715098 to 4107302 Compare March 4, 2024 15:10

buggmagnet approved these changes Mar 4, 2024

View reviewed changes

buggmagnet merged commit 29ec8a5 into main Mar 4, 2024
5 of 6 checks passed

buggmagnet deleted the split-select-location-view-into-two-sections-ios-514 branch March 4, 2024 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor location data type and dependencies #5872

Refactor location data type and dependencies #5872

rablador commented Feb 28, 2024 •

edited

Loading

linear bot commented Feb 28, 2024

rablador left a comment

acb-mv left a comment

mojganii left a comment

rablador left a comment

rablador left a comment

rablador left a comment

buggmagnet left a comment

rablador left a comment

buggmagnet left a comment

mojganii left a comment

mojganii left a comment

buggmagnet left a comment

rablador left a comment

buggmagnet left a comment

Refactor location data type and dependencies #5872

Refactor location data type and dependencies #5872

Conversation

rablador commented Feb 28, 2024 • edited Loading

linear bot commented Feb 28, 2024

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

mojganii 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

buggmagnet 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

mojganii left a comment

Choose a reason for hiding this comment

mojganii 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

buggmagnet left a comment

Choose a reason for hiding this comment

rablador commented Feb 28, 2024 •

edited

Loading