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.

Sign up for GitHub

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

Jump to bottom

fix(ios): guard new picker types #12020

Merged
merged 3 commits into from Sep 9, 2020
Merged

fix(ios): guard new picker types #12020

merged 3 commits into from Sep 9, 2020

Conversation

ewanharris
Copy link
Collaborator

@ewanharris ewanharris commented Sep 8, 2020 •
edited

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

Keeps compatibility with the stated xcode versions, from what I can tell this seems safe to use? It fixes the issue on 11.3.1 for me and I assume it's backwards compatible

@ewanharris ewanharris requested a review from a team September 8, 2020 13:58
@build build added this to the 9.2.0 milestone Sep 8, 2020
@build
Copy link
Contributor

build commented Sep 8, 2020 •
edited

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.
Messages
📖

💾 Here's the generated SDK zipfile.
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

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

Generated by 🚫 dangerJS against 6ad4e42

sgtcoolguy
sgtcoolguy reviewed Sep 8, 2020
View reviewed changes
iphone/Classes/TiUIiOSProxy.m Outdated
@@ -192,6 +192,7 @@ - (NSNumber *)ROW_ACTION_STYLE_NORMAL

#ifdef USE_TI_UIPICKER

#if __IPHONE_13_4
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure of the typical guards used on the iOS codebase. Looks like we tend to use __IPHONE_OS_VERSION_MAX_ALLOWED? Also, I think e may want the guards to be placed inside each method so if it's not defined we return the -1 value that we return in the runtime guard case where iOS < 13.4

Copy link
Collaborator Author

@ewanharris ewanharris Sep 8, 2020 •
edited

Choose a reason for hiding this comment

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

I tried to see if there was any form of existing IS_SDK_IOS_XYZ and the only similar thing I found was this in TiUIWebViewProxy https://github.com/appcelerator/titanium_mobile/blob/840b0d279f79248d1511fc518fa28fda9573be73/iphone/Classes/TiUIWebViewProxy.m#L619

I wasn't sure which form is right, a new IS_SDK_IOS_134 or this one that seems to be built in (or the difference of them).

Edit: For the positioning, I just matched the same as the iOS 14 guard. Should that also be moved into the method to allow returning -1 on <14?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ewanharris 1. __IPHONE_OS_VERSION_MAX_ALLOWED is preferred way.
2. Positioning is fine.
3. Need to guard in function https://github.com/appcelerator/titanium_mobile/blob/00f4b58d03678d1a266fa9a75713f938556f105a/iphone/Classes/TiUIPicker.m#L182 as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated as requested!

vijaysingh-axway
vijaysingh-axway reviewed Sep 8, 2020
View reviewed changes
iphone/iphone/Titanium_Prefix.pch Outdated
@@ -17,6 +17,12 @@
#define IS_SDK_IOS_14 false
#endif

#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 130400
#define IS_SDK_IOS_134 true
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to change this macro to IS_SDK_IOS_13_4. Convention should be IS_SDK_IOS_major_minor.
Everything else looks good.

@sgtcoolguy sgtcoolguy merged commit fa8f547 into tidev:master Sep 9, 2020
@build build mentioned this pull request Sep 9, 2020
Merged
@ewanharris ewanharris deleted the TIMOB-28112 branch August 31, 2021 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgtcoolguy sgtcoolguy sgtcoolguy left review comments

@vijaysingh-axway vijaysingh-axway vijaysingh-axway left review comments

Assignees
No one assigned
Labels
bug ios needs tests 🚨
Projects
None yet
Milestone
9.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ewanharris @build @sgtcoolguy @vijaysingh-axway