Skip to content
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

feat: observe key window changes and cache screen size #252

Merged
merged 7 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## Next

- fix: reading screen size could sometimes lead to a deadlock ([#252](https://github.com/PostHog/posthog-ios/pull/252))

## 3.15.3 - 2024-11-18

- fix: mangled wireframe layouts ([#250](https://github.com/PostHog/posthog-ios/pull/250))
Expand Down
151 changes: 132 additions & 19 deletions PostHog/PostHogContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,13 @@ import Foundation
#endif

class PostHogContext {
@ReadWriteLock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private var screenSize: CGSize?

#if !os(watchOS)
private let reachability: Reachability?
#endif

private var screenSize: CGSize? {
let getWindowSize: () -> CGSize? = {
#if os(iOS) || os(tvOS)
return UIApplication.getCurrentWindow(filterForegrounded: false)?.bounds.size
#elseif os(macOS)
return NSScreen.main?.visibleFrame.size
#elseif os(watchOS)
return WKInterfaceDevice.current().screenBounds.size
#else
return nil
#endif
}

return Thread.isMainThread
? getWindowSize()
: DispatchQueue.main.sync { getWindowSize() }
}

private lazy var theStaticContext: [String: Any] = {
// Properties that do not change over the lifecycle of an application
var properties: [String: Any] = [:]
Expand Down Expand Up @@ -111,11 +96,28 @@ class PostHogContext {
#if !os(watchOS)
init(_ reachability: Reachability?) {
self.reachability = reachability
registerNotifications()
}
#else
init() {}
init() {
if #available(watchOS 7.0, *) {
registerNotifications()
} else {
onShouldUpdateScreenSize()
}
}
#endif

deinit {
#if !os(watchOS)
unregisterNotifications()
#else
if #available(watchOS 7.0, *) {
unregisterNotifications()
}
#endif
}

private lazy var theSdkInfo: [String: Any] = {
var sdkInfo: [String: Any] = [:]
sdkInfo["$lib"] = postHogSdkName
Expand Down Expand Up @@ -161,4 +163,115 @@ class PostHogContext {

return properties
}

private func registerNotifications() {
#if os(iOS) || os(tvOS)
#if os(iOS)
NotificationCenter.default.addObserver(self,
selector: #selector(onOrientationDidChange),
name: UIDevice.orientationDidChangeNotification,
object: nil)
#endif
NotificationCenter.default.addObserver(self,
selector: #selector(onShouldUpdateScreenSize),
name: UIWindow.didBecomeKeyNotification,
object: nil)
#elseif os(macOS)
NotificationCenter.default.addObserver(self,
selector: #selector(onShouldUpdateScreenSize),
name: NSWindow.didBecomeKeyNotification,
object: nil)
NotificationCenter.default.addObserver(self,
selector: #selector(onShouldUpdateScreenSize),
name: NSWindow.didChangeScreenNotification,
object: nil)
NotificationCenter.default.addObserver(self,
selector: #selector(onShouldUpdateScreenSize),
name: NSApplication.didBecomeActiveNotification,
object: nil)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.addObserver(self,
selector: #selector(onShouldUpdateScreenSize),
name: WKApplication.didBecomeActiveNotification,
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
object: nil)
}
#endif
}

private func unregisterNotifications() {
#if os(iOS) || os(tvOS)
#if os(iOS)
NotificationCenter.default.removeObserver(self,
name: UIDevice.orientationDidChangeNotification,
object: nil)
#endif
NotificationCenter.default.removeObserver(self,
name: UIWindow.didBecomeKeyNotification,
object: nil)

#elseif os(macOS)
NotificationCenter.default.removeObserver(self,
name: NSWindow.didBecomeKeyNotification,
object: nil)
NotificationCenter.default.removeObserver(self,
name: NSWindow.didChangeScreenNotification,
object: nil)
NotificationCenter.default.removeObserver(self,
name: NSApplication.didBecomeActiveNotification,
object: nil)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.removeObserver(self,
name: WKApplication.didBecomeActiveNotification,
object: nil)
}
#endif
}

/// Retrieves the current screen size of the application window based on platform
private func getScreenSize() -> CGSize? {
#if os(iOS) || os(tvOS)
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
Copy link
Contributor Author

@ioannisj ioannisj Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#elseif os(watchOS)
return WKInterfaceDevice.current().screenBounds.size
#else
return nil
#endif
}

#if os(iOS)
// Special treatment for `orientationDidChangeNotification` since the notification seems to be _sometimes_ called early, before screen bounds are flipped
@objc private func onOrientationDidChange() {
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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))
Comment on lines +253 to +255
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need the max and min?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL ios-resolution

}
}
}
}
#endif

@objc private func onShouldUpdateScreenSize() {
updateScreenSize(getScreenSize)
}

private func updateScreenSize(_ getSize: @escaping () -> CGSize?) {
let block = {
self.screenSize = getSize()
}
// ensure block is executed on `main` since closure accesses non thread-safe UI objects like UIApplication
if Thread.isMainThread {
block()
} else {
DispatchQueue.main.async(execute: block)
}
}
}
20 changes: 20 additions & 0 deletions PostHog/PostHogSDK.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import Foundation
import UIKit
#elseif os(macOS)
import AppKit
#elseif os(watchOS)
import WatchKit
#endif

let retryDelay = 5.0
Expand Down Expand Up @@ -1042,6 +1044,12 @@ let maxRetryDelay = 30.0
defaultCenter.removeObserver(self, name: NSApplication.didFinishLaunchingNotification, object: nil)
defaultCenter.removeObserver(self, name: NSApplication.didResignActiveNotification, object: nil)
defaultCenter.removeObserver(self, name: NSApplication.didBecomeActiveNotification, object: nil)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.removeObserver(self,
name: WKApplication.didBecomeActiveNotification,
object: nil)
}
#endif
}

Expand Down Expand Up @@ -1075,6 +1083,18 @@ let maxRetryDelay = 30.0
selector: #selector(handleAppDidBecomeActive),
name: NSApplication.didBecomeActiveNotification,
object: nil)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.addObserver(self,
selector: #selector(handleAppDidBecomeActive),
name: WKApplication.didBecomeActiveNotification,
object: nil)
} else {
NotificationCenter.default.addObserver(self,
selector: #selector(handleAppDidBecomeActive),
name: .init("UIApplicationDidBecomeActiveNotification"),
object: nil)
}
#endif
}

Expand Down
Loading