[homekit] Update for Xcode 9 beta 1 & 2#2276
Conversation
VincentDondain
requested review from
dalexsoto,
mandel-macaque,
rolfbjarne and
spouliot
|
|
||
| partial class HMMutablePresenceEvent { | ||
|
|
||
| public HMPresenceType PresenceType { |
There was a problem hiding this comment.
make it virtual so we can replace this with normal bindings (once support is added to the generator)
There was a problem hiding this comment.
Mmh sure but I don't follow the logic. Why is virtual needed? Wound't we just remove the manual bindings?
There was a problem hiding this comment.
Because BindAs will generate a virtual member so making it virtual will be a breaking change, also we will need to use the same property name too so it is also not a breaking change
There was a problem hiding this comment.
Right to avoid a breaking change, fair enough. However that is in the eventuality the generator fix doesn't make it before our release of the Xcode 9 APIs right? We can break before that.
There was a problem hiding this comment.
yup we can break before it becomes stable, but we can keep the binary compat since now :)
|
|
||
| partial class HMMutableSignificantTimeEvent { | ||
|
|
||
| public HMSignificantEvent SignificantEvent { |
| [BaseType (typeof (HMEvent))] | ||
| [DisableDefaultCtor] | ||
| interface HMLocationEvent : NSMutableCopying { | ||
| interface HMLocationEvent : NSCopying, NSMutableCopying { |
There was a problem hiding this comment.
unneeded since NSMutableCopying subclass NSCopying
| [Watch (4,0), TV (11,0), iOS (11,0)] | ||
| [BaseType (typeof(HMTimeEvent))] | ||
| [DisableDefaultCtor] | ||
| interface HMCalendarEvent : NSCopying, NSMutableCopying { |
There was a problem hiding this comment.
same NSCopying not needed
| IntPtr Constructor (NSDateComponents fireDateComponents); | ||
|
|
||
| [Export ("fireDateComponents", ArgumentSemantic.Strong)] | ||
| NSDateComponents FireDateComponents { get; set; } |
There was a problem hiding this comment.
don't you get warnings from this ? c# does not allow overriding properties that don't match (no setter vs setter)
the non-mutable version should have both a getter and a setter, the later should be decorated with [NotImplemented]. That solves the issue and makes the code work like expected.
There was a problem hiding this comment.
Ok sounds good, but no I did not get any warning (make all in /src).
There was a problem hiding this comment.
Isn't the lack of warning due to the fact that we generate: public virtual NSDateComponents FireDateComponents for HMMutableCalendarEvent.g.cs? No override.
There was a problem hiding this comment.
weird, double check (with a simple test case) with csc (since pmcs is still mcs based)
There was a problem hiding this comment.
@spouliot this is because @VincentDondain is missing [Override] in the property of the mutable objects, he needs
[Export ("fireDateComponents", ArgumentSemantic.Strong)]
[Override]
NSDateComponents FireDateComponents { get; set; }There was a problem hiding this comment.
Sounds like it could be indeed, I did not know of this attribute, never had to use it. Seemed weird to me that the generated code did not have override.
| [Watch (4,0), TV (11,0), iOS (11,0)] | ||
| [BaseType (typeof(HMTimeEvent))] | ||
| [DisableDefaultCtor] | ||
| interface HMDurationEvent : NSCopying, NSMutableCopying { |
| IntPtr Constructor (double duration); | ||
|
|
||
| [Export ("duration")] | ||
| double Duration { get; set; } |
There was a problem hiding this comment.
same (lack of setter on non-mutable property)
|
|
||
| // FIXME: Bug https://bugzilla.xamarin.com/show_bug.cgi?id=57870 | ||
| // [Wrap ("HMPresenceTypeExtensions.GetValue (_PresenceType)")] | ||
| // HMPresenceType PresenceType { get; set; } |
There was a problem hiding this comment.
same (non mutable should expose both)
| [BaseType (typeof(HMSignificantTimeEvent))] | ||
| interface HMMutableSignificantTimeEvent { | ||
|
|
||
| [Internal] |
There was a problem hiding this comment.
not sure it should be internal, it might be needed in some cases (some NSString are user extensible).
Maybe just [Advanced] to hide it (by default) from intelligence ?
|
|
||
| // FIXME: Bug https://bugzilla.xamarin.com/show_bug.cgi?id=57870 | ||
| // [Wrap ("HMSignificantEventExtensions.GetValue (_SignificantEvent)")] | ||
| // HMSignificantEvent SignificantEvent { get; set; } |
There was a problem hiding this comment.
same, non mutable version should match signature
|
Build failure |
|
Build failure |
1 similar comment
|
Build failure |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
@VincentDondain please verify you have [Override] in your mutable properties
|
|
||
| [Watch (4,0), TV (11,0), iOS (11,0)] | ||
| [Export ("homeDidUpdateAccessControlForCurrentUser:")] | ||
| void DidUpdateAccessControl (HMHome home); |
There was a problem hiding this comment.
mmm this is kinda not making sense in my head if home type parameter is ok then let's keep DidUpdateAccessControlForCurrentUser since CurrentUser ctx is lost
There was a problem hiding this comment.
I guess you're right, I understood it as the "Home" is the user or represents the user if I give it a bit more thoughts I don't think it's actually the case.
@brief Informs the delegate when the access control for current user has been updated.
| IntPtr Constructor (NSDateComponents fireDateComponents); | ||
|
|
||
| [Export ("fireDateComponents", ArgumentSemantic.Strong)] | ||
| NSDateComponents FireDateComponents { get; set; } |
There was a problem hiding this comment.
@spouliot this is because @VincentDondain is missing [Override] in the property of the mutable objects, he needs
[Export ("fireDateComponents", ArgumentSemantic.Strong)]
[Override]
NSDateComponents FireDateComponents { get; set; }| IntPtr Constructor (double duration); | ||
|
|
||
| [Export ("duration")] | ||
| double Duration { get; set; } |
|
Build failure |
|
Unrelated test failure: https://bugzilla.xamarin.com/show_bug.cgi?id=57762 |
6 participants
Reviewed iOS/watchOS/tvOS API diffs for beta 1 & 2.