Add filtering to location selection

[rablador]

---

This change is 

[linear]

IOS-180 Update relay list view to allow filtering by provider

Users should be able to select a subset of owners for which relay should be displayed in the relay list.

The UI design is available in the spec - https://mullvad.atlassian.net/l/c/76oiCwJA

[rablador]

Reviewable status: 0 of 38 files reviewed, 1 unresolved discussion

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 82 at r1 (raw file):

    }

    enum FilterView {

I am overdoing it here...?

[buggmagnet]

Reviewed 10 of 36 files at r1, 1 of 11 files at r2, all commit messages.
Reviewable status: 11 of 38 files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 82 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I am overdoing it here...?

No it's beautiful 💜

[buggmagnet]

Reviewable status: 11 of 38 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterCellFactory.swift line 63 at r2 (raw file):

        guard let cell = cell as? CheckableSettingsCell else { return }

        var title = ""

nit: This doesn't need to be a var if you do it the following way. The intent declaration is stronger IMHO as a let

        let title: String
        switch item {
        case .allProviders:
            title = "All providers"
            setFontWeight(.semibold, to: cell.titleLabel)
        case let .provider(name):
            title = name
            setFontWeight(.regular, to: cell.titleLabel)
        default:
            title = ""
            assertionFailure("Item mismatch. Got: \(item)")
        }

        cell.titleLabel.text = NSLocalizedString(
            "RELAY_FILTER_CELL_LABEL",
            tableName: "Relay filter provider cell",
            value: title,
            comment: ""
        )

[pronebird]

Reviewed 15 of 36 files at r1, 10 of 11 files at r2.
Reviewable status: 36 of 38 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/Coordinators/App/SelectLocationCoordinator.swift line 107 at r2 (raw file):

    }

    private func makeRelayFilterCoordinator(forModalPresentation isModalPresentation: Bool)

On desktop we push filter into navigation stack.

[pronebird]

Reviewed 1 of 36 files at r1.
Reviewable status: 37 of 38 files reviewed, 3 unresolved discussions (waiting on @rablador)

[pronebird]

Reviewed 11 of 36 files at r1, 1 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)

[buggmagnet]

