-
Notifications
You must be signed in to change notification settings - Fork 511
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
[StoreKit] Update framework to Xcode 12 beta 4. #9387
[StoreKit] Update framework to Xcode 12 beta 4. #9387
Conversation
src/storekit.cs
Outdated
|
||
[Watch (7, 0), TV (14, 0), Mac (11, 0), iOS (14, 0)] | ||
[Export ("transactionObservers")] | ||
[Protocolize] |
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.
we stopped using this after XAMCORE_2_0 - that was only useful when supporting classic
use the interface for the protocol instead
side note: we have warnings about exposing concrete (vs interface) types in bindings but I'm not 💯% sure they all work with [Protocolize]
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.
They do not, and I used because it was already in the file, doing it the right way.
@@ -525,34 +545,51 @@ interface SKStoreProductViewController { | |||
void LoadProduct (StoreProductParameters parameters, [NullAllowed] Action<bool,NSError> callback); | |||
} | |||
|
|||
[Mac (11,0), NoTV, NoWatch] |
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.
missing [iOS]
attribute
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.
hmm... that was moved :(
I rather have more #if XXX
than moving code (to preserve git history)
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.
Was added in iOS 6,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.
yeah, larger diff (than required) due to code being moved
src/storekit.cs
Outdated
@@ -797,9 +834,16 @@ interface SKProductStorePromotionController { | |||
[DisableDefaultCtor] // Not specified but very likely | |||
interface SKStoreReviewController { | |||
|
|||
[Deprecated (PlatformName.iOS, 9, 0, message : "Use the 'RequestReview (UIWindowScene windowScene)' API instead.")] |
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.
nope, because
- this API was added in iOS 10.3 - so it can't be deprecated in iOS 9.0
UIWindowScene
was added in iOS 13 (iirc)
src/storekit.cs
Outdated
[Static] | ||
[Export ("requestReview")] | ||
void RequestReview (); | ||
|
||
[Introduced (PlatformName.MacCatalyst, 14, 0)] | ||
[NoWatch, NoTV, iOS (14,0), Mac (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.
[Mac (11,0)]
is incorrect as UIWindowScene
does not exists in macOS
This compiled because you defined it to be NSObject
but that won't work at runtime
src/storekit.cs
Outdated
[DesignatedInitializer] | ||
IntPtr Constructor (SKOverlayConfiguration configuration); | ||
|
||
[iOS (14,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.
not needed (already on type)
src/storekit.cs
Outdated
[Export ("presentInScene:")] | ||
void PresentInScene (UIWindowScene scene); | ||
|
||
[iOS (14,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.
not needed (already on type)
Build failure Test results1 tests failed, 75 tests passed.Failed tests
|
#if XAMCORE_3_0 // Avoid breaking change in iOS | ||
[DisableDefaultCtor] | ||
#endif | ||
interface SKCloudServiceController { |
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.
Any reason to move this class? It makes the diff harder to review :/
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.
Moved it out of the #if over adding extra #if #else
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.
yeah, better the extra #if
next time
or do a separate PR to move things
moving and adding makes the diff more difficult to review
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
…marin-macios into storekit-xcode12-beta4
src/storekit.cs
Outdated
@@ -525,34 +546,59 @@ interface SKStoreProductViewController { | |||
void LoadProduct (StoreProductParameters parameters, [NullAllowed] Action<bool,NSError> callback); | |||
} | |||
|
|||
[Mac (11,0), NoTV, NoWatch] | |||
[BaseType (typeof (NSObject))] | |||
[Model (AutoGeneratedName = true)] |
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.
did you add AutoGeneratedName = true
iirc that's a potential breaking change in existing models
c.c. @rolfbjarne
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.
@spouliot added as per @rolfbjarne request
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.
hmm... is this a new type ? (then it needs it)
or does it look new because the diff is hard to read (and the code just moved) ? (then I don't think it should be there)
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.
It was moved. So no need.
[Model (AutoGeneratedName = true)] | |
[Model] |
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.
Please don't resolve a conversation you have not started. Makes things harder to review again.
Build failure ✅ Build succeeded |
Build failure Test results1 tests failed, 75 tests passed.Failed tests
|
} | ||
#endif | ||
} | ||
#endif |
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.
No new line at end of file
} | ||
#endif | ||
} | ||
#endif |
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.
No newline at end of file
…marin-macios into storekit-xcode12-beta4
Build failure Test results2 tests failed, 74 tests passed.Failed tests
|
Build success |
No description provided.