[Photos] Update bindings for Xcode 13.0 beta 1 and beta 2 #12163

rachelkang · 2021-07-21T16:14:15Z

No beta 3 updates

mandel-macaque · 2021-07-21T17:06:55Z

src/photos.cs

+ [TV (15,0), Mac (12,0), iOS (15,0)]
+ [Export ("localIdentifierMappingsForCloudIdentifiers:")]
+ NSDictionary<PHCloudIdentifier, PHLocalIdentifierMapping> LocalIdentifierMappingsForCloudIdentifiers (PHCloudIdentifier[] cloudIdentifiers);
+
+ [TV (15,0), Mac (12,0), iOS (15,0)]
+ [Export ("cloudIdentifierMappingsForLocalIdentifiers:")]
+ NSDictionary<NSString, PHCloudIdentifierMapping> CloudIdentifierMappingsForLocalIdentifiers (string[] localIdentifiers);


You need to add MacCatalyst (15,0) else it will be using the default min version.

mandel-macaque · 2021-07-21T17:07:25Z

src/photos.cs

 [Export ("localIdentifiersForCloudIdentifiers:")]
 string[] GetLocalIdentifiers (PHCloudIdentifier[] cloudIdentifiers);


Do we want this to be added in MacCatalyst if it is deprecated, We should add NotMacCatalyst if we are deprecating. Is it deprecated on iOS and Tv?

yup, it looks like this needs a [NoTV][NoiOS][NoMacCatalyst] since they are unlikely to be both added and deprecated
and if that's the case then it's generally better not to include them anyway

mandel-macaque · 2021-07-21T17:07:33Z

src/photos.cs

 [Export ("localIdentifiersForCloudIdentifiers:")]
 string[] GetLocalIdentifiers (PHCloudIdentifier[] cloudIdentifiers);

+ [Deprecated (PlatformName.MacOSX, 12, 0, message: "Use 'cloudIdentifierMappingsForCloudIdentifiers:' instead.")]


mandel-macaque · 2021-07-21T17:07:43Z

src/photos.cs

 [Export ("cloudIdentifiersForLocalIdentifiers:")]
 PHCloudIdentifier[] GetCloudIdentifiers (string[] localIdentifiers);

+ [Deprecated (PlatformName.MacOSX, 12, 0, message: "Use 'PHPhotosErrorIdentifierNotFound' instead.")]


spouliot · 2021-07-21T17:25:56Z

src/Photos/Enums.cs

