Fix SwiftUI screen size handling on iOS #238
Conversation
UIScreen.main is deprecated and calling it appears to reset the accent color of the UI. Fixes PostHog#237
There was a problem hiding this comment.
Hey @terabyte128! Thanx for this. I was not aware of this scenario. Of course I'd be happy to move away from a deprecated call.
I replicated locally as well and here’s what I observed for some more context:
- This issue only appears when accessing
UIScreen.mainwithin the app'sinit()method. However, if I use aUIApplicationDelegateAdaptorand access UIScreen.main from a delegate, it doesn’t seem to trigger the issue. This could be due to differences in the SwiftUI and UIKit lifecycles. - Likewise, since PH only accesses
UIScreen.mainwhen building event properties, the issue only surfaces when capturing an event directly in the app'sinit(). Capturing an event inAppDelegatedoesn’t trigger it.
| @@ -123,8 +123,10 @@ class PostHogContext { | |||
| var properties: [String: Any] = [:] | |||
|
|
|||
| #if os(iOS) || os(tvOS) | |||
| properties["$screen_width"] = Float(UIScreen.main.bounds.width) | |||
| properties["$screen_height"] = Float(UIScreen.main.bounds.height) | |||
| if let screen = UIApplication.shared.windows.first?.screen { | |||
There was a problem hiding this comment.
This change means that all iOS/tvOS consumers receive a warning unless we're only calling into PostHog from the main thread. If a client's logging system runs off of main, accessing UIApplication.shared will cause warnings in Xcode. Not sure if that's a priority for y'all, but figured I'd raise.
There was a problem hiding this comment.
hey @dfed, yeah that is indeed true. Hopping and blocking to main thread for every single event is not realistic so we should probably promote these to static properties. NSScreen.main will probably need to stay as dynamic context since it represents the screen of the currently focused window which can change, but I think for iOS/tvOS it's probably safe to calculate this once. Let me think about it a bit and I'll get back soon
There was a problem hiding this comment.
We can check if the captured event is on the main thread and only do the thread jumping if needed to reduce the perf impact as well.
UIApplication.shared.windows is also deprecated so the change from A to B didn't make sense IMO.
There was a problem hiding this comment.
PostHogReplayIntegration.getCurrentWindow() is likely the correct way right now, we should move getCurrentWindow() out of PostHogReplayIntegration and reuse across the SDK, I see more places using UIApplication.shared that is just trying to find the active window, eg UIViewController.activeController and UIApplication.ph_currentWindow
There was a problem hiding this comment.
PostHogReplayIntegration.getCurrentWindow()is likely the correct way right now, we should movegetCurrentWindow()out ofPostHogReplayIntegrationand reuse across the SDK, I see more places usingUIApplication.sharedthat is just trying to find the activewindow, egUIViewController.activeControllerandUIApplication.ph_currentWindow
Agreed. For capturing events we probably want to skip scene state checks
UIScreen.main is deprecated and calling it appears to reset the accent color of the UI.
💡 Motivation and Context
See #237
💚 How did you test it?
I verified that the change fixes the accent color issue. I further verified that calling
UIScreen.mainactually causes the issue in the first place, by inserting the following into my code and observing the accent color change:📝 Checklist