[GameController][Xcode12] Updates through beta 4 #9265

whitneyschmidt · 2020-08-03T19:39:00Z

No description provided.

whitneyschmidt · 2020-08-03T19:41:32Z

src/gamecontroller.cs

 	[DisableDefaultCtor] // return nil handle -> only exposed as getter
 	partial interface GCGamepad {

-		[NullAllowed]
+		//[NullAllowed] This seems like it's a breaking change...


Is this a breaking change? 🤔 It doesn't seem like it. Any code that assumes that a null may be returned will still work here - we're restricting the return value to a subset of what it used to be.

I don't think changing this has any effect on our generated code (IOW it's fine).

AFAIK it is not an issue at all. It is a get and not a set, so we do not care. @whitneyschmidt on a 'set' we do check for the value and throw a NRE depending on the value, but since we only return, we are fine.

Nope not a breaking change

whitneyschmidt · 2020-08-03T19:41:53Z

src/gamecontroller.cs

+		[Export ("supportedLocalities", ArgumentSemantic.Strong)]
+		NSSet<NSString> SupportedLocalities { get; }
+
+		// Dependency on Xcode 12 CoreHaptics bindings


FYI @mandel-macaque.

I know this guy. It is in my feedback report, thanks for the headsup :)

Do we want to link to the issue here for easier record keeping? :)

You could add it here #8943

Looks like CoreHaptics is blocked on AVAudioSession for Mac bindings :(

https://github.com/xamarin/maccore/issues/2261

whitneyschmidt · 2020-08-03T19:42:33Z

src/gamecontroller.cs

+		GCControllerButtonInput PaddleButton4 { get; }
+	}
+
+	[Static] // I tried moving GCKey and GCKeyCode to separate files, but ran into


Is there a way for GCKey and GCKeyCode to live in their own files?

It's possible, but it's somewhat of a special case.

Basically you'd have to add it like xkit.cs is added here:

xamarin-macios/src/frameworks.sources

Line 1799 in 53dcedc

AppKit XKit \

I don't think it's worth it to have binding code in a separate file, xkit.cs is literally the only exception, and that's only because we want the code in both UIKit and AppKit at the same time.

rolfbjarne · 2020-08-03T21:07:39Z

src/gamecontroller.cs

 	[DisableDefaultCtor] // return nil handle -> only exposed as getter
 	partial interface GCGamepad {

-		[NullAllowed]
+		//[NullAllowed] This seems like it's a breaking change...


I don't think changing this has any effect on our generated code (IOW it's fine).

src/gamecontroller.cs

rolfbjarne · 2020-08-03T21:15:55Z

src/gamecontroller.cs

+		GCControllerButtonInput PaddleButton4 { get; }
+	}
+
+	[Static] // I tried moving GCKey and GCKeyCode to separate files, but ran into


It's possible, but it's somewhat of a special case.

Basically you'd have to add it like xkit.cs is added here:

xamarin-macios/src/frameworks.sources

Line 1799 in 53dcedc

AppKit XKit \

I don't think it's worth it to have binding code in a separate file, xkit.cs is literally the only exception, and that's only because we want the code in both UIKit and AppKit at the same time.

src/gamecontroller.cs

mandel-macaque · 2020-08-03T21:48:36Z

