Thanks Miguel,<br><br>Your suggestions make sense to me, except one. The MIDIObjectRef values returned by the various functions in the CoreMIDI API are not pointers but rather unsigned 32-bit integers used as unique identifiers for the various &quot;midi objects&quot;. Given that fact, I don&#39;t think it makes sense to use INativeObject, unless I am confused about something.<br>
<br>-- Pete<br><br><div class="gmail_quote">On Mon, Jan 10, 2011 at 4:59 PM, Miguel de Icaza <span dir="ltr">&lt;<a href="mailto:miguel@novell.com">miguel@novell.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hello Peter,<br>
    I have started to review the code in CoreMIDI, and here are some<br>
of my suggestions:<br>
<br>
Defs.cs:<br>
======<br>
* Make sure that all public field values start with an uppercase letter,<br>
  to match the Framework Design Guidelines (example: MIDIPacket<br>
  field names)<br>
<br>
* The word &quot;MIDI&quot; in the class names needs to be renamed Midi,<br>
  per the Framework Design Guidelines.<br>
<br>
* Drop the kMIDIXXX prefix from enumerations, these are necessary<br>
  in C, because they act as a mechanism to avoid name clashes, but<br>
  in C# we are using namespaces, and the enums themselves, so<br>
  what today passes as MIDIObjectType.kMIDIObjectType_Other should<br>
  be renamed to just Other, so that you reference it as<br>
  MIDIObjectType.Other<br>
<br>
* The above applies to all of the enumerations<br>
<br>
* The same applies to the public static IntPtr fields that are used as<br>
   keys<br>
<br>
MIDIObject.cs<br>
==========<br>
<br>
Although this is an abstract class, it would be best if it would follow<br>
the same patterns that are used in CoreGraphics for object management<br>
and if it implemented the INativeObject interface as this will allow<br>
passing Midi objects to Objective-C functions if they ever exist (the<br>
marshaller and runtime depend on this).<br>
<br>
This means that the ObjectRef property should be called handle, and<br>
it should expose a public property Handle.<br>
<br>
MIDIObject Derived classes<br>
====================<br>
<br>
Since they will be implementing the INativeObject interface, they should<br>
also follow the convention for the marshaller to have two constructors<br>
the Foo (IntPtr handle) and Foo (IntPtr handle, bool owns) to do the<br>
memory management.   And add the proper codepath to take the<br>
reference when we do not own the object.<br>
<br>
The Dispose method needs to follow the same pattern as the objects<br>
in CoreGraphics, as it is important to expose a virtual method that<br>
subclasses can override.   Disposing of the object also needs to be<br>
guarded.<br>
<br>
In general this is a good resource on how to implement the IDisposable<br>
interface:<br>
<br>
<a href="http://msdn.microsoft.com/en-us/library/ms244737%28v=vs.80%29.aspx" target="_blank">http://msdn.microsoft.com/en-us/library/ms244737(v=vs.80).aspx</a><br>
<br>
Since MIDIEndpoint could return 2 separate instances for the same<br>
handle (due to the GetSourceByIndex), it should really implement<br>
the Equals, operator ==, operator != (and by extension GetHashCode)<br>
methods.<br>
<br>
As a matter of consistency, GetSourceByIndex should be renamed<br>
FromSourceIndex, and the same pattern applies to the destination one.<br>
<br>
MIDIClient<br>
=======<br>
<br>
You can use the CFString in the constructor with the using clause,<br>
to dispose it as soon as you are done with it.<br>
<br>
When throwing exception from a constructor, we typically also add<br>
a throw-less variant as a static method, that returns null on error,<br>
so a FromName () would do in this case.<br>
<br>
Since you create MidiInputPorts here and again you could end up<br>
with two objects created for the same underlying handle, this<br>
requires the operator ==, operator != and Equals to be implemented<br>
(and again, by extension, GetHashCode)<br>
<font color="#888888"><br>
Miguel<br>
</font></blockquote></div><br>