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

Update device type #63

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Update device type #63

merged 4 commits into from
Oct 6, 2023

Conversation

bddq
Copy link
Contributor

@bddq bddq commented Sep 4, 2023

What does this PR do?
This updates the device type property sent by SDK to conform examples seen in documentation.

Where should the reviewer start?
Just check PHGPostHogIntegration file.

How should this be manually tested?
Trigger any event and check that $device_type is set with one of the implemented values.

Any background context you want to provide?
Old value "ios" doesn't represent the a device type. Even less for tvOS apps.

@bddq
Copy link
Contributor Author

bddq commented Sep 29, 2023

Any news @marandaneto?

@marandaneto
Copy link
Member

Any news @marandaneto?

Sorry for the delay, there's a similar issue here PostHog/posthog-flutter#49
I believe the change makes sense, but not sure if using the current approach.

I've been doing this with compile flags, eg.

#if TARGET_OS_OSX || TARGET_OS_MACCATALYST
    ..
#elif TARGET_OS_IOS
    ..
#elif TARGET_OS_TV
    ..
#elif TARGET_OS_WATCH
    ..
#endif

They come from https://github.com/apple/swift-corelibs-foundation/blob/main/CoreFoundation/Base.subproj/SwiftRuntime/TargetConditionals.h IIRC, would you mind testing this out?

but never used userInterfaceIdiom.

Constants that indicate the interface type for the device or an object that has a trait environment, such as a view and view controller.

So it might be that this gives a false positive, what do you think?

@bddq
Copy link
Contributor Author

bddq commented Sep 29, 2023

Conditional flags mentioned can work but cannot identify a CarPlay device (maybe PostHog will support that).

Because userInterfaceIdiom is read from the current device, it's safe to rely on that value to determine the device type.
It can also help the user to differentiate an iPhone and an iPad that are two types of iOS devices.

@marandaneto
Copy link
Member

Conditional flags mentioned can work but cannot identify a CarPlay device (maybe PostHog will support that).

Because userInterfaceIdiom is read from the current device, it's safe to rely on that value to determine the device type. It can also help the user to differentiate an iPhone and an iPad that are two types of iOS devices.

That's also possible with the $device_model, so I'd be fine with both solutions.
@benjackwhite anything to add? Since this is a breaking change and add new values that are yet not documented, eg CarPlay.

@benjackwhite
Copy link
Collaborator

I think adding new values is no big deal. Changing existing data though... Less sure. I feel like we could make this a minor change or wait till the Swift rewrite and include it as part of the major change there 🤔

If it can wait I would personally opt for the rewrite as we may want to make more breaking changes at that point as wel

@marandaneto
Copy link
Member

@bddq do you mind adding an entry to the changelog? so we can get it merged.

@bddq
Copy link
Contributor Author

bddq commented Oct 5, 2023

@marandaneto Done!

CHANGELOG.md Outdated Show resolved Hide resolved
@marandaneto marandaneto merged commit aab0004 into PostHog:master Oct 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants