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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b98b7c7
feat: observe key window changes and cache screen size
ioannisj Nov 15, 2024
eb0cecc
fix: tvOS build
ioannisj Nov 15, 2024
bfbfa11
feat: add didBecomeActiveNotification notifications for watchOS
ioannisj Nov 18, 2024
e468535
fix: failing build
ioannisj Nov 18, 2024
e72c278
fix: avoid unecessary code runs in next run-loops
ioannisj Nov 19, 2024
de4107d
Merge branch 'main' into fix/sync-deadlock
ioannisj Nov 19, 2024
1387d8c
fix: remove unsupported notifications for watchOS
ioannisj Nov 19, 2024
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
View file
Open in desktop
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.2 - 2024-11-13

- fix: allow changing person properties after identify ([#249](https://github.com/PostHog/posthog-ios/pull/249))
Expand Down
120 changes: 101 additions & 19 deletions PostHog/PostHogContext.swift
View file
Open in desktop
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,18 @@ class PostHogContext {
#if !os(watchOS)
init(_ reachability: Reachability?) {
self.reachability = reachability
registerNotifications()
}
#else
init() {}
init() {
registerNotifications()
}
#endif

deinit {
unregisterNotifications()
}

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

return properties
}

private func registerNotifications() {
#if os(iOS) || os(tvOS)
#if os(iOS)
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: UIDevice.orientationDidChangeNotification,
object: nil)
#endif
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: UIWindow.didBecomeKeyNotification,
object: nil)
#elseif os(macOS)
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: NSWindow.didBecomeKeyNotification,
object: nil)
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: NSWindow.didChangeScreenNotification,
object: nil)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: WKApplication.didBecomeActiveNotification,
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
object: nil)
} else {
NotificationCenter.default.addObserver(self,
selector: #selector(cacheScreenSize),
name: .init("UIApplicationDidBecomeActiveNotification"),
object: nil)
}
#else
cacheScreenSize()
#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)
#elseif os(watchOS)
if #available(watchOS 7.0, *) {
NotificationCenter.default.removeObserver(self,
name: WKApplication.didBecomeActiveNotification,
object: nil)
} else {
NotificationCenter.default.removeObserver(self,
name: .init("UIApplicationDidBecomeActiveNotification"),
object: nil)
}
#endif
}

private func updateScreenSize() {
screenSize = {
#if os(iOS) || os(tvOS)
return UIApplication.getCurrentWindow(filterForegrounded: false)?.bounds.size
#elseif os(macOS)
return NSApplication.shared.keyWindow?.screen?.visibleFrame.size
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
#elseif os(watchOS)
return WKInterfaceDevice.current().screenBounds.size
#else
return nil
#endif
}()
}

@objc private func cacheScreenSize() {
// orientation change need a nudge on next run-loop
Copy link
Member

Choose a reason for hiding this comment

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

the notifications are not always delivered in the main thread already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

DispatchQueue.main.async {
self.updateScreenSize()
}
}
}
Loading