src/gamecontroller.cs

 	[DisableDefaultCtor] // return nil handle -> only exposed as getter
 	partial interface GCGamepad {

-		[NullAllowed]
+		//[NullAllowed] This seems like it's a breaking change...


AFAIK it is not an issue at all. It is a get and not a set, so we do not care. @whitneyschmidt on a 'set' we do check for the value and throw a NRE depending on the value, but since we only return, we are fine.

mandel-macaque · 2020-08-03T21:49:52Z

src/gamecontroller.cs

@@ -202,7 +226,7 @@ partial interface GCGamepadSnapshot {

 	[iOS (7,0)]
 	[Mac (10,9)]
-	[BaseType (typeof (NSObject))]
+	[BaseType (typeof (GCPhysicalInputProfile))]


All this base changes are ok, but do remember that changing a base COULD be a breaking change. Not in this case since GCPhysicalInputProfile is a NSObject and it was the parent of the old class.

It will be listed as a breaking change but there is nothing we can do about it, if a base changes we have to honor that

This case technically should not be an issue. Others might: https://docs.microsoft.com/en-us/dotnet/core/compatibility/#types

REQUIRES JUDGMENT: Introducing a new base class

A type can be introduced into a hierarchy between two existing types if it doesn't introduce any new abstract members or change the semantics or behavior of existing types. For example, in .NET Framework 2.0, the DbConnection class became a new base class for SqlConnection, which had previously derived directly from Component.

mandel-macaque · 2020-08-03T21:54:03Z

src/gamecontroller.cs

+		[TV (14, 0), Mac (11, 0), iOS (14, 0)]
+		[Export ("acceleration")]
+		GCAcceleration Acceleration { get; }
+
+		[TV (14,0), Mac (11,0), iOS (14,0)]
+		[Export ("setAcceleration:")]
+		void SetAcceleration (GCAcceleration acceleration);


Method and a property? That is very weird? Acceleration and SetAcceleration do look like a perfect candidate for a property.

mandel-macaque · 2020-08-03T21:54:48Z

src/gamecontroller.cs

+		[Export ("supportedLocalities", ArgumentSemantic.Strong)]
+		NSSet<NSString> SupportedLocalities { get; }
+
+		// Dependency on Xcode 12 CoreHaptics bindings


I know this guy. It is in my feedback report, thanks for the headsup :)

monojenkins · 2020-08-03T21:56:56Z

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

dalexsoto · 2020-08-04T14:54:27Z

src/gamecontroller.cs

+		[TV (14, 0), Mac (11, 0), iOS (14, 0)]
+		[Export ("acceleration")]
+		GCAcceleration Acceleration { get; }
+
+		[TV (14,0), Mac (11,0), iOS (14,0)]
+		[Export ("setAcceleration:")]
+		void SetAcceleration (GCAcceleration acceleration);


Suggested change

[TV (14, 0), Mac (11, 0), iOS (14, 0)]

[Export ("acceleration")]

GCAcceleration Acceleration { get; }

[TV (14,0), Mac (11,0), iOS (14,0)]

[Export ("setAcceleration:")]

void SetAcceleration (GCAcceleration acceleration);

[TV (14, 0), Mac (11, 0), iOS (14, 0)]

[Export ("acceleration")]

GCAcceleration Acceleration { get; set; }

dalexsoto · 2020-08-04T14:57:58Z

src/gamecontroller.cs

+		[Export ("supportedLocalities", ArgumentSemantic.Strong)]
+		NSSet<NSString> SupportedLocalities { get; }
+
+		// Dependency on Xcode 12 CoreHaptics bindings


You could add it here #8943

dalexsoto · 2020-08-04T15:02:05Z

src/gamecontroller.cs

+		[Field ("GCHapticsLocalityDefault")]
+		NSString LocalityDefault { get; }
+
+		[Field ("GCHapticsLocalityAll")]
+		NSString LocalityAll { get; }
+
+		[Field ("GCHapticsLocalityHandles")]
+		NSString LocalityHandles { get; }
+
+		[Field ("GCHapticsLocalityLeftHandle")]
+		NSString LocalityLeftHandle { get; }
+
+		[Field ("GCHapticsLocalityRightHandle")]
+		NSString LocalityRightHandle { get; }
+
+		[Field ("GCHapticsLocalityTriggers")]
+		NSString LocalityTriggers { get; }
+
+		[Field ("GCHapticsLocalityLeftTrigger")]
+		NSString LocalityLeftTrigger { get; }
+
+		[Field ("GCHapticsLocalityRightTrigger")]
+		NSString LocalityRightTrigger { get; }


I wonder if these should be grouped on their own static class or smart enum depending on how they are used for example

[Static] interface GCHapticsLocality { [Field ("GCHapticsLocalityDefault")] NSString Default { get; } [Field ("GCHapticsLocalityAll")] NSString All { get; } [Field ("GCHapticsLocalityHandles")] NSString Handles { get; } [Field ("GCHapticsLocalityLeftHandle")] NSString LeftHandle { get; } [Field ("GCHapticsLocalityRightHandle")] NSString RightHandle { get; } [Field ("GCHapticsLocalityTriggers")] NSString Triggers { get; } [Field ("GCHapticsLocalityLeftTrigger")] NSString LeftTrigger { get; } [Field ("GCHapticsLocalityRightTrigger")] NSString RightTrigger { get; } }

dalexsoto · 2020-08-04T15:03:48Z

src/gamecontroller.cs

+		[Field ("GCHapticDurationInfinite")]
+		float DurationInfinite { get; }


I am not sure if we should remove the Haptic part, else I am unsure what infinite duration is it about

Suggested change

[Field ("GCHapticDurationInfinite")]

float DurationInfinite { get; }

[Field ("GCHapticDurationInfinite")]

float HapticDurationInfinite { get; }

dalexsoto · 2020-08-04T15:05:17Z

src/gamecontroller.cs

+		[Notification, Field ("GCKeyboardDidDisconnectNotification")]
+		NSString DisconnectNotification { get; }


Suggested change

[Notification, Field ("GCKeyboardDidDisconnectNotification")]

NSString DisconnectNotification { get; }

[Notification, Field ("GCKeyboardDidDisconnectNotification")]

NSString DidDisconnectNotification { get; }

src/gamecontroller.cs

dalexsoto · 2020-08-04T15:09:10Z

src/gamecontroller.cs

+	[BaseType (typeof (NSObject))]
+	[Protocol]
+	[Model]


Suggested change

[BaseType (typeof (NSObject))]

[Protocol]

[Model]

[Protocol]

Aha! So we can have just [Protocol] if we remove [Model] and [BaseType...]! So [Model] and [BaseType...] always go together but [Protocol] can be independent. Thank you @dalexsoto :)

