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: get current window screen size #247

Merged
merged 10 commits into from
Nov 12, 2024
Merged

feat: get current window screen size #247

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4ffb8f1
feat: get current window screen size
ioannisj Nov 11, 2024
93c471a
fix: wrap in compiler directive
ioannisj Nov 11, 2024
dc2b0cb
fix: lint
ioannisj Nov 11, 2024
10e21ed
fix: build for tvOS and watchOS
ioannisj Nov 11, 2024
b3fb499
fix: filter foregrounded scenes by default
ioannisj Nov 11, 2024
6fe5573
feat: search for UIWindowSceneDelegate when getting current window
ioannisj Nov 11, 2024
e9bf754
feat: get device size for watchOS
ioannisj Nov 11, 2024
da14bbd
fix: iterate over window scenes once
ioannisj Nov 12, 2024
a792255
fix: iOS15 fallback
ioannisj Nov 12, 2024
b34f4a2
chore: update CHANGELOG
ioannisj Nov 12, 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
4 changes: 4 additions & 0 deletions PostHog.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@
69F518382BB2BA0100F52C14 /* PostHogSwizzler.swift in Sources */ = {isa = PBXBuildFile; fileRef = 69F518372BB2BA0100F52C14 /* PostHogSwizzler.swift */; };
69F5183A2BB2BA8300F52C14 /* UIApplicationTracker.swift in Sources */ = {isa = PBXBuildFile; fileRef = 69F518392BB2BA8300F52C14 /* UIApplicationTracker.swift */; };
DA26419C2CC0499300CB427B /* PostHogAutocaptureEventTracker.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA26419A2CC0499300CB427B /* PostHogAutocaptureEventTracker.swift */; };
DA5AA7192CE245D2004EFB99 /* UIApplication+.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA5AA7132CE245CD004EFB99 /* UIApplication+.swift */; };
DA5B85882CD21CBB00686389 /* AutocaptureEventProcessing.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA5B85872CD21CBB00686389 /* AutocaptureEventProcessing.swift */; };
DA979D7B2CD370B700F56BAE /* PostHogAutocaptureEventTrackerSpec.swift in Sources */ = {isa = PBXBuildFile; fileRef = DA979D7A2CD370B700F56BAE /* PostHogAutocaptureEventTrackerSpec.swift */; };
DAC699D62CC790D9000D1D6B /* PostHogAutocaptureIntegration.swift in Sources */ = {isa = PBXBuildFile; fileRef = DAC699D52CC790D9000D1D6B /* PostHogAutocaptureIntegration.swift */; };
Expand Down Expand Up @@ -390,6 +391,7 @@
69F518372BB2BA0100F52C14 /* PostHogSwizzler.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PostHogSwizzler.swift; sourceTree = "<group>"; };
69F518392BB2BA8300F52C14 /* UIApplicationTracker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UIApplicationTracker.swift; sourceTree = "<group>"; };
DA26419A2CC0499300CB427B /* PostHogAutocaptureEventTracker.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PostHogAutocaptureEventTracker.swift; sourceTree = "<group>"; };
DA5AA7132CE245CD004EFB99 /* UIApplication+.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIApplication+.swift"; sourceTree = "<group>"; };
DA5B85872CD21CBB00686389 /* AutocaptureEventProcessing.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AutocaptureEventProcessing.swift; sourceTree = "<group>"; };
DA8D37242CBEAC02005EBD27 /* PostHogExampleAutocapture.xcodeproj */ = {isa = PBXFileReference; lastKnownFileType = "wrapper.pb-project"; name = PostHogExampleAutocapture.xcodeproj; path = PostHogExampleAutocapture/PostHogExampleAutocapture.xcodeproj; sourceTree = "<group>"; };
DA979D7A2CD370B700F56BAE /* PostHogAutocaptureEventTrackerSpec.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PostHogAutocaptureEventTrackerSpec.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -506,6 +508,7 @@
3AA4C09B2988315D006C4731 /* Utils */ = {
isa = PBXGroup;
children = (
DA5AA7132CE245CD004EFB99 /* UIApplication+.swift */,
3AE3FB422992985A00AFFC18 /* Reachability.swift */,
3AE3FB462992AB0000AFFC18 /* Hedgelog.swift */,
3A0F108429C9ABB6002C0084 /* ReadWriteLock.swift */,
Expand Down Expand Up @@ -1176,6 +1179,7 @@
69F23A7A2BB309F3001194F6 /* MethodSwizzler.swift in Sources */,
69261D1B2AD9678C00232EC7 /* PostHogEvent.swift in Sources */,
69EE82BC2BA9C53000EB9542 /* PostHogSessionReplayConfig.swift in Sources */,
DA5AA7192CE245D2004EFB99 /* UIApplication+.swift in Sources */,
69EE82CE2BAAC76000EB9542 /* ViewTreeSnapshotStatus.swift in Sources */,
69ED1AD42C90A0F100FE7A91 /* URLSessionExtension.swift in Sources */,
69ED1A9F2C8F451B00FE7A91 /* PostHogPersonProfiles.swift in Sources */,
Expand Down
11 changes: 1 addition & 10 deletions PostHog/Autocapture/PostHogAutocaptureEventTracker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,17 +342,8 @@
}
}

