Show number of available relays in filter view

[mojganii]

This PR introduces the ability to show the number of relays after choosing filters. for having a better way to guide user when the filters are set but there is no match server, a message is shown to users as footer to realise with the current filter, there is no match relays then for having match relays the filter should be revised.

---

This change is 

[linear]

IOS-1067 Implementation: Improved filter view feedback

[rablador]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

---

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

extension RelayFilterViewModel {
    func getFilteredRelays(with filter: RelayFilter) -> [String] {

We should have a unit test for this.

---

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

                configuration: LocationSectionHeaderFooterView.Configuration(
                    name: LocationSection.allLocations.header,
                    style: LocationSectionHeaderFooterView.Style(

We could create a reusable style for this I think.

[rablador]

Reviewed 10 of 25 files at r6, all commit messages.
Reviewable status: 11 of 29 files reviewed, 7 unresolved discussions (waiting on @mojganii)

---

ios/MullvadREST/Relay/RelaysCandidates.swift line 9 at r6 (raw file):

//

public struct RelaysCandidates {

Nit: RelayCandidates without s is better I think.

---

ios/CHANGELOG.md line 26 at r6 (raw file):

## Unreleased
### Changed
- Improve the filter view to display the number of available servers based on selected criteria. 

Whitespace after sentence.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 50 at r6 (raw file):

        // If multihop is enabled, we should check if there's a DAITA related error when opening the location
        // view. If there is, help the user by showing the entry instead of the exit view.
//        var startContext: LocationViewControllerWrapper.MultihopContext = .exit

Forgot to remove?

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 72 at r6 (raw file):

        )

//        let locationViewControllerWrapper = LocationViewControllerWrapper(

Forgot to remove?

---

ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 12 at r6 (raw file):

import MullvadTypes
import Network

Unintentional whitespace deletion?

[mojganii]

Reviewable status: 6 of 29 files reviewed, 6 unresolved discussions (waiting on @rablador)

---

ios/CHANGELOG.md line 26 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Whitespace after sentence.

Done.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 50 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Forgot to remove?

Done.

---

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 72 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Forgot to remove?

Done.

---

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

Previously, rablador (Jon Petersson) wrote…

We could create a reusable style for this I think.

Done

---

ios/MullvadREST/Relay/RelayPicking/RelayPicking.swift line 12 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Unintentional whitespace deletion?

Done.

---

ios/MullvadREST/Relay/RelaysCandidates.swift line 9 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: RelayCandidates without s is better I think.

Done

[rablador]

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

---

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

                    let ownership = filter.ownership

                    Task {

Any specific reason to do this async?

[rablador]

Reviewable status: 30 of 32 files reviewed, 7 unresolved discussions (waiting on @mojganii)

---

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

                            for provider in viewModel.availableProviders(for: ownership) {
                                group.addTask {
                                    await self.viewModel.providerItem(for: provider)

This call will result in a file read from the relay cache for every provider in the list, every time the snapshot is updated (see RelaySelectorWrapper.prepareObfuscation). This is probably the cause for the performance issues we're seeing.

[rablador]

Reviewable status: 30 of 32 files reviewed, 7 unresolved discussions (waiting on @mojganii)

---

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

Previously, rablador (Jon Petersson) wrote…

This call will result in a file read from the relay cache for every provider in the list, every time the snapshot is updated (see RelaySelectorWrapper.prepareObfuscation). This is probably the cause for the performance issues we're seeing.

Same thing in updateSelection and handleCollapseProviders.

[mojganii]

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

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 10 at r6 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: Space after imports.

Done

---

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

Previously, rablador (Jon Petersson) wrote…

Drop first?

For the first time there are no cells to be shown since both sections are collapsed.

---

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

Previously, rablador (Jon Petersson) wrote…

Why only when expanding?

Since when we remove items from snapshot the animation is somehow flaky

---

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

Previously, rablador (Jon Petersson) wrote…

I think we should do the localization here rather when they're used. That way we don't risk missing a translation of the names are used in multiple places.

Done.

---

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

Previously, rablador (Jon Petersson) wrote…

Any specific reason to do this async?

I found a better solution, instead of doing expensive operations.

---

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

Previously, rablador (Jon Petersson) wrote…

Same thing in updateSelection and handleCollapseProviders.

That was totally out of mind. thank you for reminding!

---

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

Previously, rablador (Jon Petersson) wrote…

We should have a unit test for this.

Removed!

---

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

Previously, rablador (Jon Petersson) wrote…

Nit: self.relayFilter = settings.relayConstraints.filter.value ?? RelayFilter()?

Done

---

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

Previously, rablador (Jon Petersson) wrote…

I think we can map ownershipItem here directly, just like ownership above. Referring to an array could lead to future bugs if the order changes.

Done.

---

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

Previously, rablador (Jon Petersson) wrote…

Nit: self.relayFilter = settings.relayConstraints.filter.value ?? RelayFilter()?

done

[rablador]

Reviewed 12 of 12 files at r10, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions

---

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

        let exitCount = relayFilterResult.exitRelays.count
        let entryCount = relayFilterResult.entryRelays?.count ?? 0
        let isMultihopEnabled = settings.tunnelMultihopState.isEnabled || settings.daita.isAutomaticRouting

This isn't necessarily true. Automatic rounting can be active in settings without multihop being active.

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 66 at r10 (raw file):

    private func createTitleForAvailableServers(
        entryCount: Int,

entryCount and exitCount are not used.

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 75 at r10 (raw file):

        }

        let numberOfRelays = Set(relayFilterResult.entryRelays ?? []).union(relayFilterResult.exitRelays).count

Nit: let count = ((relayFilterResult.entryRelays ?? []) + relayFilterResult.exitRelays).count?

---

ios/MullvadREST/Relay/RelayWithLocation.swift line 63 at r10 (raw file):

}

extension RelayWithLocation: Hashable {

I think equatable would be sufficient here.

---

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

            .receive(on: DispatchQueue.main)
            .sink { [weak self] filter in
                guard let self = self else { return }

Nit: I don't think we need to add a guard here, calling self as optional has no negative implications.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSourceCells.swift line 1 at r10 (raw file):

//

This file seems to not exist in xcode project. Remove?

[mojganii]

Reviewable status: 32 of 34 files reviewed, 6 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/RelayWithLocation.swift line 63 at r10 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think equatable would be sufficient here.

Since I need to create a set of relays in FilterDescriptor for counting the number of available relays, It should be Hashable

---

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

Previously, rablador (Jon Petersson) wrote…

This isn't necessarily true. Automatic rounting can be active in settings without multihop being active.

But if one is true, it means we are multi-hopping under the hood. Therefore, whether it's multi-hopping directly or indirectly, we consider the number of available relays for entry to determine if the filter satisfies the constraint or not.

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 66 at r10 (raw file):

Previously, rablador (Jon Petersson) wrote…

entryCount and exitCount are not used.

Good catch.

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 75 at r10 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: let count = ((relayFilterResult.entryRelays ?? []) + relayFilterResult.exitRelays).count?

there is a possibility to have duplicate relays after concatenating.

---

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

Previously, rablador (Jon Petersson) wrote…

Nit: I don't think we need to add a guard here, calling self as optional has no negative implications.

updateDataSnapshot(filter:) relies on self.snapshot(), then you need to ensure self is fully retained before calling it.

---

ios/MullvadVPN/View controllers/RelayFilter/RelayFilterDataSourceCells.swift line 1 at r10 (raw file):

Previously, rablador (Jon Petersson) wrote…

This file seems to not exist in xcode project. Remove?

Fixed. xcode retained that.

[rablador]

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

---

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

Previously, mojganii wrote…

But if one is true, it means we are multi-hopping under the hood. Therefore, whether it's multi-hopping directly or indirectly, we consider the number of available relays for entry to determine if the filter satisfies the constraint or not.

Fair enough.

[buggmagnet]

Reviewed 1 of 6 files at r1, 1 of 7 files at r2, 1 of 9 files at r5, 8 of 25 files at r6, 8 of 13 files at r7, 2 of 5 files at r8, 9 of 12 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions

---

ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 72 at r11 (raw file):

        startContext: MultihopContext
    ) {
        self.selectedEntry = settings.relayConstraints.entryLocations.value

Not sure exactly how I got this bug anymore, but the folding of a given country looks really jarring here
RPReplay_Final1741341883.MP4

---

ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 126 at r11 (raw file):

    private func setRelaysWithLocation() {
        let emptyResult = LocationRelays(relays: [], locations: [:])
        let relaysCandidates = try? relaySelectorWrapper.findCandidates(tunnelSettings: self.settings)

No need for self

---

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

    init(tableView: UITableView, viewModel: RelayFilterViewModel) {
        self.tableView = tableView

We should probably disable multi touch input in that view
RPReplay_Final1741342281.MP4

---

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

            .receive(on: DispatchQueue.main)
            .sink { [weak self] filter in
                guard let self = self else { return }

No need to strongify self if it's deallocated at this point, since the UI will be torn down, updateDataSnaphot will do work for nothing.

We can just do self?.updateDataSnapshot(filter: filter) instead

---

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

        var snapshot = NSDiffableDataSourceSnapshot<Section, Item>()
        snapshot.appendSections(Section.allCases)
        apply(snapshot, animatingDifferences: false)

The animation when folding a data source doens't look animated

RPReplay_Final1741340123.MP4

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 15 at r11 (raw file):

    let settings: LatestTunnelSettings

    var isEnabled: Bool {

Please add tests for this logic
Given the multitude of possible outcomes for this, it looks like a perfect candidate to use the Swift testing framework instead, so we can test all outcomes in a single test using parametrized testing

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 60 at r11 (raw file):

    private func createTitleForAvailableServers() -> String {
        let displayNumber: (Int) -> String = { number in
            number > 100 ? "99+" : "\(number)"

It looks like this should be number >= 100

---

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

        // and serve as the primary source of truth for subsequent filtering operations.
        // Further filtering will be applied based on specific criteria such as `ownership` or `provider`.
        var copy = settings

Please add tests for this class

[buggmagnet]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii)

---

ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 72 at r11 (raw file):

Previously, buggmagnet wrote…

Not sure exactly how I got this bug anymore, but the folding of a given country looks really jarring here
RPReplay_Final1741341883.MP4

It seems to happen in certain scroll positions, I'm not sure why exactly.
Here's how to reproduce the issue :

• Create exactly 1 custom list with 0 relays inside it
• Enable Daita and Direct only
• Expand Swizterland and UK
• Scroll to the bottom most point of the Location View
• Toggle Sweden to reproduce the bug

[mojganii]

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

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 60 at r11 (raw file):

Previously, buggmagnet wrote…

It looks like this should be number >= 100

Done

---

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

Previously, buggmagnet wrote…

We should probably disable multi touch input in that view
RPReplay_Final1741342281.MP4

it's a good idea to prevent unintended interactions, such as selecting multiple cells simultaneously.

---

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

Previously, buggmagnet wrote…

No need to strongify self if it's deallocated at this point, since the UI will be torn down, updateDataSnaphot will do work for nothing.

We can just do self?.updateDataSnapshot(filter: filter) instead

it's discussed before. this ensures self doesn’t become nil while executing the closure. do we really want to skip the that when updating?

---

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

Previously, buggmagnet wrote…

The animation when folding a data source doens't look animated

RPReplay_Final1741340123.MP4

it's disabled since the animation is flakey for collapsing.

---

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

Previously, buggmagnet wrote…

Please add tests for this class

I'll do that.

---

ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 126 at r11 (raw file):

Previously, buggmagnet wrote…

No need for self

Done.

[buggmagnet]

Reviewed 1 of 9 files at r5, 2 of 2 files at r12, 2 of 4 files at r13, all commit messages.
Reviewable status: 32 of 34 files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 72 at r11 (raw file):

Previously, buggmagnet wrote…

It seems to happen in certain scroll positions, I'm not sure why exactly.
Here's how to reproduce the issue :

• Create exactly 1 custom list with 0 relays inside it
• Enable Daita and Direct only
• Expand Swizterland and UK
• Scroll to the bottom most point of the Location View
• Toggle Sweden to reproduce the bug

After discussing this offline, we can also reproduce the same behaviour on main therefore this is not an issue.

[mojganii]

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

---

ios/MullvadVPN/View controllers/RelayFilter/FilterDescriptor.swift line 15 at r11 (raw file):

Previously, buggmagnet wrote…

Please add tests for this logic
Given the multitude of possible outcomes for this, it looks like a perfect candidate to use the Swift testing framework instead, so we can test all outcomes in a single test using parametrized testing

Done.

---

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

Previously, mojganii wrote…

it's discussed before. this ensures self doesn’t become nil while executing the closure. do we really want to skip the that when updating?

Since just there is one line code in the block, I agree we can remove.

[buggmagnet]

Reviewed 10 of 17 files at r14, 3 of 3 files at r15, all commit messages.
Reviewable status: 35 of 39 files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/MullvadMockData/MullvadREST/ServerRelaysResponse+Stubs.swift line 13 at r15 (raw file):

import WireGuardKitTypes

public enum ServerRelaysResponseStubs {

I don't mind that we move this to MullvadMockData but in that case we should delete the other copy we have in MullvadVPNTests

---

ios/MullvadVPNTests/MullvadVPN/View controllers/Filter/FilterDescriptorTests.swift line 41 at r15 (raw file):

                true,
                true
            ),

We are missing the two following cases

(
    LatestTunnelSettings(daita: DAITASettings(daitaState: .on, directOnlyState: .off)),
    RelayCandidates(entryRelays: nil, exitRelays: createRelayWithLocation()),
    false,
    true
),
(
    LatestTunnelSettings(daita: DAITASettings(daitaState: .off, directOnlyState: .off)),
    RelayCandidates(entryRelays: createRelayWithLocation(), exitRelays: []),
    false,
    false
),

---

ios/MullvadVPNTests/MullvadVPN/View controllers/Filter/FilterDescriptorTests.swift line 48 at r15 (raw file):

        _ relayCandidates: RelayCandidates,
        _ expectedEnabledState: Bool,
        _ expectDescription: Bool

nit
This could be named expectationDescription

---

ios/MullvadVPNTests/MullvadVPN/View controllers/Filter/FilterDescriptorTests.swift line 50 at r15 (raw file):

        _ expectDescription: Bool
    ) {
        let exitRelays = FilterDescriptorTests.createRelayWithLocation()

⚠️ Initialization of immutable value 'exitRelays' was never used; consider replacing with assignment to '_' or removing it

[rablador]

Reviewed 1 of 4 files at r13, 13 of 17 files at r14, 1 of 3 files at r15, 5 of 5 files at r16, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

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

                        value: "DAITA-%@",
                        comment: "Format for DAITA provider description"
                    ), statusText

If statusText is always enabled we can skip the interpolation and simply go with DAITA-enabled.

[pinkisemils]

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

---

ios/CHANGELOG.md line 29 at r16 (raw file):

### Changed
- Improve the filter view to display the number of available servers based on selected criteria.

I love that we are updating the changelog now :)