-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix: identify mac catalyst #287
base: main
Are you sure you want to change the base?
Conversation
PostHog/PostHogContext.swift
Outdated
properties["$os_version"] = "macOS \(macOSVersion) / \(underlyingOS) \(underlyingOSVersion)" | ||
properties["$os_name"] = isMacCatalystApp | ||
? "macOS (Mac Catalyst)" | ||
: "macOS (iOS running on Mac)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is great but we need an alternative solution here.
People use the os_name and os_version with matching values in feature flags, hogql, etc.
Maybe we need a new property to tell if it's a mac catalyst or not then? like a boolean one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think whether it's catalyst or not is more about the app itself and not the OS, so maybe this info should be with one of the $app_X
properties. the underlying OS is still the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, we can add something like $app_mac_catalyst
and $app_ios_on_mac
(or just one of the two), but I'm wondering how much this will be overcomplicating things for people. It will maintain backwards compatibility but will introduce additional steps and conventions when generating reports.
Also, what about $device_x
family? Especially $device_type
- currently evaluates to either Mobile
or Tablet
which doesn't really reflect reality. I'm assuming people are using these properties for their reports/flags as well. Do we also keep those unchanged and continue misrepresenting these cases here?
the underlying OS is still the same
True for iOS App on Mac since it's basically the same app just running in compatibility mode on Mac. However, Mac Catalyst apps are a bit of a mix - they can have access to macos specific features and interact with macOS. i.e they share codebase with iPad code, but can add additional mac functionality as well.
It's a tricky situation, since we need to balance correctness with backwards compatibility. I think I lean towards correctness here tbh.
A potential solution could be a mix:
- keep
$os_
family unchanged - correct values for
$device_
family - add a new bool
$app_ios_on_mac
property for both mac catalyst and ios running on mac cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think $os_
should be about the running OS on the physical device, if that's wrong, we have to fix it.
I think $device_
should be about the physical device, if that's wrong, we have to fix it.
Adding a new app flag makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed some changes and updated PR body
- For both cases,
$device_
reflects desktop/mac - For Catalyst,
$os_
points to macOS - For iOS running on Mac,
$os_
points to iPadOS - Added
$is_ios_running_on_mac
- Added
$is_mac_catalyst_app
(will create a PR on taxonomy next)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For iOS running on Mac, $os_ points to iPadOS
@ioannisj not sure if this is correct? its a mac, so the OS is the mac os, will people think they are running the app on an ipadoS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have misunderstood the feedback here.
For iOS running on Apple Silicon, there's no porting process, so I think it's essentially iOS or iPadOS running under the hood on top of Mac Catalyst. It's essentially the identical code and frameworks that runs on iPhone or iPad (docs).
It's all a bit confusing and inconsistent though because in this specific case:
ProcessInfo.processInfo.operatingSystemVersionString
returns the MacOS version - (e.g Version 15.1.1 (Build 24B91))ProcessInfo.processInfo.operatingSystemVersion
returns iPadOS version (e.g 18.1.0)
If we want to simplify things, we can treat these two cases (Mac Catalyst, iOS running on Mac) identical and use just one additional property is_mac_catalyst_app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So again just to confirm, the os
and device
props should be related to the physical device.
if the app is in compat. mode, this info should go either with app
props or new props.
if you want to add the info of the emulated OS in compat mode, this should go under new props, right now device
and os
props are meant for the physical device.
|
||
if isMacCatalystApp { | ||
let processInfo = ProcessInfo.processInfo | ||
properties["$os_name"] = "macOS \(processInfo.operatingSystemVersionString)" // eg Version 14.2.1 (Build 23C71) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be os_name and not version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just copied over exactly what we use for macOS targets here so that they match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm i feel like the naming should be deterministic since it can be used for filtering.
having the version as part of it feels wrong.
we have os_version for that.
I'd remove from both.
// | ||
// Source: https://developer.apple.com/documentation/apple-silicon/adapting-ios-code-to-run-in-the-macos-environment#Handle-unknown-device-types-gracefully | ||
properties["$device_type"] = "Desktop" | ||
properties["$device_name"] = processInfo.hostName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does hostName return here? this is about the device name (Model), and not the user's name or device's nickname
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to Host.current().localizedName
that we already use on macOS targets (Host is not available on Catalyst).
I think both return the computer name which could leak PII actually, which I agree is concerning. We describe $device_name
, however, as "Name of the device" in taxonomy
Regarding the actual device model, we capture as $device_model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be the info of the physical device.
So if we're using Host.current().localizedName
because device.model
isn't possible, then lets keep it
💡 Motivation and Context
see: https://posthog.com/questions/i-os-app-also-available-for-mac-os-is-not-showing-any-mac-os-users-in-product-analytics
Identify as
macOS
when running on Mac Catalyst or compatibility mode (Designed for iPad/iPhone).Here are the keys for the two cases:
Mac Catalyst
iOS App running on Mac (Designed for iPad/iPhone)
💚 How did you test it?
📝 Checklist