Show VPN onboarding tips #3410
Show VPN onboarding tips #3410
Conversation
diegoreymendez
force-pushed
the
diego/add-vpn-tips
branch
from
cd61d52 to
607feb0
Compare
Task/Issue URL: https://app.asana.com/0/1206580121312550/1208341440402810/f macOS PR: duckduckgo/macos-browser#3410 BSK PR: duckduckgo/BrowserServicesKit#1024 ## Description Shows VPN onboarding tips.
| @@ -19,7 +19,7 @@ | |||
| import AppKit | |||
| import Foundation | |||
|
|
|||
| public struct SiteTroubleshootingInfo { | |||
| public struct ActiveSiteInfo { | |||
There was a problem hiding this comment.
Just a rename because the original name didn't describe this very well and this is really describing what's the active site.
| let tipsModel = VPNTipsModel(featureFlagPublisher: tipsFeatureFlagPublisher, | ||
| statusObserver: statusReporter.statusObserver, | ||
| activeSitePublisher: activeSitePublisher, | ||
| forMenuApp: false, | ||
| vpnSettings: vpnSettings, | ||
| logger: Logger(subsystem: "DuckDuckGo", category: "TipKit")) |
There was a problem hiding this comment.
A new model for handling the tips.
| guard settings.connectOnLogin != connectOnLogin else { | ||
| return | ||
| } |
There was a problem hiding this comment.
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.
| subscribeToConnectOnLoginSettingChanges() | ||
| subscribeToExcludeLocalNetworksSettingChanges() |
There was a problem hiding this comment.
These were missing so if settings was open, the checkboxes weren't refreshing live.
There was a problem hiding this comment.
Damn, really good catch - thanks!
| @@ -375,6 +375,10 @@ final class DuckDuckGoVPNAppDelegate: NSObject, NSApplicationDelegate { | |||
| // Load cached config (if any) | |||
| privacyConfigurationManager.reload(etag: configurationStore.loadEtag(for: .privacyConfiguration), data: configurationStore.loadData(for: .privacyConfiguration)) | |||
|
|
|||
| // It's important for this to be set-up after the privacy configuration is loaded | |||
| // as it relies on it for the remote feature flag. | |||
| TipKitAppEventHandler(featureFlagger: featureFlagger).appDidFinishLaunching() | |||
There was a problem hiding this comment.
Even though we're not showing tips in the VPN menu app, I left this in, in hopes we'll be able to eventually enable it again.
| import Combine | ||
| import Foundation | ||
|
|
||
| public final class CurrentValuePublisher<Output, Failure: Error> { |
There was a problem hiding this comment.
This could be moved to shared space, or be replaced with CurrentValueSubject if we don't mind it being mutable.
The gist of it is I wanted to have an initial value to use for the UI and avoid flickering (there still is some we can improve using this).
| @@ -28,14 +28,14 @@ extension LottieView where Placeholder: View { | |||
| let loopEndFrame: AnimationFrameTime | |||
| } | |||
|
|
|||
| public func playing(withIntro timing: LoopWithIntroTiming, isAnimating: Binding<Bool> = .constant(true)) -> Lottie.LottieView<Placeholder> { | |||
| public func playing(withIntro timing: LoopWithIntroTiming, isAnimating: Bool = true) -> Lottie.LottieView<Placeholder> { | |||
There was a problem hiding this comment.
This method wasn't using the binding at all, just the wrapped value... so we might as well just request a Bool instead and offer a cleaner method.
| let tipsFeatureFlagPublisher = CurrentValuePublisher<Bool, Never>( | ||
| initialValue: false, | ||
| publisher: Just(false).eraseToAnyPublisher()) |
There was a problem hiding this comment.
Feature flag disabled, but we could wire this easily to test.
| @Published | ||
| public var isVPNEnabled = false | ||
| /// | ||
| public var isVPNEnabled: Bool { | ||
| get { | ||
| switch connectionStatus { | ||
| case .connected, .connecting: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
No need to store the same state twice if this can be a calculated var.
| Task { @MainActor in | ||
| self.connectionStatus = status | ||
| switch status { | ||
| case .connected, .connecting: | ||
| self.isVPNEnabled = true | ||
| default: | ||
| self.isVPNEnabled = false | ||
| } | ||
| } |
There was a problem hiding this comment.
This was duplicated in the code I simplified above... so it can now be reduced and simplified.
| .store(in: &cancellables) | ||
| } | ||
|
|
||
| private func subscribeToServerInfoChanges() { | ||
| statusReporter.serverInfoObserver.publisher | ||
| .subscribe(on: Self.serverInfoDispatchQueue) | ||
| .receive(on: DispatchQueue.main) |
There was a problem hiding this comment.
Moving all to the main dispatch queue because I was getting odd flickering in the status view.
| private static let statusDispatchQueue = DispatchQueue(label: "com.duckduckgo.NetworkProtectionStatusView.statusDispatchQueue", qos: .userInteractive) | ||
| private static let connectivityIssuesDispatchQueue = DispatchQueue(label: "com.duckduckgo.NetworkProtectionStatusView.connectivityIssuesDispatchQueue", qos: .userInteractive) | ||
| private static let serverInfoDispatchQueue = DispatchQueue(label: "com.duckduckgo.NetworkProtectionStatusView.serverInfoDispatchQueue", qos: .userInteractive) | ||
| private static let dataVolumeDispatchQueue = DispatchQueue(label: "com.duckduckgo.NetworkProtectionStatusView.dataVolumeDispatchQueue", qos: .userInteractive) |
There was a problem hiding this comment.
Most of these are for refreshing the UI so I moved everything to the main queue.
| @@ -180,7 +166,6 @@ public final class TunnelControllerViewModel: ObservableObject { | |||
|
|
|||
| private func subscribeToDataVolumeUpdates() { | |||
| statusReporter.dataVolumeObserver.publisher | |||
| .subscribe(on: Self.dataVolumeDispatchQueue) | |||
There was a problem hiding this comment.
Note do self: I have to dispatch this on main queue.
| @EnvironmentObject | ||
| private var tipsModel: VPNTipsModel |
There was a problem hiding this comment.
Note to self: the tips model should likely be handled by the main models. Would be cleaner.
Task/Issue URL: https://app.asana.com/0/1206580121312550/1208795272851000/f
iOS PR: duckduckgo/iOS#3429
BSK PR: duckduckgo/BrowserServicesKit#1024
Description
Shows VPN onboarding tips.
Steps to test this PR:
Testing Pixels:
Pixels are fired when the tips are shown, ignored (popover closed with no action), dismissed (x button) or actioned.
This is an example run:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation