Create view with custom lists to edit

[rablador]

When a user decides to edit a custom list they will be sent to a view with all available custom lists. It should look like the view with similar functionality in api access methods. Clicking on one of lists will take the user to an edit screen.

---

This change is 

[linear]

IOS-542 Create view with custom lists to edit

[buggmagnet]

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 147 at r1 (raw file):

                "CUSTOM_LIST_ACTION_SHEET_NEW_LIST",
                tableName: "CustomLists",
                value: "Add new list",

Shall we add a TODO comment for localisation purposes ?

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListInteractor.swift line 25 at r1 (raw file):

    func saveCustomList(viewModel: CustomListViewModel) throws {
        try _ = repository.save(list: viewModel.customList)

Did you mean try repository.save(list: viewModel.customList) ?

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 97 at r1 (raw file):

            "LIST_CUSTOM_LIST_NAVIGATION_TITLE",
            tableName: "CustomList",
            value: "Edit custom list",

Let's add / reuse localized strings here as well

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 101 at r1 (raw file):

        )

        navigationItem.rightBarButtonItem = UIBarButtonItem(

If I'm not mistaken, rightBarButtonItem has a strong reference to the UIBarButtonItem which means by extension, a strong reference to the closure passed to UIAction's handler and therefore, to self.

Long story short, I think we want to do the following instead

primaryAction: UIAction(handler: {[weak self] _ in
    self?.didFinish?()
})

[mojganii]

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

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 131 at r1 (raw file):

    }

    private func showCustomListActionSheet() {

nit: do really we need to make a function for this which is called once? IMO this can be placed into didRequestRouteToCustomLists

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 145 at r1 (raw file):

        actionSheet.addAction(UIAlertAction(
            title: NSLocalizedString(
                "CUSTOM_LIST_ACTION_SHEET_NEW_LIST",

it seems the sentence is not desprictive.

Code snippet:

  actionSheet.addAction(UIAlertAction(
            title: NSLocalizedString(
                "CUSTOM_LIST_ACTION_SHEET_ADD_NEW_LIST",
                tableName: "CustomLists",
                value: "Add new list",
                comment: ""
            ),
            style: .default,
            handler: { _ in
                self.showAddCustomList()
            }
        ))

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListInteractor.swift line 12 at r1 (raw file):

protocol CustomListInteractorProtocol {
    func fetchAllCustomLists() -> [CustomList]

CustomList keywords in function names are redundant .

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 101 at r1 (raw file):

Previously, buggmagnet wrote…

If I'm not mistaken, rightBarButtonItem has a strong reference to the UIBarButtonItem which means by extension, a strong reference to the closure passed to UIAction's handler and therefore, to self.

Long story short, I think we want to do the following instead

primaryAction: UIAction(handler: {[weak self] _ in
    self?.didFinish?()
})

+1

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 145 at r1 (raw file):

extension ListCustomListViewController: UITableViewDelegate {
    func tableView(_ tableView: UITableView, heightForRowAt indexPath: IndexPath) -> CGFloat {
        UIMetrics.SettingsCell.apiAccessCellHeight

it has noting to do with api access.let's table view use UITableView.automaticDimension or set hardcoded value.

[mojganii]

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

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 145 at r1 (raw file):

Previously, mojganii wrote…

it has noting to do with api access.let's table view use UITableView.automaticDimension or set hardcoded value.

moreover, if all cells have the same height I would suggest to do that through tableView.rowHeight = X instead of recalculating them every time by tableview

[rablador]

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

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 131 at r1 (raw file):

Previously, mojganii wrote…

nit: do really we need to make a function for this which is called once? IMO this can be placed into didRequestRouteToCustomLists

Done.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 145 at r1 (raw file):

Previously, mojganii wrote…

it seems the sentence is not desprictive.

Done.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 147 at r1 (raw file):

Previously, buggmagnet wrote…

Shall we add a TODO comment for localisation purposes ?

Not sure I follow.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListInteractor.swift line 12 at r1 (raw file):

Previously, mojganii wrote…

CustomList keywords in function names are redundant .

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListInteractor.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

Did you mean try repository.save(list: viewModel.customList) ?

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 97 at r1 (raw file):

Previously, buggmagnet wrote…

Let's add / reuse localized strings here as well

Resuse from where?

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 101 at r1 (raw file):

Previously, mojganii wrote…

+1

Done.

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListViewController.swift line 145 at r1 (raw file):

Previously, mojganii wrote…

moreover, if all cells have the same height I would suggest to do that through tableView.rowHeight = X instead of recalculating them every time by tableview

Done.

[rablador]

Reviewable status: 5 of 24 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadVPN/Coordinators/CustomLists/ListCustomListCoordinator.swift line 73 at r2 (raw file):

    }

    private func updateRelayConstraints(for action: EditCustomListCoordinator.FinishAction, in list: CustomList) {

Makes sure we update the relay constraints / relay selector whenever we make changes to a list we're currently using.

[buggmagnet]

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

---

ios/MullvadTypes/RelayConstraints.swift line 32 at r3 (raw file):

    // Added in 2024.1
    public var locations: RelayConstraint<UserSelectedRelays>

As it stands right now, if the user updates their application whilst being connected, it will force them into blocked mode because their currently selected relay constraints will not match anymore.

Can we prevent this from happening ?

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 147 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Not sure I follow.

Nevermind, I think I misread the original code

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 45 at r3 (raw file):

    deinit {
        print("deinit LocationCoordinator")

We probably want to remove this

---

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

    deinit {
        print("deinit ListCustomListCoordinator")

Let's remove this too

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 22 at r3 (raw file):

    }
  ],
  "version" : 3

This comes from Xcode 15.3 which we do not use yet in the CI, please revert this file with the following command
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/

[rablador]

Dismissed @buggmagnet from a discussion.
Reviewable status: 20 of 25 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadTypes/RelayConstraints.swift line 32 at r3 (raw file):

Previously, buggmagnet wrote…

As it stands right now, if the user updates their application whilst being connected, it will force them into blocked mode because their currently selected relay constraints will not match anymore.

Can we prevent this from happening ?

I'll do a migration for it.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 147 at r1 (raw file):

Previously, buggmagnet wrote…

Nevermind, I think I misread the original code

Ok, no change then.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 45 at r3 (raw file):

Previously, buggmagnet wrote…

We probably want to remove this

Done.

---

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

Previously, buggmagnet wrote…

Let's remove this too

Done.

---

ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved line 22 at r3 (raw file):

Previously, buggmagnet wrote…

This comes from Xcode 15.3 which we do not use yet in the CI, please revert this file with the following command
git checkout main -- MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/

Done.

[rablador]

Reviewable status: 20 of 25 files reviewed, 9 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadTypes/RelayLocation.swift line 134 at r4 (raw file):

}

/// - Warning: Deprecated, use UserSelectedRelays instead.

Opting not to use @available(*, deprecated) here since it will forever give us a deprecation warning when migrating from RelayLocations to UserSelectedLocations in RelayConstraints.

[buggmagnet]

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

[buggmagnet]

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

[mojganii]

when I delete a custom list which is selected as an active RelayConstarints the app doesn't go to block state.

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

[rablador]

Have you tested on device? It doesn't work on simulator.

Reviewable status: 24 of 26 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

[buggmagnet]

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

[mojganii]

In the real device it goes on blocked state.

Reviewable status: all files reviewed, 4 unresolved discussions