Modularize debug menu and login items logic #1378

diegoreymendez · 2023-07-21T09:10:26Z

Task/Issue URL: https://app.asana.com/0/0/1205112155849082/f

Stack:

Modularize debug menu and login items logic #1378 <- This PR
- Improves NetworkProtectionDebugMenuController #1389
- Modularize debug menu and login items logic #3 #1397

Description

This PR:

Moves the debug logic that's currently in NetworkProtectionTunnelProvider into its own class.
Moves the login item management logic into its own class.

This will have the following advantages:

Since most of the debug logic is platform specific, moving it out of NetworkProtectionTunnelProvider means we're closer to being able to share it with iOS, which IMHO would be great many ways.
I'll be able to tackle fixes to the login items quicker, without having to deal with unrelated code. Additionally I'll be able to add unit tests more easily.

Testing

Test 1: Debug Menu

Test the debug menu and make sure all works well.

Reset NetP's status.
Remove the system extension and login items.
Change your selected server
Simulate a failure.

Test 2: Login Items

Reset NetP's status.
Launch the app.
Start NetP.
Make sure the login items are launched.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

github-actions · 2023-07-21T09:13:00Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against d2ff642

DuckDuckGo.xcodeproj/project.pbxproj

DuckDuckGo/AppDelegate/AppDelegate.swift

DuckDuckGo/Menus/MainMenu.storyboard

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/LoginItem.swift

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift

DuckDuckGo/Menus/MainMenu.swift

DuckDuckGo/Menus/MainMenuActions.swift

DuckDuckGo/AppDelegate/AppDelegate.swift

graeme

A few comments. Nothing blocking, mainly cosmetic. If you do change anything else and want me to have another look, gimme a shout.

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionLoginItemsManager.swift

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

mallexxx

I like the changes, that‘s so much cleaner! 🥳 See some comments inline

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

…e builds

diegoreymendez · 2023-07-25T13:02:45Z

For history and context: the remaining feedback is addressed in a follow up PR here: #1389

mallexxx · 2023-07-26T08:59:07Z

DuckDuckGo/Menus/MainMenuActions.swift

#if NETWORK_PROTECTION import NetworkProtection #endif can now be removed

mallexxx · 2023-07-26T09:17:01Z

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

