Investigate crash in DiffableDataSource

[mojganii]

This PR fixes the carsh we have on UITableViewDiffableDataSource.

---

This change is 

[pinkisemils]

Is there any way we can unit test concurrent updates? Spawn two queues, both thrashing their own updates to the data source?

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @mojganii)

---

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

import MullvadTypes
import UIKit

How did this file ever work without having these imports?
Also, are we using Combine here? I can't identify anything Combine specific in the changes here, and I was under the impression we were not using Combine anywhere in our code.

[rablador]

The crash seems to be fixed! I have found two other things though:

1. When first showing the location view, the selected location is not centered on screen.

2. When selecting items between custom list and all locations, the snapshot state is sometimes lost. I believe this is related to not letting the LocationNode tree be the one source of truth. When toggling and updating selection the snapshot keeps a working state that doesn't reflect the node tree. It may be possible to make it work this way, but I doubt it will be completely reliable. What keeps track of expanded status is the showsChildren attribute on the node, but right now we don't update expanded status when toggling cells visible, which is a miss/bug from the original implementation. We should fix this and explicitly reapply the expanded status on the whole data set when data is reloaded (when creating snapshot items somewhere in setRelays). Ie. we only look at the node tree for data/structure and recreate the whole snapshot based on that.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

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

Previously, pinkisemils (Emīls Piņķis) wrote…

How did this file ever work without having these imports?
Also, are we using Combine here? I can't identify anything Combine specific in the changes here, and I was under the impression we were not using Combine anywhere in our code.

They have been duplicated and a copy can be found a few lines below. This part should be removed.

[rablador]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

[buggmagnet]

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

[pinkisemils]

Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)

---

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

Previously, rablador (Jon Petersson) wrote…

They have been duplicated and a copy can be found a few lines below. This part should be removed.

Yeah, this duplication should be removed.

[mojganii]

@rablador it seems my recent changes will solve the problem you've mentioned above. just we need to add some more uint test for that, which I'll do that in a bit. let me know if you found any new issue with that.
for checking and simulating concurrency issue you can get help from this snippet code :

    let concurrentQueue = DispatchQueue(label: "LocationDataSource.concurrentQueue", attributes: .concurrent)

        let letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"

        let numberOfConcurrentTasks = 1000

        let maxWordLength = 100

        for _ in 0 ..< numberOfConcurrentTasks {

            concurrentQueue.async {

                let randomText = String((0 ..< maxWordLength).compactMap { _ in

                    letters.randomElement()!

                })

                if (0..<maxWordLength).randomElement()! % 2 == 0 {

                    self.dataSource?.setRelays(relaysWithLocation, selectedRelays: self.selectedRelays)

                } else {

                    self.dataSource?.filterRelays(by: randomText)

                }

            }

        }

Reviewable status: 1 of 9 files reviewed, 1 unresolved discussion (waiting on @buggmagnet, @pinkisemils, and @rablador)

---

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

Previously, pinkisemils (Emīls Piņķis) wrote…

Yeah, this duplication should be removed.

that was my mistake. it's removed.

[rablador]

Reviewed 1 of 6 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @pinkisemils)

---

ios/MullvadVPN/View controllers/SelectLocation/LocationDiffableDataSourceProtocol.swift line 42 at r4 (raw file):

    }

    func toggledItems(for cell: LocationCell, completion: (() -> Void)? = nil) {

Since this function no longer returns anything we should call it toggleItems instead.

---

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

        setUpSegmentedControl()
        addSubviews()
        entryLocationViewController.flatMap {

Although nifty we decided some time ago that we should not use flatMap in this way.

[rablador]

Reviewed 4 of 4 files at r5, all commit messages.
Dismissed @pinkisemils from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

[rablador]

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

[mojganii]

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

---

ios/MullvadVPN/View controllers/SelectLocation/LocationDiffableDataSourceProtocol.swift line 42 at r4 (raw file):

Previously, rablador (Jon Petersson) wrote…

Since this function no longer returns anything we should call it toggleItems instead.

Done.

---

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

Previously, rablador (Jon Petersson) wrote…

Although nifty we decided some time ago that we should not use flatMap in this way.

Done.

[mojganii]

@pinkisemils Unit testing for this will be addressed in a separate task. Here is the link: Testing Location DataSource for Stability

Reviewable status: 8 of 9 files reviewed, all discussions resolved (waiting on @rablador)

[rablador]

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

[buggmagnet]

Reviewed 5 of 8 files at r4, 3 of 4 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved