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): sf symbol handling for application shortcut #11220

Merged
merged 8 commits into from Sep 24, 2019

Conversation

vijaysingh-axway
Copy link
Contributor

@build build added this to the 8.3.0 milestone Sep 16, 2019
@build build requested review from a team September 16, 2019 22:30
@build
Copy link
Contributor

build commented Sep 16, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3899 tests are passing.
(There are 475 skipped tests not included in that total)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against f56e152

}

if (self = [super init]) {
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 130000
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 130000
#if IS_SDK_IOS_13

I feel a little uncomfortable only wrapping the image creation in the iOS 13 condition. Sure, this code block should never be reached when not built with iOS 13 SDK but only wrapping that single line inside it calls for unexpected behavior in the following lines, e.g. when module developers use TiBlob and wrongfully use initWithSystemImage:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Wrapped whole function inside macro.

Macro IS_SDK_IOS_13 will not be available inside TitaniumKit framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah damn, i keep forgetting that the macro is for our other source files only. Maybe we should define the same macro in TitaniumKit as well, so we have some consistency across our source files.

@ssekhri
Copy link

ssekhri commented Sep 24, 2019

FR Passed. An image blob is shown as part fo the app shortcut. Also no error is shown when adding the shortcut.
Verified on:
Mac OS 10.14.6
Ti SDK: 8.3.0.v20190923105712
Appc CLI: 7.1.1
Node: 10.5.0
JDK: 10.0.2
XCode: 11.0
Studio: 5.1.4.201909061933
Device: iPhone X(v13.0)

@ssekhri ssekhri merged commit 28907e0 into tidev:master Sep 24, 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