[modelio] Update for iOS 10.3 beta 1 by VincentDondain · Pull Request #1601 · dotnet/macios

VincentDondain · 2017-01-31T20:47:14Z

Added MDLAssetTest (copy from MDLObjectTest).
Indexers tests.

spouliot

I'm not sure we want to expose Protocol in the API

spouliot · 2017-01-31T21:36:29Z

+		public void IndexerTest ()
+		{
+			using (var obj = new MDLObject ()) {
+				var key = new Protocol ("MDLComponent");


I'm not sure we want to expose Protocol like this in the API. At best it's not very type safe, I'd better use

var key = new Protocol (typeof (MDLComponent));

My first thought was to abuse [BindAs] but a conversion from Protocol to System.Type, with additional checks to ensure it's a protocol, can be costly if done repetitively...

Maybe we can just provide a [Wrapper] that calls a conversion method ? and also expose that as a new Protocol.ctor(System.Type) ?

@rolfbjarne @dalexsoto thoughts ?

I think just a [Wrapper] that calls a conversion method (or a new Protocol ctor) would be fine.

BTW your example is wrong 😄 It should be:

var key = new Protocol (typeof (IMDLComponent));

Yep, agree on just the Wrapper 👍

spouliot · 2017-01-31T21:38:36Z

+				return ObjectForKeyedSubscript (key);
+			}
+			set {
+				SetObject (value, key);


that's confusing because your new SetObject (even if internal) is not symmetric with the existing GetObject

spouliot · 2017-01-31T21:40:33Z

+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		[Export ("componentConformingToProtocol:")]
+		[return: NullAllowed]
+		IMDLComponent IsComponentConforming (Protocol protocol);


Is makes it sound boolean, but this is not what it returns. Seems to me that Get is the action (not question) being done.

Also it seems to be related with the previous method, so GetComponent would match.

@spouliot I totally agree with you but I wanted to keep it close to MDLObject's IsComponentConforming...

Most of the changes in this PR are about aligning MDLObject and MDLAsset.

Let's fix the bad one with an [Obsolete] and add a new, accurate, name :)

Maybe GetConformingComponent? That's a bit more descriptive and closer to what ObjC calls it (which makes finding it easier).

Ah, I see the SetComponent method now, GetComponent would be the opposite method (I assume at least, since Apple named them quite differently).

Yea so in the headers setComponent:forProtocol: and componentConformingToProtocol: have the exact same description:

Extensible component support that allows user of ModelIO to customize MDLAssets to fit their format and workflow.

So I'm going for GetComponent instead of IsComponentConforming.

spouliot · 2017-01-31T21:43:23Z

 		}
+
+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		public IMDLComponent this [Protocol key] {


there's already a this that returns something else... check FxDG but I don't think we should expose it this way. That can get confusing source wise when the argument type is not clear.

FxDG (Framework Design Guideline)?

Mmh here's the guideline: https://msdn.microsoft.com/en-us/library/ms229006(v=vs.110).aspx

AVOID indexers with parameter types other than System.Int32, System.Int64, System.String, System.Object, or an enum.

So Protocol is definitely not recommended.

DO NOT provide more than one family of overloaded indexers in one type.

I guess that validates your point @spouliot. As I understand it it's fine to "overload" an indexer (as in have an indexer that's using an other indexer in the same type) but it's wrong to create a 2nd and different family of indexers.

spouliot · 2017-01-31T21:43:55Z

+namespace XamCore.ModelIO {
+	public partial class MDLObject {
+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		public IMDLComponent this [Protocol key] {


should match how MDLAsset turns out...

spouliot · 2017-01-31T21:45:33Z

+
+		[Internal]
+		[Export ("setObject:forKeyedSubscript:")]
+		void SetObject ([NullAllowed] IMDLComponent obj, Protocol key);


are those just doing the same as the setComponent:forProtocol: and componentConformingToProtocol: above ? in that case maybe we should not expose them ?

Yes so objectForKeyedSubscript:'s description is:

Allows shorthand [key] syntax for componentConformingToProtocol:

@spouliot correct me if I'm wrong but this is basically the indexer I implemented initially (public IMDLComponent this [Protocol key] which was sentenced to die because there's already one).

spouliot · 2017-01-31T21:46:00Z

+
+		[Internal]
+		[Export ("setObject:forKeyedSubscript:")]
+		void SetObject ([NullAllowed] IMDLComponent obj, Protocol key);


same (not sure we need those, tests would confirm)

spouliot · 2017-01-31T21:46:24Z


 		[Export ("componentConformingToProtocol:")]
 		[return: NullAllowed]
 		IMDLComponent IsComponentConforming (Protocol protocol);


that's old, but badly named :(

Ah well you found the origin of my IsComponentConforming...

spouliot · 2017-01-31T21:47:31Z

 		void RemoveObject (MDLObject @object);

+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		[Abstract]


breaking change, you cannot add abstract methods as existing subclasses won't have them

Ah damn, good point I missed that.

spouliot · 2017-01-31T21:47:36Z

+		MDLObject GetObjectAtIndexedSubscript (nuint index);
+
+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		[Abstract]


monojenkins · 2017-01-31T22:02:52Z

Build failure

VincentDondain · 2017-01-31T22:21:35Z

Introspection-mac

[FAIL] Selector not found for ModelIO.MDLAsset : objectForKeyedSubscript:
[FAIL] Selector not found for ModelIO.MDLAsset : setObject:forKeyedSubscript:
[FAIL] Selector not found for ModelIO.MDLObject : objectForKeyedSubscript:
[FAIL] Selector not found for ModelIO.MDLObject : setObject:forKeyedSubscript:

VincentDondain · 2017-01-31T22:29:07Z

Unrelated tests failures (due to the move to Xcode8.3):
apitest - https://bugzilla.xamarin.com/show_bug.cgi?id=51799
monotouch-test - https://bugzilla.xamarin.com/show_bug.cgi?id=51801
mini watchOS - https://bugzilla.xamarin.com/show_bug.cgi?id=50419
don’t link watchOS - https://bugzilla.xamarin.com/show_bug.cgi?id=50419

rolfbjarne · 2017-02-01T07:47:42Z

+		[iOS (10,3), TV (10,2), Mac (10,12,4)]
+		[Abstract]
+		[Export ("objectAtIndexedSubscript:")]
+		MDLObject GetObjectAtIndexedSubscript (nuint index);


Just GetObject would be better.

rolfbjarne · 2017-02-01T07:49:53Z

+		[Test]
+		public void IndexerTest ()
+		{
+			using (var obj = new MDLAsset ()) {


I think you need a version check here, for Xcode 8.3.

rolfbjarne · 2017-02-01T07:53:56Z

+		public void IndexerTest ()
+		{
+			using (var obj = new MDLObject ()) {
+				var key = new Protocol ("MDLComponent");


I think just a [Wrapper] that calls a conversion method (or a new Protocol ctor) would be fine.

BTW your example is wrong 😄 It should be:

var key = new Protocol (typeof (IMDLComponent));

VincentDondain · 2017-02-02T03:00:18Z

Alright so:

I'm open to suggestions on how to assert that Type is a Protocol in ObjCRuntime/Protocol.cs (not sure at all I'm doing the right thing but it works :P)
I removed the Indexers I added for both MDLAsset and MDLObject because they did not follow the Framework Design Guidelines, therefore I removed MDLObject.cs that was new in this PR and the tests I created since they were related to the new manual code.

For MDLObject, based on the description, I'm really not sure that objectAtPath should become an indexer like MDLAsset's objectAtIndex.

The first one being:

Return the object at the specified path, or nil if none exists there

The second being:

Returns the top-level object at the specified index in the asset.
index: An index in the asset’s list of top-level objects; between zero and the value of the count property.

monojenkins · 2017-02-02T03:52:13Z

Build failure

rolfbjarne

Also please add tests for the new Protocol constructor.

rolfbjarne · 2017-02-02T07:01:44Z


+		public Protocol (Type type)
+		{
+			this.handle = Class.GetHandle (type);


This is not correct.

First check that the type is an interface, otherwise it can't be a protocol:

if (!type.IsInterface) throw new ArgumentException (string.Format ("'{0}' is not a protocol.", type.FullName));

Then get the Protocol attribute, and get the name of the protocol from there:

var protocols = type.GetCustomAttributes (typeof (ProtocolAttribute), false); if (protocols.Count == 0) throw ... var protocol = protocols [0]; var protocolName = protocol.Name; handle = objc_getProtocol (protocolName); if (handle == IntPtr.Zero) throw ...

Mmh didn't realized I could dive into the managed type like that! Much better thanks.

monojenkins · 2017-02-02T20:53:41Z

Build failure

spouliot · 2017-02-02T22:06:20Z


+		public Protocol (Type type)
+		{
+			var exception = new ArgumentException (string.Format ("'{0}' is an unknown protocol", type.FullName));


This creates an exception instance for every protocol created (thru this .ctor). This is not efficient, use something like:

if (type.IsInterface) { foreach (var pa in type.GetCustomAttributes<ProtocolAttribute> (false)) { handle = objc_getProtocol (pa.Name); if (handle != IntPtr.Zero) return; } } if (handle == IntPtr.Zero) new ArgumentException (string.Format ("'{0}' is an unknown protocol", type.FullName));

Also having different exception messages for each condition gives you valuable information (which check failed?).

spouliot · 2017-02-02T22:09:51Z

+		[Export ("components", ArgumentSemantic.Copy)]
+		IMDLComponent[] Components { get; }
+
+		[Internal]


maybe [Advanced] but not [Internal]. The main reason is that Type is easier (and type safe) to use, but it would be costly in a loop, e.g.

foreach (var component in list) x.SetComponent (type);

vs

var p = new Protocol (type); foreach (var component in list) x.SetComponent (p);

It also makes it possible to override the method in a subclass.

spouliot · 2017-02-02T22:09:59Z

+		[Wrap ("SetComponent (component, new Protocol (type))")]
+		void SetComponent (IMDLComponent component, Type type);
+
+		[Internal]


spouliot · 2017-02-02T22:10:45Z

+#if XAMCORE_4_0
+		[Internal]
+#endif
+		[Obsolete ("Use SetComponent (Type protocol)")]


remove both [Obsolete] and [Internal]

spouliot · 2017-02-02T22:11:53Z

+#if XAMCORE_4_0
+		[Internal]
+#endif
+		[Obsolete ("Use GetComponent (Type protocol)")]


Obsolete IsComponentConforming but expose GetComponent (Protocol protocol) for the same reasons as above

spouliot

👍

dalexsoto · 2017-02-03T20:39:22Z

 				throw new ArgumentException (String.Format ("'{0}' is an unknown protocol", name));
 		}

+		public Protocol (Type type)


monojenkins · 2017-02-03T21:33:34Z

Build failure

monojenkins · 2017-02-06T17:46:56Z

Build failure

VincentDondain · 2017-02-06T18:04:10Z

Unrelated test failures

https://bugzilla.xamarin.com/show_bug.cgi?id=52162 - monotouch-test - Unified 32bits
https://bugzilla.xamarin.com/show_bug.cgi?id=52290 - monotouch-test - Unified 64bits

@rolfbjarne add one simple test in MDLObjectTest covering the new Protocol constructor.

Also fixed the introspection-mac tests.

[modelio] Update for iOS 10.3 beta 1

8369702

dnfclas added the cla-already-signed label Jan 31, 2017

spouliot suggested changes Jan 31, 2017

View reviewed changes

rolfbjarne requested changes Feb 1, 2017

View reviewed changes

[modelio] Fixes based on comments

a5c2034

VincentDondain force-pushed the modelio-b1 branch from 847fe43 to a5c2034 Compare February 2, 2017 02:47

rolfbjarne requested changes Feb 2, 2017

View reviewed changes

[modelio] Fixes based on comments (2)

d7882fb

spouliot suggested changes Feb 2, 2017

View reviewed changes

[modelio] Fixes based on comments (3)

5a3e862

spouliot approved these changes Feb 3, 2017

View reviewed changes

dalexsoto approved these changes Feb 3, 2017

View reviewed changes

rolfbjarne approved these changes Feb 6, 2017

View reviewed changes

[modelio] Fixes (4)

640892f

VincentDondain force-pushed the modelio-b1 branch from 0238842 to 640892f Compare February 6, 2017 16:39

spouliot approved these changes Feb 6, 2017

View reviewed changes

VincentDondain merged commit 23ba6df into dotnet:xcode8.3 Feb 6, 2017

VincentDondain deleted the modelio-b1 branch February 6, 2017 20:20

VincentDondain mentioned this pull request Feb 10, 2017

Beta 1 Mac changes for ModelIO, SpriteKit, MediaPlayer #1681

Closed

spouliot pushed a commit to spouliot/xamarin-macios that referenced this pull request Mar 28, 2017

[modelio] Update for iOS 10.3 beta 1 (dotnet#1601)

b7dd01c

Uh oh!

Conversation

VincentDondain commented Jan 31, 2017

Uh oh!

spouliot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Jan 31, 2017

Uh oh!

VincentDondain commented Jan 31, 2017

Uh oh!

VincentDondain commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentDondain commented Feb 2, 2017

Uh oh!

monojenkins commented Feb 2, 2017

Uh oh!

rolfbjarne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

VincentDondain commented Jan 31, 2017 •

edited

Loading