VPN menus improvements (#1979)

Task/Issue URL: https://app.asana.com/0/1203137811378537/1206144067554516/f ## Description Makes some improvements to our status menu and in-app menu for the VPN. More specifically: - The macOS status bar app now shows a contextual menu when right clicked that allows users to hide our menu app. - Our in-app VPN icon can now be hidden even when the VPN is connected.
duckduckgo · Dec 22, 2023 · 45c37be · 45c37be
1 parent ae8a4e0
commit 45c37be
Show file tree

Hide file tree

Showing 9 changed files with 180 additions and 62 deletions.
diff --git a/DuckDuckGo/Menus/MainMenu.swift b/DuckDuckGo/Menus/MainMenu.swift
@@ -519,14 +519,14 @@ import Subscription
     }
 
     private func updateShortcutMenuItems() {
-        toggleAutofillShortcutMenuItem.title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .autofill)
-        toggleBookmarksShortcutMenuItem.title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .bookmarks)
-        toggleDownloadsShortcutMenuItem.title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .downloads)
+        toggleAutofillShortcutMenuItem.title = LocalPinningManager.shared.shortcutTitle(for: .autofill)
+        toggleBookmarksShortcutMenuItem.title = LocalPinningManager.shared.shortcutTitle(for: .bookmarks)
+        toggleDownloadsShortcutMenuItem.title = LocalPinningManager.shared.shortcutTitle(for: .downloads)
 
 #if NETWORK_PROTECTION
         if NetworkProtectionKeychainTokenStore().isFeatureActivated {
             toggleNetworkProtectionShortcutMenuItem.isHidden = false
-            toggleNetworkProtectionShortcutMenuItem.title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .networkProtection)
+            toggleNetworkProtectionShortcutMenuItem.title = LocalPinningManager.shared.shortcutTitle(for: .networkProtection)
         } else {
             toggleNetworkProtectionShortcutMenuItem.isHidden = true
         }

diff --git a/DuckDuckGo/Menus/MainMenuActions.swift b/DuckDuckGo/Menus/MainMenuActions.swift
@@ -380,7 +380,9 @@ extension MainViewController {
     }
 
     @objc func toggleNetworkProtectionShortcut(_ sender: Any) {
+#if NETWORK_PROTECTION
         LocalPinningManager.shared.togglePinning(for: .networkProtection)
+#endif
     }
 
     // MARK: - History

