-
Notifications
You must be signed in to change notification settings - Fork 71
feat: Implement Manual SceneDelegate Support #453
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
feat: Implement Manual SceneDelegate Support #453
Conversation
7394c34 to
75d68a4
Compare
75d68a4 to
775315c
Compare
985e4aa to
e1ba42d
Compare
|
|
||
| NSData *ekConfigData = [NSJSONSerialization dataWithJSONObject:configurationDictionary options:0 error:nil]; | ||
| NSJSONWritingOptions options = 0; | ||
| if (@available(iOS 11.0, tvOS 11.0, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this check? Our min iOS version is 15.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current minimum deployment target is 9.0 though I do plan on upping that in future PRs/tickets. Would you like me to bring that into this ticket? @denischilik
| NSString *messageAnnotation = [NSString stringWithFormat:@"Annotation: %@", urlContext.options.annotation]; | ||
| [logger debug:messageAnnotation]; | ||
| #if TARGET_OS_IOS == 1 | ||
| if (@available(iOS 14.5, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we also don't need this check
| #endif | ||
| import XCTest | ||
|
|
||
| @available(iOS 13.0, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same
UnitTests/SwiftTests/MParticle/MParticleSceneDelegateTests.swift
Outdated
Show resolved
Hide resolved
UnitTests/SwiftTests/MParticle/MParticleSceneDelegateTests.swift
Outdated
Show resolved
Hide resolved
UnitTests/SwiftTests/MParticle/MParticleSceneDelegateTests.swift
Outdated
Show resolved
Hide resolved
UnitTests/SwiftTests/MParticle/MParticleSceneDelegateTests.swift
Outdated
Show resolved
Hide resolved
|
Thanks for the comments @denischilik I'm happy to make the additional change. I just wanted to be cautious around it maybe do the version update and removal of the obsolete code in its own PR. But my opinion could be swayed. |
|
Separating the bump of minimum version makes sense. Wondering if this PR should be targeting main though or if it should be a workstation for code freeze? On the Rokt side I've created a major version bump workstation to collate everything |
This code at least, I would like to get out in the current major version. It's adding functionality we're missing for sceneDelegate clients and doesn't remove or change (besides marking as deprecated) any existing functionality. Though for clarity, the major version bump your talking about is the one your planning for post holidays right? The bump of the minimum version (and removal of deprecated code) should certainly be in that. |
This reverts commit e5cbe84.
Background
What Has Changed
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)