extension UIApplication {
static var ph_currentWindow: UIWindow? {
Array(UIApplication.shared.connectedScenes)
.compactMap { $0 as? UIWindowScene }
.flatMap(\.windows)
.first { $0.windowLevel != .statusBar }
}
}

extension UIViewController {
class func ph_topViewController(base: UIViewController? = UIApplication.ph_currentWindow?.rootViewController) -> UIViewController? {
class func ph_topViewController(base: UIViewController? = UIApplication.getCurrentWindow()?.rootViewController) -> UIViewController? {
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
if let nav = base as? UINavigationController {
return ph_topViewController(base: nav.visibleViewController)

Expand Down
36 changes: 24 additions & 12 deletions PostHog/PostHogContext.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,33 @@ import Foundation
import UIKit
#elseif os(macOS)
import AppKit
#elseif os(watchOS)
import WatchKit
#endif

class PostHogContext {
#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
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
#elseif os(watchOS)
return WKInterfaceDevice.current().screenBounds.size
#else
return nil
#endif
}

return Thread.isMainThread
? getWindowSize()
: DispatchQueue.main.sync { getWindowSize() }
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

@ioannisj this .sync call creates a deadlock in my app. We wrap PostHogSDK.shared.isFeatureEnabled calls in a lock so that we can return stable values for a feature flag within one app lifecycle, like so:

	public func isEnabled(_ key: String) -> Bool {
		lock.sync {
			if let deliveredIsEnabled = unsafe_keyToPreviouslyDeliveredIsEnabled[key] {
				// Avoid potential bugs by always returning a stable value.
				// Do not allow a feature flag to change mid app lifecycle.
				return deliveredIsEnabled
			} else {
				let isEnabled = PostHogSDK.shared.isFeatureEnabled(key)
				unsafe_keyToPreviouslyDeliveredIsEnabled[key] = isEnabled
				return isEnabled
			}
		}
	}

On app launch, we call into this isEnabled(…) method from multiple queues, including main. If our background queue calls into this method just before main calls into this method, then the background queue gets the lock, the main queue waits on the lock, and then your code here waits forever for main to continue.

While this code exists in PostHog we will be unable to upgrade from our current version.

Copy link

Choose a reason for hiding this comment

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

Ideally you wouldn't need to synchronously access this data just-in-time: a possible approach would be to intercept changes to the current window synchronously, capturing the window-specific values you need, and then storing those values in a locked container that background queues can access without getting a lock on all of main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @dfed thanx for raising this! Great oversight from our side that this could be potentially problematic. I'll take a look asap

Copy link
Member

@marandaneto marandaneto Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

maybe a simpler approach/quick workaround would be:

        if Thread.isMainThread {
            return getWindowSize()
        } else {
            var size: CGSize?
            let group = DispatchGroup()
            group.enter()
            DispatchQueue.main.async {
                size = getWindowSize()
                group.leave()
                
            }
            let timeout = DispatchTime.now() + .seconds(1)
            let _ = group.wait(timeout: timeout)
            return size
        }

or yes, listening to window changes and caching the values

Copy link

@dfed dfed Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That quick workaround would alleviate the deadlock but would cause a 1s hang on the main queue any time we would have deadlocked 😬

Certainly better than a deadlock, but also not something I can ship with.

Quickest fix imo is to cache the value behind a lock any time you're on main and read that cached value any time you're on a non-main queue. This would mean that you'd have to call into PostHog from main at least once before you'd get screen data attached to events, but that seems like a reasonable requirement.

Copy link
Member

Choose a reason for hiding this comment

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

If the SDK is init in a background thread, and the main thread is still holding a lock, the problem would persist though.

btw @dfed PostHogSDK.shared.isFeatureEnabled is already thread-safe, unless your app has to do many operations within that lock, but reading/writing a flag is thread-safe.
I understand your use case, but feels like the deadlock is caused by your own locks/logic, the main thread should be available since most features require main thread access, including our SDK and some of its features.
I agree that we should not contribute to a deadlock hence trying to figure out the best approach here but I'd not hold a lock in the main thread.

@ioannisj it's important to return the correct screen size info if we do caching, or better to not attach any data at all, I'd prioritize correct data over skewed data.

What's about this notification? Would that be reliable to observe and cache the current active window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marandaneto yeah, already on that currently (along with some other relevant notifications). For now, we'll cache size but later on we can use this implementation to always keep a ref to key window for replay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just need to make sure that this will work on all platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @dfed, would it be possible for you to test your app against this PR when you have a chance? (possibly also verifying that you get the correct $screen_width, $screen_height on your events? 🙏)

Copy link

Choose a reason for hiding this comment

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

PostHogSDK.shared.isFeatureEnabled is already thread-safe, unless your app has to do many operations within that lock, but reading/writing a flag is thread-safe.
I understand your use case, but feels like the deadlock is caused by your own locks/logic, the main thread should be available since most features require main thread access, including our SDK and some of its features.
I agree that we should not contribute to a deadlock hence trying to figure out the best approach here but I'd not hold a lock in the main thread.

You're absolutely right that it's my code, not yours, that is causing the deadlock. However, what I'm doing should be supported by PostHog – PostHog should not be synchronously calling out to a global resource like main. Being able to ensure that PostHog always returns stable data is not possible without a lock or single-threading my calls into PostHog. Holding a lock on main is fine if the work is quick, and this work should be quick.

would it be possible for you to test your app against this #252 when you have a chance? (possibly also verifying that you get the correct $screen_width, $screen_height on your events? 🙏)

done! Looking good to me!

}

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 @@ -122,18 +142,10 @@ class PostHogContext {
func dynamicContext() -> [String: Any] {
var properties: [String: Any] = [:]

#if os(iOS) || os(tvOS)
if let screen = UIApplication.shared.windows.first?.screen {
properties["$screen_width"] = Float(screen.bounds.width)
properties["$screen_height"] = Float(screen.bounds.height)
}
#elseif os(macOS)
if let mainScreen = NSScreen.main {
let screenFrame = mainScreen.visibleFrame
properties["$screen_width"] = Float(screenFrame.size.width)
properties["$screen_height"] = Float(screenFrame.size.height)
}
#endif
if let screenSize {
properties["$screen_width"] = Float(screenSize.width)
properties["$screen_height"] = Float(screenSize.height)
}

if Locale.current.languageCode != nil {
properties["$locale"] = Locale.current.languageCode
Expand Down
26 changes: 1 addition & 25 deletions PostHog/Replay/PostHogReplayIntegration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,30 +509,6 @@
return wireframe
}

static func getCurrentWindow() -> UIWindow? {
// TODO: support multi windows

// UIApplication.shared.windows is deprecated
for scene in UIApplication.shared.connectedScenes {
if scene is UIWindowScene,
scene.activationState == .foregroundActive,
let windowScene = scene as? UIWindowScene
{
if #available(iOS 15.0, *) {
if let keyWindow = windowScene.keyWindow {
return keyWindow
}
}

for window in windowScene.windows where window.isKeyWindow {
return window
}
}
}

return nil
}

@objc private func snapshot() {
if !PostHogSDK.shared.isSessionReplayActive() {
return
Expand All @@ -543,7 +519,7 @@
}
ViewLayoutTracker.clear()

guard let window = PostHogReplayIntegration.getCurrentWindow() else {
guard let window = UIApplication.getCurrentWindow() else {
return
}

Expand Down
2 changes: 1 addition & 1 deletion PostHog/Replay/UIApplicationTracker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
guard event.type == .touches else {
return
}
guard let window = PostHogReplayIntegration.getCurrentWindow() else {
guard let window = UIApplication.getCurrentWindow() else {
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
return
}
guard let touches = event.touches(for: window) else {
Expand Down
12 changes: 2 additions & 10 deletions PostHog/UIViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,9 @@
// if a view is being dismissed, this will return nil
if let root = viewIfLoaded?.window?.rootViewController {
return root
} else {
// preferred way to get active controller in ios 13+
for scene in UIApplication.shared.connectedScenes where scene.activationState == .foregroundActive {
let windowScene = scene as? UIWindowScene
let sceneDelegate = windowScene?.delegate as? UIWindowSceneDelegate
if let target = sceneDelegate, let window = target.window {
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
return window?.rootViewController
}
}
}
return nil
// TODO: handle container controllers (see ph_topViewController)
return UIApplication.getCurrentWindow()?.rootViewController
ioannisj marked this conversation as resolved.
Show resolved Hide resolved
}

static func getViewControllerName(_ viewController: UIViewController) -> String? {
Expand Down
45 changes: 45 additions & 0 deletions PostHog/Utils/UIApplication+.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//
// UIApplication+.swift
// PostHog
//
// Created by Yiannis Josephides on 11/11/2024.
//

#if os(iOS) || os(tvOS)
import UIKit

extension UIApplication {
static func getCurrentWindow(filterForegrounded: Bool = true) -> UIWindow? {
let windowScenes = UIApplication.shared
.connectedScenes
.compactMap { $0 as? UIWindowScene }
.filter {
!filterForegrounded || $0.activationState == .foregroundActive
}

for scene in windowScenes {
// attempt to retrieve directly from UIWindowScene
if #available(iOS 15.0, tvOS 15.0, *) {
if let keyWindow = scene.keyWindow {
return keyWindow
}
}

// check scene.windows.isKeyWindow
for window in scene.windows {
if window.isKeyWindow {
return window
}
}
ioannisj marked this conversation as resolved.
Show resolved Hide resolved

// check scene.delegate.window property
let sceneDelegate = scene.delegate as? UIWindowSceneDelegate
if let target = sceneDelegate, let window = target.window {
return window
}
}

return nil
}
}
#endif
Loading