-
Notifications
You must be signed in to change notification settings - Fork 518
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
[AppKit] Add support for Xcode 15. #18762
[AppKit] Add support for Xcode 15. #18762
Conversation
|
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.
It might also be an idea to do the bindings for UIKit before merging this one, in case there's identical API that should be bound identically.
[UnsupportedOSPlatform ("ios17.0")] | ||
[UnsupportedOSPlatform ("maccatalyst17.0")] | ||
[UnsupportedOSPlatform ("macos14.0")] | ||
#if __MACCATALYST__ | ||
[Obsolete ("Starting with maccatalyst17.0 will always return null.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")] | ||
#elif IOS | ||
[Obsolete ("Starting with ios17.0 will always return null.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")] | ||
#elif MONOMAC | ||
[Obsolete ("Starting with macos14.0 wll always return null.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")] | ||
#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.
Use [ObsoletedOSPlatform ("ios17.0", "Starting with ...")]
instead of UnsupportedOSPlatform
+ Obsolete
.
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 I am outdated
// This field is really inside WebKit | ||
[NoWatch, NoTV] | ||
[iOS (13, 0)] | ||
[MacCatalyst (13, 1)] | ||
[Field ("NSReadAccessURLDocumentOption", "WebKit")] | ||
NSString NSReadAccessUrlDocumentOption { get; } |
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 this is still available?
https://developer.apple.com/documentation/webkit/nsreadaccessurldocumentoption?language=objc
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.
Intro && xtro say the opposite. Should we leave it?
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.
Yea, this is weird, seems like something people would use fairly frequently. I think it's best to leave it in for now.
tests/cecil-tests/ConstructorTest.cs
Outdated
if (intptrCtor is not null) { | ||
if (intptrCtor is not null && intptrCtor.Name == "init:") { |
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.
What failed here to make this necessary?
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 following constructor:
[Export ("initWithCount:")]
NativeHandle Constructor (nint itemCount);
Yet now that you mention it, is intptrConstr.Name the selector name, I think I added a big 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.
I updated the code accordingly checking the ExportAttr.
|
||
# the method is a static constructor that return a label for a New item, they used New not to represent the intent of the | ||
# method, but what label it returns (probably a new objc developer at apple) | ||
!missing-release-attribute-on-return-value! AppKit.NSMenuItemBadge AppKit.NSMenuItemBadge::CreateNewItems(System.IntPtr)'s selector's ('newItemsWithCount:') Objective-C method family ('new') indicates that the native method returns a retained object, and as such a '[return: Release]' attribute is required. |
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 test that the selector in question doesn't return a retained object? AFAIK that's automatic in the Objective-C compiler, so unless Apple's developer did something specific to get a different behavior, we'll get a retained object.
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.
Yes, this is from someone that did not know about Objc, the Badge it creates is fo "New Items" as opossed to Update Items.. a very poorly chosen API if you consider the objc common practice :/
src/appkit.cs
Outdated
@@ -10551,6 +10691,65 @@ interface NSImageView : NSAccessibilityImage, NSMenuItemValidation { | |||
[Mac (11, 0)] | |||
[Export ("symbolConfiguration", ArgumentSemantic.Copy)] | |||
NSImageSymbolConfiguration SymbolConfiguration { get; set; } | |||
|
|||
// from NSSymbolEffect (NSImageView) |
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.
Documentation says NSSymbolEffect is a base class, so why not bind it as such?
https://developer.apple.com/documentation/symbols/nssymboleffect
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.
Xtro reports the following:
Looking at xtro we get:
!missing-selector! +NSImageView::defaultPreferredImageDynamicRange not bound
!missing-selector! +NSImageView::setDefaultPreferredImageDynamicRange: not bound
!missing-selector! NSImageView::addSymbolEffect: not bound
!missing-selector! NSImageView::addSymbolEffect:options: not bound
!missing-selector! NSImageView::addSymbolEffect:options:animated: not bound
!missing-selector! NSImageView::imageDynamicRange not bound
!missing-selector! NSImageView::preferredImageDynamicRange not bound
!missing-selector! NSImageView::removeAllSymbolEffects not bound
!missing-selector! NSImageView::removeAllSymbolEffectsWithOptions: not bound
!missing-selector! NSImageView::removeAllSymbolEffectsWithOptions:animated: not bound
!missing-selector! NSImageView::removeSymbolEffectOfType: not bound
!missing-selector! NSImageView::removeSymbolEffectOfType:options: not bound
!missing-selector! NSImageView::removeSymbolEffectOfType:options:animated: not bound
!missing-selector! NSImageView::setPreferredImageDynamicRange: not bound
!missing-selector! NSImageView::setSymbolImage:withContentTransition: not bound
!missing-selector! NSImageView::setSymbolImage:withContentTransition:options: not bound
We are looking at the category on the NSImageView class.
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.
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.
This comment has been minimized.
This comment has been minimized.
src/appkit.cs
Outdated
|
||
[Mac (14, 0)] | ||
[Export ("curveToPoint:controlPoint:")] | ||
void Curve (CGPoint endPoint, CGPoint controlPoint); |
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.
void Curve (CGPoint endPoint, CGPoint controlPoint); | |
void CurveTo (CGPoint endPoint, CGPoint controlPoint); |
Maybe verbify - looks like the style from other methods is adding the 'To" after
src/appkit.cs
Outdated
|
||
[Mac (14, 0)] | ||
[Export ("relativeCurveToPoint:controlPoint:")] | ||
void RelativeCurve (CGPoint endPoint, CGPoint controlPoint); |
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.
void RelativeCurve (CGPoint endPoint, CGPoint controlPoint); | |
void RelativeCurveTo (CGPoint endPoint, CGPoint controlPoint); |
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 reverting literally a previos comment added by @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.
In this PR or a different one? I'm not sure if I follow
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.
An other PR. But looking at the fact that we already have a ConvertTo, is best to be consistent within the framework. I'll change it back.
src/appkit.cs
Outdated
[Mac (14, 0)] | ||
[Abstract] | ||
[Export ("accessibilityUserInputLabels", ArgumentSemantic.Copy)] | ||
string [] AccessibilityUserInputLabels { get; set; } | ||
|
||
[Mac (14, 0)] | ||
[Abstract] | ||
[Export ("accessibilityAttributedUserInputLabels", ArgumentSemantic.Copy)] | ||
NSAttributedString [] AccessibilityAttributedUserInputLabels { get; set; } |
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.
Isn't it a breaking change to add abstract members to an existing protocol?
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 missed that, you are correct.
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.
after other comments are addressed 👍
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.
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] 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 stable (Breaking changes)Legacy Xamarin (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:).NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)ℹ️ 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 0 tests crashed, 28 tests failed, 209 tests passed. Failures❌ monotouch tests
Html Report (VSDrops) Download ❌ xammac tests
Html Report (VSDrops) Download ❌ xtro tests
Html Report (VSDrops) Download Successes✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
ed462d5
to
c862bf6
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.