Refactor app startup code - extracting services #3859
Conversation
There was a problem hiding this comment.
Great effort 💪 ! I'm not approving yet because I haven't smoke tested it and there are several features to test thoroughly, but overall I have no major concerns. I left a few suggestions though.
I think you can also consider *LifecycleController naming instead of *Service for those newly created objects. In fact they're controlling the encapsulated logic based on the app events, similarly to how ViewController controls view based on UI events.
| NotificationCenter.default.addObserver(forName: AppUserDefaults.Notifications.autofillEnabledChange, | ||
| object: nil, | ||
| queue: nil) { _ in | ||
| self.autofillPixelReporter.updateAutofillEnabledStatus(AppDependencyProvider.shared.appSettings.autofillCredentialsEnabled) |
There was a problem hiding this comment.
Closure is prolonging the life of AutofillService. Is it expected?
| override init() { | ||
| super.init() | ||
| NotificationCenter.default.addObserver(forName: .databaseDidEncounterInsufficientDiskSpace, object: nil, queue: .main) { _ in | ||
| self.application(UIApplication.shared, willTerminateWithReason: .insufficientDiskSpace) |
There was a problem hiding this comment.
AppDelegate is retained here. It should be present throughout app lifecycle regardless but I'd probably advise to not keep it strongly via closure.
| uiService = UIService(window: window) | ||
|
|
||
| // Task handler registration needs to happen before the end of `didFinishLaunching`, otherwise submitting a task can throw an exception. | ||
| // Having both in `didBecomeActive` can sometimes cause the exception when running on a physical device, so registration happens here. | ||
| AppConfigurationFetch.registerBackgroundRefreshTaskHandler() | ||
|
|
||
| UNUserNotificationCenter.current().delegate = unService |
There was a problem hiding this comment.
I'm not sure it's the best solution to encapsulate UserNotificationCenter inside VPN. Given VPNService conforms to UNUserNotificationCenterDelegate there can be NotificationService which holds an array of conforming instances, where one of those will be VPNService. This way those two are decoupled from each other and we're ready for easier handling of other cases outside of the VPN purpose. It's only a suggestion, since it may increase already quite big scope of the project.
Task/Issue URL: TBD
Description:
TBD
Steps to test this PR:
1.
2.