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)(8_2_X): sf symbol handling for application shortcut #11222

Merged
merged 5 commits into from Sep 24, 2019

Conversation

vijaysingh-axway
Copy link
Contributor

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

build commented Sep 16, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4336 tests are passing.
(There are 471 tests skipped)

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

Generated by 🚫 dangerJS against 9427a25

@sgtcoolguy sgtcoolguy modified the milestones: 8.2.0, 8.2.1 Sep 17, 2019
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

docs need a tweak

apidoc/Titanium/UI/iOS/ApplicationShortcuts.yml Outdated Show resolved Hide resolved
@@ -211,6 +212,14 @@ - (UIApplicationShortcutIcon *)findIcon:(id)value
return [UIApplicationShortcutIcon iconWithTemplateImageName:[self urlInAssetCatalog:value]];
}

#ifdef IS_SDK_IOS_13
if ([value isKindOfClass:[TiBlob class]] && [TiUtils isIOSVersionOrGreater:@"13.0"]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to check the iOS version here if you have the ifdef guard as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Its needed. UIApplicationShortcutIcon's api 'iconWithSystemImageName' is not available for iOS < 13. Let's say I ran my app using sdk 8.2.1 with Xcode 11 and run on iOS 12, in that case it will give error at run time.

@@ -211,6 +212,15 @@ - (UIApplicationShortcutIcon *)findIcon:(id)value
return [UIApplicationShortcutIcon iconWithTemplateImageName:[self urlInAssetCatalog:value]];
}

#ifdef IS_SDK_IOS_13
if ([value isKindOfClass:[TiBlob class]] && [TiUtils isIOSVersionOrGreater:@"13.0"]) {
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 above...

Co-Authored-By: Christopher Williams <chris.a.williams@gmail.com>
if ([value isKindOfClass:[TiBlob class]] && [TiUtils isIOSVersionOrGreater:@"13.0"]) {
TiBlob *blob = (TiBlob *)value;
if (blob.type == TiBlobTypeSystemImage) {
return [UIApplicationShortcutIcon iconWithSystemImageName:blob.systemImageName];
Copy link
Collaborator

Choose a reason for hiding this comment

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

A wild guess here: Instead of creating new blob types and all this handling, why not just introduce a new systemIcon String proxy property and pass it directly to the iconWithSystemImageName constructor? That'd probably be around 10 lines of code in total.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn 1. UIApplicationShortcutIcon has 4 type of icons, which are getting handled from same place in SDK. And developer have to pass proper value to 'icon' property and it gets internally handled on basis of type.
2. Only one type of icon can be set at a time. Having 2 property will create confusion.
3. Similar type of handling sf symbols will be better for developer.

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

LGTM!

@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.2.0.v20190920114538
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 b1d6ce0 into tidev:8_2_X Sep 24, 2019
rlustemberg added a commit to inzdr/titanium_mobile that referenced this pull request Sep 25, 2019
* 8_2_X:
  fix(ios): properly set tint-color on image-view (tidev#11233)
  fix(ios)(8_2_X): sf symbol handling for application shortcut (tidev#11222)
  fix(ios): volume event handling (tidev#11227)
  fix(ios): also lookup semnantic colors in correct location for classic
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

6 participants