From 45c37be1e1087f36f855c38b1efe01296018c3ae Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 22 Dec 2023 12:37:39 +0100 Subject: [PATCH] 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/Menus/MainMenu.swift | 8 +-- DuckDuckGo/Menus/MainMenuActions.swift | 2 + DuckDuckGo/NavigationBar/PinningManager.swift | 16 ++--- .../View/NavigationBarViewController.swift | 53 ++++++++------ .../NetworkProtectionNavBarButtonModel.swift | 35 ++++----- DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift | 3 + ...tatusBarMenu.swift => StatusBarMenu.swift} | 71 +++++++++++++++---- .../Menu/StatusBarMenuModel.swift | 46 ++++++++++++ .../NetworkProtectionStatusBarMenuTests.swift | 8 +++ 9 files changed, 180 insertions(+), 62 deletions(-) rename LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/{NetworkProtectionStatusBarMenu.swift => StatusBarMenu.swift} (63%) create mode 100644 LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenuModel.swift diff --git a/DuckDuckGo/Menus/MainMenu.swift b/DuckDuckGo/Menus/MainMenu.swift index 9c3fdd9ac3..574076a5fa 100644 --- 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 index e07e537eea..88d6e141bd 100644 --- 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 index 3824e04bab..64d29c104e 100644 --- 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 index 9b6225e63f..86dcf60fad 100644 --- 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() private var downloadsCancellables = Set() - private var networkProtectionCancellable: AnyCancellable? - private var networkProtectionInterruptionCancellable: AnyCancellable? private var cancellables = Set() @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/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift b/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift index 3c9f1fda50..0d85b52574 100644 --- a/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift +++ b/DuckDuckGo/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 index 71f37ea3ba..3654968a57 100644 --- 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, diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/NetworkProtectionStatusBarMenu.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenu.swift similarity index 63% rename from LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/NetworkProtectionStatusBarMenu.swift rename to LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenu.swift index 890d1bee8f..7384586cba 100644 --- a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/NetworkProtectionStatusBarMenu.swift +++ b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenu.swift @@ -1,5 +1,5 @@ // -// NetworkProtectionStatusBarMenu.swift +// StatusBarMenu.swift // // Copyright © 2022 DuckDuckGo. All rights reserved. // @@ -24,9 +24,12 @@ import NetworkProtection /// Abstraction of the the Network Protection status bar menu with a simple interface. /// -public final class StatusBarMenu { +@objc +public final class StatusBarMenu: NSObject { public typealias MenuItem = NetworkProtectionStatusView.Model.MenuItem + private let model: StatusBarMenuModel + private let statusItem: NSStatusItem private let popover: NetworkProtectionPopover @@ -43,28 +46,56 @@ public final class StatusBarMenu { /// - statusItem: (meant for testing) this allows us to inject our own status `NSStatusItem` to make automated testing easier.. /// @MainActor - public init(statusItem: NSStatusItem? = nil, + public init(model: StatusBarMenuModel, + statusItem: NSStatusItem? = nil, onboardingStatusPublisher: OnboardingStatusPublisher, statusReporter: NetworkProtectionStatusReporter, controller: TunnelController, iconProvider: IconProvider, menuItems: @escaping () -> [MenuItem]) { - self.statusItem = statusItem ?? NSStatusBar.system.statusItem(withLength: NSStatusItem.variableLength) + self.model = model + let statusItem = statusItem ?? NSStatusBar.system.statusItem(withLength: NSStatusItem.variableLength) + self.statusItem = statusItem self.iconPublisher = NetworkProtectionIconPublisher(statusReporter: statusReporter, iconProvider: iconProvider) popover = NetworkProtectionPopover(controller: controller, onboardingStatusPublisher: onboardingStatusPublisher, statusReporter: statusReporter, menuItems: menuItems) popover.behavior = .transient - self.statusItem.button?.image = .image(for: iconPublisher.icon) - self.statusItem.button?.target = self - self.statusItem.button?.action = #selector(statusBarButtonTapped) + super.init() + + statusItem.button?.image = .image(for: iconPublisher.icon) + statusItem.button?.target = self + statusItem.button?.action = #selector(statusBarButtonTapped) + statusItem.button?.sendAction(on: [.leftMouseUp, .rightMouseUp]) subscribeToIconUpdates() } @objc private func statusBarButtonTapped() { + let isRightClick = NSApp.currentEvent?.type == .rightMouseUp + + guard !isRightClick else { + showContextMenu() + return + } + + togglePopover() + } + + private func subscribeToIconUpdates() { + iconPublisherCancellable = iconPublisher.$icon + .receive(on: DispatchQueue.main) + .sink { [weak self] icon in + + self?.statusItem.button?.image = .image(for: icon) + } + } + + // MARK: - Popover + + private func togglePopover() { if popover.isShown { popover.close() } else { @@ -76,13 +107,20 @@ public final class StatusBarMenu { } } - private func subscribeToIconUpdates() { - iconPublisherCancellable = iconPublisher.$icon - .receive(on: DispatchQueue.main) - .sink { [weak self] icon in + // MARK: - Context - self?.statusItem.button?.image = .image(for: icon) + private func showContextMenu() { + if popover.isShown { + popover.close() } + + let menu = NSMenu() + menu.delegate = self + menu.items = model.contextMenuItems + + menu.popUp(positioning: nil, + at: .zero, + in: statusItem.button) } // MARK: - Showing & Hiding the menu @@ -95,3 +133,12 @@ public final class StatusBarMenu { statusItem.isVisible = false } } + +extension StatusBarMenu: NSMenuDelegate { + public func menuDidClose(_ menu: NSMenu) { + // We need to remove the context menu when it's closed because otherwise + // macOS will bypass our custom click-handling code and will proceed directly + // to always showing the context menu (ignoring if it's a left or right click). + statusItem.menu = nil + } +} diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenuModel.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenuModel.swift new file mode 100644 index 0000000000..7077dcb1bd --- /dev/null +++ b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenuModel.swift @@ -0,0 +1,46 @@ +// +// StatusBarMenuModel.swift +// +// Copyright © 2023 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import AppKit +import Foundation +import NetworkProtection + +public final class StatusBarMenuModel { + private let vpnSettings: VPNSettings + + public init(vpnSettings: VPNSettings) { + self.vpnSettings = vpnSettings + } + + var hideVPNMenu: NSMenuItem { + let item = NSMenuItem(title: "Hide VPN from menu bar", action: #selector(hideVPNMenuItemAction), keyEquivalent: "") + item.target = self + return item + } + + var contextMenuItems: [NSMenuItem] { + [ + hideVPNMenu + ] + } + + @objc + private func hideVPNMenuItemAction() { + vpnSettings.showInMenuBar = false + } +} diff --git a/LocalPackages/NetworkProtectionMac/Tests/NetworkProtectionUITests/NetworkProtectionStatusBarMenuTests.swift b/LocalPackages/NetworkProtectionMac/Tests/NetworkProtectionUITests/NetworkProtectionStatusBarMenuTests.swift index 56842e070e..d557ef5e5e 100644 --- a/LocalPackages/NetworkProtectionMac/Tests/NetworkProtectionUITests/NetworkProtectionStatusBarMenuTests.swift +++ b/LocalPackages/NetworkProtectionMac/Tests/NetworkProtectionUITests/NetworkProtectionStatusBarMenuTests.swift @@ -42,8 +42,12 @@ final class StatusBarMenuTests: XCTestCase { @MainActor func testShowStatusBarMenu() { + let defaults = UserDefaults(suiteName: UUID().uuidString)! let item = NSStatusItem() + let model = StatusBarMenuModel(vpnSettings: .init(defaults: defaults)) + let menu = StatusBarMenu( + model: model, statusItem: item, onboardingStatusPublisher: Just(OnboardingStatus.completed).eraseToAnyPublisher(), statusReporter: MockNetworkProtectionStatusReporter(), @@ -58,8 +62,12 @@ final class StatusBarMenuTests: XCTestCase { @MainActor func testHideStatusBarMenu() { + let defaults = UserDefaults(suiteName: UUID().uuidString)! let item = NSStatusItem() + let model = StatusBarMenuModel(vpnSettings: .init(defaults: defaults)) + let menu = StatusBarMenu( + model: model, statusItem: item, onboardingStatusPublisher: Just(OnboardingStatus.completed).eraseToAnyPublisher(), statusReporter: MockNetworkProtectionStatusReporter(),