Intercept back button when leaving an unsaved custom list

[rablador]

Due to the complexity of changes that can be applied to a custom list, it's best to have the users confirm their changes with a deliberate save action.

Thus, when a user is clicking to go back from editing a custom list and the list has not been saved yet, the user should be prompted if they want to discard their changes or to go back to editing.

We also need to take menu navigation (long press on back button) into consideration by enforcing the same behaviour when eg. the user tries to navigate directly from add locations view to the list of custom lists.

---

This change is 

[linear]

IOS-584 Intercept back button when leaving an unsaved custom list

[rablador]

Clarifications:
I opted to go with Combine and a subject to keep track of changes made to the custom list being added/edited. This constant tracking is necessary becuase the user can either pop the last view controller from the navigation controller by simply pressing back, OR pop to a specific view controller from anywhere by long pressing back. Passing custom list objects around and trying to account for this unpredictable behavior seemed less than ideal.

Reviewable status: 0 of 12 files reviewed, all discussions resolved

[mojganii]

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/Containers/Navigation/CustomNavigationController.swift line 12 at r1 (raw file):

/// Custom navigation controller that applies the custom appearance to itself.
class CustomNavigationController: InterceptibleNavigationController {

Is there any reason to use InterceptibleNavigationController solely during editing mode in a custom list, rather than inheriting CustomNavigationController from InterceptibleNavigationController?

Code snippet (i):

class InterceptibleNavigationController: CustomNavigationController {
    var shouldPopViewController: ((UIViewController) -> Bool)?
    var shouldPopToViewController: ((UIViewController) -> Bool)?

    // Called when popping the last view controller, eg. by pressing a nacvigation bar back button.
    override func popViewController(animated: Bool) -> UIViewController? {
        guard let viewController = viewControllers.last else { return nil }

        if shouldPopViewController?(viewController) == true {
            return super.popViewController(animated: animated)
        } else {
            return nil
        }
    }

    // Called when popping to a specific view controller, eg. by long pressing a nacvigation bar
    // back button (revealing a navigation menu) and selecting a destination view controller.
    override func popToViewController(_ viewController: UIViewController, animated: Bool) -> [UIViewController]? {
        if shouldPopToViewController?(viewController) == true {
            return super.popToViewController(viewController, animated: animated)
        } else {
            return nil
        }
    }
}

Code snippet (ii):

 private func showAddCustomList(nodes: [LocationNode]) {
        let coordinator = AddCustomListCoordinator(
            navigationController: CustomNavigationController(),
            interactor: CustomListInteractor(
                repository: customListRepository
            ),
            nodes: nodes
        )

        coordinator.didFinish = { [weak self] addCustomListCoordinator in
            addCustomListCoordinator.dismiss(animated: true)
            self?.locationViewController?.refreshCustomLists()
        }

        coordinator.start()
        presentChild(coordinator, animated: true)
    }

    private func showEditCustomLists(nodes: [LocationNode]) {
        let coordinator = ListCustomListCoordinator(
            navigationController: InterceptibleNavigationController(),
            interactor: CustomListInteractor(repository: customListRepository),
            tunnelManager: tunnelManager,
            nodes: nodes
        )

        coordinator.didFinish = { [weak self] listCustomListCoordinator in
            listCustomListCoordinator.dismiss(animated: true)
            self?.locationViewController?.refreshCustomLists()
        }

        coordinator.start()
        presentChild(coordinator, animated: true)

        coordinator.presentedViewController.presentationController?.delegate = self
    }

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 30 at r1 (raw file):

    private var validationErrors: Set<CustomListFieldValidationError> = []

