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

[StoreKit] Update framework to Xcode 12 beta 4. #9387

Merged
merged 9 commits into from
Aug 19, 2020

Conversation

mandel-macaque
Copy link
Member

No description provided.

src/storekit.cs Outdated

[Watch (7, 0), TV (14, 0), Mac (11, 0), iOS (14, 0)]
[Export ("transactionObservers")]
[Protocolize]
Copy link
Contributor

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]

Copy link
Member Author

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

missing [iOS] attribute

Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Contributor

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 Show resolved Hide resolved
src/storekit.cs Show resolved Hide resolved
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.")]
Copy link
Contributor

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)]
Copy link
Contributor

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 Show resolved Hide resolved
src/storekit.cs Outdated
[DesignatedInitializer]
IntPtr Constructor (SKOverlayConfiguration configuration);

[iOS (14,0)]
Copy link
Contributor

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)]
Copy link
Contributor

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)

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 75 tests passed.

Failed tests

  • introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

src/storekit.cs Show resolved Hide resolved
#if XAMCORE_3_0 // Avoid breaking change in iOS
[DisableDefaultCtor]
#endif
interface SKCloudServiceController {
Copy link
Member

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 :/

Copy link
Member Author

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

Copy link
Contributor

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

mandel-macaque and others added 3 commits August 17, 2020 13:43
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
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)]
Copy link
Contributor

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

Copy link
Member Author

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

Copy link
Contributor

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)

Copy link
Member Author

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.

Suggested change
[Model (AutoGeneratedName = true)]
[Model]

Copy link
Contributor

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.

@monojenkins
Copy link
Collaborator

Build failure
Build failed or was aborted

Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 75 tests passed.

Failed tests

  • introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

}
#endif
}
#endif
Copy link
Member

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
Copy link
Member

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

@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

2 tests failed, 74 tests passed.

Failed tests

  • Xtro/Mac: Failed (Test run failed.)
  • introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
Test run succeeded

@mandel-macaque mandel-macaque merged commit 5ba2042 into xamarin:xcode12 Aug 19, 2020
@mandel-macaque mandel-macaque deleted the storekit-xcode12-beta4 branch August 19, 2020 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants