Improve OpenVPN Endpoint UX (#332)

- Make selection linear by address - Do not hide endpoints when automatic, show disabled - Suggest manual endpoint required - Pre-expand selected endpoint address - Do not dismiss on selection, because selected value is not visible in ProfileView
2023-07-22 17:10:16 +02:00 · 2023-07-22 17:10:16 +02:00 · 1c3cbe02e5
parent 0734816c05
commit 1c3cbe02e5
8 changed files with 143 additions and 135 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased

+### Changed
+
+- OpenVPN: Endpoint UX. [#332](https://github.com/passepartoutvpn/passepartout-apple/pull/332)
+
 ## 2.1.2 (2023-07-06)

 ### Fixed
--- a/Passepartout.xcodeproj/project.pbxproj
+++ b/Passepartout.xcodeproj/project.pbxproj
@ -41,7 +41,7 @@
 		0E2AC24522EC3AC10037B4B0 /* Settings.bundle in Resources */ = {isa = PBXBuildFile; fileRef = 0E2AC24422EC3AC10037B4B0 /* Settings.bundle */; };
 		0E2C171B27CB5A3B007E8488 /* GenericCreditsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2C171A27CB5A3A007E8488 /* GenericCreditsView.swift */; };
 		0E2C172B27CB63F9007E8488 /* Reviewer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2C172A27CB63F9007E8488 /* Reviewer.swift */; };
-		0E2DE71C27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift */; };
+		0E2DE71C27DCCFE80067B9E1 /* TunnelKit+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Extensions.swift */; };
 		0E2DE71F27DCD0290067B9E1 /* TunnelKit+L10n.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2DE71E27DCD0290067B9E1 /* TunnelKit+L10n.swift */; };
 		0E2DE72527DCDF550067B9E1 /* WireGuard+L10n.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E2DE72427DCDF550067B9E1 /* WireGuard+L10n.swift */; };
 		0E34A2B627CAA8CC00C73B67 /* Core+L10n.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0E34A2B527CAA8CC00C73B67 /* Core+L10n.swift */; };
@ -330,7 +330,7 @@
 		0E2AC24422EC3AC10037B4B0 /* Settings.bundle */ = {isa = PBXFileReference; lastKnownFileType = "wrapper.plug-in"; path = Settings.bundle; sourceTree = "<group>"; };
 		0E2C171A27CB5A3A007E8488 /* GenericCreditsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GenericCreditsView.swift; sourceTree = "<group>"; };
 		0E2C172A27CB63F9007E8488 /* Reviewer.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Reviewer.swift; sourceTree = "<group>"; };
-		0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+Identifiable.swift"; sourceTree = "<group>"; };
+		0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Extensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+Extensions.swift"; sourceTree = "<group>"; };
 		0E2DE71E27DCD0290067B9E1 /* TunnelKit+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+L10n.swift"; sourceTree = "<group>"; };
 		0E2DE72427DCDF550067B9E1 /* WireGuard+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WireGuard+L10n.swift"; sourceTree = "<group>"; };
 		0E34A2AF27CAA84500C73B67 /* OpenVPN+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "OpenVPN+L10n.swift"; sourceTree = "<group>"; };
@ -734,7 +734,7 @@
 				0EBC075C27EC529000208AD9 /* DebugLog+Constants.swift */,
 				0EB17EB927D2560300D473B5 /* PassepartoutProviders+Extensions.swift */,
 				0E35C099280E95BB0071FA35 /* ProviderProfileAvailability.swift */,
-				0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift */,
+				0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Extensions.swift */,
 				0EE8B7E227FF340F00B68621 /* VPNProtocolType+FileExtensions.swift */,
 			);
 			path = Extensions;
@ -1425,7 +1425,7 @@
 				0E34AC7827F840890042F2AB /* OrganizerView+Scene.swift in Sources */,
 				0E0BD27927B2EBE500583AC5 /* ShortcutsView.swift in Sources */,
 				0E92D7C627F103300033CB7B /* ProfileView+Configuration.swift in Sources */,
-				0E2DE71C27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift in Sources */,
+				0E2DE71C27DCCFE80067B9E1 /* TunnelKit+Extensions.swift in Sources */,
 				0ED1D6DE27DBA42100983466 /* DiagnosticsView+WireGuard.swift in Sources */,
 				0EF2213127E674BD001D0BD7 /* AddProviderViewModel.swift in Sources */,
 				0E90DFE627BACC1500EF5078 /* AddHostViewModel.swift in Sources */,
