- Notifications
You must be signed in to change notification settings - Fork548
[corelocation] Update for Xcode 11 beta 1 to 4#6600
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
monojenkins commentedJul 17, 2019
Build failure Test results9 tests failed, 82 tests passed.Failed tests
|
src/corelocation.cs Outdated
| [NoWatch][NoTV] | ||
| [iOS(6,0)] | ||
| [Mac(10,9)] | ||
| [Deprecated(PlatformName.iOS,13,0,message:"Not used anymore. It will always return .false'.")] |
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 wondering if ".false" is a typo?
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 it should have been'false'
good catch !
monojenkins commentedJul 17, 2019
Build success |
mandel-macaque commentedJul 17, 2019
We probably want to double check with the new Xcode 11 beta 4. |
| [NoWatch,NoTV,NoMac,iOS(13,0)] | ||
| [Export("locationManager:didRangeBeacons:satisfyingConstraint:")] | ||
| [EventArgs("CLRegionBeaconsConstraintRanged")] | ||
| voidDidRangeBeaconsSatisfyingConstraint(CLLocationManagermanager,CLBeacon[]beacons,CLBeaconIdentityConstraintbeaconConstraint); |
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.
CanDidRangeBeaconsSatisfyingConstraint be shortened toDidRangeBeacons - since the params are different it won't cause a namespace issue, right?
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.
Good catch! It was my first attempt too - but sadly it can't :(
The problem are that:
- the name is already used in the type (even if overloads are fine most of the time);
- a
*Delegatetype; - it uses
[EventArgs];
so this cause a conflict in the generated code.
I don't have this PR built right now - but I'll update the comment with the code (easier to see what's happening this way).
Update
So the[EventArgs] attribute is used in conjunction with theEvents property of[BaseType]. This allows us to expose C# events as a better alternative (there are gotchas) to implementing a*Delegate.
[BaseType(typeof(NSObject),Delegates=newstring[]{"WeakDelegate"},Events=newType[]{typeof(CLLocationManagerDelegate)})]partialinterfaceCLLocationManager{
But the extra events means a conflict because we already have aDidRangeBeacons method inCLLocationManagerDelegate, so the generator already created an event using that name:
publiceventEventHandler<CLRegionBeaconsRangedEventArgs> DidRangeBeacons{add{ EnsureCLLocationManagerDelegate().didRangeBeacons+= value;}remove{EnsureCLLocationManagerDelegate().didRangeBeacons-=value;}}
and we can't overload events in C# (even if the args are different).
So to avoid the compiler error a new, different name must be used - and I chooseDidRangeBeaconsSatisfyingConstraint
| NSUuidUuid{get;} | ||
| [NullAllowed,Export("major",ArgumentSemantic.Copy)] | ||
| [BindAs(typeof(short?))] |
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 do we useBindAs?
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.
There's a few cases, the most common one isNSNumber, like above. Another one are to support "smart enums" (using a C# enum that maps to severalNSString constants).
So for some API Apple prefers to use a reference type, likeNSNumber, over a more specific value type. Why ? mainly because they want a nullable properties
But
NSNumberMajor{get;}
does not tell the API consumer if it should except ashort (like it's meant to), afloat or even abool. That does not create a very discoverable API and we can do a lot better in C# with nullable? types :)
Why not
short?Major{get;}
Because our runtime must know this anNSNumber natively - e.g. it's needed to allow us to override the property in a subclass and have some ObjC code call us. So the declaration remains anNSNumber but we give the generator a hint to add extra (conversion) code to expose a nicer API to our developers.
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.
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.
Thank you both for the detailed explanation! 👍
spouliot commentedJul 18, 2019
@mandel-macaque there's no change in corelocation for beta 4 (but I'll fix the typo found by@tj-devel709) |
monojenkins commentedJul 18, 2019
Build success |
No description provided.