This repository has been archived by the owner on Jan 17, 2019. It is now read-only.

Add tvOS support #263

Open

marmelroy wants to merge 7 commits into spotify:master from marmelroy:tvOS

Contributor

marmelroy commented Dec 26, 2016 •

edited

Loading

Fixes https://github.com/spotify/HubFramework/issues/164.

There are several things going on here:

Made HubFramework compile for tvOS (lots of compile flags).
Added support for the focus engine (a little trickier than expected, especially with nested collection views).
Added a HubComponentWithFocusState protocol so that a component can update its view when in focus.
Added a tvOS test target.
Updated the podspec.

Missing:

I did not update the CI setup. I'm not super familiar with using fastlane for this (often use a simple shell script) and wanted to avoid breaking something.
A tvOS demo app (to come in a separate PR later on).

spotify-ci-bot commented Dec 26, 2016 •

edited

Loading

	1 Warning
⚠️	External contributor has edited the HubFramework.podspec

	3 Messages
📖	Executed 340 tests, with 0 failures (0 unexpected) in 8.045 (9.913) seconds
📖	Executed 337 tests, with 0 failures (0 unexpected) in 6.807 (8.331) seconds
📖	Executed 9 tests, with 0 failures (0 unexpected) in 146.199 (146.239) seconds

Generated by 🚫 danger

marmelroy force-pushed the tvOS branch from 167a5c1 to 69ae7b4 Compare

December 26, 2016 17:21

codecov-io commented Dec 26, 2016 •

edited

Loading

Current coverage is 93.11% (diff: 36.66%)

Merging #263 into master will increase coverage by 93.11%

@@           master       #263   diff @@
========================================
  Files          72         72           
  Lines        5019       5042     +23   
  Methods         0          0           
  Messages        0          0           
  Branches        0          0           
========================================
+ Hits            0       4695   +4695   
+ Misses       5019        347   -4672   
  Partials        0          0

Powered by Codecov. Last update 6e9595b...43eb2c4

marmelroy force-pushed the tvOS branch from 078a6d6 to 711728d Compare

December 27, 2016 05:36

cerihughes reviewed

View reviewed changes

