Adjust UI to allow adding elements to a custom list ios 490

[mojganii]

This pull request introduces a feature that allows users to add/edit locations within a custom list from all available locations. However, there are certain limitations in place to ensure the integrity of the process. Specifically, users cannot add elements to the list that are already encompassed within the list above. Additionally, if a user selects all children directly from the list, it does not imply that the parent was explicitly chosen.

---

This change is 

[linear]

IOS-490 Adjust UI to allow adding elements to a custom list

[rablador]

Reviewed 19 of 19 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationCell.swift line 1 at r1 (raw file):

//

Could this class perhaps have inherited from LocationCell to reuse most of the base functionality there?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 36 at r1 (raw file):

    ) {
        self.tableView = tableView
        self.nodes = allLocations.map {

I think it'd be better if the nodes passed into the class are already copied and "flattened" rather than having a non-related state.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 74 at r1 (raw file):

            locationsList.append(viewModel)

            // Determine the node should be expanded.

Nit: Determine "if"

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 75 at r1 (raw file):

            // Determine the node should be expanded.
            guard customListLocationNode.children.contains(where: { node.allChildren.contains($0) }) else {

contains in contains can be hard to grasp. Also, this code is present in more places in this file. Perhaps we could make an aptly named function of it?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 91 at r1 (raw file):

        }
        updateDataSnapshot(with: [locationsList], completion: {
            // Scroll to the first selected location

I'm not sure if this is the best behavior. Since we don't have just one particular item of interest, I think it feels odd (and a little random) to jump to a selected item at all. I might as well want to add items from a node at the top, but with the scrolling I need to go up there manually again.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 198 at r1 (raw file):

        var locationList = snapshot().itemIdentifiers
        let item = locationList[index]
        let isSelected = !item.isSelected

Nit: I think the ! should be applied when used below instead. That way isSelected = item.isSelected translates 1:1.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 216 at r1 (raw file):

extension AddLocationsDataSource {
    private func scroll(to item: AddLocationCellViewModel, animated: Bool) {

This function is almost identical to a function in LocationDataSource. The only this that differs is the view model, but perhaps we can replace that will a protocol?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 242 at r1 (raw file):

fileprivate extension [AddLocationCellViewModel] {
    mutating func addSubNodes(from item: AddLocationCellViewModel, at indexPath: IndexPath) {

Just like above, this function only differs from its LocationDataSource counterpart in terms of view model (and section, but that can be handled through a param or something I think).

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 255 at r1 (raw file):

    }

    mutating func recursivelyRemoveSubNodes(from node: LocationNode) {

This function IS identical to a function in LocationDataSource. Can we reuse somehow?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 267 at r1 (raw file):

fileprivate extension [AddLocationCellViewModel] {
    mutating func deselectAncestors(from node: LocationNode?) {

Could node.forEachAncestor be used here instead (to reuse logic already covered by tests)?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 277 at r1 (raw file):

    }

    mutating func toggleSelectionSubNodes(from node: LocationNode, isSelected: Bool) {

Could node.forEachDescendant be used here instead (to reuse logic already covered by tests)?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 303 at r1 (raw file):

        removeSubNodes(node: selectedLocation)
    }

The functions below all use recursion for traversing the node tree. Could we make use of .forEachDescendant and .forEachAncestor here?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 23 at r1 (raw file):

    weak var delegate: AddLocationsViewControllerDelegate?
    private let tableView: UITableView = {

I think we could create a config or something for both this table view and the one in LocationViewController. They should always be the same except for selection and accessibilityIdentifier.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 35 at r1 (raw file):

    init(
        allLocationsNodes: [LocationNode],

Nit: Simply call nodes?

---

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

    }

    var allChildren: [LocationNode] {

Nit: "var flattened: [LocationNode]"?

[buggmagnet]

Reviewed 9 of 19 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/Coordinators/CustomLists/AddCustomListCoordinator.swift line 87 at r3 (raw file):

        )

        coordinator.didFinish = { customList in

I think there is a cyclic reference between AddLocationsCoordinator and AddCustomListCoordinator here.

AddCustomListCoordinator adds AddLocationsCoordinator as a child.
But also AddLocationsCoordinator captures self (AddCustomListCoordinator in this case) in its didFinish closure (which it retains) )

---

ios/MullvadVPN/Coordinators/CustomLists/AddCustomListCoordinator.swift line 94 at r3 (raw file):

                tableSections: self.subject.value.tableSections
            ))
            self.removeFromParent()

This gets called everytime we select a different location when editing location,
Correct me if I'm wrong, but I think this is not the intended behaviour ?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationCell.swift line 1 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Could this class perhaps have inherited from LocationCell to reuse most of the base functionality there?

Heavily agree !

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 119 at r3 (raw file):

        )

        dataSourceConfiguration?.didSelectItem = { item in

[weak self] to avoid a retain cycle here

---

ios/MullvadVPN/Coordinators/CustomLists/EditCustomListCoordinator.swift line 84 at r3 (raw file):

        )

        coordinator.didFinish = { customList in

[weak self] to avoid a retain cycle here

[buggmagnet]

Reviewed 2 of 19 files at r1, 12 of 14 files at r4, all commit messages.
Reviewable status: 20 of 22 files reviewed, 21 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 42 at r4 (raw file):

    }

    deinit {

Let's remove this

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsCoordinator.swift line 60 at r4 (raw file):

    }

    func didOnBack() {

didOnBack doesn't read well, how about onBack ? Or didBack if you want to keep the did convention.

[mojganii]

Reviewable status: 20 of 22 files reviewed, 19 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 42 at r4 (raw file):

Previously, buggmagnet wrote…

Let's remove this

It was underdevelopment.Sure

---

ios/MullvadVPN/Coordinators/CustomLists/AddCustomListCoordinator.swift line 87 at r3 (raw file):

Previously, buggmagnet wrote…

I think there is a cyclic reference between AddLocationsCoordinator and AddCustomListCoordinator here.

AddCustomListCoordinator adds AddLocationsCoordinator as a child.
But also AddLocationsCoordinator captures self (AddCustomListCoordinator in this case) in its didFinish closure (which it retains) )

there were some retain cycle in different part of the Custom list which were fixed.

---

ios/MullvadVPN/Coordinators/CustomLists/AddCustomListCoordinator.swift line 94 at r3 (raw file):

Previously, buggmagnet wrote…

This gets called everytime we select a different location when editing location,
Correct me if I'm wrong, but I think this is not the intended behaviour ?

indeed this code should be revoked when the controller is popped .

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationCell.swift line 1 at r1 (raw file):

Previously, buggmagnet wrote…

Heavily agree !

let me disagree with this.it's the ui for adding location to custom list and the other one in using in select location, however cells have similar appearance but the fictionality is different. to be honest I don't see any advantage to do this.

I tried to implement them with different configuration through UIContentConfiguration but It brought much more than complexity for no good reason

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsCoordinator.swift line 60 at r4 (raw file):

Previously, buggmagnet wrote…

didOnBack doesn't read well, how about onBack ? Or didBack if you want to keep the did convention.

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 36 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think it'd be better if the nodes passed into the class are already copied and "flattened" rather than having a non-related state.

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 74 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Determine "if"

done

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 91 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'm not sure if this is the best behavior. Since we don't have just one particular item of interest, I think it feels odd (and a little random) to jump to a selected item at all. I might as well want to add items from a node at the top, but with the scrolling I need to go up there manually again.

I don't have a strong opinion on this matter, especially considering that we have the freedom to make changes to iOS for the initial iteration.I removed that

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 242 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Just like above, this function only differs from its LocationDataSource counterpart in terms of view model (and section, but that can be handled through a param or something I think).

I tried to take care of reusability as much as I can. I have a draft version for that work.I'm pressed the time let's do that any refactoring when I'm back or I would be happy if you would like to do that.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 255 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This function IS identical to a function in LocationDataSource. Can we reuse somehow?

they have different view model.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 267 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Could node.forEachAncestor be used here instead (to reuse logic already covered by tests)?

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 277 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Could node.forEachDescendant be used here instead (to reuse logic already covered by tests)?

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 303 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

The functions below all use recursion for traversing the node tree. Could we make use of .forEachDescendant and .forEachAncestor here?

Indeed. I forgot to refactor this part after resolving memory issue at LocationNode

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 23 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think we could create a config or something for both this table view and the one in LocationViewController. They should always be the same except for selection and accessibilityIdentifier.

I would prefer keep them separated as much as I can. in my opinion they are different views with diffrent functionality. tiding them to gather specially when it comes to appearance doesn't appeal to me

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 35 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Simply call nodes?

Done

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 119 at r3 (raw file):

Previously, buggmagnet wrote…

[weak self] to avoid a retain cycle here

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/EditCustomListCoordinator.swift line 84 at r3 (raw file):

Previously, buggmagnet wrote…

[weak self] to avoid a retain cycle here

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListCoordinator.swift line 51 at r3 (raw file):

    private func edit(list: CustomList) {
        // Remove previous edit coordinator to prevent accumulation.

if we remove children when their are no longer exsist then there is no need to remove possible accumulation coordinator.

---

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

Previously, rablador (Jon Petersson) wrote…

Nit: "var flattened: [LocationNode]"?

Done

[mojganii]

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

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 75 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

contains in contains can be hard to grasp. Also, this code is present in more places in this file. Perhaps we could make an aptly named function of it?

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 198 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: I think the ! should be applied when used below instead. That way isSelected = item.isSelected translates 1:1.

it contains the fresh toggled value after selection or 'deselection'.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 216 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This function is almost identical to a function in LocationDataSource. The only this that differs is the view model, but perhaps we can replace that will a protocol?

it's tidying them up or reusing UI elements, doesn't offer any benefits. Is that correct? If so, could you provide more context or clarify your statement further?

[rablador]

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

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 23 at r1 (raw file):

Previously, mojganii wrote…

I would prefer keep them separated as much as I can. in my opinion they are different views with diffrent functionality. tiding them to gather specially when it comes to appearance doesn't appeal to me

@buggmagnet or @acb-mv What are your takes on this?

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsViewController.swift line 84 at r5 (raw file):

        dataSource = AddLocationsDataSource(
            tableView: tableView,
            allLocations: nodes.copy(),

Nit: "copyAndFlatten"?

[rablador]

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

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 216 at r1 (raw file):

Previously, mojganii wrote…

it's tidying them up or reusing UI elements, doesn't offer any benefits. Is that correct? If so, could you provide more context or clarify your statement further?

Did some shuffling and refactoring to combine as much as possible.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 242 at r1 (raw file):

Previously, mojganii wrote…

I tried to take care of reusability as much as I can. I have a draft version for that work.I'm pressed the time let's do that any refactoring when I'm back or I would be happy if you would like to do that.

Did some shuffling and refactoring to combine as much as possible.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationsDataSource.swift line 255 at r1 (raw file):

Previously, mojganii wrote…

they have different view model.

Did some shuffling and refactoring to combine as much as possible.

---

ios/MullvadVPN/Coordinators/CustomLists/AddLocationCell.swift line 1 at r1 (raw file):

Previously, mojganii wrote…

let me disagree with this.it's the ui for adding location to custom list and the other one in using in select location, however cells have similar appearance but the fictionality is different. to be honest I don't see any advantage to do this.

I tried to implement them with different configuration through UIContentConfiguration but It brought much more than complexity for no good reason

Did some shuffling and refactoring to combine as much as possible.

[buggmagnet]

CustomFail.mov

Those changes are really good !
However, now we are automatically connecting whenever we edit a custom list even if it's not a user selection, or if another relay was already selected.

Reviewed 5 of 13 files at r5, 9 of 17 files at r6, 3 of 6 files at r7, 7 of 7 files at r8, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv and @mojganii)

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 19 at r8 (raw file):

    private let relayCacheTracker: RelayCacheTracker
    private var cachedRelays: CachedRelays?
    private var customListRepository: CustomListRepositoryProtocol

This should be declared as let now

[rablador]

We run tunnelManager.startTunnel() when saving the lists, but changing to tunnelManager.reconnectTunnel() will to the trick.

Reviewed 9 of 17 files at r6, 3 of 6 files at r7, 7 of 7 files at r8, 1 of 1 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 19 at r8 (raw file):

Previously, buggmagnet wrote…

This should be declared as let now

Done.

[rablador]

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

[buggmagnet]

Reviewed 1 of 1 files at r9, 3 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

[rablador]

Reviewable status: all files reviewed, 3 unresolved discussions

[buggmagnet]

Reviewable status: complete! all files reviewed, all discussions resolved

[rablador]

Reviewed 6 of 6 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 6 of 6 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved