-
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
Do not reload DBP tab when switching to it #1942
Conversation
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.
Sorry but the tabWillClose
is incorrect, and dataBrokerProtectionHomeViewController
deallocating or reloading doesn‘t work in all cases as intended. Probably it should be a singleton view controller (something like a pinned tab)
DuckDuckGo/Tab/Model/Tab.swift
Outdated
@@ -463,6 +464,8 @@ protocol NewWindowPolicyDecisionMaker { | |||
userContentController?.cleanUpBeforeClosing() | |||
webView.assertObjectDeallocated(after: 4.0) | |||
} | |||
delegate?.tabWillClose(self) |
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.
the method is being called on Tab.dealloc, passing self
to a call here is a critical issue that will eventually lead to crashes. this should be removed
@@ -684,6 +683,12 @@ extension BrowserTabViewController: TabDelegate { | |||
} | |||
} | |||
|
|||
func tabWillClose(_ tab: Tab) { | |||
if tab.content == .dataBrokerProtection { | |||
dataBrokerProtectionHomeViewController = nil |
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.
this won‘t remove dataBrokerProtectionHomeViewController when a Tab is navigated to another URL. Or when dragging a tab to another window. (in this case DBP page data will be reloaded for another window)
If there‘s 2 DBP tabs open (dragged from another window), switching between them will show the same content. If one of them is closed, the DBP page will be reloaded on another one activation.
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.
If you don‘t mind I‘ve fixed the sandboxed target and removed !tabs.isEmpty
so it works when the last tab is removed
LGTM
# By Dominik Kapusta (41) and others # Via Dominik Kapusta (9) and others * main: (138 commits) Make sure when we set custom config url, we don't expect etag in return (#1994) Add PixelKit source parameter (#1989) Fix internal user toggling (#2000) Show alert and display warning icon in Sync Settings when data syncing is disabled (#1996) DBP: Integrate subscription account authentication to DBP (#1995) Improve bookmarks html reader (#1986) Add Sync feature flags (#1992) Add daily stats pixel (#1993) Do not reload DBP tab when switching to it (#1942) Fix: external application requests via redirect URLs shows wrong origin. (#1900) Update clean-app.sh to work on macOS Sonoma and include NetP containers (#1988) Fix: "SwiftLintPlugin" must be enabled before it can be used (#1987) Prevent VPN server list persistence failures (#1985) add test can remove data (#1980) Remove VPN upgrade card (#1983) Fix low-res VPN warning asset (#1984) DBP: Fix unreliable date tests (#1981) Add search retention pixel for NetP (#1964) Sabrina/sync e2e tests (#1959) swiftlint build plugin (#1318) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved # DuckDuckGo/Application/AppDelegate.swift
# By Dominik Kapusta (2) and others # Via GitHub * main: Make sure when we set custom config url, we don't expect etag in return (#1994) Add PixelKit source parameter (#1989) Fix internal user toggling (#2000) Show alert and display warning icon in Sync Settings when data syncing is disabled (#1996) DBP: Integrate subscription account authentication to DBP (#1995) Improve bookmarks html reader (#1986) Add Sync feature flags (#1992) Add daily stats pixel (#1993) Do not reload DBP tab when switching to it (#1942) Fix: external application requests via redirect URLs shows wrong origin. (#1900) # Conflicts: # DuckDuckGo/Statistics/PixelEvent.swift
Task/Issue URL: https://app.asana.com/0/1203581873609357/1206115814306009/f
Description:
Do not reload the DBP tab when switching to it
Steps to test this PR:
[ Prepare to use DBP]
false
from this linemacos-browser/LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Services/RedeemCodeServices.swift
Line 72 in 2bd8ca8
true
from this line if you're not already logged as in internal usermacos-browser/DuckDuckGo/DBP/DataBrokerProtectionFeatureVisibility.swift
Line 62 in 2bd8ca8
DataBrokerProtectionViewController
was removed from memory, open it again, check if the UI loads againInternal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation