[CarPlay] [Xcode 12] Update through beta 6 #9479

whitneyschmidt · 2020-08-24T22:40:53Z

TODO:
https://github.com/xamarin/maccore/issues/2297
https://github.com/xamarin/maccore/issues/2298

Add tests for manual code.

mandel-macaque · 2020-08-24T22:43:16Z

src/CarPlay/CPMessageListItem.cs

+	public partial class CPMessageListItem {
+
+		public CPMessageListItem (string identifier, string text, CPMessageListItemLeadingConfiguration leadingConfiguration,
+


Weird new line here, right?

mandel-macaque · 2020-08-24T22:44:05Z

src/carplay.cs

+		TwoColumn,
+	}
+
+	[Native]


Missing availability attrs.

mandel-macaque · 2020-08-24T22:44:10Z

src/carplay.cs

+		Cloud,
+	}
+
+	[Native]


mandel-macaque · 2020-08-24T22:44:15Z

src/carplay.cs

+		Trailing,
+	}
+
+	[Native]


mandel-macaque · 2020-08-24T22:44:21Z

src/carplay.cs

+		Star,
+	}
+
+	[Native]


mandel-macaque · 2020-08-24T22:50:34Z

src/carplay.cs

 		[Export ("searchTemplate:updatedSearchText:completionHandler:")]
 		void UpdatedSearchText (CPSearchTemplate searchTemplate, string searchText, CPSearchTemplateDelegateUpdateHandler completionHandler);

 		[Abstract]
+		[Async]


src/carplay.cs

mandel-macaque · 2020-08-24T22:53:25Z

src/carplay.cs

+	}
+
+
+#if false


On purpose?

Yes, see the linked issue: https://github.com/xamarin/maccore/issues/2298

Is it better to leave out for now until we know Apple is actually adding it?

either way is fine
you can add a link to the issue in the source (since the original commit message takes longer to get)

mandel-macaque · 2020-08-24T22:53:52Z

src/CarPlay/CPMessageListItem.cs

+            {
+            }
+
+		public CPMessageListItem (string identifier, string text, CPMessageListItemLeadingConfiguration leadingConfiguration, CPMessageListItemTrailingConfiguration? trailingConfiguration,


Manual code requires tests.

@whitneyschmidt this has not been fixed

mandel-macaque · 2020-08-24T22:55:10Z

src/carplay.cs

@@ -297,13 +417,15 @@ interface CPApplicationDelegate : UIApplicationDelegate {
 	}

 	[NoWatch, NoTV, NoMac, iOS (12,0)]
-	[BaseType (typeof (NSObject))]
+	[BaseType (typeof (CPBaseListItem))]


@spouliot question about API. The class is added in iOS 14, won't this be an issue in previous versions that do have access to the class in iOS 12 and iOS 13?

@mandel-macaque iirc, we saw a similar change in a recent PR.

I believe this is ok here because CPBaseListItem is a type of NSObject. Also worth noting - it looks like Apple is going to get rid of CPBaseListItem in favor of CPSelectableListItem...which isn't available and has been filed as a radar here: https://github.com/xamarin/maccore/issues/2298

[Deprecated (PlatformName.iOS, 14, 0)] [NoWatch, NoTV, NoMac, iOS (14,0)] [BaseType (typeof (NSObject))] [DisableDefaultCtor] interface CPBaseListItem : NSSecureCoding

@mandel-macaque it's not a problem, nor a breaking change.

We did have a bug (last year or so) where this caused an issue for 32bits code (because the new type would throw an exception as not being available on 32bits OS). This was fixed.

mandel-macaque · 2020-08-24T22:58:48Z

tests/introspection/iOS/iOSApiProtocolTest.cs

@@ -666,6 +676,15 @@ protected override bool Skip (Type type, string protocolName)
 				case "SRVisit":
 				case "SRWebUsage":
 					return true;
+				// Xcode 12 beta 4


beta 4 or beta 5?

good catch! beta 5 :)

monojenkins · 2020-08-25T00:02:31Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 75 tests passed.

Failed tests

Xtro/Mac: Failed (Test run failed.)

spouliot · 2020-08-25T00:01:43Z

src/carplay.cs

+		// Not sure about this one. ObjC header says: extern NSString * const CarPlayErrorDomain;
+		[Field ("CarPlayErrorDomain")]
+		NSString CarPlayErrorDomain { get; }


indeed, there's no enum.

