-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show VPN onboarding tips #3410
Show VPN onboarding tips #3410
Changes from 34 commits
1beb4c9
12c6d00
c5723b2
9c54287
59b9348
2492fee
80b97e0
5d0687f
3c6260f
e6ccb08
607feb0
aa937d5
7d0404a
f5b3b71
83899f3
a2fa5dc
9ca31ec
31f022c
865a5a7
d807463
a32cb01
a63b154
ed60c48
bf91f74
3ae5700
18b2b7a
986777c
112a301
cd57672
3554610
6d99f53
36d251c
5a77984
5cd2212
d1bb000
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,11 +24,14 @@ import Foundation | |
import LoginItems | ||
import NetworkProtection | ||
import NetworkProtectionIPC | ||
import NetworkProtectionProxy | ||
import NetworkProtectionUI | ||
import os.log | ||
import Subscription | ||
import VPNAppLauncher | ||
import SwiftUI | ||
import NetworkProtectionProxy | ||
import VPNAppLauncher | ||
import BrowserServicesKit | ||
import FeatureFlags | ||
|
||
protocol NetworkProtectionIPCClient { | ||
var ipcStatusObserver: ConnectionStatusObserver { get } | ||
|
@@ -55,8 +58,8 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager { | |
let vpnUninstaller: VPNUninstalling | ||
|
||
@Published | ||
private var siteInfo: SiteTroubleshootingInfo? | ||
private let siteTroubleshootingInfoPublisher: SiteTroubleshootingInfoPublisher | ||
private var siteInfo: ActiveSiteInfo? | ||
private let activeSitePublisher: ActiveSiteInfoPublisher | ||
private var cancellables = Set<AnyCancellable>() | ||
|
||
init(ipcClient: VPNControllerXPCClient, | ||
|
@@ -67,15 +70,15 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager { | |
|
||
let activeDomainPublisher = ActiveDomainPublisher(windowControllersManager: .shared) | ||
|
||
siteTroubleshootingInfoPublisher = SiteTroubleshootingInfoPublisher( | ||
activeSitePublisher = ActiveSiteInfoPublisher( | ||
activeDomainPublisher: activeDomainPublisher.eraseToAnyPublisher(), | ||
proxySettings: TransparentProxySettings(defaults: .netP)) | ||
|
||
subscribeToCurrentSitePublisher() | ||
} | ||
|
||
private func subscribeToCurrentSitePublisher() { | ||
siteTroubleshootingInfoPublisher | ||
activeSitePublisher | ||
.assign(to: \.siteInfo, onWeaklyHeld: self) | ||
.store(in: &cancellables) | ||
} | ||
|
@@ -87,9 +90,10 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager { | |
func show(positionedBelow view: NSView, withDelegate delegate: NSPopoverDelegate) -> NSPopover { | ||
|
||
/// Since the favicon doesn't have a publisher we force refreshing here | ||
siteTroubleshootingInfoPublisher.refreshSiteTroubleshootingInfo() | ||
activeSitePublisher.refreshActiveSiteInfo() | ||
|
||
let popover: NSPopover = { | ||
let vpnSettings = VPNSettings(defaults: .netP) | ||
let controller = NetworkProtectionIPCTunnelController(ipcClient: ipcClient) | ||
|
||
let statusReporter = DefaultNetworkProtectionStatusReporter( | ||
|
@@ -103,15 +107,22 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager { | |
) | ||
|
||
let onboardingStatusPublisher = UserDefaults.netP.networkProtectionOnboardingStatusPublisher | ||
_ = VPNSettings(defaults: .netP) | ||
let appLauncher = AppLauncher(appBundleURL: Bundle.main.bundleURL) | ||
let vpnURLEventHandler = VPNURLEventHandler() | ||
let proxySettings = TransparentProxySettings(defaults: .netP) | ||
let uiActionHandler = VPNUIActionHandler(vpnURLEventHandler: vpnURLEventHandler, proxySettings: proxySettings) | ||
|
||
let connectionStatusPublisher = CurrentValuePublisher( | ||
initialValue: statusReporter.statusObserver.recentValue, | ||
publisher: statusReporter.statusObserver.publisher) | ||
|
||
let activeSitePublisher = CurrentValuePublisher( | ||
initialValue: siteInfo, | ||
publisher: $siteInfo.eraseToAnyPublisher()) | ||
|
||
let siteTroubleshootingViewModel = SiteTroubleshootingView.Model( | ||
connectionStatusPublisher: statusReporter.statusObserver.publisher, | ||
siteTroubleshootingInfoPublisher: $siteInfo.eraseToAnyPublisher(), | ||
connectionStatusPublisher: connectionStatusPublisher, | ||
activeSitePublisher: activeSitePublisher, | ||
uiActionHandler: uiActionHandler) | ||
|
||
let statusViewModel = NetworkProtectionStatusView.Model(controller: controller, | ||
|
@@ -157,10 +168,36 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager { | |
_ = try? await self?.vpnUninstaller.uninstall(removeSystemExtension: true) | ||
}) | ||
|
||
let featureFlagger = NSApp.delegateTyped.featureFlagger | ||
let tipsFeatureFlagInitialValue = featureFlagger.isFeatureOn(.networkProtectionUserTips) | ||
let tipsFeatureFlagPublisher: CurrentValuePublisher<Bool, Never> | ||
|
||
if let overridesHandler = featureFlagger.localOverrides?.actionHandler as? FeatureFlagOverridesPublishingHandler<FeatureFlag> { | ||
|
||
let featureFlagPublisher = overridesHandler.flagDidChangePublisher | ||
.filter { $0.0 == .networkProtectionUserTips } | ||
|
||
tipsFeatureFlagPublisher = CurrentValuePublisher( | ||
initialValue: tipsFeatureFlagInitialValue, | ||
publisher: Just(tipsFeatureFlagInitialValue).eraseToAnyPublisher()) | ||
} else { | ||
tipsFeatureFlagPublisher = CurrentValuePublisher( | ||
initialValue: tipsFeatureFlagInitialValue, | ||
publisher: Just(tipsFeatureFlagInitialValue).eraseToAnyPublisher()) | ||
} | ||
Comment on lines
+171
to
+187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is all to handle the feature flag. The I think some of this can be simplified in the feature flag handling logic, as it's mostly boilerplate. But here I wasn't too worried because FF code is temporary and this will go away anyway. I can try to work on this if we see value, though. |
||
|
||
let tipsModel = VPNTipsModel(featureFlagPublisher: tipsFeatureFlagPublisher, | ||
statusObserver: statusReporter.statusObserver, | ||
activeSitePublisher: activeSitePublisher, | ||
forMenuApp: false, | ||
vpnSettings: vpnSettings, | ||
logger: Logger(subsystem: "DuckDuckGo", category: "TipKit")) | ||
Comment on lines
+189
to
+194
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A new model for handling the tips. |
||
|
||
let popover = NetworkProtectionPopover( | ||
statusViewModel: statusViewModel, | ||
statusReporter: statusReporter, | ||
siteTroubleshootingViewModel: siteTroubleshootingViewModel, | ||
tipsModel: tipsModel, | ||
debugInformationViewModel: DebugInformationViewModel(showDebugInformation: false)) | ||
popover.delegate = delegate | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,12 +32,20 @@ final class VPNPreferencesModel: ObservableObject { | |
|
||
@Published var connectOnLogin: Bool { | ||
didSet { | ||
guard settings.connectOnLogin != connectOnLogin else { | ||
return | ||
} | ||
Comment on lines
+35
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To prevent an infinite loop when the change is coming from a different place. I actually got this to happen when enabling connect on login from the tips. |
||
|
||
settings.connectOnLogin = connectOnLogin | ||
} | ||
} | ||
|
||
@Published var excludeLocalNetworks: Bool { | ||
didSet { | ||
guard settings.excludeLocalNetworks != excludeLocalNetworks else { | ||
return | ||
} | ||
|
||
settings.excludeLocalNetworks = excludeLocalNetworks | ||
|
||
Task { | ||
|
@@ -49,8 +57,6 @@ final class VPNPreferencesModel: ObservableObject { | |
} | ||
} | ||
|
||
@Published var secureDNS: Bool = true | ||
|
||
@Published var showInMenuBar: Bool { | ||
didSet { | ||
settings.showInMenuBar = showInMenuBar | ||
|
@@ -117,6 +123,8 @@ final class VPNPreferencesModel: ObservableObject { | |
locationItem = VPNLocationPreferenceItemModel(selectedLocation: settings.selectedLocation) | ||
|
||
subscribeToOnboardingStatusChanges(defaults: defaults) | ||
subscribeToConnectOnLoginSettingChanges() | ||
subscribeToExcludeLocalNetworksSettingChanges() | ||
Comment on lines
+126
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were missing so if settings was open, the checkboxes weren't refreshing live. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Damn, really good catch - thanks! |
||
subscribeToShowInMenuBarSettingChanges() | ||
subscribeToShowInBrowserToolbarSettingsChanges() | ||
subscribeToLocationSettingChanges() | ||
|
@@ -129,6 +137,18 @@ final class VPNPreferencesModel: ObservableObject { | |
.store(in: &cancellables) | ||
} | ||
|
||
func subscribeToConnectOnLoginSettingChanges() { | ||
settings.connectOnLoginPublisher | ||
.assign(to: \.connectOnLogin, onWeaklyHeld: self) | ||
.store(in: &cancellables) | ||
} | ||
|
||
func subscribeToExcludeLocalNetworksSettingChanges() { | ||
settings.excludeLocalNetworksPublisher | ||
.assign(to: \.excludeLocalNetworks, onWeaklyHeld: self) | ||
.store(in: &cancellables) | ||
} | ||
|
||
func subscribeToShowInMenuBarSettingChanges() { | ||
settings.showInMenuBarPublisher | ||
.removeDuplicates() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// | ||
// Logger+TipKit.swift | ||
// | ||
// Copyright © 2024 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 Foundation | ||
import os.log | ||
|
||
extension Logger { | ||
|
||
static var tipKit: Logger = { | ||
Logger(subsystem: Bundle.main.bundleIdentifier ?? "DuckDuckGo", category: "TipKit") | ||
}() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convenience menu to test TipKit.