dalexsoto · 2020-08-04T15:11:12Z

src/gamecontroller.cs

+		[TV (9, 0), Mac (10, 9), iOS (7, 0)]
+		[Abstract]
+		[Export ("handlerQueue", ArgumentSemantic.Strong)]
+		DispatchQueue HandlerQueue { get; set; }
+
+		[TV (9, 0), Mac (10, 9), iOS (7, 0)]
+		[Abstract]
+		[NullAllowed, Export ("vendorName")]
+		string VendorName { get; }
+
+		[TV (13, 0), Mac (10, 15), iOS (13, 0)]
+		[Abstract]
+		[Export ("productCategory")]
+		string ProductCategory { get; }
+
+		[TV (14, 0), Mac (11, 0), iOS (14, 0)]
+		[Abstract]
+		[Export ("physicalInputProfile", ArgumentSemantic.Strong)]
+		GCPhysicalInputProfile PhysicalInputProfile { get; }


The availability attributes do not make sense here, specially since the container protocol is [TV (14,0), Mac (11,0), iOS (14,0)] so someone is lying to us

This is bizarre because the availability attributes in the header match these ones...but it's impossible for that to be true. I'm thinking we should just leave the container protocol's availability attribute and remove the rest of them so that everything has [TV (14,0), Mac (11,0), iOS (14,0)].

dalexsoto · 2020-08-04T15:11:42Z

src/gamecontroller.cs

+
+		[Export ("objectForKeyedSubscript:")]
+		[return: NullAllowed]
+		GCControllerElement ObjectForKeyedSubscript (string key);


Suggested change

GCControllerElement ObjectForKeyedSubscript (string key);

GCControllerElement GetObjectForKeyedSubscript (string key);

dalexsoto · 2020-08-04T15:15:29Z

src/gamecontroller.cs

+        NSString RightGui { get; }
+    }
+
+	[TV (14, 0), Mac (11, 0), iOS (14, 0)]


indentation :)

monojenkins · 2020-08-04T22:24:35Z

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

3 tests failed, 73 tests passed.

Failed tests

introspection/Mac Modern/Debug: Failed (Test run failed.)
introspection/iOS Unified 64-bits - simulator/Debug (iOS 10.3): TimedOut
introspection/tvOS - simulator/Debug (tvOS 10.2): Failed

rolfbjarne · 2020-08-05T07:20:35Z

src/gamecontroller.cs

