feat: observe key window changes and cache screen size #252
Conversation
PostHog/PostHogContext.swift
Outdated
| } | ||
|
|
||
| @objc private func cacheScreenSize() { | ||
| // orientation change need a nudge on next run-loop |
There was a problem hiding this comment.
the notifications are not always delivered in the main thread already?
There was a problem hiding this comment.
Not guaranteed per se, but each notification will usually document if it does or not. I'll go through the documentation and let you know. I left a comment right above, for orientationDidChangeNotification the screen size is still the old orientation, so it needs a bump on next run-loop to get the correct value (we can handle it in a special way and just manually flip the values but for sake of simplicity I just reschedule on next run-loop instead)
There was a problem hiding this comment.
yeah that would be good, also if that's not the case, check Thread.isMainThread and only do DispatchQueue.main.async if needed, I think this is also good so the value is updated as soon as possible
| updateScreenSize { | ||
| self.getScreenSize().map { size in | ||
| // manually set width and height based on device orientation. (Needed for fast orientation changes) | ||
| if UIDevice.current.orientation.isLandscape { |
There was a problem hiding this comment.
We need to do some manual work here cause on fast orientation changes, sometimes the size is correct for the given orientation and sometimes it's not. On simulator you can quickly rotate the device with command+arrow
| return UIApplication.getCurrentWindow(filterForegrounded: false)?.bounds.size | ||
| #elseif os(macOS) | ||
| // NSScreen.frame represents the full screen rectangle and includes any space occupied by menu, dock or camera bezel | ||
| return NSApplication.shared.windows.first { $0.isKeyWindow }?.screen?.frame.size |
There was a problem hiding this comment.
Per documentation, NSApplication.keyWindow is the window that is currently receiving keyboard events, which sounds like the way to go. When testing, however, I run into cases where keyWindow would be nil, but windows[0].isKeyWindow was true. So I opted in for using isKeyWindow here.
Also, dropped visibleFrame for frame since the former will report the drawble screen rect (rect that excludes menu bar, camera bezel and sticky dock). I think for our context we want the actual full screen size here which is represented by frame
There was a problem hiding this comment.
cool, for macos session replay we can double-check this because if it's wrong, the screen bound will be off since all positions are absolute
| @@ -16,28 +16,13 @@ import Foundation | |||
| #endif | |||
|
|
|||
| class PostHogContext { | |||
| @ReadWriteLock | |||
There was a problem hiding this comment.
I have been using NSLock for almost everything but the PostHogFileBackedQueue (came from the older version)
I intend to remove ReadWriteLock later since NSLock gives the ability of atomicity for multiple operations.
No need to change it now but keep that in mind.
PostHog/PostHogContext.swift
Outdated
| NotificationCenter.default.addObserver(self, | ||
| selector: #selector(onShouldUpdateScreenSize), | ||
| name: .init("UIApplicationDidBecomeActiveNotification"), | ||
| object: nil) |
There was a problem hiding this comment.
is that available for watchos? https://developer.apple.com/documentation/uikit/uiapplicationdidbecomeactivenotification says its not
There was a problem hiding this comment.
I tested this manually for WatchOS 6 & 7 and it fired in both cases, but not 100% confident tbh. I think it's safe to keep since this won't crash the app
If you prefer we can replace with a direct call to onShouldUpdateScreenSize() in watchOS init. For watchOS we are actually reading the screen size directly from the device, and the info is readily available.
Just wanted to keep a consistent approach for all platforms here
There was a problem hiding this comment.
Surprised that would trigger but it's not documented and in the code is UIKIT_EXTERN NSNotificationName const UIApplicationDidBecomeActiveNotification API_UNAVAILABLE(watchos) NS_SWIFT_NONISOLATED; (API_UNAVAILABLE(watchos).
your call here, I'd just double check it's really fired because I'd be super surprised.
There was a problem hiding this comment.
Double checked just now and it does fire. Downloading now a runtime for older watchOS sim to test (out of curiosity) but I think the safest approach here is to skip registerNotifications() in watchOS init and just call onShouldUpdateScreenSize directly
| CGSize(width: max(size.width, size.height), height: min(size.height, size.width)) | ||
| } else { | ||
| CGSize(width: min(size.width, size.height), height: max(size.height, size.width)) |
There was a problem hiding this comment.
why do we need the max and min?
There was a problem hiding this comment.
This relates to the comment here.
This is an attempt to grab to "correct" size for the current orientation and avoid jumping to next run-loop. If it's landscape we want the width to be the bigger of the two values, if it's portrait we want it to be the min of the two.
I'm not sure why this is happening but when the notification fires we sometimes get the correct screen size (for current orientation) and sometimes we get an "old" screen size with wrong proportions. Pushing this to next run-loop resolves the issue so I assume it's related to CoreAnimation works (there's a considerable visual delay in iPhones between orientation change and when the screen begins animating the rotation effect).
I think this transformation should be safe for all content size categories for iPhones and iPads https://www.ios-resolution.com/
|
@ioannisj left a few questions but otherwise LGTM |
💡 Motivation and Context
discussion: #247 (comment)
React to the following notifications and cache current screen size:
macOS:
watchOS:
iOS:
tvOS:
💚 How did you test it?
Spent some time writing a unit test to detect deadlocks, but dropped it since it would hang the test execution in case of a deadlock (old code).
Tested on sample apps for WatchOS, iOS and macOS
📝 Checklist