Reviewed 10 of 36 files at r1, 7 of 11 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewController.swift line 104 at r2 (raw file):

    }

    @objc private func didTapApplyButton() {

nit: How about renaming applyFilter and renaming didApplyFilter to onApplyFilter ?

Here we haven't applied anything yet, we're just bubbling up the interaction, so I'm not super keen on calling this didApplyFilter

Doing this would make the call site look like this

        relayFilterViewController.onApplyFilter = { [weak self] filter in
            guard let self else { return }

            var relayConstraints = tunnelManager.settings.relayConstraints
            relayConstraints.filter = .only(filter) // Here we actually apply the filter

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 18 at r2 (raw file):

    var uniqueProviders: [String] {
        return Array(Set(relays.map { $0.provider })).caseInsensitiveSorted()

Set already conforms to Collection where you added the caseInsensitiveSorted method, so you don't need to wrap this in an Array

func uniqueProviders(_ relays: [Relay]) -> [String] {
    Set(relays.map { $0.provider }).caseInsensitiveSorted()
}

func uniqueProvidersWithArray(_ relays: [Relay]) -> [String] {
    Array(Set(relays.map { $0.provider })).caseInsensitiveSorted()
}

print(uniqueProviders(providers) == uniqueProvidersWithArray(providers)) // prints "true"

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 65 at r2 (raw file):

    }

    func getOwnership(for item: RelayFilterDataSource.Item?) -> RelayFilter.Ownership? {

We can remove all those get prefixes, they don't add anything meaningful to the method names

---

ios/MullvadVPN/Views/CheckboxView.swift line 32 at r2 (raw file):

    init() {
        isChecked = false

Is there any reason why we do this before the call to super.init ?
It looks like it's an innocent mistake, or a code smell, but it doesn't look right IMHO

---

ios/RelaySelector/RelaySelector.swift line 71 at r2 (raw file):

    /// Determines whether a `REST.ServerRelay` satisfies the given relay filter.
    public static func relayMatchesFilter(_ relay: REST.ServerRelay, filter: RelayFilter) -> Bool {

If you switch the checks around, it's much, much easier to understand, here's what it would look like

    public static func relayMatchesFilter(_ relay: REST.ServerRelay, filter: RelayFilter) -> Bool {
        if case let .only(providers) = filter.providers {
            if providers.contains(relay.provider) == false {
                return false
            }
        }

        switch filter.ownership {
        case .any:
            return true
        case .owned:
            return relay.owned
        case .rented:
            return relay.owned == false
        }
    }

[rablador]

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

---

ios/MullvadVPN/Coordinators/App/SelectLocationCoordinator.swift line 107 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

On desktop we push filter into navigation stack.

I know, but it doesn't really make sense to me. AFAIK navigation items (like items in a list etc) usually go horisontally while new flows start vertically. The select location vc is an example of this, popping up over the map. I can change it, but especially on mobile I really think it feels natural to have it as a modal.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterCellFactory.swift line 63 at r2 (raw file):

Previously, buggmagnet wrote…

nit: This doesn't need to be a var if you do it the following way. The intent declaration is stronger IMHO as a let

        let title: String
        switch item {
        case .allProviders:
            title = "All providers"
            setFontWeight(.semibold, to: cell.titleLabel)
        case let .provider(name):
            title = name
            setFontWeight(.regular, to: cell.titleLabel)
        default:
            title = ""
            assertionFailure("Item mismatch. Got: \(item)")
        }

        cell.titleLabel.text = NSLocalizedString(
            "RELAY_FILTER_CELL_LABEL",
            tableName: "Relay filter provider cell",
            value: title,
            comment: ""
        )

Done

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewController.swift line 104 at r2 (raw file):

Previously, buggmagnet wrote…

nit: How about renaming applyFilter and renaming didApplyFilter to onApplyFilter ?

Here we haven't applied anything yet, we're just bubbling up the interaction, so I'm not super keen on calling this didApplyFilter

Doing this would make the call site look like this

        relayFilterViewController.onApplyFilter = { [weak self] filter in
            guard let self else { return }

            var relayConstraints = tunnelManager.settings.relayConstraints
            relayConstraints.filter = .only(filter) // Here we actually apply the filter

From a UI standpoint the user did apply the filter, but I can also agree that it makes more sense with "on" from a system standpoint. Will change it.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 18 at r2 (raw file):

Previously, buggmagnet wrote…

Set already conforms to Collection where you added the caseInsensitiveSorted method, so you don't need to wrap this in an Array

func uniqueProviders(_ relays: [Relay]) -> [String] {
    Set(relays.map { $0.provider }).caseInsensitiveSorted()
}

func uniqueProvidersWithArray(_ relays: [Relay]) -> [String] {
    Array(Set(relays.map { $0.provider })).caseInsensitiveSorted()
}

print(uniqueProviders(providers) == uniqueProvidersWithArray(providers)) // prints "true"

Indeed!

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 65 at r2 (raw file):

Previously, buggmagnet wrote…

We can remove all those get prefixes, they don't add anything meaningful to the method names

I like them when part of function declarations (denoting an action), as opposed to variable declarations. @mojganii @pronebird, please chip in so that we can have consensus going forward.

---

ios/MullvadVPN/Views/CheckboxView.swift line 32 at r2 (raw file):

Previously, buggmagnet wrote…

Is there any reason why we do this before the call to super.init ?
It looks like it's an innocent mistake, or a code smell, but it doesn't look right IMHO

Compiler reasons: Property 'self.isChecked' not initialized at super.init call
Can give isChecked a default value if that's better.

---

ios/RelaySelector/RelaySelector.swift line 71 at r2 (raw file):

Previously, buggmagnet wrote…

If you switch the checks around, it's much, much easier to understand, here's what it would look like

    public static func relayMatchesFilter(_ relay: REST.ServerRelay, filter: RelayFilter) -> Bool {
        if case let .only(providers) = filter.providers {
            if providers.contains(relay.provider) == false {
                return false
            }
        }

        switch filter.ownership {
        case .any:
            return true
        case .owned:
            return relay.owned
        case .rented:
            return relay.owned == false
        }
    }

Done.

[buggmagnet]

Reviewable status: 33 of 38 files reviewed, 6 unresolved discussions (waiting on @mojganii and @pronebird)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 65 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I like them when part of function declarations (denoting an action), as opposed to variable declarations. @mojganii @pronebird, please chip in so that we can have consensus going forward.

It's part of the Swift API design guidelines
Namely "Every word should convey salient information at the use site"

Here the word get doesn't add anything. ownership(for: myItem) already states that calling this gives us the ownership for the item.

[rablador]

Reviewable status: 33 of 38 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pronebird)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 65 at r2 (raw file):

Previously, buggmagnet wrote…

It's part of the Swift API design guidelines
Namely "Every word should convey salient information at the use site"

Here the word get doesn't add anything. ownership(for: myItem) already states that calling this gives us the ownership for the item.

Fair enough. Will change.

[pronebird]

Reviewed 2 of 10 files at r10.
Reviewable status: 37 of 39 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

[pronebird]

Reviewed 1 of 17 files at r9, 2 of 10 files at r10.
Reviewable status: 37 of 39 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

[rablador]

Reviewable status: 37 of 39 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 100 at r10 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

See constants under UIMetrics.UITableView, seems like duplication.

Fixed. Rebase issue?

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 103 at r10 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I think this was moved under UIMetrics.SettingsCell.layoutMargins

Fixed. Rebase issue?

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 106 at r10 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

See if you can find a similar constant in UIMetrics.SettingsCell or perhaps define one in there

Fixed. Rebase issue?

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 117 at r10 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I think that was moved to UIMetrics.TableView

Fixed. Rebase issue?

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 132 at r10 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Was it also moved to UIMetrics.UITableView namespace?

Fixed. Rebase issue?

[pronebird]

Reviewed 1 of 2 files at r11, 3 of 3 files at r12, all commit messages.
Reviewable status: 39 of 40 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 123 at r12 (raw file):

        case .rented:
            return rentedProviders
        default:

Maybe use any instead of default, unless I miss something Ownership is an enum type with three variants right?

[pronebird]

Reviewed 1 of 2 files at r11.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

[rablador]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pronebird)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterViewModel.swift line 123 at r12 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Maybe use any instead of default, unless I miss something Ownership is an enum type with three variants right?

Indeed, updated.

[pronebird]

Reviewed 1 of 1 files at r13.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 2 of 36 files at r1, 1 of 19 files at r4, 13 of 24 files at r14, all commit messages.
Reviewable status: 31 of 42 files reviewed, 4 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/Extensions/UIBarButtonItem+Blocks.swift line 2 at r14 (raw file):

//
//  UIBarButtonItem+Blocks.swift

This file was deleted and shouldn't be here.

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 68 at r1 (raw file):

    }

    enum SettingsCell {

A lot of these deletions look like regressions, did you get a merge issue when rebasing from main ?
IMHO we should checkout the version from the current main and retroactively add what's missing for the work needed for the filters here.

---

ios/MullvadVPN/View controllers/Account/AccountContentView.swift line 84 at r4 (raw file):

        stackView.translatesAutoresizingMaskIntoConstraints = false
        stackView.axis = .vertical
        stackView.spacing = UIMetrics.TableView.sectionSpacing

Why are there changes here ? It seems unrelated to filtering location in the relay filter

---

ios/MullvadVPN/View controllers/DeviceList/DeviceManagementContentView.swift line 215 at r14 (raw file):

        }

        if animated {

Same as the previous comment, please remove those changes as they are not related to this PR

[buggmagnet]

It looks like there are many unrelated changes in this PR, likely due to a rebasing conflict.
I think we should extract the fundamental changes in this PR (all the new elements, such as the pill cells, the relay filter data source changes etc...), delete this branch and start again from a fresh main.

It's hard to figure out what changes are necessary in this PR, but it looks like a lot of them are regressions rather than changes.

Reviewable status: 31 of 42 files reviewed, 4 unresolved discussions (waiting on @pronebird and @rablador)

[rablador]

The rebase from main was huge due to this branch going stale. I have quickly looked through the full diff and it looks sane.

Reviewable status: 31 of 42 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/Extensions/UIBarButtonItem+Blocks.swift line 2 at r14 (raw file):

Previously, buggmagnet wrote…

This file was deleted and shouldn't be here.

Probably an old change due to rebase. I have deleted the file now.

---

ios/MullvadVPN/UI appearance/UIMetrics.swift line 68 at r1 (raw file):

Previously, buggmagnet wrote…

A lot of these deletions look like regressions, did you get a merge issue when rebasing from main ?
IMHO we should checkout the version from the current main and retroactively add what's missing for the work needed for the filters here.

UIMetrics had a lot of merge conflicts, so I cleaned up things a bit. This has led to some changes trickling down the code base, but nothing big or complicated.

---

ios/MullvadVPN/View controllers/Account/AccountContentView.swift line 84 at r4 (raw file):

Previously, buggmagnet wrote…

Why are there changes here ? It seems unrelated to filtering location in the relay filter

Probably an old change due to rebase. It's correct in its current form.

---

ios/MullvadVPN/View controllers/DeviceList/DeviceManagementContentView.swift line 215 at r14 (raw file):

Previously, buggmagnet wrote…

Same as the previous comment, please remove those changes as they are not related to this PR

Probably an old change due to rebase. It's correct in its current form.

[buggmagnet]

Reviewed 1 of 17 files at r9, 1 of 10 files at r10, 1 of 3 files at r12, 1 of 1 files at r13, 9 of 24 files at r14, 2 of 2 files at r15, 1 of 2 files at r16.
Reviewable status: 41 of 43 files reviewed, 4 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/Extensions/UIBarButtonItem+Blocks.swift line 2 at r14 (raw file):

Previously, rablador (Jon Petersson) wrote…

Probably an old change due to rebase. I have deleted the file now.

The file is still there

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSource.swift line 53 at r15 (raw file):

        tableView.delegate = self

        registerClasses()

We probably want to register those and create the data snapshots before we set ourselves to be the tableView.delegate

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSource.swift line 297 at r15 (raw file):

        if select {
            tableView?.selectRow(at: indexPath, animated: false, scrollPosition: .none)

We can remove the last parameter, it's the default value already

[pronebird]

Reviewed 12 of 24 files at r14, 2 of 2 files at r16.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/Coordinators/SelectLocationCoordinator.swift line 25 at r16 (raw file):

    }

    var presentationContext: UIViewController {

Nit: we have default implementation for Presenting when Presentable is implemented.

---

ios/MullvadVPN/Extensions/UIBarButtonItem+Blocks.swift line 2 at r14 (raw file):

Previously, buggmagnet wrote…

The file is still there

This file is absent in main. I believe we switched to using UIAction since migration to iOS 14.

[rablador]

Reviewable status: 36 of 43 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)

---

ios/MullvadVPN/Coordinators/SelectLocationCoordinator.swift line 25 at r16 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: we have default implementation for Presenting when Presentable is implemented.

Removed.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSource.swift line 53 at r15 (raw file):

Previously, buggmagnet wrote…

We probably want to register those and create the data snapshots before we set ourselves to be the tableView.delegate

Doesn't matter, but it might look more logical so I can change it.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSource.swift line 297 at r15 (raw file):

Previously, buggmagnet wrote…

We can remove the last parameter, it's the default value already

It's not optional.

---

ios/MullvadVPN/Extensions/UIBarButtonItem+Blocks.swift line 2 at r14 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This file is absent in main. I believe we switched to using UIAction since migration to iOS 14.

Weird, I deleted it. Well, deleted again.

[buggmagnet]

Reviewed 1 of 10 files at r10, 6 of 8 files at r17.
Reviewable status: 42 of 44 files reviewed, 4 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSource.swift line 297 at r15 (raw file):

Previously, rablador (Jon Petersson) wrote…

It's not optional.

Indeed !

[buggmagnet]

It feels really nice to be able to finally merge this !

Reviewable status: 42 of 44 files reviewed, 1 unresolved discussion (waiting on @pronebird)