+	[Static]
+	[TV (14, 0), Mac (11, 0), iOS (14, 0)]
+	partial interface GCKey
+    {


Indentation is space here.

Good catch! Thank you 😄

mandel-macaque · 2020-08-05T16:25:26Z

src/gamecontroller.cs


 using System;

 using CoreFoundation;
 using Foundation;
 using ObjCRuntime;
 using OpenTK;
+//#using CoreHaptics; // need CoreHaptics update for TV 14.0 inclusion


PR with support for CoreHaptics on tvOS landed on the xcode12 branch: #9273

monojenkins · 2020-08-05T23:18:11Z

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

dalexsoto · 2020-08-05T23:51:06Z

tests/introspection/iOS/iOSApiProtocolTest.cs

+				// iOS 14 beta 3 (to be reviewed)
+				case "GCDeviceBattery":
+				case "GCDeviceLight":
+				case "GCDualShockGamepad":
+				case "GCKeyboard":
+				case "GCMouse":
+				case "GCXboxGamepad":


Is this and the one below filled as an issue somewhere? So we do not forget to review these in the future or you could check for a specific Xcode Version in the tests so they fail if not fixed in the future, also if you consider this to be an Apple mistake file a bug :)

rolfbjarne · 2020-08-06T09:15:06Z

src/gamecontroller.cs

@@ -535,4 +616,1157 @@ interface GCEventViewController {
 		[Export ("controllerUserInteractionEnabled")]
 		bool ControllerUserInteractionEnabled { get; set; }
 	}
+
+	[TV (14,0), Mac (11,0), iOS (14,0)]
+	[BaseType (typeof( NSObject))]


Suggested change

[BaseType (typeof( NSObject))]

[BaseType (typeof (NSObject))]

monojenkins · 2020-08-07T06:35:41Z

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

whitneyschmidt requested a review from spouliot as a code owner August 3, 2020 19:39

whitneyschmidt requested review from mandel-macaque, dalexsoto and chamons August 3, 2020 19:39

whitneyschmidt commented Aug 3, 2020

View reviewed changes

rolfbjarne requested changes Aug 3, 2020

View reviewed changes

mandel-macaque requested changes Aug 3, 2020

View reviewed changes

dalexsoto requested changes Aug 4, 2020

View reviewed changes

whitneyschmidt added this to the xcode12 milestone Aug 4, 2020

spouliot mentioned this pull request Aug 4, 2020

Xcode 12 final audit checklist #8943

Open

46 tasks

rolfbjarne approved these changes Aug 5, 2020

View reviewed changes

mandel-macaque reviewed Aug 5, 2020

View reviewed changes

whitneyschmidt added 7 commits August 5, 2020 15:04

initial commit for GameController

446371e

remove frameworks.sources changes

894325c

small tidying up

5ebc9de

remove noise

3686e56

remove more noise

c046fa0

tidy up more

5f35ce7

feedback fixes

72dcf63

whitneyschmidt changed the title ~~[GameController][Xcode12] Updates through beta 3~~ [GameController][Xcode12] Updates through beta 4 Aug 5, 2020

update w/ CoreHaptics changes, fix intro failures

05766d6

whitneyschmidt force-pushed the new_gamecontroller branch from e230e75 to 05766d6 Compare August 5, 2020 20:59

whitneyschmidt requested review from mandel-macaque, rolfbjarne and dalexsoto August 5, 2020 23:38

dalexsoto approved these changes Aug 5, 2020

View reviewed changes

rolfbjarne approved these changes Aug 6, 2020

View reviewed changes

whitneyschmidt added 2 commits August 6, 2020 11:36

fix space

61312a1

update protocol conformance

32e0b4a

whitneyschmidt merged commit 1f77d1b into xamarin:xcode12 Aug 7, 2020

whitneyschmidt added iOS Issues affecting Xamarin.iOS macOS Issues affecting Xamarin.Mac note-highlight Worth calling out specifically in release notes labels Sep 9, 2020

		[Field ("GCHapticDurationInfinite")]
		float DurationInfinite { get; }

		[Notification, Field ("GCKeyboardDidDisconnectNotification")]
		NSString DisconnectNotification { get; }

	GCControllerElement ObjectForKeyedSubscript (string key);
	GCControllerElement GetObjectForKeyedSubscript (string key);

	[BaseType (typeof( NSObject))]
	[BaseType (typeof (NSObject))]

[GameController][Xcode12] Updates through beta 4 #9265

[GameController][Xcode12] Updates through beta 4 #9265

Conversation

whitneyschmidt commented Aug 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Aug 3, 2020

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 4, 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

monojenkins commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Aug 7, 2020

whitneyschmidt commented Aug 3, 2020 •

edited

Loading