@@ -356,7 +358,9 @@ public enum PHProjectSectionType : long {
 [Native]
 [ErrorDomain ("PHLivePhotoEditingErrorDomain")]
 public enum PHLivePhotoEditingError : long {
+ [Deprecated (PlatformName.MacOSX, 10, 15, message: "Use 'InternalError' instead.")]


the suggested alternative is inside a different enum, which might be hard to use. Sadly not much we can do about it :(

However we should name that enum Use 'PHPhotosError.InternalError' instead.

spouliot · 2021-07-21T17:26:20Z

src/Photos/Enums.cs

 Unknown,
+ [Deprecated (PlatformName.MacOSX, 10, 15, message: "Use 'UserCancelled' instead.")]


same, prefix value with PHPhotosError.

spouliot · 2021-07-21T17:32:15Z

src/Photos/Enums.cs

 UserCancelled = 3072,
 LibraryVolumeOffline = 3114,
 RelinquishingLibraryBundleToWriter = 3142,
 SwitchingSystemPhotoLibrary = 3143,
+ [TV (14,0), Mac (11,0), iOS (14,0)]


It's a special case but we do not annotate error codes with OS versions.

Why ? They are are output-only and, as enum values, they do not have to exists to be used. E.g.

var error = CallApi (); switch (error) case PHPhotosError. NetworkAccessRequired: // it does not make sense to add a version check here // and we don't want compiler/tooling warnings for them if we don't Console.WriteLine ("Network Access Required"); break; // ... more cases ... default: Console.WriteLine ($"Error {error}"); break; }

It's also a bunch of (not really useful) metadata to include inside platform assemblies.

that makes sense, thank you! removing now

spouliot · 2021-07-21T17:32:47Z

src/photos.cs

@@ -146,9 +146,14 @@ interface PHAsset {
 [Export ("playbackStyle", ArgumentSemantic.Assign)]
 PHAssetPlaybackStyle PlaybackStyle { get; }

+ [Deprecated (PlatformName.MacOSX, 12, 0, message: "Use 'PHPhotosErrorIdentifierNotFound' instead.")]


missing dot . -> PHPhotosError.IdentifierNotFound

spouliot · 2021-07-21T17:35:05Z

src/photos.cs

+
+ [TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)]
+ [Field ("PHLocalIdentifiersErrorKey")]
+ NSString PHLocalIdentifiersErrorKey { get; }


not sure if it's the best place for that key
but it's not named correctly, IOW you do not want to use PHPhotoLibrary.PHLocalIdentifiersErrorKey
minimally remove PH prefix
also check if other constants exists - there might be a better place for this one (or not)

WE already did that :) That key is only for when the indentifer is not matched and we noticed that the only Object with methods for identifiers is this class, so it would be something like:

PHPhotoLibrary.LocalIdentifierMappingsForCloudIdentifiers(.., out var error); // access the error info error.UserInfo[PHPhotoLibrary.LocalIdentifiersErrorKey]

Yet, I did not know that @rachelkang was going to add those two methods as a Category in a diff object, I would have added them in the library and not in PHPhotoLibrary_CloudIdentifiers. If we want a diff class for the category, lets move the key there, else lets move the category methods to PHPhotoLibrary. My preference is PHPhotoLibrary but I'm happy either way.

sounds good - but the

remove PH prefix
remains :)

removed the prefix!

Let me know if I should definitively move the key to PHPhotoLibrary_CloudIdentifiers or move the category methods to PHPhotoLibrary. Moving neither for now, although I think it would make sense to move the key to PHPhotoLibrary_CloudIdentifiers where the category methods are. I put the category methods there because the older versions of them (deprecated) also live there

spouliot · 2021-07-21T17:37:38Z

src/photos.cs

 [Export ("localIdentifiersForCloudIdentifiers:")]
 string[] GetLocalIdentifiers (PHCloudIdentifier[] cloudIdentifiers);


yup, it looks like this needs a [NoTV][NoiOS][NoMacCatalyst] since they are unlikely to be both added and deprecated
and if that's the case then it's generally better not to include them anyway

spouliot · 2021-07-21T17:38:15Z

src/photos.cs

- [NoiOS][NoTV]
- [Unavailable (PlatformName.MacCatalyst)]
+ [TV (15,0)]
+ [iOS (15,0)]
 [Advice ("This API is not available when using UIKit on macOS.")]


that needs to be removed as you already removed the [Unavailable (PlatformName.MacCatalyst)]

The [Advice]? Doesn't that apply to macOS?

"UIKit on macOS" => Mac Catalyst (it's an old name).

ah, good to know! thanks!

spouliot · 2021-07-21T17:38:25Z

src/photos.cs

 [Category]
- [Unavailable (PlatformName.MacCatalyst)]
 [Advice ("This API is not available when using UIKit on macOS.")]


that needs to be removed as you already removed the [Unavailable (PlatformName.MacCatalyst)]

spouliot · 2021-07-21T17:38:47Z

src/photos.cs

 [Advice ("This API is not available when using UIKit on macOS.")]
 [BaseType (typeof (NSObject))]
 [DisableDefaultCtor]
 interface PHCloudIdentifier : NSSecureCoding {

+ [Deprecated (PlatformName.MacOSX, 12, 0, message: "Use 'PHPhotosErrorIdentifierNotFound' instead.")]


missing dot .

src/Photos/Enums.cs

src/photos.cs

rolfbjarne · 2021-07-22T08:53:16Z

src/photos.cs

- [NoiOS][NoTV]
- [Unavailable (PlatformName.MacCatalyst)]
+ [TV (15,0)]
+ [iOS (15,0)]
 [Advice ("This API is not available when using UIKit on macOS.")]


"UIKit on macOS" => Mac Catalyst (it's an old name).

tj-devel709 · 2021-07-22T17:27:05Z

src/Photos/Enums.cs

 Invalid = -1,
+#endif
+ InternalError = -1,


Will having 2 enums with the value -1 cause issues?

Nope, it's totally valid to have multiple enum fields with the same numerical value.

Oh wait, the [Deprecated] attribute goes inside the #if as well.

tj-devel709 · 2021-07-22T17:33:31Z

src/photos.cs

@@ -1423,4 +1445,28 @@ interface PHCloudIdentifier : NSSecureCoding {
 [Export ("initWithStringValue:")]
 IntPtr Constructor (string stringValue);
 }
+
+ [TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)]
+ [BaseType (typeof(NSObject))]


Suggested change

[BaseType (typeof(NSObject))]

[BaseType (typeof (NSObject))]

small

fixed! having seen both spacing styles all over (even in this file), I'm curious if this is the actual stylistic guideline we should stick with?

tj-devel709 · 2021-07-22T17:34:05Z

src/photos.cs

+ }
+
+ [TV (15,0), Mac (12,0), iOS (15,0), MacCatalyst (15,0)]
+ [BaseType (typeof(NSObject))]


Suggested change

[BaseType (typeof(NSObject))]

[BaseType (typeof (NSObject))]

small also

tj-devel709 · 2021-07-22T17:37:13Z

src/photos.cs

 [NoTV][NoiOS]
 [Field ("PHLocalIdentifierNotFound")]
 NSString LocalIdentifierNotFound { get; }
+
+ [TV (15, 0), Mac (12, 0), iOS (15, 0), MacCatalyst (15,0)]


Extra extra small but maybe try to be consistent with the spacing in the attribute versions

nice catch, thanks!

rolfbjarne · 2021-07-22T19:06:23Z

src/Photos/Enums.cs

 Invalid = -1,
+#endif
+ InternalError = -1,


Oh wait, the [Deprecated] attribute goes inside the #if as well.

vs-mobiletools-engineering-service2 · 2021-07-22T20:51:30Z

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

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

2 tests failed, 87 tests passed.

Failed tests

monotouch-test/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 2629 Passed: 2491 Inconclusive: 35 Failed: 2 Ignored: 136)
link sdk/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 129 Passed: 105 Inconclusive: 6 Failed: 1 Ignored: 23)

