-
Notifications
You must be signed in to change notification settings - Fork 415
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
ensure only one history database context #3068
Conversation
func isHistoryFeatureEnabled() -> Bool | ||
var isEnabledByUser: Bool { get } | ||
func removeAllHistory() async |
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.
protocol was not in use widely so added these functions which are used and made all dependents use the protocol
@@ -106,25 +103,12 @@ public class HistoryManager: HistoryManaging { | |||
|
|||
public func removeAllHistory() async { | |||
await withCheckedContinuation { continuation in | |||
historyCoordinator.burnAll { | |||
dbCoordinator.burnAll { |
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.
Always use the db coordinator when burning, even if disabled in settings.
extension HistoryManager { | ||
|
||
/// Should only be called once in the app | ||
public static func make(isAutocompleteEnabledByUser: @autoclosure @escaping () -> Bool, |
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.
Moved creation logic here. These auto closures mean I don't need to pass in AppSettings into a Core class.
} else { | ||
self.presentPreemptiveCrashAlert() | ||
} | ||
return NullHistoryManager() |
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.
Will never actually get used because the app will get killed, but gotta return something for the compiler.
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.
There is some linter error, but its minor.
I also think we could limit the work we do on startup (or move it off main thread in some cases) but I've perf-tested this with 5k visits on the device and it still performed well, so I'm not that concerned.
Great job :)
Task/Issue URL: https://app.asana.com/0/392891325557410/1207755453158856/f
Tech Design URL:
CC:
Description:
Ensures there's only 1 database context for the history store to avoid having merge conflicts.
Steps to test this PR:
Fire button:
Settings:
2. Visit some sites and generate history.
3. Disable the autocomplete setting
4. Check history not visible and visit different sites
6. Enable the autocomplete setting
7. Check previous history not visible
8. Repeat but by turning off the recently visited sites setting
Autoclear: