TelemetryDeck / SwiftSDK

Swift SDK for TelemetryDeck, a privacy-conscious analytics service for apps and websites.
https://telemetrydeck.com/
Other
149 stars 30 forks source link

`UIApplication.willEnterForegroundNotification` is called each time an app enters the foreground on iOS, but only once on macCatalyst. #110

Open michaellenaghan opened 1 year ago

michaellenaghan commented 1 year ago

This isn't necessarily a bug, but it may be unexpected: UIApplication.willEnterForegroundNotification is called each time an app enters the foreground on iOS, but only once on macCatalyst.

https://github.com/TelemetryDeck/SwiftClient/blob/3f8bd438c8681ce7ccdf035b529cb5c4cd82cd7b/Sources/TelemetryClient/TelemetryClient.swift#L145

That means that an iOS client will generate a new session each time the app is activated, while a macCatalyst client will generate a new session each time the app is launched — even if it's left running for days, weeks or months. Again, not necessarily a bug, but someone who's focusing on session stats might think there's a lot less Mac usage than there really is.

It might also throw off TelemetryDeck's Daily Active Users and User Retention stats?

Here's a suggestion:

That would treat each launch and each additional day as a new session (if the app is activated by the user on that day).

winsmith commented 12 months ago

Love that idea!

michaellenaghan commented 12 months ago

TL;DR

On macCatalyst, observe NSWindowDidBecomeMainNotification rather than UIScene.willEnterForegroundNotification. Since that's an NSWindow notification name, and since NSWindow notification names aren't available in macCatalyst without special effort, subscribe using the stringified name, like this:

        self.token = NotificationCenter.default.addObserver(
            forName: Notification.Name("NSWindowDidBecomeMainNotification"), 
            object: nil, 
            queue: nil
        ) {
            notification in
            ...
        }

Background

macCatalyst sends out a number of UIScene.willEnterForegroundNotification notifications — all undocumented. Here's just one example: a popover sends a UIScene.willEnterForegroundNotification notification with a _UIPopoverScene object when it's presented. Observing NSWindowDidBecomeMainNotification is much simpler (and more reliable) than figuring out which notifications to pay attention to, and which to ignore.

winsmith commented 11 months ago

This is very compatible, doesn't break on other systems, and does what is needed, fantastic! I'm adding this to the roadmap – unless you'd like to submit a PR?

michaellenaghan commented 11 months ago

Well, it depends what you mean by "break." Stats would change. Since it takes time for people to update, that change would happen over a period of time rather than all at once, making it a bit harder to "see" and reason about.

If you don't think that's a problem I think I could submit a PR? If you do, we should talk it through a bit.

The "breaking" thing is more obvious with something like #106 where old data will have the wrong values, etc. (I'm aware of yet another wrong value that I haven't reported yet.) One option: bundle this change and those changes into a bigger release with a new major version number. The question is: would there be any effort to continue to support the old behaviour, under a flag of some kind, or would there just be a clean break…?

winsmith commented 8 months ago

I think that's fine. With "breaking" I meant more something like the app crashing because a notification type is not supported on macOS.

I know its been a minute, but if you're up for it I'd love the PR.