Pipeline on Agent XAMBOT-1104.BigSur'
Merge addedb1 into 23d3fa5

spouliot · 2021-07-23T17:43:54Z

src/photos.cs

 [BaseType (typeof (PHPhotoLibrary))]
 interface PHPhotoLibrary_CloudIdentifiers {

+ [Mac (12,0)]
+ [Export ("localIdentifierMappingsForCloudIdentifiers:")]
+ NSDictionary<PHCloudIdentifier, PHLocalIdentifierMapping> LocalIdentifierMappingsForCloudIdentifiers (PHCloudIdentifier[] cloudIdentifiers);


-> GetLocalIdentifierMappings

prefix with Get so there's a verb/action in the method name

remove ForCloudIdentifiers suffix, it's the parameter

spouliot · 2021-07-23T17:44:16Z

src/photos.cs

+
+ [Mac (12,0)]
+ [Export ("cloudIdentifierMappingsForLocalIdentifiers:")]
+ NSDictionary<NSString, PHCloudIdentifierMapping> CloudIdentifierMappingsForLocalIdentifiers (string[] localIdentifiers);


-> GetCloudIdentifierMappings for the same reasons as above

spouliot · 2021-07-23T17:45:04Z

src/photos.cs

 [BaseType (typeof (NSObject))]
 [DisableDefaultCtor]
 interface PHCloudIdentifier : NSSecureCoding {

+ [Deprecated (PlatformName.MacOSX, 12, 0, message: "Use 'PHPhotosError.IdentifierNotFound' instead.")]
+ [NoTV, NoiOS]


you can add [NoMacCatalyst] like the cases above

src/photos.cs

src/Photos/Enums.cs

spouliot · 2021-07-23T17:47:22Z

src/Photos/Enums.cs

@@ -182,6 +182,8 @@ public enum PHAssetCollectionSubtype : long {
 SmartAlbumLongExposures = 215,
 [iOS (13,0)][TV(13,0)][Mac (10,15)]
 SmartAlbumUnableToUpload = 216,
+ [iOS (15,0), TV (15,0), Mac (12,0)]


looks like it should also have [MacCatalyst (15,0)]

vs-mobiletools-engineering-service2 · 2021-07-23T18:41:45Z

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

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

2 tests failed, 87 tests passed.

Failed tests

monotouch-test/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 2629 Passed: 2485 Inconclusive: 35 Failed: 3 Ignored: 141)
monotouch-test/tvOS - simulator/Debug (LinkSdk): Failed

Pipeline on Agent XAMBOT-1097.BigSur'
Merge c7f8c25 into 28bb8bc

vs-mobiletools-engineering-service2 · 2021-07-29T00:24:45Z

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

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

12 tests failed, 77 tests passed.

Failed tests

xammac tests/Mac Modern/Debug: Failed (Test run failed.
Tests run: 2463 Passed: 2407 Inconclusive: 10 Failed: 1 Ignored: 55)
xammac tests/Mac Modern/Release: Failed (Test run failed.
Tests run: 2460 Passed: 2403 Inconclusive: 10 Failed: 1 Ignored: 56)
xammac tests/Mac Modern/Release (all optimizations): Failed (Test run failed.
Tests run: 2460 Passed: 2402 Inconclusive: 10 Failed: 1 Ignored: 57)
xcframework-test/Mac Catalyst/Debug: TimedOut (Execution timed out after 1200 seconds.
No test log file was produced)
monotouch-test/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (LinkSdk): Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
monotouch-test/tvOS - simulator/Debug: Failed
monotouch-test/tvOS - simulator/Debug (LinkSdk): Failed
monotouch-test/tvOS - simulator/Debug (static registrar): Failed
monotouch-test/tvOS - simulator/Release (all optimizations): Failed

Pipeline on Agent XAMBOT-1100.BigSur'
Merge a516b77 into 3336910

vs-mobiletools-engineering-service2 · 2021-07-30T03:34:32Z

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

iOS vsdrops gist
macOS vsdrops gist
macCat vsdrops gist
macCatiOS gist
tvOS vsdrops gist
watchOS vsdrops gist

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):

🎉 All 89 tests passed 🎉

Pipeline on Agent XAMBOT-1099.BigSur'
Merge c4ad54e into 448cc6e

[Photos] Update bindings for Xcode 13.0 beta 1 and beta 2

463cc31

rachelkang added the note-highlight Worth calling out specifically in release notes label Jul 21, 2021

rachelkang added this to the xcode13.0 milestone Jul 21, 2021

rachelkang requested review from dalexsoto and spouliot July 21, 2021 16:14

Keep Catalyst availability on the same line as others

f56bf00

mandel-macaque requested changes Jul 21, 2021

View reviewed changes

spouliot suggested changes Jul 21, 2021

View reviewed changes

rolfbjarne requested changes Jul 22, 2021

View reviewed changes

Update based on feedback

addedb1

rachelkang requested review from spouliot, rolfbjarne and mandel-macaque July 22, 2021 17:05

tj-devel709 reviewed Jul 22, 2021

View reviewed changes

rolfbjarne requested changes Jul 22, 2021

View reviewed changes

Update based on feedback pt 2

c7f8c25

rachelkang requested review from rolfbjarne and tj-devel709 July 23, 2021 14:47

rolfbjarne approved these changes Jul 23, 2021

View reviewed changes

spouliot suggested changes Jul 23, 2021

View reviewed changes

Update based on feedback

78694f9

spouliot approved these changes Jul 28, 2021

View reviewed changes

Merge branch 'main' into xcode13.0-photos-b1-b2

a516b77

mandel-macaque approved these changes Jul 28, 2021

View reviewed changes

Merge branch 'main' into xcode13.0-photos-b1-b2

c4ad54e

mandel-macaque merged commit af2fea2 into xamarin:main Jul 30, 2021

		[Export ("localIdentifiersForCloudIdentifiers:")]
		string[] GetLocalIdentifiers (PHCloudIdentifier[] cloudIdentifiers);

		Unknown,
		[Deprecated (PlatformName.MacOSX, 10, 15, message: "Use 'UserCancelled' instead.")]

[Photos] Update bindings for Xcode 13.0 beta 1 and beta 2 #12163

[Photos] Update bindings for Xcode 13.0 beta 1 and beta 2 #12163

Conversation

rachelkang commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vs-mobiletools-engineering-service2 commented Jul 22, 2021

❌ [PR Build] Tests failed on Build ❌

API diff

API & Generator diff

GitHub pages

Test results

Failed tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vs-mobiletools-engineering-service2 commented Jul 23, 2021

❌ [PR Build] Tests failed on Build ❌

API diff

API & Generator diff

GitHub pages

Test results

Failed tests

vs-mobiletools-engineering-service2 commented Jul 29, 2021

❌ [PR Build] Tests failed on Build ❌

API diff

API & Generator diff

GitHub pages

Test results

Failed tests

vs-mobiletools-engineering-service2 commented Jul 30, 2021

✅ [PR Build] Tests passed on Build. ✅

API diff

API & Generator diff

GitHub pages

🎉 All 89 tests passed 🎉