UI adjustments for improved VPN user control (#2043)

Task/Issue URL: https://app.asana.com/0/0/1206318102194992/f BSK PR: duckduckgo/BrowserServicesKit#622 iOS PR: duckduckgo/iOS#2317 ## Description This PR brings some adjustments to recent VPN user control improvements. - If you hide the VPN menu app icon when the browser is showing VPN preferences, the VPN preferences "Show VPN in Menu Bar" will be updated dynamically. - The copy shown when right clicking on the VPN icon in the browser's navigation bar was adjusted to include "shortcut" at the end, like for other navigation bar buttons.
duckduckgo · Jan 16, 2024 · 2a7d10d · 2a7d10d
1 parent f77ec1e
commit 2a7d10d
Show file tree

Hide file tree

Showing 16 changed files with 38 additions and 30 deletions.
diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj
@@ -13127,7 +13127,7 @@
 			repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
 			requirement = {
 				kind = exactVersion;
-				version = 101.1.0;
+				version = 101.1.1;
 			};
 		};
 		AA06B6B52672AF8100F541C5 /* XCRemoteSwiftPackageReference "Sparkle" */ = {

diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -14,8 +14,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/duckduckgo/BrowserServicesKit",
       "state" : {
-        "revision" : "851187f38974b87889b21259eb442e95aedffafe",
-        "version" : "101.1.0"
+        "revision" : "202dc0540c214e21b89395370177873e090a7633",
+        "version" : "101.1.1"
       }
     },
     {

diff --git a/DuckDuckGo/Common/Localizables/UserText.swift b/DuckDuckGo/Common/Localizables/UserText.swift
@@ -828,8 +828,8 @@ struct UserText {
     static let showDownloadsShortcut = NSLocalizedString("pinning.show-downloads-shortcut", value: "Show Downloads Shortcut", comment: "Menu item for showing the downloads shortcut")
     static let hideDownloadsShortcut = NSLocalizedString("pinning.hide-downloads-shortcut", value: "Hide Downloads Shortcut", comment: "Menu item for hiding the downloads shortcut")
 
-    static let showNetworkProtectionShortcut = NSLocalizedString("pinning.show-netp-shortcut", value: "Show Network Protection", comment: "Menu item for showing the NetP shortcut")
-    static let hideNetworkProtectionShortcut = NSLocalizedString("pinning.hide-netp-shortcut", value: "Hide Network Protection", comment: "Menu item for hiding the NetP shortcut")
+    static let showNetworkProtectionShortcut = NSLocalizedString("pinning.show-netp-shortcut", value: "Show VPN Shortcut", comment: "Menu item for showing the NetP shortcut")
+    static let hideNetworkProtectionShortcut = NSLocalizedString("pinning.hide-netp-shortcut", value: "Hide VPN Shortcut", comment: "Menu item for hiding the NetP shortcut")
 
     static let showHomeShortcut = NSLocalizedString("pinning.show-home-shortcut", value: "Show Home Button", comment: "Menu item for showing the Home shortcut")
     static let hideHomeShortcut = NSLocalizedString("pinning.hide-home-shortcut", value: "Hide Home Button", comment: "Menu item for hiding the Home shortcut")

diff --git a/DuckDuckGo/Localizable.xcstrings b/DuckDuckGo/Localizable.xcstrings
@@ -6471,7 +6471,7 @@
         "en" : {
           "stringUnit" : {
             "state" : "new",
-            "value" : "Hide Network Protection"
+            "value" : "Hide VPN Shortcut"
           }
         }
       }
@@ -6531,7 +6531,7 @@
         "en" : {
           "stringUnit" : {
             "state" : "new",
-            "value" : "Show Network Protection"
+            "value" : "Show VPN Shortcut"
           }
         }
       }

diff --git a/...ckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift b/...ckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarButtonModel.swift
@@ -219,19 +219,15 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
         }
 
         guard !isPinned,
-              !popoverManager.isShown else {
+              !popoverManager.isShown,
+              !isHavingConnectivityIssues else {
+
+            pinNetworkProtectionToNavBarIfNeverPinnedBefore()
             showButton = true
             return
         }
 
-        Task {
-            guard !isHavingConnectivityIssues else {
-                showButton = true
-                return
-            }
-
-            showButton = false
-        }
+        showButton = false
     }
 
     // MARK: - Pinning
