-
Notifications
You must be signed in to change notification settings - Fork 511
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ClockKit] Add support for Xcode 12 beta 1. #9010
[ClockKit] Add support for Xcode 12 beta 1. #9010
Conversation
Build success |
|
||
[Watch (7, 0), NoiOS] | ||
[Static] | ||
[Export ("templateWithRow1ImageProvider:row1Column1TextProvider:row1Column2TextProvider:row2ImageProvider:row2Column1TextProvider:row2Column2TextProvider:row3ImageProvider:row3Column1TextProvider:row3Column2TextProvider:")] |
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.
Looks like someone never heard of arrays before...
|
||
[Watch (7, 0), NoiOS] | ||
[BaseType (typeof (CLKComplicationTemplateGraphicExtraLargeCircular))] | ||
interface CLKComplicationTemplateGraphicExtraLargeCircularOpenGaugeRangeText { |
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.
Say that 10 times fast as you can!
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
src/Constants.iOS.cs.in
Outdated
@@ -146,5 +146,7 @@ namespace ObjCRuntime { | |||
public const string AutomaticAssessmentConfigurationLibrary = "/System/Library/Frameworks/AutomaticAssessmentConfiguration.framework/AutomaticAssessmentConfiguration"; | |||
// iOS 14.0 | |||
public const string AppClipLibrary = "/System/Library/Frameworks/AppClip.framework/AppClip"; | |||
// waiting on apple to fix the headers https://github.com/xamarin/maccore/issues/2258 | |||
// public const string ClockKitLibrary = "/System/Library/Frameworks/ClockKit.framework/ClockKit"; |
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 don't think ClockKit is supposed to be available on iOS, looks like an header error in Xcode (which would explain the lack of framework on iOS 14)
tests/mtouch/RegistrarTest.cs
Outdated
@@ -346,6 +346,8 @@ public void MT4134 () | |||
new { Framework = "BackgroundTasks", Version = "13.0" }, | |||
new { Framework = "QuickLookThumbnailing", Version = "13.0" }, | |||
new { Framework = "AutomaticAssessmentConfiguration", Version = "13.4" }, | |||
// waiting on apple to ifx the headers https://github.com/xamarin/maccore/issues/2258 | |||
// new { Framework = "ClockKit", Version = "14.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.
I would not add the lines for this, since the fix will likely mean we don't need them (and they won't merge cleanly with other new frameworks to be added)
tools/common/Frameworks.cs
Outdated
@@ -333,6 +333,9 @@ public static Frameworks GetiOSFrameworks (Application app) | |||
|
|||
{ "AppClip", "AppClip", 14,0 }, | |||
|
|||
// waiting on apple to fix it: https://github.com/xamarin/maccore/issues/2258 | |||
// { "ClockKit", "ClockKit", 14,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.
same
tools/mtouch/Makefile
Outdated
@@ -162,6 +162,9 @@ SIMLAUNCHER_FRAMEWORKS = \ | |||
-weak_framework AutomaticAssessmentConfiguration \ | |||
\ | |||
-weak_framework AppClip \ | |||
# waiting on apple to fix the framework on ios | |||
# https://github.com/xamarin/maccore/issues/2258 | |||
# -weak_framework ClockKit \ |
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
!incorrect-protocol-member! CLKComplicationDataSource::getSupportedTimeTravelDirectionsForComplication:withHandler: is OPTIONAL and should NOT be abstract | ||
|
||
# no needed, families is a smart enum and the same can be achived with .net | ||
!missing-pinvoke! CLKAllComplicationFamilies is not bound |
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 it needs to be bound
sounds like it can return a different subset based on OS version (or even watch model at some point) while our smart enum will always have the same values (and the availability attributes are removed by the linker)
src/clockkit.cs
Outdated
[Export ("initWithTextProvider:")] | ||
IntPtr Constructor (CLKTextProvider textProvider); | ||
|
||
[Watch (7, 0), NoiOS] |
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/clockkit.cs
Outdated
} | ||
|
||
[BaseType (typeof (CLKComplicationTemplate))] | ||
interface CLKComplicationTemplateModularSmallSimpleImage { | ||
|
||
[Export ("imageProvider", ArgumentSemantic.Copy)] | ||
CLKImageProvider ImageProvider { get; set; } | ||
|
||
[Watch (7, 0), NoiOS] |
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.
double check all [NoiOS]
src/clockkit.cs
Outdated
interface CLKImageProvider : NSCopying { | ||
|
||
[Deprecated (PlatformName.iOS, 14, 0, message: "The default constructor is deprecated.")] |
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 need for this message - unless we can provide what should be used
src/clockkit.cs
Outdated
interface CLKTextProvider : NSCopying { | ||
|
||
[Deprecated (PlatformName.iOS, 14, 0, message: "The default constructor is deprecated. Use the provided factories instead.")] |
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, only last part is useful
src/clockkit.cs
Outdated
CLKComplicationTemplateGraphicRectangularFullImage Create (CLKFullColorImageProvider imageProvider); | ||
} | ||
|
||
[Abstract] // from docs 'An abstract superclass for all the extra large circular graphic templates.' |
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.
we had problems with [Abstract]
before, since Apple can return instance of them.
Might be better to have no public ctor (and no member) and an [Advice]
this is meant to abstract
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.
Do we want to deal with the one in https://github.com/xamarin/xamarin-macios/blob/main/src/clockkit.cs#L715 too? That is the main reason I used Abstract here, to follow the same pattern.
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 wonder if we should be fixing this at the generator level, i.e. [Abstract]
on a type to mean [Advice ("blah!")]
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 wonder if we should be fixing this at the generator level, i.e.
[Abstract]
on a type to mean[Advice ("blah!")]
It's already filed: #4969
Build success |
|
||
[Watch (7, 0), NoiOS] | ||
[Export ("initWithRow1Column1TextProvider:row1Column2TextProvider:row2Column1TextProvider:row2Column2TextProvider:")] | ||
IntPtr Constructor (CLKTextProvider row1Column1TextProvider, CLKTextProvider row1Column2TextProvider, CLKTextProvider row2Column1TextProvider, CLKTextProvider row2Column2TextProvider); |
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 wonder whether you could simplify all of these param names to row1Column1Provider since they're all text providers?
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.
Open to suggestions, but the name has to point to the row and the column, we could remove TextProvider, but doesn't make it much nicer :/
As rolf said, someone does not know about arrays
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 custom .ctor (CLKTextProvider[,] textProviders)
with additional validations (ranges) could be manually added (and tested) and this one could become internal (or protected for subclassing).
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 might know a trick I don't with C# because the way to call another constructor on c# is:
public CLKComplicationTemplateModularLargeTable (CLKTextProvider[] data) : this (data[0], data[1], data[2], data[3], data[4]) {
}
And this code is super error prone. I cannot validate that data is not null AND I cannot check the number of items before I call this. Factory methods, Ok, no problem, the constructors, I don't see how unless I do a binding to a IntPtr for the objc msg send etc..
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 an ugly trick using a static method : this (CheckData (data), data [1], ...)
But better call objc_msgSend (and not this
)
Not sure it's worth it... at least right now (can be added at any time)
|
||
[Watch (7, 0), NoiOS] | ||
[Export ("initWithHeaderTextProvider:row1Column1TextProvider:row1Column2TextProvider:row2Column1TextProvider:row2Column2TextProvider:")] | ||
IntPtr Constructor (CLKTextProvider headerTextProvider, CLKTextProvider row1Column1TextProvider, CLKTextProvider row1Column2TextProvider, CLKTextProvider row2Column1TextProvider, CLKTextProvider row2Column2TextProvider); |
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 above.
[Watch (7, 0), NoiOS] | ||
[Static] | ||
[Export ("templateWithHeaderImageProvider:headerTextProvider:row1Column1TextProvider:row1Column2TextProvider:row2Column1TextProvider:row2Column2TextProvider:")] | ||
CLKComplicationTemplateModularLargeTable Create ([NullAllowed] CLKImageProvider headerImageProvider, CLKTextProvider headerTextProvider, CLKTextProvider row1Column1TextProvider, CLKTextProvider row1Column2TextProvider, CLKTextProvider row2Column1TextProvider, CLKTextProvider row2Column2TextProvider); |
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
|
||
[Watch (7, 0), NoiOS] | ||
[Export ("initWithRow1Column1TextProvider:row1Column2TextProvider:row2Column1TextProvider:row2Column2TextProvider:row3Column1TextProvider:row3Column2TextProvider:")] | ||
IntPtr Constructor (CLKTextProvider row1Column1TextProvider, CLKTextProvider row1Column2TextProvider, CLKTextProvider row2Column1TextProvider, CLKTextProvider row2Column2TextProvider, CLKTextProvider row3Column1TextProvider, CLKTextProvider row3Column2TextProvider); |
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.
And more of the same?
Build failure |
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.
how did you merge ?
your PR now includes stuff that should not be there
src/ClockKit/CLKComplication.cs
Outdated
public partial class CLKComplication { | ||
[Watch (7,0)] | ||
[DllImport (Constants.ClockKitLibrary, EntryPoint = "CLKAllComplicationFamilies")] | ||
public static extern NSNumber[] GetAllComplicationFamilies (); |
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, that needs a test because it defy my understanding of p/invokes wrt ObjC ;-)
src/clockkit.cs
Outdated
@@ -13,12 +13,38 @@ | |||
using UIKit; | |||
|
|||
namespace ClockKit { | |||
|
|||
[Watch (7,0), iOS (14, 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.
remove [iOS]
src/clockkit.cs
Outdated
[Export ("getSupportedTimeTravelDirectionsForComplication:withHandler:")] | ||
void GetSupportedTimeTravelDirections (CLKComplication complication, Action<CLKComplicationTimeTravelDirections> handler); | ||
|
||
[Deprecated (PlatformName.WatchOS, 7, 0, message: "Backwards extension and time travel is not longer supported.")] |
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 not
-> are no
?
src/clockkit.cs
Outdated
@@ -51,6 +79,7 @@ interface CLKComplicationDataSource { | |||
[Export ("getCurrentTimelineEntryForComplication:withHandler:")] | |||
void GetCurrentTimelineEntry (CLKComplication complication, Action<CLKComplicationTimelineEntry> handler); | |||
|
|||
[Deprecated (PlatformName.WatchOS, 7, 0, message: "Backwards extension and time travel is not longer supported.")] |
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 not
-> are no
?
src/clockkit.cs
Outdated
interface CLKComplicationTemplate : NSCopying { | ||
|
||
[NullAllowed, Export ("tintColor", ArgumentSemantic.Copy)] | ||
UIColor TintColor { get; set; } | ||
|
||
[Deprecated (PlatformName.iOS, 14, 0, message: "Use the provided factories instead.")] |
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.
watchOS ?
it was not available before on iOS so it can't be deprecated in 14
src/clockkit.cs
Outdated
interface CLKImageProvider : NSCopying { | ||
|
||
[Deprecated (PlatformName.iOS, 14, 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.
same
Build success |
Build failure Test results16 tests failed, 77 tests passed.Failed tests
|
src/ClockKit/CLKComplication.cs
Outdated
[DllImport (Constants.ClockKitLibrary)] | ||
static extern IntPtr CLKAllComplicationFamilies (); | ||
|
||
public static NSNumber[] GetAllComplicationFamilies () |
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.
NSNumber
is not a very nice type to expose and you already did a better job elsewhere in the bindings ;-)
[Export ("supportedFamilies")]
[BindAs ( typeof (CLKComplicationFamily []))]
NSNumber[] SupportedFamilies { get; }
Build failure ✅ Build succeeded |
Build success |
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 a couple of nits but 👍
public static CLKComplicationFamily[] GetAllComplicationFamilies () | ||
{ | ||
using (var nsArray = new NSArray (CLKAllComplicationFamilies ())) { | ||
var families = new CLKComplicationFamily [(int)nsArray.Count]; | ||
for (nuint i = 0; i < nsArray.Count; i++) | ||
{ | ||
families[i] = (CLKComplicationFamily)nsArray.GetItem <NSNumber> (i).Int32Value; | ||
} | ||
return families; | ||
} | ||
} |
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.
cute but 😛
public static CLKComplicationFamily[] GetAllComplicationFamilies () | |
{ | |
using (var nsArray = new NSArray (CLKAllComplicationFamilies ())) { | |
var families = new CLKComplicationFamily [(int)nsArray.Count]; | |
for (nuint i = 0; i < nsArray.Count; i++) | |
{ | |
families[i] = (CLKComplicationFamily)nsArray.GetItem <NSNumber> (i).Int32Value; | |
} | |
return families; | |
} | |
} | |
[Watch (7,0)] | |
public static CLKComplicationFamily[] GetAllComplicationFamilies () => NSArray.EnumsFromHandle<CLKComplicationFamily> (CLKAllComplicationFamilies ()); |
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... source code beauty contest ;-)
I prefer the smallest (IL) one :)
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.
@spouliot heh I like consistency (I wonder who taught me that 😁) if we do (or don't) like NSArray.EnumsFromHandle
then we have a problem elsewhere
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, after my experience with LINQ... I'm going for the explicit implementation
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.
LINQ? There is no linq involved at all :)
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.
@mandel-macaque if you mean =>
that is just an Expression body definition and if you really do not like it you can do
public static CLKComplicationFamily[] GetAllComplicationFamilies () | |
{ | |
using (var nsArray = new NSArray (CLKAllComplicationFamilies ())) { | |
var families = new CLKComplicationFamily [(int)nsArray.Count]; | |
for (nuint i = 0; i < nsArray.Count; i++) | |
{ | |
families[i] = (CLKComplicationFamily)nsArray.GetItem <NSNumber> (i).Int32Value; | |
} | |
return families; | |
} | |
} | |
[Watch (7,0)] | |
public static CLKComplicationFamily[] GetAllComplicationFamilies () | |
{ | |
return NSArray.EnumsFromHandle<CLKComplicationFamily> (CLKAllComplicationFamilies ()); | |
} |
That said you are missing [Watch (7,0)]
for GetAllComplicationFamilies
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, I use => alot, I'm talking about the IL of EnumsFromHandle hehe
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 call to EnumsFromHandle
is a lot smaller then the code above
the total size diff will depend on the app, and if anything else uses EnumsFromHandle
or the code it depends on
so YYMV, but considering watchOS size limitations having less code is generally better
Co-authored-by: Alex Soto <alex@alexsoto.me>
Build failure ✅ Build succeeded |
Build success |
src/ClockKit/CLKComplication.cs
Outdated
} | ||
} | ||
} | ||
} |
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.
🚫
} | ||
} | ||
} | ||
#endif |
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.
🚫
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
Build failure |
Build success |
No description provided.