[CoreSpotlight] Add Xcode13 beta1 support. #11986
Conversation
mandel-macaque
added
the
note-highlight
|
|
||
| [NoTV, Mac (12,0), iOS (15,0), MacCatalyst (15,0)] | ||
| [BaseType (typeof (NSObject))] | ||
| interface CSImportExtension : NSExtensionRequestHandling |
There was a problem hiding this comment.
doc suggest this is to be subclassed inside an extension
IOW it's not usable as-is inside apps
might be worth making it abstract
There was a problem hiding this comment.
We want the abstract keyword because CSImportExtension will be subclassed and we want the user to implement it's properties and methods? (just Update in this case)
There was a problem hiding this comment.
Adding [Abstract] has its own set of problems (#4969), although those might not necessarily apply to this case.
src/corespotlight.cs
Outdated
| interface CSImportExtension : NSExtensionRequestHandling | ||
| { | ||
| [Export ("updateAttributes:forFileAtURL:error:")] | ||
| bool UpdateAttributes (CSSearchableItemAttributeSet attributes, NSUrl contentUrl, [NullAllowed] out NSError error); |
There was a problem hiding this comment.
-> Update
attributes is the first parameter, not the action
In Swift the name is update
|
|
||
| [NoTV, NoMac, iOS (15,0), MacCatalyst (15,0)] | ||
| [Export ("actionIdentifiers", ArgumentSemantic.Copy)] | ||
| string[] ActionIdentifiers { get; set; } |
There was a problem hiding this comment.
you expose ActionIdentifier as a NSString
but ActionIdentifiers as string[]
if it's to be used with constant then it's better to keep them all as NSString, it avoid conversions and remove the risk that the constants are compared using pointers
There was a problem hiding this comment.
ActionIdentifier is a Field that is used for the plist as per the docs:
Info dictionary of the specified NSUserActivity includes the corresponding Info.plist entry using the key CSActionIdentifier.
I think is safe to leave it as it is, maybe if anything move the plist to a string, but all other keys are NSStrings.
❌ [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 results2 tests failed, 84 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
|
|
||
| [NoTV, Mac (12,0), iOS (15,0), MacCatalyst (15,0)] | ||
| [BaseType (typeof (NSObject))] | ||
| interface CSImportExtension : NSExtensionRequestHandling |
There was a problem hiding this comment.
Adding [Abstract] has its own set of problems (#4969), although those might not necessarily apply to this case.
❌ [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, 85 tests passed.Failed tests
Pipeline on Agent XAMBOT-1100.BigSur' |
🔥 Tests failed catastrophically on Build (no summary found). 🔥Result file $(TEST_SUMMARY_PATH) not found. |
|
network issue :( |
|
/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 results2 tests failed, 84 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
❌ [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, 85 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
No description provided.