@@ -245,10 +241,8 @@ final class NetworkProtectionNavBarButtonModel: NSObject, ObservableObject {
     /// if the user hasn't toggled it manually before.
     /// 
     private func pinNetworkProtectionToNavBarIfNeverPinnedBefore() {
-        assert(showButton)
-
-        guard !pinningManager.wasManuallyToggled(.networkProtection),
-              !pinningManager.isPinned(.networkProtection) else {
+        guard !pinningManager.isPinned(.networkProtection),
+              !pinningManager.wasManuallyToggled(.networkProtection) else {
             return
         }
 

diff --git a/DuckDuckGo/Preferences/Model/VPNPreferencesModel.swift b/DuckDuckGo/Preferences/Model/VPNPreferencesModel.swift
@@ -84,6 +84,7 @@ final class VPNPreferencesModel: ObservableObject {
         shouldShowLocationItem = featureFlagger.isFeatureOn(.vpnGeoswitching)
 
         subscribeToOnboardingStatusChanges(defaults: defaults)
+        subscribeToShowInMenuBarSettingChanges()
         subscribeToLocationSettingChanges()
     }
 
@@ -93,6 +94,13 @@ final class VPNPreferencesModel: ObservableObject {
             .store(in: &cancellables)
     }
 
+    func subscribeToShowInMenuBarSettingChanges() {
+        settings.showInMenuBarPublisher
+            .removeDuplicates()
+            .assign(to: \.showInMenuBar, onWeaklyHeld: self)
+            .store(in: &cancellables)
+    }
+
     func subscribeToLocationSettingChanges() {
         settings.selectedLocationPublisher
             .map(VPNLocationPreferenceItemModel.init(selectedLocation:))

diff --git a/LocalPackages/DataBrokerProtection/Package.swift b/LocalPackages/DataBrokerProtection/Package.swift
@@ -29,7 +29,7 @@ let package = Package(
             targets: ["DataBrokerProtection"])
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
         .package(path: "../PixelKit"),
         .package(path: "../SwiftUIExtensions"),
         .package(path: "../XPCHelper")

diff --git a/LocalPackages/LoginItems/Package.swift b/LocalPackages/LoginItems/Package.swift
@@ -13,7 +13,7 @@ let package = Package(
         ),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         .target(

diff --git a/LocalPackages/NetworkProtectionMac/Package.swift b/LocalPackages/NetworkProtectionMac/Package.swift
@@ -30,7 +30,7 @@ let package = Package(
         .library(name: "NetworkProtectionUI", targets: ["NetworkProtectionUI"])
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
         .package(path: "../XPCHelper"),
         .package(path: "../SwiftUIExtensions")
     ],

diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenu.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionUI/Menu/StatusBarMenu.swift
@@ -123,8 +123,14 @@ public final class StatusBarMenu: NSObject {
         menu.delegate = self
         menu.items = model.contextMenuItems
 
+        // I'm not sure why +8 is needed, but that seems to be the right positioning to make this work well
+        // across all systems.  I'm seeing an issue where the menu looks right for me but not for others testing
+        // this, and this seems to fix it:
+        // Ref: https://app.asana.com/0/0/1206318017787812/1206336583680668/f
+        let yPosition = statusItem.statusBar!.thickness + 8
+
         menu.popUp(positioning: nil,
-                   at: .zero,
+                   at: NSPoint(x: 0, y: yPosition),
                    in: statusItem.button)
     }
 

diff --git a/LocalPackages/PixelKit/Package.swift b/LocalPackages/PixelKit/Package.swift
@@ -20,7 +20,7 @@ let package = Package(
         )
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         .target(

diff --git a/LocalPackages/Subscription/Package.swift b/LocalPackages/Subscription/Package.swift
@@ -12,7 +12,7 @@ let package = Package(
             targets: ["Subscription"]),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         .target(

diff --git a/LocalPackages/SwiftUIExtensions/Package.swift b/LocalPackages/SwiftUIExtensions/Package.swift
@@ -11,7 +11,7 @@ let package = Package(
         .library(name: "PreferencesViews", targets: ["PreferencesViews"]),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         .target(

diff --git a/LocalPackages/SyncUI/Package.swift b/LocalPackages/SyncUI/Package.swift
@@ -13,7 +13,7 @@ let package = Package(
     ],
     dependencies: [
         .package(path: "../SwiftUIExtensions"),
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         .target(

diff --git a/LocalPackages/SystemExtensionManager/Package.swift b/LocalPackages/SystemExtensionManager/Package.swift
@@ -16,7 +16,7 @@ let package = Package(
         ),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         // Targets are the basic building blocks of a package, defining a module or a test suite.

diff --git a/LocalPackages/XPCHelper/Package.swift b/LocalPackages/XPCHelper/Package.swift
@@ -30,7 +30,7 @@ let package = Package(
         .library(name: "XPCHelper", targets: ["XPCHelper"]),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "101.1.1"),
     ],
     targets: [
         // Targets are the basic building blocks of a package. A target can define a module or a test suite.