-
Notifications
You must be signed in to change notification settings - Fork 515
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
[GameController] Add missing API for XCode15. Fixes #15725 #19708
base: main
Are you sure you want to change the base?
[GameController] Add missing API for XCode15. Fixes #15725 #19708
Conversation
This PR adds the missing bindings and fixes xamarin#15725. What the issue reported is incorrect, the problem with the API is that it uses Swift enumerators that have a String RawValue. The way to fix this in our bindings is to use the string values that are **PRESENT** in the headers.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
I think this also needs tests, even for a sample of the generated bindings, because the API is somewhat uncommon.
public NSEnumerator<IGCPhysicalInputElement> ElementEnumerator | ||
=> Runtime.GetNSObject<NSEnumerator<IGCPhysicalInputElement>> (_ElementEnumerator)!; |
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.
This should be a property, not a field.
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.
This is a property, notice the '=>' and not a '='
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.
Ah - this is exactly why Sebastien said to not use => for properties, it's a very small difference from fields (just a single character), and can end up confusing.
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's also public
and not private
which is the better indicator for me personally.
Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
The issue with this API is that in most cases it needs a controller to test. I have an xbox controller which I can use to write a sample, but adding that in the CI will be problematic. I have added all the suggested changes. Shall we do a sample? /cc @dalexsoto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 I have to agree with @rolfbjarne and my old (younger?) self, this ideally requires a sample.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💻 [CI Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
❌ [CI Build] Tests on macOS M1 - Mac Ventura (13.0) failed ❌Failed tests are:
Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🔥 [CI Build] Test results 🔥Test results❌ Tests failed on VSTS: simulator tests 1 tests crashed, 4 tests failed, 229 tests passed. Failures❌ introspection tests
Html Report (VSDrops) Download ❌ msbuild tests🔥 Failed catastrophically on VSTS: simulator tests - msbuild (no summary found). Html Report (VSDrops) Download Successes✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
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.
public NSEnumerator<IGCPhysicalInputElement> ElementEnumerator | ||
=> Runtime.GetNSObject<NSEnumerator<IGCPhysicalInputElement>> (_ElementEnumerator)!; |
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's also public
and not private
which is the better indicator for me personally.
@@ -181,6 +181,9 @@ protected override bool Skip (Type type) | |||
if (Mac.CheckSystemVersion (10, 15)) | |||
return true; | |||
break; | |||
case "GameController.GCPhysicalInputSource": |
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.
Should these use constants, enumerations, or nameof
?
case "ASAuthorizationProviderExtensionRegistrationHandler": | ||
return true; | ||
case "GCPhysicalInputSource": | ||
return 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.
Should the case
values be using constants, enumerations, or nameof
?
This PR adds the missing bindings and fixes #15725. What the issue reported is incorrect, the problem with the API is that it uses Swift enumerators that have a String RawValue. The way to fix this in our bindings is to use the string values that are PRESENT in the headers.