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

[Intents] Update bindings for Xcode 13.0 betas 1 through 5 #12771

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

rachelkang
Copy link
Contributor

build and intro and xtro and I are all happy :)

@rachelkang rachelkang added the note-highlight Worth calling out specifically in release notes label Sep 17, 2021
@rachelkang rachelkang added this to the xcode13.0 milestone Sep 17, 2021
@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

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

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

5 tests failed, 97 tests passed.

Failed tests

  • introspection/Mac Catalyst [dotnet]/Debug [dotnet]: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)
  • monotouch-test/watchOS 32-bits - simulator/Debug: BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Debug (LinkSdk): BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Debug (static registrar): BuildFailure
  • monotouch-test/watchOS 32-bits - simulator/Release (all optimizations): BuildFailure

Pipeline on Agent XAMBOT-1100.BigSur'
Merge d1ce9ff into d94a0f5

Comment on lines 22 to 24
[SupportedOSPlatform ("macos12.0")]
#endif //!NET
public enum INPersonType {
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is off here, and it looks like the indentation is spaces too (when it should be tabs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh thanks for catching!

src/intents.cs Show resolved Hide resolved
src/intents.cs Outdated
[iOS (10, 0)]
[Watch (3, 2)]
[NoTV, Mac (11,0)]
[NoWatch] // it says it's now NoWatch, but not deprecated? (there was previous watch availability)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, if the type was already present in an earlier version of Xamarin.iOS, we can't remove it now (we have to mark it as deprecated instead).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, thanks!

src/intents.cs Show resolved Hide resolved
src/intents.cs Show resolved Hide resolved
src/intents.cs Show resolved Hide resolved
{

#if !NET
[Introduced (PlatformName.iOS, 15,0)]
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is still off for all the attributes in this file (looks like they should be indented one more level).

#else
[SupportedOSPlatform ("ios15.0")]
[SupportedOSPlatform ("macos12.0")]
#endif //!NET
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like extra indentation here

src/intents.cs Outdated

[Watch (7,0), NoTV, NoMac, iOS (14,0)]
interface INStartCallIntent : UNNotificationContentProviding {

Copy link
Contributor

Choose a reason for hiding this comment

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

extra whitespace

@tj-devel709
Copy link
Contributor

tj-devel709 commented Sep 20, 2021

I know Sebastien explained a little to me why, but I think I am still a little confused about something. It makes sense to me to turn a 'NoMac' to a 'Mac 12,0' but why are we changing the already available Mac attributes (ie. Mac 11,3) to 'Mac 12,0' inside src/intents.cs?

@rachelkang
Copy link
Contributor Author

I know Sebastien explained a little to me why, but I think I am still a little confused about something. It makes sense to me to turn a 'NoMac' to a 'Mac 12,0' but why are we changing the already available Mac attributes (ie. Mac 11,3) to 'Mac 12,0' inside src/intents.cs?

@tj-devel709 yeah, it's confusing! there are slightly different justifications based on the scenario, but for the most part, what happened was that the interface was originally NoMac and some properties inside of them appeared to have some earlier Mac availability. But if I understood Sebastien correctly, this was incorrect to begin with because "it should not exist in Xamarin.Mac if the type has [NoMac]"

@tj-devel709
Copy link
Contributor

Ah I see now! Thanks for clearing that up for me, @rachelkang!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

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

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 101 tests passed.

Failed tests

  • Xtro/Mac: Failed (Test run failed.)

Pipeline on Agent XAMBOT-1104.BigSur'
Merge b14b8cb into 499a69e

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

Awesome! Only minor things

@@ -6862,6 +6955,55 @@ interface INSetSeatSettingsInCarIntentResponse {
INSetSeatSettingsInCarIntentResponseCode Code { get; }
}

[Watch (8,0), NoTV, Mac (12,0), iOS (15,0), MacCatalyst (15,0)]
[BaseType (typeof(INIntent))]
Copy link
Member

Choose a reason for hiding this comment

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

Probably you want a DisableDefaultCtor here because there is no way to set FocusStatus unless the provided ctor is used

src/intents.cs Outdated
{
[Export ("initWithIsFocused:")]
[DesignatedInitializer]
IntPtr Constructor ([NullAllowed] NSNumber isFocused);
Copy link
Member

Choose a reason for hiding this comment

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

You can use BindAs here, see swift docs

Suggested change
IntPtr Constructor ([NullAllowed] NSNumber isFocused);
IntPtr Constructor ([NullAllowed] [BindAs (typeof (bool?))] NSNumber isFocused);

[DesignatedInitializer]
IntPtr Constructor ([NullAllowed] NSNumber isFocused);

[Export ("isFocused", ArgumentSemantic.Copy)]
Copy link
Member

Choose a reason for hiding this comment

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

Same, you can use BindAs to provide a better binding here

Suggested change
[Export ("isFocused", ArgumentSemantic.Copy)]
[Export ("isFocused", ArgumentSemantic.Copy)]
[BindAs (typeof (bool?))]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

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

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 101 tests passed.

Failed tests

  • dont link/Mac Catalyst [dotnet]/Release [dotnet]: TimedOut (Execution timed out after 1200 seconds.
    No test log file was produced)

Pipeline on Agent XAMBOT-1097.BigSur'
Merge fc58ff1 into ae08971

@dalexsoto
Copy link
Member

Unrelated test failure

@dalexsoto dalexsoto merged commit 70dd0d3 into xamarin:main Sep 22, 2021
@dalexsoto
Copy link
Member

/sudo backport xcode13-ios

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Backport Job to branch xcode13-ios Created! The magic is happening here

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Oh no! Backport failed! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5233974 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants