Refactor AppDelegate (milestone 1) #3727
Conversation
jaceklyp
changed the title
| private let appStateMachine: AppStateMachine = AppStateMachine() | ||
| private let appBehavior: AppBehavior = { | ||
| guard !ProcessInfo().arguments.contains("testing") else { return .existing } | ||
| if let appBehavior = AppDependencyProvider.shared.appSettings.appBehavior { |
There was a problem hiding this comment.
I'm afraid we can run into an unexpected behavior here, i.e. not being consistent for a single app instance. AppSettings are user defaults and this can be executed before data is unlocked. We are already observing inconsistencies with UserDefaults (https://app.asana.com/0/414235014887631/1208659072736427/f) so maybe it's a good idea to steer away from using those here?
| case stateMachine | ||
|
|
||
| } | ||
|
|
||
| @UIApplicationMain class AppDelegate: UIResponder, UIApplicationDelegate { |
There was a problem hiding this comment.
I know it'll be a bit more work at this point, but did you consider extracting the code into 2 separate types, say ExistingAppDelegate and StateMachineAppDelegate, then use them in this class? It'd have a few advantages for the future:
- It'd lower the risk of introducing unwanted changes to existing approach, for example by making changes to instance properties.
- Requirements, dependencies and instance properties for the state machine would be clearly visible and easier to maintain.
- Reasoning about the approach and overall shape will be easier.
- Switching the implementation after the migration is complete could be as easy as replacing
AppDelegatecontents with whatever is insideStateMachineAppDelegate.
|
|
||
| } | ||
|
|
||
| protocol AppEventHandler { | ||
|
|
||
| @MainActor | ||
| func handle(_ event: AppEvent) |
There was a problem hiding this comment.
Marking protocol function declaration as MainActor won't enforce it (based on my tests). It's better to mark the method as async here and defer to the implementation to isolate for specific actor.
| } | ||
|
|
||
| // onApplicationLaunch code | ||
| Task { @MainActor [self] in // is capturing self here ok? |
There was a problem hiding this comment.
is capturing self here ok?
self is a value type so you'll be capturing a copy. If self value changes you won't see those changes here.
| overlayWindow = UIWindow(frame: window.frame) | ||
| overlayWindow?.windowLevel = UIWindow.Level.alert | ||
|
|
||
| // TODO: most likely we do not need voiceSearchHelper for BlankSnapshotVC |
There was a problem hiding this comment.
It's used to toggle mic's visibility in the omni bar, so I'm afraid it's here to stay.
Task/Issue URL: https://app.asana.com/0/0/1208832732122403/f
Tech Design URL: https://app.asana.com/0/481882893211075/1208859623176995/f
CC: @bwaresiak
Description:
Steps to test this PR:
Definition of Done (Internal Only):
Device Testing:
OS Testing: