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

fix(ios): add missing runtime checks #10729

Merged
merged 6 commits into from
Apr 17, 2019
Merged

Conversation

janvennemann
Copy link
Contributor

@janvennemann janvennemann commented Feb 27, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26829

The __IPHONE_XX_X macros are evaluated at compile time which could lead to crashes on devices running a lower version than what the macros checked against. To prevent that i added runtime checks for the required version in the media module which was affected by the crash.

@build
Copy link
Contributor

build commented Feb 27, 2019

Messages
📖

✅ All tests are passing
Nice one! All 1881 tests are passing.

Generated by 🚫 dangerJS against 0c8ef3e

MPMediaItemPropertyLastPlayedDate, @"lastPlayedDate",
MPMediaItemPropertyUserGrouping, @"userGrouping",
MPMediaItemPropertyBookmarkTime, @"bookmarkTime",
#ifdef __IPHONE_10_0
Copy link
Collaborator

Choose a reason for hiding this comment

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

These have been used to ensure Xcode compatibility (in the above case to support Xcode 7 although the API would require Xcode 8). Is there a concept to support this in the future? I could think of class extensions that are injected outside TitaniumKit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh thanks for the explanation, that makes sense. So i think we still need these but with an additional runtime check like in the WebView below to prevent the runtime crash on devices that are actually below the checked iOS version.

@janvennemann
Copy link
Contributor Author

@vijaysingh-axway can you give this another look?

@janvennemann janvennemann changed the title fix(ios): replace pre-processor macros with runtime checks fix(ios): add missing runtime checks Mar 6, 2019
@vijaysingh-axway
Copy link
Contributor

@janvennemann @hansemannn As per previous history in titanium sdk, I can say that we guard current Xcode sdk and current Xcode sdk - 1. e.g in titanium SDK 8.0.0 we guard against Xcode 10 and Xcode 9.
iOS 10 comes with Xcode 8. Here we are guarding for Xcode 8 using #ifdef __IPHONE_10_0. Should we guard for Xcode 8 as well? Though this will not create any problem. Am I missing anything?

@janvennemann
Copy link
Contributor Author

Well, technically it is possible to use old iOS SDKs with newer Xcode versions. So i think the check against the iOS SDK is more sophisticated and generally more expressive. It makes clear what the check is meant to do: check the iOS SDK version.

Although Xcode versions are (usually) tied to a specific iOS SDK checking for the Xcode version seems confusing to me. If you don't know the SDK version shipped with Xcode you need to look that up.

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

CR passed.

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed Titanium.Media.openMusicLibrary no longer returns empty items on iOS 9.3.5 and no longer crashes the application. Tested using the test case from: https://jira.appcelerator.org/browse/TIMOB-26829

Test Environment

iphone 5s (9.3.5)
APPC CLI: 7.0.10
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1

@build
Copy link
Contributor

build commented Apr 17, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3787 tests are passing.

Generated by 🚫 dangerJS against ed37f1c

@ssjsamir ssjsamir merged commit 5fa3cbf into tidev:master Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants