- Notifications
You must be signed in to change notification settings - Fork548
[Photos] Add Xcode 11 Beta 1-4 bindings#6521
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
spouliot commentedJul 8, 2019
build |
src/Photos/Enums.cs Outdated
| [Mac(10,12,onlyOn64:true)] | ||
| [Native] | ||
| publicenumPHCollectionListType:long{ | ||
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] |
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.
you don't need this, it's already there with[iOS (8,0)] on the type
src/Photos/Enums.cs Outdated
| publicenumPHCollectionListType:long{ | ||
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Will be removed in a future release")] | ||
| [Introduced(PlatformName.TvOS,10,0,message:"Will be removed in a future release")] |
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 (already present)
src/Photos/Enums.cs Outdated
| [Native] | ||
| publicenumPHCollectionListType:long{ | ||
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Will be removed in a future release")] |
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 message is not helpful - since deprecated implies it's not to be used. Also do not copy/paste strings directly from sharpie (since those comes from header files).
src/Photos/Enums.cs Outdated
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Will be removed in a future release")] | ||
| [Introduced(PlatformName.TvOS,10,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.TvOS,13,0,message:"Will be removed in a future release")] |
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 (remove message)
src/Photos/Enums.cs Outdated
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Will be removed in a future release")] | ||
| [Introduced(PlatformName.TvOS,10,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.TvOS,13,0,message:"Will be removed in a future release")] |
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 ^
src/Photos/Enums.cs Outdated
| [Native] | ||
| publicenumPHCollectionEditOperation:long{ | ||
| None=0, | ||
| //None = 0, |
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.
nope :)
it's good to have a member for0 since it's the default value
src/Photos/Enums.cs Outdated
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Will be removed in a future release")] | ||
| [Introduced(PlatformName.TvOS,10,0,message:"Will be removed in a future release")] | ||
| [Deprecated(PlatformName.TvOS,13,0,message:"Will be removed in a future release")] |
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 as ^^^
src/Photos/Enums.cs Outdated
| FullSizePairedVideo=10, | ||
| [Mac(10,15,onlyOn64:true),iOS(10,0)] | ||
| AdjustmentBasePairedVideo=11, | ||
| [Mac(10,15,onlyOn64:true),iOS(13,0)] |
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.
^ you need to have the[TV] too
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 the TV attribute already included for that entire enum?
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, but the newer values wherenot available in[TV (10,0)] so they must be decorated again with the SDK version that added them
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.
Oh so are you saying that since the new ones say "API_AVAILABLE(macos(10.15), ios(10))" for example, this means that it is not available in TV?
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.
not quite, from macros Apple iOS if often equal to tvOS (if the framework is available to the later)
However we're stricter when using attributes, so we have to explicitly do a[iOS (13,0)][TV (13,0)] for the IDE to show the right information to customers
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 I was confused because I was thinking TV 10 was the newest one, sorry!
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.
still missing[TV(13,0)] 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.
Okay I think I was still confused before because I was misunderstanding you, sorry. This one needs the TV 13 since it has the iOS 13 meaning this one is actually new where as the previous properties were actually already created in previous versions, but they just weren't in the file so those ones are Tv 10 and do not need a special attribute since the header includes it?
| [Mac(10,15,onlyOn64:true),iOS(10,0)] | ||
| FullSizePairedVideo=10, | ||
| [Mac(10,15,onlyOn64:true),iOS(10,0)] | ||
| AdjustmentBasePairedVideo=11, |
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.
is it new in Xcode 11 ?
if so it's likely not available before iOS 13 (not 10)
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.
From the wiki it looks like it was already available in iOS 10, but the mac part is new. I'm not sure why it was not in the file previously
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.
indeed, seems it was forgotten last time
- PHAssetResourceTypeAdjustmentBasePairedVideo PHOTOS_AVAILABLE_IOS_TVOS(10_0, 10_0) = 11,^ here you see theold macro was mentioningtvOS too, so you need a[TV (10,0)] on the members too
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.
Okay that makes sense!
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.
Just clarifying, since it is TV 10 and the class is decorated with TV 10, there's no need to put TV 10 on the methods themselves?
| @@ -1 +1,4 @@ | |||
| !missing-protocol-conformance! PHFetchResult should conform to NSFastEnumeration | |||
| # Enum is named PHPhotosErrorDomain and is clashing with domain name | |||
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.
add a link to the feedback you filed and maccoreshadow issue
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'm sorry, what is a maccore shadow issue?
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.
when we file a feedback issue with Apple (only you can see it) we add a copy in maccore (so everyone can see it)
https://github.com/xamarin/maccore/issues?q=is%3Aissue+is%3Aopen+label%3Aapple-feedback
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 was not able to open this link, is it possible that I do not have authorization to this page?
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, you might not have access to the private repo :(
src/Photos/Enums.cs Outdated
| [Mac(10,12,onlyOn64:true)] | ||
| [Native] | ||
| publicenumPHCollectionListType:long{ | ||
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] |
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 message only makes sense for the[Deprecated] attribute.
| [Introduced(PlatformName.iOS,8,0,message:"Will be removed in a future release")] | |
| [Introduced(PlatformName.iOS,8,0)] |
src/Photos/Enums.cs Outdated
| [Native] | ||
| publicenumPHCollectionEditOperation:long{ | ||
| None=0, | ||
| //None = 0, |
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.
You can't remove enum fields, it's a breaking change.
| //None = 0, | |
| None=0, |
src/photos.cs Outdated
| PHAssetSourceTypeSourceType{get;} | ||
| [TV(11,0),iOS(11,0),NoMac] | ||
| [TV(11,0),iOS(11,0),Mac(10,15,onlyOn64: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.
| [TV(11,0),iOS(11,0),Mac(10,15,onlyOn64:true),] | |
| [TV(11,0),iOS(11,0),Mac(10,15,onlyOn64: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.
Thanks!
src/photos.cs Outdated
| [NoMac] | ||
| [BaseType(typeof(PHAssetChangeRequest))] | ||
| [Mac(10,15,onlyOn64:true)] | ||
| [BaseType(typeof(PHAssetChangeRequest))] |
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.
Unnecessary whitespace change:
| [BaseType(typeof(PHAssetChangeRequest))] | |
| [BaseType(typeof(PHAssetChangeRequest))] |
src/photos.cs Outdated
| [NoMac] | ||
| [BaseType(typeof(NSObject))] | ||
| [Mac(10,15,onlyOn64:true)] | ||
| [BaseType(typeof(NSObject))] |
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.
Unnecessary whitespace change:
| [BaseType(typeof(NSObject))] | |
| [BaseType(typeof(NSObject))] |
src/photos.cs Outdated
| [DisableDefaultCtor] | ||
| interfacePHChangeRequest{ | ||
| [Internal] |
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.
Why is this internal?
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 will make a comment of this too, but the reasoning is a little confusing to me. I remember I could not build the file and@dalexsoto may remember, but there is a file src/Photos/PHAssetChangeRequest.cs that has an Obsolete attribute in the PHAssetChangeRequest empty constructor and we deemed that is what was causing the build issue. However, now I am confused because this class is PHChangeRequest and not PHAssetChangeRequest. I can talk to Alex about this tomorrow and get back to you with more clarification.
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.
So below is the code from a file that shows that PHAssetChangeRequest disables creating the no argument constructor and since the base class of PHAssetChangeRequest changed from NSObject to the newly added PHChangeRequest, we thought this would be the way to go. Do you have a different suggestion to handle that?
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 I see: you need the default constructor in the base class so that the derived class' default constructor can call it.
I think it would be better to add this in a manually written source file, like the constructor is in PHAssetChangeRequest.
i.e. in thesrc/Photos/PHChangeRequest file (create it if it doesn't exist):
publicpartialclassPHChangeRequest{#if!XAMCORE_3_0// This constructor is required for the default constructor in PHAssetChangeRequest to compile.internalPHChangeRequest(){}#endif}
this should work around the compiler error, and it doesn't require generating a binding.
src/photos.cs Outdated
| [iOS(8,0)] | ||
| [TV(10,0)] | ||
| [Mac(10,13,onlyOn64:true)] | ||
| [Mac(10,15,onlyOn64: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.
This looks very strange: Apple retroactively removed the API from previous macOS versions?
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.
Apple has that available as Mac 10,15 on xcode, but Alex mentioned to me that we should keep the versions we have so I went back and tried to change them, but I must have missed this one. Will change back to 10,13!
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.
Don't blindly trust the headers 😄
src/photos.cs Outdated
| [TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)] | ||
| [Export("requestImageDataAndOrientationForAsset:options:resultHandler:")] | ||
| intRequestImageDataAndOrientationForAsset(PHAssetasset,[NullAllowed]PHImageRequestOptionsoptions,PHImageManagerRequestImageDataHandlerresultHandler); |
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 thinkRequestImageDataAndOrientation sounds better:
| intRequestImageDataAndOrientationForAsset(PHAssetasset,[NullAllowed]PHImageRequestOptionsoptions,PHImageManagerRequestImageDataHandlerresultHandler); | |
| intRequestImageDataAndOrientation(PHAssetasset,[NullAllowed]PHImageRequestOptionsoptions,PHImageManagerRequestImageDataHandlerresultHandler); |
src/photos.cs Outdated
| interfacePHLivePhoto:NSSecureCoding,NSCopying | ||
| { | ||
| [BaseType(typeof(NSObject))] | ||
| interfacePHLivePhoto:NSSecureCoding,NSCopying{ |
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.
Unnecessary whitespace change.
src/photos.cs Outdated
| [Static] | ||
| [Export("requestLivePhotoWithResourceFileURLs:placeholderImage:targetSize:contentMode:resultHandler:")] | ||
| intRequestLivePhoto(NSUrl[]fileUrls,[NullAllowed]UIImageimage,CGSizetargetSize,PHImageContentModecontentMode,Action<PHLivePhoto,NSDictionary>resultHandler); | ||
| intRequestLivePhoto(NSUrl[]fileUrls,[NullAllowed]UIImageimage,CGSizetargetSize,PHImageContentModecontentMode,PHLivePhotoRequestLivePhotoHandlerresultHandler); |
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 breaking change.
src/Photos/Enums.cs Outdated
| [Native] | ||
| publicenumPHAssetResourceType:long | ||
| { | ||
| publicenumPHAssetResourceType:long{ |
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.
your style is better - but don't rewrite existing code (unless required)
i.e. minimizedamage (changes) in your PRs to ease reviewers life :)
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.
Got it sorry about that!
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 will undo some of the other style changes I made
monojenkins commentedJul 8, 2019
Build failure Test results3 tests failed, 88 tests passed.Failed tests
|
spouliot commentedJul 8, 2019
build |
chamons commentedJul 8, 2019
whitelist |
monojenkins commentedJul 8, 2019
Build failure Test results3 tests failed, 88 tests passed.Failed tests
|
monojenkins commentedJul 8, 2019
Build failure Test results3 tests failed, 88 tests passed.Failed tests
|
whitneyschmidt left a comment
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.
👍
| publicenumPHCollectionListType:long{ | ||
| [Deprecated(PlatformName.iOS,13,0)] | ||
| [Deprecated(PlatformName.TvOS,13,0)] | ||
| [Unavailable(PlatformName.MacOSX)] |
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.
Generally, when do we use [Unavailable]? Is it specific to [Native] enums?
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.
[Unavailable] can be used anywhere but it's most useful in files processed by the generator (bgen/btouch). It basically tells the generator to skip the member for the specified platform.
One exception: enum values (like this case) are always generated on all platforms since
- it's compiled as a numeric value, not a string symbol and won't be rejected; and
- it makes it easier to write code without defines, e.g.
#if __IOS__
Still the IDE will read the attribute and show it as unavailable if someone is working on a Xamarin.Mac application.
Uh oh!
There was an error while loading.Please reload this page.
src/photos.cs Outdated
| [DisableDefaultCtor] | ||
| interfacePHAssetResourceManager | ||
| { | ||
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.
No needed space.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| [TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)] | ||
| [Export("requestImageDataAndOrientationForAsset:options:resultHandler:")] | ||
| intRequestImageDataAndOrientation(PHAssetasset,[NullAllowed]PHImageRequestOptionsoptions,PHImageManagerRequestImageDataHandlerresultHandler); |
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.
Async?
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 documentation says that it is only called once if I am reading it correctly, so therefore it doesn't need an [async] correct? Here's what it says "@param resultHandler A block that is called exactly once either synchronously on the current thread or asynchronously on the main thread depending on the synchronous option specified in the PHImageRequestOptions options parameter (deliveryMode is ignored). Orientation is an EXIF orientation as an CGImagePropertyOrientation. For iOS or tvOS, convert this to an UIImageOrientation."
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 async will allow to have the async/await pattern which is nicer. Even if the cb is only called once, it will mean that we do not have to bloc the current thread.
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've talk with@dalexsoto about this. While parse we could make the metho async, that long explanation make is scary and also means that the mono async/await might be doing evil things not expected by objc (for example, ConfigureAwait (false) + the wrong option).
Better leave it like this and later add the API if needed.
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.
Okay cool thank you!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
monojenkins commentedJul 9, 2019
Build failure 🔥Build failed 🔥 |
mandel-macaque left a comment
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.
All my comments have been addressed and we agreed with@dalexsoto that Async could be dangerous due to the explanation given by Apple.
monojenkins commentedJul 9, 2019
Build failure |
monojenkins commentedJul 9, 2019
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
src/photos.cs Outdated
| [DisableDefaultCtor] | ||
| interfacePHChangeRequest{ | ||
| [Internal] |
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 I see: you need the default constructor in the base class so that the derived class' default constructor can call it.
I think it would be better to add this in a manually written source file, like the constructor is in PHAssetChangeRequest.
i.e. in thesrc/Photos/PHChangeRequest file (create it if it doesn't exist):
publicpartialclassPHChangeRequest{#if!XAMCORE_3_0// This constructor is required for the default constructor in PHAssetChangeRequest to compile.internalPHChangeRequest(){}#endif}
this should work around the compiler error, and it doesn't require generating a binding.
monojenkins commentedJul 10, 2019
Build failure 🔥Build failed 🔥 |
dalexsoto left a comment
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.
You need to add your new file here to unbreak your build
https://github.com/xamarin/xamarin-macios/blob/master/src/frameworks.sources#L1240-L1251
Looks good to me after Rolf's and Sebastien's 👍
monojenkins commentedJul 10, 2019
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
rolfbjarne left a comment
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.
LGTM, but have a look at the api diff for the PR, there seems to be breaking changes for macOS (ignore the "Modified base type" breaking changes, those are actually OK).
spouliot left a comment
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.
a few more things :)
src/Photos/Enums.cs Outdated
| FullSizePairedVideo=10, | ||
| [Mac(10,15,onlyOn64:true),iOS(10,0)] | ||
| AdjustmentBasePairedVideo=11, | ||
| [Mac(10,15,onlyOn64:true),iOS(13,0)] |
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.
still missing[TV(13,0)] here ?
| namespacePhotos{ | ||
| publicpartialclassPHChangeRequest{ | ||
| #if!XAMCORE_3_0 |
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.
why notXAMCORE_4_0 ?
_3_0 was used for changes that are included in tvOS and watchOS (but could not be enabled in iOS and macOS without braking changes). It should not (99% of time) be used again.
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.
Mmm I'm not sure,@rolfbjarne sent me that code snippet, but I can change it to be XAMCORE_4_0 if need be.
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's3_0 because it's required for this code, which is using3_0:https://github.com/xamarin/xamarin-macios/blob/846bc763a881855bc455848ef15d2c78b765ffc0/src/Photos/PHAssetChangeRequest.cs#L9-L14
src/photos.cs Outdated
| [TV(10,0)] | ||
| [NoMac] | ||
| [BaseType(typeof(NSObject))] | ||
| [Mac(10,13,onlyOn64: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.
sure about 10,13 ? other related types are 10,15
if it's only in xcode11 headers assume it's 10,15 - unless tested
src/photos.cs Outdated
| PHCloudIdentifier[]GetCloudIdentifiers(string[]localIdentifiers); | ||
| [Field("PHLocalIdentifierNotFound")] | ||
| NSStringPHLocalIdentifierNotFound{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.
removePH prefix - the type already has it
src/photos.cs Outdated
| [Export("projectExtensionData",ArgumentSemantic.Copy)] | ||
| NSDataProjectExtensionData{get;set;} | ||
| [Introduced(PlatformName.MacOSX,10,13)] |
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 the shortcut[Mac (10,13, onlyOn64: true)]
easier to rememberonlyOn64: 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.
Oh cool I didn't know that they meant the same thing haha. So then I can remove it entirely since the header has Mac (10,13) already, correct?
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.
yup, no need to dupe them :)
monojenkins commentedJul 11, 2019
Build failure 🔥Build failed 🔥 |
tj-devel709 commentedJul 15, 2019
Is this one ready for a merge?@spouliot@rolfbjarne@dalexsoto |
rolfbjarne commentedJul 15, 2019
build |
monojenkins commentedJul 15, 2019
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
monojenkins commentedJul 16, 2019
Build failure 🔥Build failed 🔥 |
monojenkins commentedJul 17, 2019
Build success |
…cted whitespace and spacing
tj-devel709 commentedJul 18, 2019
@spouliot@mandel-macaque@dalexsoto@whitneyschmidt@rolfbjarne@stephen-hawley Fixed those changes! |
| [BaseType(typeof(NSObject))] | ||
| [DisableDefaultCtor] | ||
| // include the availability attributes to any new member (and don't trust the type-level ones) | ||
| interfacePHChangeRequest{} |
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.
👍
dalexsoto left a comment
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.
Great job!
monojenkins commentedJul 19, 2019
Build success |
No description provided.