+/// Controller for the Network Protection debug menu.
+///
+@objc
+final class NetworkProtectionDebugMenuController: NSObject {


In other places menu items are managed by subclassing NSMenu (ApplicationDockMenu, MainMenu, HistoryMenu, MoreoptionsMenu, SharingMenu etc..)
Let‘s convert this into : NSMenu subclass - this will remove the need of an extra object in MainMenu, networkProtectionMenuItem and networkProtectionDebugMenuController outlets

we could also use self.supermenu()?.remove(at: self.supermenu()!.indexOfItem(withSubmenu: self)) in awakeFromNib() inside the !NETWORK_PROTECTION block to remove the menu management code from MainMenu - not strong on this

Addressed in #1397

mallexxx · 2023-07-26T09:21:11Z

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugUtilities.swift

+
+    // MARK: - Registration Key Validity
+
+    static let registrationKeyValidityKey = "com.duckduckgo.network-protection.NetworkProtectionTunnelController.registrationKeyValidityKey"


Can this be replaced with @UserDefaultsWrapper?

Addressed in #1397

mallexxx · 2023-07-26T09:22:02Z

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugUtilities.swift

+    ///     - validity: the default registration key validity time interval.  A `nil` value means it will be automatically
+    ///         defined by NetP using its standard configuration.
+    ///
+    func setRegistrationKeyValidity(_ validity: TimeInterval?, defaults: UserDefaults = .standard) async throws {


this call is raising "Non-sendable type 'UserDefaults' exiting main actor-isolated context in call to non-isolated instance method 'setRegistrationKeyValidity(_:defaults:)' cannot cross actor boundary" warning in NetworkProtectionDebugMenuController - could we use @UserDefaultsWrapper instead of direct UserDefaults passing here?

Addressed in #1397

mallexxx · 2023-07-26T09:26:31Z

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

+    /// Sets the selected server.
+    ///
+    @IBAction
+    func setSelectedServer(_ sender: Any?) {


NSMenuItem sender type can be directly set in the definition instead of casting - not something critical but reduces unneeded code

Yeah I don't mind either way but the reason I don't worry too much about these is that the guard below is staying, just in a slightly different form.

Either way I'm pushing this change since the guard becomes slightly nicer to read.

I mean we could get rid of the guard statement entirely by making the sender argument non- optional since it isn't meant to be called from outside

Addressed in #1397

mallexxx · 2023-07-26T09:32:54Z

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

+        if menuItem.state == .on {
+            menuItem.state = .off
+        } else {
+            menuItem.state = .on
+        }


this should be set in the update() method - item state is not reset after the simulationOptions is reset

Tackled in #1397

mallexxx · 2023-07-26T09:33:02Z

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift

+        if menuItem.state == .on {
+            menuItem.state = .off
+        } else {
+            menuItem.state = .on
+        }


same as above

Tackled in #1397

mallexxx · 2023-07-26T09:33:21Z

DuckDuckGo/Menus/MainMenu.swift

-    @IBOutlet weak var networkProtectionMenuItem: NSMenuItem?
+    @IBOutlet weak var networkProtectionMenuItem: NSMenuItem? {
+        didSet {
+            networkProtectionMenuItem?.target = networkProtectionDebugMenuController


why is it needed?

It's actually not - I think it was necessary at some point when the code was mostly living here, but after moving it I lost track of coming back to remove this. Good catch.

mallexxx · 2023-07-26T09:36:01Z

DuckDuckGo/Menus/MainMenu.storyboard

        <scene sceneID="JPo-4y-FX3">
            <objects>
                <customObject id="Voe-Tx-rLC" customClass="AppDelegate" customModule="DuckDuckGo_Privacy_Browser" customModuleProvider="target"/>
                <customObject id="Ady-hI-5gd" userLabel="First Responder" customClass="NSResponder" sceneMemberID="firstResponder"/>


not related but since you‘re touching it, could you remove an empty Menu object from the storyboard? It seemingly got there by mistake and not used anywhere

Tackled in #1397

mallexxx · 2023-07-26T09:44:23Z

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionLoginItemsManager.swift

+    func ensureLoginItemsAreRunning(_ condition: LoginItemCheckCondition = .none, after interval: TimeInterval = .seconds(5)) {
+
+        Task {
+            try? await Task.sleep(interval: interval)


thrown error shouldn‘t be ignored for Task.sleep (meaning cancellation)

Tackled in #1397

Task/Issue URL: https://app.asana.com/0/0/1205135296966627/f This is a follow up to: #1378 ## Stack: - #1378 - #1389 <- This PR ## Description: Implements some changes based on the feedback received in these two comments: - #1378 (comment) - #1378 (comment) These changes were tackled separately for these reasons: - The base PR was mostly about moving existing code to a different location. - The base PR works fine. - Reviewing these changes separately guarantees a faster and more focused feedback loop.

diegoreymendez · 2023-07-26T14:12:35Z

@mallexxx - All feedback (but one item since I think it's ok to leave as-is, has been addressed). Most of it is in #1397 since it required huge changes and I wanted to avoid this PR going out of control.

mallexxx · 2023-07-27T09:16:14Z

@diegoreymendez, not really huge 🤷‍♂️ especially having #1397 based on develop 🙂 - but certainly makes it harder to review the requested changes
Approved.

diegoreymendez · 2023-07-27T10:13:29Z

@mallexxx - Part of the issue is that this PR's scope exploded, and I should've pushed back on it. A lot of the requested changes in this review are really out of scope.

I'll bring this up for discussion separately as a PR is probably not the best place for it.

diegoreymendez · 2023-07-27T10:57:47Z

I'm merging as the issues I'm seeing in CI have absolutely nothing to do with my changes.

Task/Issue URL: https://app.asana.com/0/1199230911884351/1205144339578924/f ## Stack: - #1378 - #1389 - #1397 <- This PR ## Description: Addresses some feedback in the base PR: - #1378 (comment) - #1378 (comment) - #1378 (comment) - #1378 (comment) - #1378 (comment) - #1378 (comment) - #1378 (comment)

# By Dominik Kapusta (7) and others # Via Sam Symons (2) and others * develop: Modularize debug menu and login items logic #3 (#1397) Modularize debug menu and login items logic (#1378) Fixes of minor issues with Fire Button after the new logic release (#1395) Fire Window (#1357) Fix reordering favorites in the last row when it's not fully populated (#1401) Close the window when cmd+w is pressed on a pinned tab and there are no regular tabs (#1390) update sparkle to 2.4.2 (#1362) disable trailing_comma (#1394) Add support for per-model Sync Data Providers initialization (#1387) update favicon logic to fix gsuite icons (#1380) Update minor version by default in code_freeze lane (#1392) Upload dSYMs from release builds to S3 for Sentry (#1393) Set version to 1.48.2. Hide Cards UI on macOS Catalina (#1386) Hide Cards UI on macOS Catalina (#1386) update shield icon only after navigation ended and tab is selected (#1314) Add Debug menu option back in for resetting the Email Protection InContext prompt (#1373) Bump version to 1.49.0 (42) Update embedded files # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

Makes several changes to our login items and NetP startup logic

4a0f1e2

diegoreymendez added 2 commits July 21, 2023 13:25

Several improvements to the NetP debug menu logic

faf6a6a

Improves some NetP code in the App delegate

446e4a0

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo.xcodeproj/project.pbxproj Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/AppDelegate/AppDelegate.swift Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/Menus/MainMenu.storyboard Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/LoginItem.swift Show resolved Hide resolved

Adds a few MARK sections

baea4bb

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift Show resolved Hide resolved

diegoreymendez added 4 commits July 21, 2023 14:11

Improves some of the code for handling NetP's debug menu

7e81e8f

Several fixes for my recent NetP menu changes

b84d836

Several fixes to my recent changes

912201f

Removes some code that should be submitted separately

25d5539

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/Menus/MainMenu.swift Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/Menus/MainMenuActions.swift Show resolved Hide resolved

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/Menus/MainMenuActions.swift Show resolved Hide resolved

Addresses some swiftlint errors

78e27c0

diegoreymendez commented Jul 21, 2023

View reviewed changes

DuckDuckGo/AppDelegate/AppDelegate.swift Show resolved Hide resolved

diegoreymendez changed the title ~~Makes several changes to our login items and NetP startup logic~~ Modularize debug and login items logic Jul 21, 2023

diegoreymendez changed the title ~~Modularize debug and login items logic~~ Modularize debug menu and login items logic Jul 21, 2023

Fixes for AppStore build

d8a99a5

diegoreymendez requested a review from graeme July 21, 2023 15:14

diegoreymendez self-assigned this Jul 21, 2023

diegoreymendez marked this pull request as ready for review July 21, 2023 15:14

Fixes a bad swiflint directive

b08acec

graeme approved these changes Jul 24, 2023

View reviewed changes

mallexxx reviewed Jul 24, 2023

View reviewed changes

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift Outdated Show resolved Hide resolved

...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenuController.swift Outdated Show resolved Hide resolved

Implements an empty NetworkProtectionDebugMenuController for app stor…

22574d6

…e builds

Removed an outdated comment

c4d1697

diegoreymendez mentioned this pull request Jul 25, 2023

Improves NetworkProtectionDebugMenuController #1389

Merged

Fixes a swiftlint warning

6572521

mallexxx suggested changes Jul 26, 2023

View reviewed changes

diegoreymendez and others added 2 commits July 26, 2023 12:28

Addresses some review feedbacl

82dbce9

diegoreymendez mentioned this pull request Jul 26, 2023

Modularize debug menu and login items logic #3 #1397

Merged

diegoreymendez requested a review from mallexxx July 26, 2023 14:12

mallexxx approved these changes Jul 27, 2023

View reviewed changes

Merges the latest from develop

d2ff642

diegoreymendez merged commit 0ba31be into develop Jul 27, 2023

diegoreymendez deleted the diego/agents-debug-menu branch July 27, 2023 10:57


		// MARK: - Registration Key Validity

		static let registrationKeyValidityKey = "com.duckduckgo.network-protection.NetworkProtectionTunnelController.registrationKeyValidityKey"

Modularize debug menu and login items logic #1378

Modularize debug menu and login items logic #1378

Uh oh!

Conversation

diegoreymendez commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack:

Description

Testing

Test 1: Debug Menu

Test 2: Login Items

Internal references:

Uh oh!

github-actions bot commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graeme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mallexxx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

diegoreymendez commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mallexxx Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

diegoreymendez commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

mallexxx Jul 26, 2023 •

edited

Loading

diegoreymendez Jul 26, 2023 •

edited

Loading