-
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] Update Xcode 13.0 bindings for betas 1,2,3,4,5 #12936
[AppKit] Update Xcode 13.0 bindings for betas 1,2,3,4,5 #12936
Conversation
src/appkit.cs
Outdated
@@ -13069,6 +13143,30 @@ partial interface NSScreen { | |||
string LocalizedName { get; } | |||
} | |||
|
|||
[NoMacCatalyst] | |||
partial interface NSScreen |
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 unsure about what to do about this interface structure - there are a couple other partial interfaces with the same name, in the same file here. Not sure why they're all separated, but sharpie does the same, and diffs suggest the interface is new for some reason. So, I just did what the diffs/sharpie seem to indicate.
But I was wondering - why is it like that? Can't I just add these new properties to the partial interface above?
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 don't know why sharpie does that, but you can just add it to any existing partial interfaces (at the very least it makes it less confusing for the next person).
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.
sounds good, thanks!
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 looks great! I mostly just had a few comments about naming.
src/appkit.cs
Outdated
[Mac (12,0)] | ||
[IgnoredInDelegate] | ||
[Export ("applicationSupportsSecureRestorableState:")] | ||
bool ApplicationSupportsSecureRestorableState (NSApplication application); |
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 'application' suffix is often emitted from the managed name in this case, because it's somewhat redundant (look at the other methods in this class, they mostly do the same thing) - in the UIApplicationDelegate class the logic is about UIApplication, which means that spelling out 'Application' all the time feels a bit unnecessary:
bool ApplicationSupportsSecureRestorableState (NSApplication application); | |
bool SupportsSecureRestorableState (NSApplication application); |
src/appkit.cs
Outdated
[IgnoredInDelegate] | ||
[Export ("application:handlerForIntent:")] | ||
[return: NullAllowed] | ||
NSObject GetApplication (NSApplication application, INIntent intent); |
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 somewhat of a similar case, the important part is what comes after 'application:', so this is a better managed name:
NSObject GetApplication (NSApplication application, INIntent intent); | |
NSObject GetHandler (NSApplication application, INIntent intent); |
src/appkit.cs
Outdated
[Mac (12,0)] | ||
[IgnoredInDelegate] | ||
[Export ("applicationShouldAutomaticallyLocalizeKeyEquivalents:")] | ||
bool ApplicationShouldAutomaticallyLocalizeKeyEquivalents (NSApplication application); |
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.
Same:
bool ApplicationShouldAutomaticallyLocalizeKeyEquivalents (NSApplication application); | |
bool ShouldAutomaticallyLocalizeKeyEquivalents (NSApplication application); |
src/appkit.cs
Outdated
|
||
[Mac (12,0)] | ||
[Export ("applicationProtectedDataWillBecomeUnavailable:")] | ||
void ApplicationProtectedDataWillBecomeUnavailable (NSNotification notification); |
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 ApplicationProtectedDataWillBecomeUnavailable (NSNotification notification); | |
void ProtectedDataWillBecomeUnavailable (NSNotification notification); |
src/appkit.cs
Outdated
|
||
[Mac (12,0)] | ||
[Export ("applicationProtectedDataDidBecomeAvailable:")] | ||
void ApplicationProtectedDataDidBecomeAvailable (NSNotification notification); |
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 ApplicationProtectedDataDidBecomeAvailable (NSNotification notification); | |
void ProtectedDataDidBecomeAvailable (NSNotification notification); |
src/appkit.cs
Outdated
[Async] | ||
[Mac (12,0)] | ||
[Export ("setDefaultApplicationAtURL:toOpenContentTypeOfFileAtURL:completionHandler:")] | ||
void SetDefaultApplicationToOpenContentType (NSUrl applicationURL, NSUrl url, [NullAllowed] Action<NSError> completionHandler); |
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 SetDefaultApplicationToOpenContentType (NSUrl applicationURL, NSUrl url, [NullAllowed] Action<NSError> completionHandler); | |
void SetDefaultApplicationToOpenContentType (NSUrl applicationUrl, NSUrl url, [NullAllowed] Action<NSError> completionHandler); |
src/appkit.cs
Outdated
[Async] | ||
[Mac (12,0)] | ||
[Export ("setDefaultApplicationAtURL:toOpenFileAtURL:completionHandler:")] | ||
void SetDefaultApplicationToOpenFile (NSUrl applicationURL, NSUrl url, [NullAllowed] Action<NSError> completionHandler); |
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 SetDefaultApplicationToOpenFile (NSUrl applicationURL, NSUrl url, [NullAllowed] Action<NSError> completionHandler); | |
void SetDefaultApplicationToOpenFile (NSUrl applicationUrl, NSUrl url, [NullAllowed] Action<NSError> completionHandler); |
src/appkit.cs
Outdated
[Mac (12,0)] | ||
[Export ("URLForApplicationToOpenContentType:")] | ||
[return: NullAllowed] | ||
NSUrl URLForApplication (UTType contentType); |
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 have to be a bit more explicit to be understandable:
NSUrl URLForApplication (UTType contentType); | |
NSUrl GetUrlForApplicationToOpenContentType (UTType contentType); |
src/appkit.cs
Outdated
|
||
[Mac (12,0)] | ||
[Export ("URLsForApplicationsToOpenContentType:")] | ||
NSUrl[] URLsForApplications (UTType contentType); |
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.
NSUrl[] URLsForApplications (UTType contentType); | |
NSUrl[] GetUrlsForApplicationsToOpenContentType (UTType contentType); |
src/appkit.cs
Outdated
[Async] | ||
[Mac (12,0)] | ||
[Export ("setDefaultApplicationAtURL:toOpenContentType:completionHandler:")] | ||
void SetDefaultApplication (NSUrl applicationURL, UTType contentType, [NullAllowed] Action<NSError> completionHandler); |
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 SetDefaultApplication (NSUrl applicationURL, UTType contentType, [NullAllowed] Action<NSError> completionHandler); | |
void SetDefaultApplicationToOpenContentType (NSUrl applicationURL, UTType contentType, [NullAllowed] Action<NSError> completionHandler); |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 102 tests passed 🎉Pipeline on Agent XAMBOT-1100.BigSur' |
@rolfbjarne thank you! updated based on your feedback |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094.BigSur' |
[Mac (12,0)] | ||
[Static] | ||
[Export ("allowedClassesForRestorableStateKeyPath:")] | ||
Class[] GetAllowedClassesForRestorableStateKeyPath (string keyPath); |
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.
Wondering if this name could exclude KeyPath since it is the parameter name.
Swift name in the docs is allowedClasses(forRestorableStateKeyPath:)
but would GetAllowedClassesForRestorableState (string keyPath)
work here?
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.
thanks! I'll just simplify to GetAllowedClasses
then - I think RestorableStateKeyPath is one thing, so it could be confusing to imply its for the restorablestate and not specify the keypath
src/appkit.cs
Outdated
[BaseType (typeof (NSCell))] | ||
interface NSTextAttachmentCell : NSTextAttachmentCellProtocol { | ||
[Export ("initImageCell:")] | ||
IntPtr Constructor (NSImage image); |
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.
IntPtr Constructor (NSImage image); | |
IntPtr Constructor (NSImage image); |
nit
Added a small formatting change and a suggestion about the KeyPath! Everything else looks great to me! :) |
🔥 Tests failed catastrophically on Build (no summary found). 🔥Result file $(TEST_SUMMARY_PATH) not found. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1097.BigSur' |
failing test not related #10833 |
No description provided.