diff --git a/DuckDuckGo/NavigationBar/PinningManager.swift b/DuckDuckGo/NavigationBar/PinningManager.swift
@@ -26,8 +26,11 @@ enum PinnableView: String {
     case autofill
     case bookmarks
     case downloads
-    case networkProtection
     case homeButton
+
+#if NETWORK_PROTECTION
+    case networkProtection
+#endif
 }
 
 protocol PinningManager {
@@ -37,6 +40,7 @@ protocol PinningManager {
     func wasManuallyToggled(_ view: PinnableView) -> Bool
     func pin(_ view: PinnableView)
     func unpin(_ view: PinnableView)
+    func shortcutTitle(for view: PinnableView) -> String
 }
 
 final class LocalPinningManager: PinningManager {
@@ -111,21 +115,15 @@ final class LocalPinningManager: PinningManager {
         return pinnedViewStrings.contains(view.rawValue)
     }
 
-    func toggleShortcutInterfaceTitle(for view: PinnableView) -> String {
+    func shortcutTitle(for view: PinnableView) -> String {
         switch view {
         case .autofill: return isPinned(.autofill) ? UserText.hideAutofillShortcut : UserText.showAutofillShortcut
         case .bookmarks: return isPinned(.bookmarks) ? UserText.hideBookmarksShortcut : UserText.showBookmarksShortcut
         case .downloads: return isPinned(.downloads) ? UserText.hideDownloadsShortcut : UserText.showDownloadsShortcut
         case .homeButton: return ""
-        case .networkProtection:
 #if NETWORK_PROTECTION
-            if !networkProtectionFeatureActivation.isFeatureActivated {
-                assertionFailure("Tried to toggle Network Protection when it was not activated")
-            }
-
+        case .networkProtection:
             return isPinned(.networkProtection) ? UserText.hideNetworkProtectionShortcut : UserText.showNetworkProtectionShortcut
-#else
-            fatalError("Tried to get Network Protection interface title when NetP was disabled")
 #endif
         }
     }

diff --git a/DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift b/DuckDuckGo/NavigationBar/View/NavigationBarViewController.swift
@@ -99,8 +99,6 @@ final class NavigationBarViewController: NSViewController {
     private var pinnedViewsNotificationCancellable: AnyCancellable?
     private var navigationButtonsCancellables = Set<AnyCancellable>()
     private var downloadsCancellables = Set<AnyCancellable>()
-    private var networkProtectionCancellable: AnyCancellable?
-    private var networkProtectionInterruptionCancellable: AnyCancellable?
     private var cancellables = Set<AnyCancellable>()
 
     @UserDefaultsWrapper(key: .homeButtonPosition, defaultValue: .right)
@@ -394,17 +392,9 @@ final class NavigationBarViewController: NSViewController {
                     self.updateDownloadsButton(updatingFromPinnedViewsNotification: true)
                 case .homeButton:
                     self.updateHomeButton()
-                case .networkProtection:
 #if NETWORK_PROTECTION
-                    guard NetworkProtectionKeychainTokenStore().isFeatureActivated else {
-                        LocalPinningManager.shared.unpin(.networkProtection)
-                        networkProtectionButtonModel.isPinned = false
-                        return
-                    }
-
-                    networkProtectionButtonModel.isPinned = LocalPinningManager.shared.isPinned(.networkProtection)
-#else
-                    assertionFailure("Tried to toggle NetP when the feature was disabled")
+                case .networkProtection:
+                    networkProtectionButtonModel.updateVisibility()
 #endif
                 }
             } else {
@@ -642,7 +632,7 @@ final class NavigationBarViewController: NSViewController {
 
     private func updatePasswordManagementButton() {
         let menu = NSMenu()
-        let title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .autofill)
+        let title = LocalPinningManager.shared.shortcutTitle(for: .autofill)
         menu.addItem(withTitle: title, action: #selector(toggleAutofillPanelPinning), keyEquivalent: "")
 
         passwordManagementButton.menu = menu
@@ -714,7 +704,7 @@ final class NavigationBarViewController: NSViewController {
 
     private func updateDownloadsButton(updatingFromPinnedViewsNotification: Bool = false) {
         let menu = NSMenu()
-        let title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .downloads)
+        let title = LocalPinningManager.shared.shortcutTitle(for: .downloads)
         menu.addItem(withTitle: title, action: #selector(toggleDownloadsPanelPinning(_:)), keyEquivalent: "")
 
         downloadsButton.menu = menu
@@ -779,7 +769,7 @@ final class NavigationBarViewController: NSViewController {
 
     private func updateBookmarksButton() {
         let menu = NSMenu()
-        let title = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .bookmarks)
+        let title = LocalPinningManager.shared.shortcutTitle(for: .bookmarks)
         menu.addItem(withTitle: title, action: #selector(toggleBookmarksPanelPinning(_:)), keyEquivalent: "")
 
         bookmarkListButton.menu = menu
@@ -877,20 +867,20 @@ extension NavigationBarViewController: NSMenuDelegate {
 
         HomeButtonMenuFactory.addToMenu(menu)
 
-        let autofillTitle = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .autofill)
+        let autofillTitle = LocalPinningManager.shared.shortcutTitle(for: .autofill)
         menu.addItem(withTitle: autofillTitle, action: #selector(toggleAutofillPanelPinning), keyEquivalent: "A")
 
-        let bookmarksTitle = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .bookmarks)
+        let bookmarksTitle = LocalPinningManager.shared.shortcutTitle(for: .bookmarks)
         menu.addItem(withTitle: bookmarksTitle, action: #selector(toggleBookmarksPanelPinning), keyEquivalent: "K")
 
-        let downloadsTitle = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .downloads)
+        let downloadsTitle = LocalPinningManager.shared.shortcutTitle(for: .downloads)
         menu.addItem(withTitle: downloadsTitle, action: #selector(toggleDownloadsPanelPinning), keyEquivalent: "J")
 
 #if NETWORK_PROTECTION
         let isPopUpWindow = view.window?.isPopUpWindow ?? false
 
         if !isPopUpWindow && networkProtectionFeatureActivation.isFeatureActivated {
-            let networkProtectionTitle = LocalPinningManager.shared.toggleShortcutInterfaceTitle(for: .networkProtection)
+            let networkProtectionTitle = LocalPinningManager.shared.shortcutTitle(for: .networkProtection)
             menu.addItem(withTitle: networkProtectionTitle, action: #selector(toggleNetworkProtectionPanelPinning), keyEquivalent: "N")
         }
 #endif
@@ -913,7 +903,9 @@ extension NavigationBarViewController: NSMenuDelegate {
 
     @objc
     private func toggleNetworkProtectionPanelPinning(_ sender: NSMenuItem) {
+#if NETWORK_PROTECTION
         LocalPinningManager.shared.togglePinning(for: .networkProtection)
+#endif
     }
 
     // MARK: - Network Protection
@@ -930,19 +922,38 @@ extension NavigationBarViewController: NSMenuDelegate {
         }
     }
 
+    /// Sets up the Network Protection button.
+    ///
+    /// This method should be run just once during the lifecycle of this view.
+    /// .
     private func setupNetworkProtectionButton() {
-        networkProtectionCancellable = networkProtectionButtonModel.$showButton
+        assert(networkProtectionButton.menu == nil)
+
+        let menuItem = NSMenuItem(title: LocalPinningManager.shared.shortcutTitle(for: .networkProtection), action: #selector(toggleNetworkProtectionPanelPinning), target: self)
+        let menu = NSMenu(items: [menuItem])
+        networkProtectionButton.menu = menu
+
+        networkProtectionButtonModel.$shortcutTitle
+            .receive(on: RunLoop.main)
+            .sink { title in
+                menuItem.title = title
+            }
+            .store(in: &cancellables)
+
+        networkProtectionButtonModel.$showButton
             .receive(on: RunLoop.main)
             .sink { [weak self] show in
                 let isPopUpWindow = self?.view.window?.isPopUpWindow ?? false
                 self?.networkProtectionButton.isHidden =  isPopUpWindow || !show
             }
+            .store(in: &cancellables)
 
-        networkProtectionInterruptionCancellable = networkProtectionButtonModel.$buttonImage
+        networkProtectionButtonModel.$buttonImage
             .receive(on: RunLoop.main)
             .sink { [weak self] image in
                 self?.networkProtectionButton.image = image
             }
+            .store(in: &cancellables)
     }
 #endif
 

diff --git a/...ckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift b/...ckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift
@@ -54,17 +54,20 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
     private let pinningManager: PinningManager
 
     @Published
-    private(set) var showButton = false
+    private(set) var showButton = false {
+        didSet {
+            shortcutTitle = pinningManager.shortcutTitle(for: .networkProtection)
+        }
+    }
+
+    @Published
+    private(set) var shortcutTitle: String
 
     @Published
     private(set) var buttonImage: NSImage?
 
     var isPinned: Bool {
-        didSet {
-            Task { @MainActor in
-                updateVisibility()
-            }
-        }
+        pinningManager.isPinned(.networkProtection)
     }
 
     // MARK: - NetP State
@@ -92,7 +95,7 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
         )
         self.iconPublisher = NetworkProtectionIconPublisher(statusReporter: networkProtectionStatusReporter, iconProvider: iconProvider)
         self.pinningManager = pinningManager
-        isPinned = pinningManager.isPinned(.networkProtection)
+        self.shortcutTitle = pinningManager.shortcutTitle(for: .networkProtection)
 
         isHavingConnectivityIssues = networkProtectionStatusReporter.connectivityIssuesObserver.recentValue
         buttonImage = .image(for: iconPublisher.icon)
@@ -190,7 +193,7 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
     }
 
     @MainActor
-    private func updateVisibility() {
+    func updateVisibility() {
         // The button is visible in the case where NetP has not been activated, but the user has been invited and they haven't accepted T&Cs.
         let networkProtectionVisibility = DefaultNetworkProtectionVisibility()
         if networkProtectionVisibility.isNetworkProtectionVisible() {
@@ -227,17 +230,17 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
                 return
             }
 
-            switch status {
-            case .connecting, .connected, .reasserting, .disconnecting:
-                showButton = true
-
-                pinNetworkProtectionToNavBarIfNeverPinnedBefore()
-            default:
-                showButton = false
-            }
+            showButton = false
         }
     }
 
+    // MARK: - Pinning
+
+    @objc
+    func togglePin() {
+        pinningManager.togglePinning(for: .networkProtection)
+    }
+
     /// We want to pin Network Protection to the navigation bar the first time it's enabled, and only
     /// if the user hasn't toggled it manually before.
     /// 

diff --git a/DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift b/DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift
@@ -133,7 +133,10 @@ final class DuckDuckGoVPNAppDelegate: NSObject, NSApplicationDelegate {
             OnboardingStatus(rawValue: rawValue) ?? .default
         }.eraseToAnyPublisher()
 
+        let model = StatusBarMenuModel(vpnSettings: .init(defaults: .netP))
+
         return StatusBarMenu(
+            model: model,
             onboardingStatusPublisher: onboardingStatusPublisher,
             statusReporter: statusReporter,
             controller: tunnelController,