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

Adds WatchKit support via CocoaPods #465

Merged
merged 13 commits into from Dec 16, 2019

Conversation

leoAsana
Copy link
Contributor

@leoAsana leoAsana commented Nov 6, 2019

As mentioned in my issue ##464 was was looking to use the tracker from the Apple Watch directly.

I went ahead and fixed the issues that prevented the project to build on watchOS and added watchOS to the podspec.

The changes a just some more #ifs in SPUtilities.m and I had to replace the use of NSURLConnection in SPEmitter.m with NSURLSession. I used a dispatch_semaphore to make it synchronous as mentioned here.

I've also added an watch app to the Swift Demo and it worked in my tests.

@snowplowcla
Copy link

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://github.com/snowplow/snowplow/wiki/CLA to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@snowplowcla snowplowcla added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Nov 6, 2019
@leoAsana
Copy link
Contributor Author

leoAsana commented Nov 6, 2019

I signed it!

@paulboocock paulboocock added cla: yes cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed. and removed cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. cla: yes labels Nov 8, 2019
@AlexBenny
Copy link
Contributor

Thanks for your contribute @leoAsana .
As you noticed some parts of the tracker haven't been touched for a while. That's the case of the WatchOS target, so your contribute is very valuable.

We are going to release soon the version 1.1.4 (early next week). Please, could you rebase your branch onto master as soon as we proceed with the release of 1.1.4 ?

@coveralls
Copy link

coveralls commented Nov 12, 2019

Coverage Status

Coverage increased (+0.05%) to 82.174% when pulling 2c0ca08 on asanarebel:watchos into d4b852b on snowplow:master.

@leoAsana
Copy link
Contributor Author

I've rebased master and edited the new example projects for carthage and cocoapods to include a watch target.
I also fixed some build settings problems on the watch target, so now it works with Carthage to.

I know this is a rather large PR, but most changes are just in the example projects.

@AlexBenny AlexBenny added this to the 1.2.0 milestone Nov 12, 2019
@AlexBenny
Copy link
Contributor

Thanks @leoAsana. That's very helpful, I'll review the changes in the next days.
Meanwhile I've set the milestone to the next planned version 1.2.0

@leoAsana
Copy link
Contributor Author

@AlexBenny Thanks! I'm already using by fork in our watch project and I havn't seen any problems yet. I noticed that the platform is pc. Is that intended or should I change the value?

I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this,
This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec.
@AlexBenny
Copy link
Contributor

@leoAsana Sorry for the delays, and happy to see that your solution is working without issues. Unfortunately we’re working on some of the core features, that slows down the integration of GitHub contributes, but once we have the high priority work done we will review your PR.

I noticed that the platform is pc. Is that intended or should I change the value?

We haven't defined anything for wearables at the moment. I think it would make sense using mob for the WatchOS platform. Much of the watch apps are iOS apps extensions.

FYI: we just released the version 1.1.5 where we fixed an issue introduced on 1.1.4 about IDFA.

Copy link
Contributor

@AlexBenny AlexBenny left a comment

Choose a reason for hiding this comment

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

Thanks for the important contribute on making our tracker suitable for the Apple Watch!

I've reviewed and tested the code on the device. It works very well.
I've looked briefly to the demo apps and it seems everything good.
We can't consider the Watch tracker ready to production, we need to discuss it internally and eventually plan further improvements, but I would be happy to merge it in the next release.

I've just added few comments to address before the merge.
I'd ask you to add also this small details:

  • SPUtilities.m > getOSType - Can we return @"watchos" for the Watch target?
  • Snowplow.m > Could you add "watchos-1.1.5" to kSPVersion?

Snowplow/SPUtilities.m Outdated Show resolved Hide resolved
Snowplow/SPUtilities.m Outdated Show resolved Hide resolved
Snowplow/SPUtilities.m Outdated Show resolved Hide resolved
} else {
NSLog(@"SPLog: Error: %@", connectionError);
[results addObject:[[SPRequestResponse alloc] initWithBool:false withIndex:indexArray]];
[_dataOperationQueue addOperationWithBlock:^{
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 👍
We should refactor all that, soon or later, but meanwhile this is a nice fix for the deprecated method!

Examples/SnowplowSwiftCarthageDemo/Cartfile Outdated Show resolved Hide resolved
@leoAsana
Copy link
Contributor Author

leoAsana commented Dec 6, 2019

Thanks for the review. I applied all the request changes.

@AlexBenny AlexBenny changed the base branch from master to issue/464-add_watchkit_support December 16, 2019 11:57
@AlexBenny AlexBenny changed the base branch from issue/464-add_watchkit_support to master December 16, 2019 12:00
@AlexBenny AlexBenny changed the base branch from master to issue/464-add_watchkit_support December 16, 2019 12:04
@AlexBenny AlexBenny merged commit 480dd97 into snowplow:issue/464-add_watchkit_support Dec 16, 2019
@AlexBenny
Copy link
Contributor

Thanks @leoAsana for your great work!
I've just merged your branch in our branch 464-add_watchkit_support for rebasing with the last changes before to merge it to the release branch.
I'll keep you informed when done!

AlexBenny pushed a commit that referenced this pull request Dec 17, 2019
* Adds WatchKit support via CocoaPods

* Fixes missing file for watchOS

* Change watchOS product name

* Adds modual map for watchos

* Adds carthage watch example

* Adds cocoapods watch example

* Update CocoaPods demo to include older watchOS version

I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this,

* Fix crash on < watchOS 6

This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec.

* Changed button name
AlexBenny pushed a commit that referenced this pull request Jan 14, 2020
* Adds WatchKit support via CocoaPods

* Fixes missing file for watchOS

* Change watchOS product name

* Adds modual map for watchos

* Adds carthage watch example

* Adds cocoapods watch example

* Update CocoaPods demo to include older watchOS version

I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this,

* Fix crash on < watchOS 6

This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec.

* Changed button name
AlexBenny added a commit that referenced this pull request Jan 14, 2020
PR #478

* Adds WatchKit support via CocoaPods (#465)

* Fixes missing file for watchOS

* Change watchOS product name

* Adds modual map for watchos

* Adds carthage watch example

* Adds cocoapods watch example

* Update CocoaPods demo to include older watchOS version

I noticed that apps using this library could not launch on older watchOS version. This adds a demo project to reproduce this,

* Fix crash on < watchOS 6

This fixes the crash mentioned in the prev. commit. It removes the frameworks for watchOS from the Podspec.

* Changed button name

* Update podspec for watchOS target

Updated podspec excluding the new Reachability feature added when we got rid of the third part dependency Reachability.swift

* Update travis scripts

* Pod update

Co-authored-by: leoAsana <56038088+leoAsana@users.noreply.github.com>
@AlexBenny AlexBenny mentioned this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants