From 1c3cbe02e5d8072f8acc664fa8ac2c89253e08df Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Sat, 22 Jul 2023 17:10:16 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 4 + Passepartout.xcodeproj/project.pbxproj | 8 +- Passepartout/App/Constants/Theme.swift | 1 - ...iable.swift => TunnelKit+Extensions.swift} | 39 +++- .../App/Views/EndpointView+OpenVPN.swift | 217 ++++++++---------- .../App/Views/ProviderLocationView.swift | 1 - Passepartout/App/en.lproj/Localizable.strings | 2 + .../Constants/SwiftGen+Strings.swift | 6 +- 8 files changed, 143 insertions(+), 135 deletions(-) rename Passepartout/App/Extensions/{TunnelKit+Identifiable.swift => TunnelKit+Extensions.swift} (59%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a20a4b38..729bafc6 100644 --- 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 diff --git a/Passepartout.xcodeproj/project.pbxproj b/Passepartout.xcodeproj/project.pbxproj index f51685e2..a0997c37 100644 --- 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 = ""; }; 0E2C171A27CB5A3A007E8488 /* GenericCreditsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GenericCreditsView.swift; sourceTree = ""; }; 0E2C172A27CB63F9007E8488 /* Reviewer.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Reviewer.swift; sourceTree = ""; }; - 0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Identifiable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+Identifiable.swift"; sourceTree = ""; }; + 0E2DE71B27DCCFE80067B9E1 /* TunnelKit+Extensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+Extensions.swift"; sourceTree = ""; }; 0E2DE71E27DCD0290067B9E1 /* TunnelKit+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TunnelKit+L10n.swift"; sourceTree = ""; }; 0E2DE72427DCDF550067B9E1 /* WireGuard+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WireGuard+L10n.swift"; sourceTree = ""; }; 0E34A2AF27CAA84500C73B67 /* OpenVPN+L10n.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "OpenVPN+L10n.swift"; sourceTree = ""; }; @@ -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 */, diff --git a/Passepartout/App/Constants/Theme.swift b/Passepartout/App/Constants/Theme.swift index e3f1fac3..49a5e12d 100644 --- 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) } diff --git a/Passepartout/App/Extensions/TunnelKit+Identifiable.swift b/Passepartout/App/Extensions/TunnelKit+Extensions.swift similarity index 59% rename from Passepartout/App/Extensions/TunnelKit+Identifiable.swift rename to Passepartout/App/Extensions/TunnelKit+Extensions.swift index fe0a7b12..e374eedc 100644 --- a/Passepartout/App/Extensions/TunnelKit+Identifiable.swift +++ b/Passepartout/App/Extensions/TunnelKit+Extensions.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: ":") diff --git a/Passepartout/App/Views/EndpointView+OpenVPN.swift b/Passepartout/App/Views/EndpointView+OpenVPN.swift index 7e354317..94faf1ad 100644 --- 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 { +private extension EndpointView.OpenVPNView { + func isExpandedBinding(address: String) -> Binding { .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 } } } diff --git a/Passepartout/App/Views/ProviderLocationView.swift b/Passepartout/App/Views/ProviderLocationView.swift index ef9647bb..f4440c8b 100644 --- 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 { diff --git a/Passepartout/App/en.lproj/Localizable.strings b/Passepartout/App/en.lproj/Localizable.strings index 852599ce..39dcd534 100644 --- 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"; diff --git a/Passepartout/AppShared/Constants/SwiftGen+Strings.swift b/Passepartout/AppShared/Constants/SwiftGen+Strings.swift index 3fa674b9..8080652c 100644 --- 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 {