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

tvOS support #571

Closed
wants to merge 9 commits into from
Closed

tvOS support #571

wants to merge 9 commits into from

Conversation

justindhill
Copy link

No description provided.

- Fixed a bug that caused an incomplete set of traits to be persisted to
  NSUserDefaults
- On tvOS, switched the order of loading of traits and the queue so it's
  attempted from the file first, then NSUserDefaults
- Updated the Podspec
  - Bumped the version to 3.2.6
  - Added a tvOS deployment target version (9.0)
  NSUserDefaults to only NSUserDefaults on tvOS
@justindhill justindhill mentioned this pull request Jul 1, 2016
@f2prateek
Copy link
Contributor

f2prateek commented Jul 1, 2016

Hey @justindhill - looks like the compilation works fine but the Podspec does not validate. you can run make lint locally to see the output yourself.

https://cloudup.com/cfQaMXWRBEO

pod lib lint

 -> Analytics (3.2.5)
    - WARN  | [tvOS] xcodebuild:  /Users/prateek/dev/src/github.com/segmentio/analytics-ios/Analytics/Classes/Internal/SEGAnalyticsRequest.m:34:48: warning: 'initWithRequest:delegate:startImmediately:' is deprecated: first deprecated in tvOS 9.0 - Use NSURLSession (see NSURLSession.h) [-Wdeprecated-declarations]
    - NOTE  | [tvOS] xcodebuild:  /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVSimulator.platform/Developer/SDKs/AppleTVSimulator9.2.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSURLConnection.h:117:1: note: 'initWithRequest:delegate:startImmediately:' has been explicitly marked deprecated here
    - WARN  | [tvOS] xcodebuild:  /Users/prateek/dev/src/github.com/segmentio/analytics-ios/Analytics/Classes/Internal/SEGLocation.m:38:17: warning: method definition for 'startUpdatingLocation' not found [-Wincomplete-implementation]
    - NOTE  | [tvOS] xcodebuild:  /Users/prateek/dev/src/github.com/segmentio/analytics-ios/Analytics/Classes/Internal/SEGLocation.h:18:1: note: method 'startUpdatingLocation' declared here
    - WARN  | [tvOS] xcodebuild:  /Users/prateek/dev/src/github.com/segmentio/analytics-ios/Analytics/Classes/Internal/SEGSegmentIntegration.m:649:12: warning: unused variable 'url' [-Wunused-variable]

[!] Analytics did not pass validation, due to 3 warnings (but you can use `--allow-warnings` to ignore them).
You can use the `--no-clean` option to inspect any issue.
make: *** [lint] Error 1

Some of these are actually the same warnings I was stuck at last time I looked into this #509 (comment). The good thing is there are no errors now, just warnings!

So if we can fix the warnings that'd be great. Additionaly I'd like to do some manual testing on tvOS for a sanity check as well.

@justindhill
Copy link
Author

Hey @f2prateek, resolved all but one of the linting errors. The last one is caused by the use of NSURLConnection in SEGSegmentIntegration. You'll need to switch to using NSURLSession to resolve that one.

@justindhill
Copy link
Author

However, that one is a warning and can be resolved at your leisure.

@f2prateek
Copy link
Contributor

@justindhill Thanks, we can take care of that separately- we do have an open issue for migrating to NSUrlSession but haven't had time to tackle it.

Do you know if it's possible to have CocoaPods ignore a specific error (other than supressing all with --allow-warnings)?

@justindhill
Copy link
Author

You can suppress warning emissions within a particular block of code using the preprocessor.

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"

// code that touches deprecated APIs here

#pragma clang diagnostic pop

[self.location startUpdatingLocation];
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to move this lower down - to right before return [context copy];?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know that I agree. CoreLocation is still available on tvOS, as well as CLLocationManager, but it doesn't provide periodic updates like it does on iOS. Presumably, this is because Apple doesn't anticipate the device moving while it's in use. 😜

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, that makes sense!

@f2prateek
Copy link
Contributor

We'll want to remove this entire block:

// Check for previous queue/track data in NSUserDefaults and remove if present
         [self dispatchBackground:^{
             if ([[NSUserDefaults standardUserDefaults] objectForKey:SEGQueueKey]) {
                 [[NSUserDefaults standardUserDefaults] removeObjectForKey:SEGQueueKey];
             }
             if ([[NSUserDefaults standardUserDefaults] objectForKey:SEGTraitsKey]) {
                 [[NSUserDefaults standardUserDefaults] removeObjectForKey:SEGTraitsKey];
             }
         }];

@justindhill
Copy link
Author

Probably only want to remove it on tvOS, right? I guess the purpose of it was to flush data from NSUserDefaults after you rolled back to file-based storage.

@@ -586,16 +611,26 @@ - (void)applicationWillTerminate
- (NSMutableArray *)queue
{
if (!_queue) {
#if TARGET_OS_TV
_queue = [[NSUserDefaults standardUserDefaults] objectForKey:SEGQueueKey];
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @justindhill , this will need to be _queue = [[NSUserDefaults standardUserDefaults] objectForKey:SEGQueueKey]; ?: [[NSMutableArray alloc] init];. Without that, the queue is always nil since nothing has been stored for that key, and none of the payloads are added to the queue, as a result of which nothing is persisted or uploaded.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good catch

@f2prateek
Copy link
Contributor

@justindhill Yup that was indeed the purpose. Let's remove those LOC just from tvOS for now.

return _queue;
}

- (NSMutableDictionary *)traits
{
if (!_traits) {
#if TARGET_OS_TV
_traits = [[NSUserDefaults standardUserDefaults] objectForKey:SEGTraitsKey];
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for the queue, this should fallback to [[NSMutableDictionary alloc] init]

_traits = [[NSUserDefaults standardUserDefaults] objectForKey:SEGTraitsKey] ?: [[NSMutableDictionary alloc] init];

@f2prateek
Copy link
Contributor

I tested this with the ?: [[NSMutableDictionary alloc] init] fallbacks and removing the code that was clearing SEGQueueKey and SEGTraitsKey and with those changes everything looked to be working fine!

@justindhill
Copy link
Author

Great, I made those changes in f9eddea!

@@ -496,6 +525,8 @@ - (void)reset
[self dispatchBackgroundAndWait:^{
[[NSUserDefaults standardUserDefaults] setValue:nil forKey:SEGUserIdKey];
[[NSUserDefaults standardUserDefaults] setValue:nil forKey:SEGAnonymousIdKey];
[[NSUserDefaults standardUserDefaults] setValue:@[] forKey:SEGQueueKey];
Copy link
Contributor

@f2prateek f2prateek Jul 1, 2016

Choose a reason for hiding this comment

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

Should this and the line below be only on tvOS as well?

Copy link
Author

Choose a reason for hiding this comment

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

I think so, yes.

  instead of nil
- When resetting, only remove storage files when we are not on tvOS
- When retrieving collection objects from NSUserDefaults, make a mutable
  copy, as all collections retrieved from NSUserDefaults are immutable
@f2prateek f2prateek mentioned this pull request Jul 1, 2016
@f2prateek
Copy link
Contributor

Thanks, tested manually and merged in #572

@f2prateek f2prateek closed this Jul 1, 2016
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.

2 participants