grep "enum CP" *.g.cs ios-14.0.g.cs:public enum CPAlertActionStyle : ulong ios-14.0.g.cs:public enum CPBarButtonStyle : long ios-14.0.g.cs:public enum CPBarButtonType : ulong ios-14.0.g.cs:public enum CPTextButtonStyle : long ios-14.0.g.cs:public enum CPInformationTemplateLayout : long ios-14.0.g.cs:public enum CPListItemAccessoryType : long ios-14.0.g.cs:public enum CPListItemPlayingIndicatorLocation : long ios-14.0.g.cs:public enum CPNavigationAlertDismissalContext : ulong ios-14.0.g.cs:public enum CPTripPauseReason : ulong ios-14.0.g.cs:public enum CPPanDirection : long ios-14.0.g.cs:public enum CPManeuverDisplayStyle : long ios-14.0.g.cs:public enum CPTimeRemainingColor : ulong ios-14.0.g.cs:public enum CPTripEstimateStyle : ulong ios-14.0.g.cs:public enum CPMessageLeadingItem : long ios-14.0.g.cs:public enum CPMessageTrailingItem : long ios-14.0.g.cs:public enum CPLimitableUserInterface : ulong ios-14.0.g.cs:public enum CPContentStyle : ulong

I guess one could be added later... the error domain is rarely used in code (at least for comparison). I suggest you remove it and add this (lack of related enum) in the .ignore file. If an enum is added later (beta or release) then it can be added to the right place.

spouliot · 2020-08-25T00:04:14Z

src/carplay.cs

@@ -297,13 +417,15 @@ interface CPApplicationDelegate : UIApplicationDelegate {
 	}

 	[NoWatch, NoTV, NoMac, iOS (12,0)]
-	[BaseType (typeof (NSObject))]
+	[BaseType (typeof (CPBaseListItem))]


@mandel-macaque it's not a problem, nor a breaking change.

We did have a bug (last year or so) where this caused an issue for 32bits code (because the new type would throw an exception as not being available on 32bits OS). This was fixed.

spouliot · 2020-08-25T00:06:01Z

src/carplay.cs

+
+		[iOS (14,0)]
+		[Export ("indexOfItem:")]
+		nuint IndexOfItem (CPBaseListItem item);


no verb in the method, use GetIndex (CPBaseListItem item); instead

spouliot · 2020-08-25T00:06:25Z

src/carplay.cs

+
+		[iOS (14,0)]
+		[Export ("itemAtIndex:")]
+		CPBaseListItem ItemAtIndex (nuint index);


same, use GetItem (nuint index); instead

spouliot · 2020-08-25T00:06:59Z

src/carplay.cs

+		[iOS (14,0)]
+		[Export ("indexPathForItem:")]
+		[return: NullAllowed]
+		NSIndexPath IndexPathForItem (CPBaseListItem item);


same, GetIndexPath (CPBaseListItem item);

src/carplay.cs

spouliot · 2020-08-25T00:27:19Z

src/carplay.cs

+	[NoWatch, NoTV, NoMac, iOS (14,0)]
+	[Protocol]
+	[Model]
+	[BaseType (typeof (NSObject))]


not *Delegate
remove [Model] and [BaseType] attributes

src/carplay.cs

spouliot · 2020-08-25T00:30:00Z

src/carplay.cs

+	}
+
+
+#if false


either way is fine
you can add a link to the issue in the source (since the original commit message takes longer to get)

rolfbjarne · 2020-08-25T08:46:01Z

src/CarPlay/CPMessageListItem.cs

