TunnelsManager: get rid of index management

No need for premature optimization. There aren't that many tunnels most
of the time, and calling sort on a partially sorted array is fast.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

3 files changed, 33 insertions, 51 deletions

diff --git a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift
index cb63867..a73c057 100644
--- a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift
+++ b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift
@@ -11,6 +11,8 @@ class ErrorPresenter {
         // TunnelManagementError
         case TunnelManagementError.tunnelAlreadyExistsWithThatName:
             return ("Name already exists", "A tunnel with that name already exists")
+        case TunnelManagementError.tunnelInvalidName:
+            return ("Name already exists", "The tunnel name is invalid")
         case TunnelManagementError.vpnSystemErrorOnAddTunnel:
             return ("Unable to create tunnel", "Internal error")
         case TunnelManagementError.vpnSystemErrorOnModifyTunnel:
diff --git a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift
index 9515885..1ab7dd7 100644
--- a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift
+++ b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift
@@ -312,11 +312,11 @@ extension TunnelsListTableViewController: TunnelsManagerDelegate {
     func tunnelModified(at index: Int) {
         tableView.reloadRows(at: [IndexPath(row: index, section: 0)], with: .automatic)
     }
-
-    func tunnelsChanged() {
-        tableView.reloadData()
+    
+    func tunnelMoved(at oldIndex: Int, to newIndex: Int) {
+        tableView.moveRow(at: IndexPath(row: oldIndex, section: 0), to: IndexPath(row: newIndex, section: 0))
     }
-
+    
     func tunnelRemoved(at index: Int) {
         tableView.deleteRows(at: [IndexPath(row: index, section: 0)], with: .automatic)
     }
diff --git a/WireGuard/WireGuard/VPN/TunnelsManager.swift b/WireGuard/WireGuard/VPN/TunnelsManager.swift
index 4a43755..5a89201 100644
--- a/WireGuard/WireGuard/VPN/TunnelsManager.swift
+++ b/WireGuard/WireGuard/VPN/TunnelsManager.swift
@@ -8,7 +8,7 @@ import os.log
 protocol TunnelsManagerDelegate: class {
     func tunnelAdded(at: Int)
     func tunnelModified(at: Int)
-    func tunnelsChanged()
+    func tunnelMoved(at oldIndex: Int, to newIndex: Int)
     func tunnelRemoved(at: Int)
 }
 
@@ -23,6 +23,7 @@ enum TunnelActivationError: Error {
 
 enum TunnelManagementError: Error {
     case tunnelAlreadyExistsWithThatName
+    case tunnelInvalidName
     case vpnSystemErrorOnAddTunnel
     case vpnSystemErrorOnModifyTunnel
     case vpnSystemErrorOnRemoveTunnel
@@ -43,12 +44,10 @@ class TunnelsManager {
 
     init(tunnelProviders: [NETunnelProviderManager]) {
         var tunnelNames: Set<String> = []
-        var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0, index: 0) }
+        var tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0) }
         tunnels.sort { $0.name < $1.name }
         var currentTunnel: TunnelContainer? = nil
-        for i in 0 ..< tunnels.count {
-            let tunnel = tunnels[i]
-            tunnel.index = i
+        for tunnel in tunnels {
             tunnelNames.insert(tunnel.name)
             if (tunnel.status != .inactive) {
                 currentTunnel = tunnel
@@ -75,17 +74,12 @@ class TunnelsManager {
         return tunnelNames.contains(name)
     }
 
-    private func insertionIndexFor(tunnelName: String) -> Int {
-        // Wishlist: Use binary search instead
-        for i in 0 ..< tunnels.count {
-            if (tunnelName.lexicographicallyPrecedes(tunnels[i].name)) { return i }
-        }
-        return tunnels.count
-    }
-
     func add(tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelContainer?, TunnelManagementError?) -> Void) {
         let tunnelName = tunnelConfiguration.interface.name
-        assert(!tunnelName.isEmpty)
+        if tunnelName.isEmpty {
+            completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName)
+            return
+        }
 
         guard (!containsTunnel(named: tunnelName)) else {
             completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName)
@@ -106,14 +100,11 @@ class TunnelsManager {
                 return
             }
             if let s = self {
-                let index = s.insertionIndexFor(tunnelName: tunnelName)
-                let tunnel = TunnelContainer(tunnel: tunnelProviderManager, index: index)
-                for i in index ..< s.tunnels.count {
-                    s.tunnels[i].index = s.tunnels[i].index + 1
-                }
-                s.tunnels.insert(tunnel, at: index)
+                let tunnel = TunnelContainer(tunnel: tunnelProviderManager)
+                s.tunnels.append(tunnel)
+                s.tunnels.sort { $0.name < $1.name }
                 s.tunnelNames.insert(tunnel.name)
-                s.delegate?.tunnelAdded(at: index)
+                s.delegate?.tunnelAdded(at: s.tunnels.firstIndex(of: tunnel)!)
                 completionHandler(tunnel, nil)
             }
         }
@@ -139,7 +130,10 @@ class TunnelsManager {
 
     func modify(tunnel: TunnelContainer, with tunnelConfiguration: TunnelConfiguration, completionHandler: @escaping (TunnelManagementError?) -> Void) {
         let tunnelName = tunnelConfiguration.interface.name
-        assert(!tunnelName.isEmpty)
+        if tunnelName.isEmpty {
+            completionHandler(TunnelManagementError.tunnelAlreadyExistsWithThatName)
+            return
+        }
 
         isModifyingTunnel = true
 
@@ -167,22 +161,14 @@ class TunnelsManager {
             }
             if let s = self {
                 if (isNameChanged) {
-                    s.tunnels.remove(at: tunnel.index)
+                    let oldIndex = s.tunnels.firstIndex(of: tunnel)!
                     s.tunnelNames.remove(oldName!)
-                    for i in tunnel.index ..< s.tunnels.count {
-                        s.tunnels[i].index = s.tunnels[i].index - 1
-                    }
-                    let index = s.insertionIndexFor(tunnelName: tunnelName)
-                    tunnel.index = index
-                    for i in index ..< s.tunnels.count {
-                        s.tunnels[i].index = s.tunnels[i].index + 1
-                    }
-                    s.tunnels.insert(tunnel, at: index)
                     s.tunnelNames.insert(tunnel.name)
-                    s.delegate?.tunnelsChanged()
-                } else {
-                    s.delegate?.tunnelModified(at: tunnel.index)
+                    s.tunnels.sort { $0.name < $1.name }
+                    let newIndex = s.tunnels.firstIndex(of: tunnel)!
+                    s.delegate?.tunnelMoved(at: oldIndex, to: newIndex)
                 }
+                s.delegate?.tunnelModified(at: s.tunnels.firstIndex(of: tunnel)!)
 
                 if (tunnel.status == .active || tunnel.status == .activating || tunnel.status == .reasserting) {
                     // Turn off the tunnel, and then turn it back on, so the changes are made effective
@@ -196,8 +182,6 @@ class TunnelsManager {
 
     func remove(tunnel: TunnelContainer, completionHandler: @escaping (TunnelManagementError?) -> Void) {
         let tunnelProviderManager = tunnel.tunnelProvider
-        let tunnelIndex = tunnel.index
-        let tunnelName = tunnel.name
 
         isDeletingTunnel = true
 
@@ -209,12 +193,10 @@ class TunnelsManager {
                 return
             }
             if let s = self {
-                for i in ((tunnelIndex + 1) ..< s.tunnels.count) {
-                    s.tunnels[i].index = s.tunnels[i].index - 1
-                }
-                s.tunnels.remove(at: tunnelIndex)
-                s.tunnelNames.remove(tunnelName)
-                s.delegate?.tunnelRemoved(at: tunnelIndex)
+                let index = s.tunnels.firstIndex(of: tunnel)!
+                s.tunnels.remove(at: index)
+                s.tunnelNames.remove(tunnel.name)
+                s.delegate?.tunnelRemoved(at: index)
             }
             completionHandler(nil)
         }
@@ -246,7 +228,7 @@ class TunnelsManager {
             completionHandler(TunnelActivationError.attemptingDeactivationWhenTunnelIsInactive)
             return
         }
-        assert(tunnel.index == currentTunnel!.index)
+        assert(tunnel == currentTunnel!)
 
         tunnel.startDeactivation()
     }
@@ -293,17 +275,15 @@ class TunnelContainer: NSObject {
     @objc dynamic var status: TunnelStatus
 
     fileprivate let tunnelProvider: NETunnelProviderManager
-    fileprivate var index: Int
     fileprivate var statusObservationToken: AnyObject?
 
     private var dnsResolver: DNSResolver? = nil
 
-    init(tunnel: NETunnelProviderManager, index: Int) {
+    init(tunnel: NETunnelProviderManager) {
         self.name = tunnel.localizedDescription ?? "Unnamed"
         let status = TunnelStatus(from: tunnel.connection.status)
         self.status = status
         self.tunnelProvider = tunnel
-        self.index = index
         super.init()
         if (status != .inactive) {
             startObservingTunnelStatus()