    private var customListHasUnsavedChanges: Bool {

we can make it shorter.

Code snippet:

    private var customListHasUnsavedChanges: Bool {
        return interactor.fetchAll().first(where: { $0.id == subject.value.id }) != subject.value.customList
    }

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 109 at r1 (raw file):

    private func interceptNavigation(_ navigationController: InterceptibleNavigationController) {
        navigationController.shouldPopViewController = { [weak self] viewController in

When I took these steps at the custom list the SelectLocation doesn't refresh:
1- update the locations of a custom list and save it
3- the location list didn't get updated at Select location view

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 260 at r1 (raw file):

                    handler: {
                        // Reset subject/view model to no longer having unsaved changes.
                        if let list = self.interactor.fetchAll().first(where: { $0.id == self.subject.value.id }) {

nit : use flatMap seems much more sensiable.

Code snippet:

     // Reset subject/view model to no longer having unsaved changes.
                        self.interactor.fetchAll().first(where: { $0.id == self.subject.value.id }).flatMap({
                      
                                self.subject.value.update(with: $0)
                        })

[acb-mv]

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

---

ios/MullvadVPN/Classes/InterceptibleNavigationController.swift line 19 at r1 (raw file):

        guard let viewController = viewControllers.last else { return nil }

        if shouldPopViewController?(viewController) == true {

Firstly, the == true is redundant, and secondly, if shouldPopViewController is nil, this will be regarded as false. Did you by any chance mean if shouldPopViewController?(viewController) ?? true?

---

ios/MullvadVPN/Classes/InterceptibleNavigationController.swift line 29 at r1 (raw file):

    // back button (revealing a navigation menu) and selecting a destination view controller.
    override func popToViewController(_ viewController: UIViewController, animated: Bool) -> [UIViewController]? {
        if shouldPopToViewController?(viewController) == true {

See the comment for popViewController, in particular about what to do if the callbacks are nil. My instincts would suggest that if the instantiator neglected to provide one of these callbacks, they'd want it to be assumed to be true, giving unmodified behaviour.

[rablador]

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

---

ios/MullvadVPN/Classes/InterceptibleNavigationController.swift line 19 at r1 (raw file):

Previously, acb-mv wrote…

Firstly, the == true is redundant, and secondly, if shouldPopViewController is nil, this will be regarded as false. Did you by any chance mean if shouldPopViewController?(viewController) ?? true?

if shouldPopViewController?(viewController) { triggers a complaint from the compiler since an optional (in this case Bool?) cannot be used as boolean. By doing the above we can fall back on false if shouldPopViewController is nil as you mentioned, or true if it's not and indeed true.

You are right in the comment below though, we should probably evaluate to true if the closure is nil. I'll change it.

---

ios/MullvadVPN/Classes/InterceptibleNavigationController.swift line 29 at r1 (raw file):

Previously, acb-mv wrote…

See the comment for popViewController, in particular about what to do if the callbacks are nil. My instincts would suggest that if the instantiator neglected to provide one of these callbacks, they'd want it to be assumed to be true, giving unmodified behaviour.

See comment above.

---

ios/MullvadVPN/Containers/Navigation/CustomNavigationController.swift line 12 at r1 (raw file):

Previously, mojganii wrote…

Is there any reason to use InterceptibleNavigationController solely during editing mode in a custom list, rather than inheriting CustomNavigationController from InterceptibleNavigationController?

Yeah, it's a better idea to do it the other way around, thereby only using it in custom list edit mode. I'll change it.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 30 at r1 (raw file):

Previously, mojganii wrote…

we can make it shorter.

In my opinion that's also harder to read, so I'd rather keep it a little longer.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 109 at r1 (raw file):

Previously, mojganii wrote…

When I took these steps at the custom list the SelectLocation doesn't refresh:
1- update the locations of a custom list and save it
3- the location list didn't get updated at Select location view

That's strange. I'm testing it now and it gets updated as expected. Can you reproduce it easily?

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 260 at r1 (raw file):

Previously, mojganii wrote…

nit : use flatMap seems much more sensiable.

I agree that it's neat, but I believe the team agreed on not using flatmap this way. @buggmagnet @acb-mv ?

[buggmagnet]

Reviewed 12 of 12 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

---

ios/MullvadVPN/Classes/InterceptibleNavigationController.swift line 15 at r1 (raw file):

    var shouldPopToViewController: ((UIViewController) -> Bool)?

    // Called when popping the last view controller, eg. by pressing a nacvigation bar back button.

nit
navigation instead of nacvigation
Same for the function below

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 30 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

In my opinion that's also harder to read, so I'd rather keep it a little longer.

I agree with @rablador here, readability is better than having a one liner.
If I wanted to be nit picky, I'd say that this should be a function instead of a variable however.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 260 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I agree that it's neat, but I believe the team agreed on not using flatmap this way. @buggmagnet @acb-mv ?

What I don't like in @mojganii's suggestion is that $0 is used twice in a row, which makes it harder to understand what argument is passed where.

[mojganii]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 109 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

That's strange. I'm testing it now and it gets updated as expected. Can you reproduce it easily?

talked offline.

[rablador]

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

---

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

    }

    /// Refreshes the custom list section and keeps all modifications intact (selection and expanded states).

It's all so complex and convoluted... 😭

[mojganii]

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

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 30 at r4 (raw file):

    private var validationErrors: Set<CustomListFieldValidationError> = []

    private var persistedCustomList: CustomList? {

nit : you can eliminate return. moreover, customList name is enough, isn't it?

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 34 at r4 (raw file):

    }

    private var customListHasUnsavedChanges: Bool {

isn't hasUnsavedChanges good enough?

[rablador]

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

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 30 at r4 (raw file):

Previously, mojganii wrote…

nit : you can eliminate return. moreover, customList name is enough, isn't it?

I'd like to keep the full name to clarify that it comes from persisted storage rather than from the subject.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 34 at r4 (raw file):

Previously, mojganii wrote…

isn't hasUnsavedChanges good enough?

Done.

[buggmagnet]

Reviewed 3 of 6 files at r3, 3 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @acb-mv and @mojganii)

[acb-mv]

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

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift line 260 at r1 (raw file):

Previously, buggmagnet wrote…

What I don't like in @mojganii's suggestion is that $0 is used twice in a row, which makes it harder to understand what argument is passed where.

flatMap should be used for functional operations which return a value, rather than producing side effects. I'd rewrite @mojganii 's example as

Code snippet:

if let match = self.interactor.fetchAll().first(where: { $0.id == self.subject.value.id }) {
    subject.value.update(with: match)
}

---

ios/MullvadVPN/Classes/InterceptableNavigationController.swift line 11 at r6 (raw file):

import UIKit

class InterceptableNavigationController: CustomNavigationController {

IMHO, "Interceptible" is the correct spelling (technically)

[rablador]

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

---

ios/MullvadVPN/Classes/InterceptableNavigationController.swift line 11 at r6 (raw file):

Previously, acb-mv wrote…

IMHO, "Interceptible" is the correct spelling (technically)

Done.

[acb-mv]

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

[mojganii]

Reviewable status: all files reviewed, 3 unresolved discussions

[buggmagnet]

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

---

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

Previously, rablador (Jon Petersson) wrote…

It's all so complex and convoluted... 😭

🫂

[github-actions]

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