+
+namespace CarPlay {
+
+    public enum  CPMessageListItemType {


Extra space:

Suggested change

public enum CPMessageListItemType {

public enum CPMessageListItemType {

rolfbjarne · 2020-08-25T08:47:23Z

src/CarPlay/CPMessageListItem.cs

+                    "initWithFullName:phoneOrEmailAddress:leadingConfiguration:trailingConfiguration:detailText:trailingText:");
+                break;
+            default:
+                throw new ArgumentException ("The 'CPMessageListItemType type' argument needs a value.");


I think this is simpler and more accurate:

Suggested change

throw new ArgumentException ("The 'CPMessageListItemType type' argument needs a value.");

throw new ArgumentOutOfRangeException (nameof (type));

mandel-macaque · 2020-08-26T23:12:03Z

src/CarPlay/CPMessageListItem.cs

+
+namespace CarPlay {
+
+    public enum CPMessageListItemType {


Availability attributes missing.

monojenkins · 2020-08-27T00:27:06Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

1 tests failed, 75 tests passed.

Failed tests

Xtro/Mac: Failed (Test run failed.)

spouliot

Please update xtro before merging

monojenkins · 2020-08-27T22:05:13Z

Build failure
❌ Build failed or was aborted

monojenkins · 2020-08-27T23:26:13Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

whitneyschmidt · 2020-08-27T23:53:08Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
⚠️ API Diff (from PR only) (🔥 breaking changes 🔥)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

In beta 6:

API_AVAILABLE(ios(12.0)) API_UNAVAILABLE(macos, watchos, tvos)
@interface CPListItem : NSObject <CPSelectableListItem>

The api diff in beta 1 shows:

 API_AVAILABLE(ios(12.0)) API_UNAVAILABLE(macos, watchos, tvos)
-@interface CPListItem : NSObject <NSSecureCoding>
+@interface CPListItem : CPBaseListItem

It seems that the breaking change is valid if Apple meant to remove conformance to NSSecureCoding for this type. However, that seem very strange for an API that's been available since iOS 12.

dalexsoto · 2020-08-28T01:21:58Z

I would file an apple feedback ticked and add this to our tracking issue #8943

spouliot · 2020-08-28T01:34:16Z

I agree with @dalexsoto, file an feedback with Apple. But in any case you re-add INSSecureCoding to avoid breaking changes.

…ange

monojenkins · 2020-08-28T17:27:51Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)
✅ Test run succeeded

whitneyschmidt · 2023-02-06T22:40:36Z

#17461

whitneyschmidt added the iOS Issues affecting Xamarin.iOS label Aug 24, 2020

whitneyschmidt added this to the xcode12 milestone Aug 24, 2020

whitneyschmidt requested review from dalexsoto and spouliot as code owners August 24, 2020 22:40

whitneyschmidt requested a review from tj-devel709 August 24, 2020 22:41

mandel-macaque requested changes Aug 24, 2020

View reviewed changes

mandel-macaque reviewed Aug 24, 2020

View reviewed changes

spouliot mentioned this pull request Aug 24, 2020

Xcode 12 final audit checklist #8943

Open

46 tasks

spouliot suggested changes Aug 25, 2020

View reviewed changes

rolfbjarne approved these changes Aug 25, 2020

View reviewed changes

whitneyschmidt added the note-highlight Worth calling out specifically in release notes label Aug 25, 2020

stephen-hawley approved these changes Aug 26, 2020

View reviewed changes

whitneyschmidt force-pushed the carplay branch from 0edb449 to 62765cf Compare August 26, 2020 23:05

mandel-macaque requested changes Aug 26, 2020

View reviewed changes

src/CarPlay/CPMessageListItem.cs

namespace CarPlay {

public enum CPMessageListItemType {

Copy link

Member

mandel-macaque Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Availability attributes missing.

spouliot approved these changes Aug 27, 2020

View reviewed changes

whitneyschmidt added 9 commits August 27, 2020 14:54

carplay initial updates through beta 5

9ebca03

add async for completion handlers

7b0285d

remove noise

e03c179

re-add new carplay file

6dbb09a

remove sharpie comments

e5ab5bb

fix minor formatting

92cd16e

remove more whitespace nosie

1629fb1

update for beta 6, respond to feedback

f2b54eb

update to add ut, fix xtro failures

bf487d9

whitneyschmidt force-pushed the carplay branch from 62765cf to bf487d9 Compare August 27, 2020 18:56

whitneyschmidt added 2 commits August 27, 2020 15:11

tidy up tests file

d8760d2

add availability to manual code enum

317b04b

mandel-macaque approved these changes Aug 27, 2020

View reviewed changes

whitneyschmidt changed the title ~~[CarPlay] [Xcode 12] Update through beta 5~~ [CarPlay] [Xcode 12] Update through beta 6 Aug 27, 2020

dalexsoto approved these changes Aug 28, 2020

View reviewed changes

tj-devel709 approved these changes Aug 28, 2020

View reviewed changes

re-add conformance to NSSecureCoding to CPListItem to fix breaking ch…

1b7e0e4

…ange

whitneyschmidt merged commit efe8abd into xamarin:xcode12 Aug 28, 2020

		public partial class CPMessageListItem {

		public CPMessageListItem (string identifier, string text, CPMessageListItemLeadingConfiguration leadingConfiguration,

	public enum CPMessageListItemType {
	public enum CPMessageListItemType {

	throw new ArgumentException ("The 'CPMessageListItemType type' argument needs a value.");
	throw new ArgumentOutOfRangeException (nameof (type));

		}


		#if false

		}


		#if false

[CarPlay] [Xcode 12] Update through beta 6 #9479

[CarPlay] [Xcode 12] Update through beta 6 #9479

Conversation

whitneyschmidt commented Aug 24, 2020 • edited Loading

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

monojenkins commented Aug 25, 2020

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

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

monojenkins commented Aug 27, 2020

Test results

Failed tests

spouliot left a comment

Choose a reason for hiding this comment

monojenkins commented Aug 27, 2020

monojenkins commented Aug 27, 2020

whitneyschmidt commented Aug 27, 2020

dalexsoto commented Aug 28, 2020

spouliot commented Aug 28, 2020

monojenkins commented Aug 28, 2020

whitneyschmidt commented Feb 6, 2023

whitneyschmidt commented Aug 24, 2020 •

edited

Loading