--- a/Passepartout/App/Constants/Theme.swift
+++ b/Passepartout/App/Constants/Theme.swift
@ -344,7 +344,6 @@ extension View {
        foregroundColor(themeLightTextColor)
    }

-    @available(iOS 15, *)
    func themePrimaryTintStyle() -> some View {
        tint(themePrimaryBackgroundColor)
    }
--- a/Passepartout/App/Extensions/TunnelKit+Identifiable.swift
+++ b/Passepartout/App/Extensions/TunnelKit+Identifiable.swift
@ -1,5 +1,5 @@
 //
-//  TunnelKit+Identifiable.swift
+//  TunnelKit+Extensions.swift
 //  Passepartout
 //
 //  Created by Davide De Rosa on 3/12/22.
@ -32,6 +32,43 @@ extension Endpoint: Identifiable {
    }
 }

+extension Endpoint: Comparable {
+    public static func < (lhs: Self, rhs: Self) -> Bool {
+        guard lhs.address != rhs.address else {
+            return lhs.proto < rhs.proto
+        }
+        guard lhs.isHostname == rhs.isHostname else {
+            return lhs.isHostname
+        }
+        return lhs.address < rhs.address
+    }
+}
+
+extension Endpoint: Hashable {
+}
+
+extension EndpointProtocol: Comparable {
+    public static func < (lhs: Self, rhs: Self) -> Bool {
+        guard lhs.socketType != rhs.socketType else {
+            return lhs.port < rhs.port
+        }
+        return lhs.socketType.orderValue < rhs.socketType.orderValue
+    }
+}
+
+private extension SocketType {
+    var orderValue: Int {
+        switch self {
+        case .udp: return 1
+        case .udp4: return 2
+        case .udp6: return 3
+        case .tcp: return 4
+        case .tcp4: return 5
+        case .tcp6: return 6
+        }
+    }
+}
+
 extension IPv4Settings.Route: Identifiable {
    public var id: String {
        [destination, mask, gateway ?? "*"].joined(separator: ":")
--- a/Passepartout/App/Views/EndpointView+OpenVPN.swift
+++ b/Passepartout/App/Views/EndpointView+OpenVPN.swift
@ -29,23 +29,17 @@ import TunnelKitOpenVPN

 extension EndpointView {
    struct OpenVPNView: View {
-        @Environment(\.presentationMode) private var presentationMode
-
        @ObservedObject private var providerManager: ProviderManager

        @ObservedObject private var currentProfile: ObservableProfile

        @Binding private var builder: OpenVPN.ConfigurationBuilder

-        @Binding private var customEndpoint: Endpoint?
-
        @State private var isFirstAppearance = true

        @State private var isAutomatic = false

-        @State private var selectedSocketType: SocketType = .udp
-
-        @State private var selectedPort: UInt16 = 0
+        @State private var isExpanded: [String: Bool] = [:]

        init(currentProfile: ObservableProfile) {
            let providerManager: ProviderManager = .shared
@ -54,28 +48,21 @@ extension EndpointView {
            self.currentProfile = currentProfile

            _builder = currentProfile.builderBinding(providerManager: providerManager)
-            _customEndpoint = currentProfile.customEndpointBinding
        }

        var body: some View {
            ScrollViewReader { scrollProxy in
                List {
                    mainSection
-                    if !isAutomatic {
-                        filtersSection
-                        addressesSection
-                    }
+                    endpointsSections
                    advancedSection
                }.onAppear {
-                    scrollToCustomEndpoint(scrollProxy)
-                    preselectFilters(once: true)
-                }.onChange(of: isAutomatic, perform: onToggleAutomatic)
-                .onChange(of: selectedSocketType, perform: preselectPort)
-                .onChange(of: customEndpoint) { _ in
-                    withAnimation {
-                        preselectFilters(once: false)
+                    isAutomatic = (currentProfile.value.customEndpoint == nil)
+                    if let customEndpoint = currentProfile.value.customEndpoint {
+                        isExpanded[customEndpoint.address] = true
                    }
-                }
+                    scrollToCustomEndpoint(scrollProxy)
+                }.onChange(of: isAutomatic, perform: onToggleAutomatic)
            }.navigationTitle(L10n.Global.Strings.endpoint)
        }
    }
@ -87,36 +74,41 @@ private extension EndpointView.OpenVPNView {
    var mainSection: some View {
        Section {
            Toggle(L10n.Global.Strings.automatic, isOn: $isAutomatic.themeAnimation())
+        } footer: {
+            // FIXME: l10n
+            themeErrorMessage(isManualEndpointRequired ? L10n.Endpoint.Errors.endpointRequired : nil)
        }
    }

-    var filtersSection: some View {
-        Section {
-            themeTextPicker(
-                L10n.Global.Strings.protocol,
-                selection: $selectedSocketType,
-                values: availableSocketTypes,
-                description: \.rawValue
-            )
-            themeTextPicker(
-                L10n.Global.Strings.port,
-                selection: $selectedPort,
-                values: allPorts(forSocketType: selectedSocketType),
-                description: \.description
-            )
-        }
+    var endpointsSections: some View {
+        ForEach(endpointsByAddress, content: endpointsGroup(forSection:))
+            .disabled(isAutomatic)
    }

-    var addressesSection: some View {
+    // TODO: OpenVPN, make endpoints editable
+    func endpointsGroup(forSection section: EndpointsByAddress) -> some View {
        Section {
-            filteredRemotes.map {
-                ForEach($0, content: button(forEndpoint:))
+            DisclosureGroup(isExpanded: isExpandedBinding(address: section.address)) {
+                ForEach(section.endpoints) {
+                    row(forEndpoint: $0)
+                }
+            } label: {
+                Text(L10n.Global.Strings.address)
+                    .withTrailingText(section.address)
            }
-        } header: {
-            Text(L10n.Global.Strings.addresses)
        }
    }

+    func row(forEndpoint endpoint: Endpoint) -> some View {
+        Button {
+            withAnimation {
+                currentProfile.value.customEndpoint = endpoint
+            }
+        } label: {
+            Text(endpoint.proto.rawValue)
+        }.withTrailingCheckmark(when: currentProfile.value.customEndpoint == endpoint)
+    }
+
    var advancedSection: some View {
        Section {
            let caption = L10n.Endpoint.Advanced.title
@ -130,56 +122,24 @@ private extension EndpointView.OpenVPNView {
        }
    }

-    func button(forEndpoint endpoint: Endpoint?) -> some View {
-        Button {
-            customEndpoint = endpoint
-            presentationMode.wrappedValue.dismiss()
-        } label: {
-            text(forEndpoint: endpoint)
-        }.withTrailingCheckmark(when: customEndpoint == endpoint)
-    }
-
-    func text(forEndpoint endpoint: Endpoint?) -> some View {
-        Text(endpoint?.address ?? L10n.Global.Strings.automatic)
-            .themeLongTextStyle()
-    }
-
-    var availableSocketTypes: [SocketType] {
-        guard let remotes = builder.remotes else {
+    var endpointsByAddress: [EndpointsByAddress] {
+        guard let remotes = builder.remotes, !remotes.isEmpty else {
            return []
        }
-        let allTypes: [SocketType] = [
-            SocketType.udp,
-            SocketType.tcp,
-            SocketType.udp4,
-            SocketType.tcp4
-        ]
-        var availableTypes: [SocketType] = []
-        allTypes.forEach { socketType in
-            guard remotes.contains(where: {
-                $0.proto.socketType == socketType
-            }) else {
+        var uniqueAddresses: [String] = []
+        remotes.forEach {
+            guard !uniqueAddresses.contains($0.address) else {
                return
            }
-            availableTypes.append(socketType)
+            uniqueAddresses.append($0.address)
+        }
+        return uniqueAddresses.map {
+            EndpointsByAddress(address: $0, remotes: remotes)
        }
-        return availableTypes
    }

-    func allPorts(forSocketType socketType: SocketType) -> [UInt16] {
-        guard let remotes = builder.remotes else {
-            return []
-        }
-        let allPorts = Set(remotes.filter {
-            $0.proto.socketType == socketType
-        }.map(\.proto.port))
-        return Array(allPorts).sorted()
-    }
-
-    var filteredRemotes: [Endpoint]? {
-        builder.remotes?.filter {
-            $0.proto.socketType == selectedSocketType && $0.proto.port == selectedPort
-        }
+    var isManualEndpointRequired: Bool {
+        !isAutomatic && currentProfile.value.customEndpoint == nil
    }

    var isConfigurationReadonly: Bool {
@ -187,53 +147,43 @@ private extension EndpointView.OpenVPNView {
    }
 }

+private struct EndpointsByAddress: Identifiable {
+    let address: String
+
+    let endpoints: [Endpoint]
+
+    init(address: String, remotes: [Endpoint]?) {
+        self.address = address
+        endpoints = remotes?.filter {
+            $0.address == address
+        }.sorted() ?? []
+    }
+
+    // MARK: Identifiable
+
+    var id: String {
+        address
+    }
+}
+
 // MARK: -

 private extension EndpointView.OpenVPNView {
    func onToggleAutomatic(_ value: Bool) {
-        if value {
-            guard customEndpoint != nil else {
-                return
-            }
-            customEndpoint = nil
-        }
-    }
-
-    func preselectFilters(once: Bool) {
-        guard !once || isFirstAppearance else {
+        guard value else {
            return
        }
-        isFirstAppearance = false
-
-        if let customEndpoint = customEndpoint {
-            isAutomatic = false
-            selectedSocketType = customEndpoint.proto.socketType
-            selectedPort = customEndpoint.proto.port
-        } else {
-            isAutomatic = true
-            guard let socketType = availableSocketTypes.first else {
-                assertionFailure("No socket types, empty remotes?")
-                return
-            }
-            selectedSocketType = socketType
-            preselectPort(forSocketType: socketType)
-        }
-    }
-
-    func preselectPort(forSocketType socketType: SocketType) {
-        let supported = allPorts(forSocketType: socketType)
-        guard !supported.contains(selectedPort) else {
+        guard currentProfile.value.customEndpoint != nil else {
            return
        }
-        guard let port = supported.first else {
-            assertionFailure("No ports, empty remotes?")
-            return
+        withAnimation {
+            currentProfile.value.customEndpoint = nil
+            isExpanded.removeAll()
        }
-        selectedPort = port
    }

    func scrollToCustomEndpoint(_ proxy: ScrollViewProxy) {
-        proxy.maybeScrollTo(customEndpoint?.id)
+        proxy.maybeScrollTo(currentProfile.value.customEndpoint?.id)
    }
 }

@ -274,19 +224,32 @@ private extension ObservableProfile {
            }
        }
    }
+}

-    var customEndpointBinding: Binding<Endpoint?> {
+private extension EndpointView.OpenVPNView {
+    func isExpandedBinding(address: String) -> Binding<Bool> {
        .init {
-            if self.value.isProvider {
-                return self.value.providerCustomEndpoint
-            } else {
-                return self.value.hostOpenVPNSettings?.customEndpoint
-            }
+            isExpanded[address] ?? false
        } set: {
-            if self.value.isProvider {
-                self.value.providerCustomEndpoint = $0
+            isExpanded[address] = $0
+        }
+    }
+}
+
+private extension Profile {
+    var customEndpoint: Endpoint? {
+        get {
+            if isProvider {
+                return providerCustomEndpoint
            } else {
-                self.value.hostOpenVPNSettings?.customEndpoint = $0
+                return hostOpenVPNSettings?.customEndpoint
+            }
+        }
+        set {
+            if isProvider {
+                providerCustomEndpoint = newValue
+            } else {
+                hostOpenVPNSettings?.customEndpoint = newValue
            }
        }
    }
--- a/Passepartout/App/Views/ProviderLocationView.swift
+++ b/Passepartout/App/Views/ProviderLocationView.swift
@ -216,7 +216,6 @@ private extension ProviderLocationView {
        }
    }

-    @available(iOS 15, *)
    func favoriteActions(_ location: ProviderLocation) -> some View {
        Button {
            withAnimation {
--- a/Passepartout/App/en.lproj/Localizable.strings
+++ b/Passepartout/App/en.lproj/Localizable.strings
@ -203,6 +203,8 @@

 /* MARK: ProfileView -> EndpointView */

+"endpoint.errors.endpoint_required" = "Please select an endpoint";
+
 "endpoint.wireguard.items.peer.caption" = "Peer";
 "endpoint.wireguard.items.preshared_key.caption" = "Preshared key";
 "endpoint.wireguard.items.allowed_ip.caption" = "Allowed IP";
--- a/Passepartout/AppShared/Constants/SwiftGen+Strings.swift
+++ b/Passepartout/AppShared/Constants/SwiftGen+Strings.swift
@ -403,6 +403,10 @@ internal enum L10n {
        }
      }
    }
+    internal enum Errors {
+      /// MARK: ProfileView -> EndpointView
+      internal static let endpointRequired = L10n.tr("Localizable", "endpoint.errors.endpoint_required", fallback: "Please select an endpoint")
+    }
    internal enum Wireguard {
      internal enum Items {
        internal enum AllowedIp {
@ -410,7 +414,7 @@ internal enum L10n {
          internal static let caption = L10n.tr("Localizable", "endpoint.wireguard.items.allowed_ip.caption", fallback: "Allowed IP")
        }
        internal enum Peer {
-          /// MARK: ProfileView -> EndpointView
+          /// Peer
          internal static let caption = L10n.tr("Localizable", "endpoint.wireguard.items.peer.caption", fallback: "Peer")
        }
        internal enum PresharedKey {