tests/HUBViewControllerTests.m

               - (void)testProposedContentInsetIsDefaultIfHeaderMissing
               {
+                  #if !TARGET_OS_TV

Contributor

cerihughes Jan 2, 2017

Should this encompass the whole test (including the method signature) as with testAdaptingOverlayComponentCenterPointToKeyboard?

rastersize added framework tvos labels

rastersize reviewed

View reviewed changes

Contributor

rastersize left a comment

Nice stuff! 😺

You might also be interested in @JohnSundell’s pass at the same task: master...JohnSundell:tvos

I haven’t look that much at the linked code. Main difference I know about it that the framework is universal. I.e. the same target is used for both iOS and tvOS. Not sure which approach we want though. Xcode doesn’t seem to like multi-platform targets, showing way to many destinations in the build device destination dropdown menu,

HubFramework.xcodeproj/project.pbxproj

+C5F101E09CF9A00597B61 /* Debug */ = {
+              			isa = XCBuildConfiguration;
+              			buildSettings = {
+              				ALWAYS_SEARCH_USER_PATHS = NO;

Contributor

rastersize Jan 3, 2017

We keep all of these settings in our spotify_os.xcconfig file. Could you instead set the target to use the project.xcconfig file and then remove all of these overrides.

Unless a config option is specifically required for a framework or differs from the normal xcconfig. We’ll later look into moving those values to an xcconfig as well.

Contributor Author

marmelroy Jan 10, 2017

Finally had the chance to look into this. So... I basically just duplicated the setup of the current HubFramework-iOS dynamic target and made my tvOS modifications. All of these overrides are currently in production.

I will make this modification to both dynamic targets then.

HubFramework.xcodeproj/project.pbxproj

+C5F111E09CF9A00597B61 /* Release */ = {
+              			isa = XCBuildConfiguration;
+              			buildSettings = {
+              				ALWAYS_SEARCH_USER_PATHS = NO;

Contributor

rastersize Jan 3, 2017

Same for release as for debug :)

HubFramework.xcodeproj/project.pbxproj

+              				SDKROOT = appletvos10.1;
+              				SUPPORTED_PLATFORMS = "appletvsimulator appletvos";
+              				WARNING_CFLAGS = (
+              					"$(inherited)",

Contributor

rastersize Jan 3, 2017

Same here as for the debug commands (remove overrides and use xcconfig values).

HubFramework.xcodeproj/project.pbxproj

+              				INFOPLIST_FILE = "$(SRCROOT)/sources/Info.plist";
+              				PRODUCT_BUNDLE_IDENTIFIER = com.spotify.HubFrameworkTests;
+              				PRODUCT_NAME = "$(TARGET_NAME)";
+              				SDKROOT = appletvos10.1;

Contributor

rastersize Jan 3, 2017

This should probably be set to just appletvos, right? Since we want to always use the latests SDK to build with.

HubFramework.xcodeproj/project.pbxproj

+              			buildSettings = {
+              				HUB_NON_ERROR_WARNINGS_0720 = "";
+              				HUB_NON_ERROR_WARNINGS_0730 = "$(HUB_NON_ERROR_WARNING_0720) -Wno-error=partial-availability";
+              				INFOPLIST_FILE = "$(SRCROOT)/sources/Info.plist";

Contributor

rastersize Jan 3, 2017

Shouldn’t we use Info-tvOS.plist?

include/HubFramework/HUBComponentWithFocusState.h

+              #import "HUBComponent.h"
+              /// Enum defining various focus states that a component can be in
+              typedef NS_ENUM(NSUInteger, HUBComponentFocusState) {

Contributor

rastersize Jan 3, 2017

NS_ENUM should use NSInteger as the backing value, unless there’s a specific reason for it being unsigned?

Contributor Author

marmelroy Jan 9, 2017

Erm... I was just being consistent with most of the other NS_ENUMs in the project (see HUBComponentSelectionState, HUBActionTrigger, HUBComponentLayoutContentEdge, HUBComponentType, HUBConnectivityState etc).

There's no specific reason I can think of for any of them to be unsigned. Should we change all enums?

Contributor Author

marmelroy commented Jan 4, 2017

Thanks for the feedback.

A universal framework is much easier to maintain and that's a huge plus. The main technical argument against a universal framework was that Carthage couldn't build it but that's no longer the case.

See these for context:
Alamofire/Alamofire#326
Alamofire/Alamofire#1064

Any other argument we should consider in favour of a platform-specific framework?

An additional difference with @JohnSundell's implementation is that I added an explicit HubComponentWithFocusState protocol whereas John merged focus state with highlighted state.

I will update the PR with the changes requested by @rastersize and @cerihughes and will steal a couple of ideas from @JohnSundell's branch 😄

Contributor

rastersize commented Jan 5, 2017

@marmelroy Thank you for the links and update, much appreciated! From reading the Alamofire discussions I think we should probably go with multiple targets. Like you’ve done in this PR.

marmelroy added 5 commits

January 9, 2017 23:10


          Adding tvOS support

d5cadc3


          Add tvOS to podspec

64d214d


          Style improvement on test if statement

6347ec8


          NS_enums are no longer unsigned across the framework

2794fb7


          Removing a siginificant chunk of overrides from both dynamic framewor…

37f37c4

…k targets

marmelroy force-pushed the tvOS branch from ade5616 to 37f37c4 Compare

January 10, 2017 07:05


          Improving tvOS test target

53017b1

Contributor Author

marmelroy commented Jan 10, 2017 •

edited

Loading

Sorry for the delay in fixing this - moving countries is quite time exhaustive. This PR should be ready for another review (cc @cerihughes, @rastersize ).

Summary of changes:

Syntax was improved as per @cerihughes request.
Change requested re unsigned enums was applied framework-wide.
Change requested re configuration overrides was due to duplication of the dynamic target on production. Overrides were removed from both iOS and tvOS targets.
The test target was improved a little.

Note:
testScrollingToBottomOfViewLoadsPaginatedContent was being flaky. I improved the log we get when it fails. We should monitor the issue.


          More descriptive error for flaky pagination UI test

43eb2c4

Contributor

hallski commented Jan 19, 2017

I would prefer we wait with this until we have the API more stable, we also need to refactor HUBViewController to do less and be easier to reason about which will become more difficult by having to care for tvOS as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels