WebAudio / web-midi-api

The Web MIDI API, developed by the W3C Audio WG
http://webaudio.github.io/web-midi-api/
Other
323 stars 49 forks source link

Refactor getInput, getOutput -> getPort(), inputs[], outputs[] ? #2

Closed jussi-kalliokoski closed 10 years ago

jussi-kalliokoski commented 11 years ago

Originally reported on W3C Bugzilla ISSUE-20505 Mon, 24 Dec 2012 08:15:56 GMT Reported by Marcos Caceres Assigned to This bug has no owner yet - up for the taking

The methods getInput() and getOutput() effectively do the same thing (evident also by the amount of duplicate text in the spec); as such, they should be merged. Furthermore, it's not clear (in the spec) as why there are 3 different ways to get a MIDIPort. I would like to suggest that these two methods be merged into a single method and the way to be one way to get a MIDIPort: by its ID… hence getPort(id) or getPortById(id);

Consider:

  1. getInput/Output(MIDIPort)… doesn't make much sense, as why is a developer requesting a MIDIPort with a MIDIPort they already have? I'm sure there is something I'm missing here.
  2. getInput/Output(short): first, short here doesn't make much sense, given that a WebIDL short is defined as: "The short type is a signed integer type that has values in the range [−32768, 32767]". So requesting negative indexes don't make any sense here. In addition, this is basically the same as doing:

var port = midiAccess.getInputs()[x];

Additionally, the ordering of the ports may or may not be consistent after each session, so saying getInput(number) is inherently unreliable. Also, it's kinda unhelpful is you want to, for instance, get the last available port. This is because midiAccess has no length property. The only way to get the length is to first call getInputs(), and get the length from there… but then you already have the list of inputs, in so you don't then need to call getInput( number ), because it's already the same as inputs[number].

  1. getInput/Output( id ): this is probably the only one that makes sense. It's a cheap way of checking if a previously used exists:

var favInstrument = midiAccess.getInput(localStorage.fav) if(favInstrument){ … } Also, if target is is not found, just return null. Please don't throw an exception. Throwing exceptions should be done in "exceptional" circumstances, not on simple lookups.

marcoscaceres commented 11 years ago

it feels to me that to then cover the use case all we really need is:

 sequence<MIDIPort>  getInputs();
 sequence<MIDIPort>  getOutputs();
 //assuming no input will have the same id as an output 
 MIDIPort getPort(DOMString id); 
marcoscaceres commented 11 years ago

We've been going around in circles for a few months on this, and I think we would probably benefit from some fresh eyes: I really think we should ask public-script-coord@w3.org for guidance here (they promised to help W3C groups with API design).

@cwilso or @jussi-kalliokoski, could you please send and email to them asking for a general review and about this in particular? I've been flooding them with API review questions lately, so I don't feel comfortable sending them another.

jussi-kalliokoski commented 11 years ago

If we still have getInput(), then I would want an index to work there.

I've still yet to see a user-friendly use case for this. :)

@marcoscaceres Generally I don't like functions that can return more than one different type, even less that IDs for two different lists of items have to avoid taking an ID that's in use in another list. Separation of concerns and all that. But I guess I could live with that.

Of course, we have more options than what's been proposed so far:

partial interface MIDIAccess {
  MIDIInput getInputById(DOMString id);
  MIDIOutput getOutputById(DOMString id);
}
MIDI{Input,Output} open();

Where you could e.g. get access to the first input like this:

var input = midiAccess.getInputs()[0].open();

I've just sent an email to public-script-coord asking for feedback on this.

tabatkins commented 11 years ago

It sounds like, from the replies in this thread, that the port index isn't stable across devices (the [0] port could be an audio device on one machine, and a lighting device on another). It also sounds like this may not even be stable across a single page's lifetime, if devices are disconnected then reconnected. Is this correct?

What's the use-case for enumerating ports? It sounds like there is one, or else you wouldn't worry about exposing sequences in the first place, but I'm not sure. If there's a use-case for enumerating, but indexes are always (potentially) unstable, it sounds like what you really want is a Map type, not a sequence. You can enumerate a Map by specifying whether you want an array of its keys (in this case, id strings), its values (in this case, ports), or tuples of both. This gives you enumeration, without accidentally smuggling in the ability to use unstable indexes.

I have a thread in public-script-coord about adding Map-likes to WebIDL, which would make this very easy to specify. It would look something like:

partial interface MIDIAccess {
  readonly attribute MapLike<DOMString, MIDIPort> inputs;
  readonly attribute MapLike<DOMString, MIDIPort> outputs;
}

@cwilso and @jussi-kalliokoski bring up another important issue, though. It appears that some OSes might only be able to grant exclusive access, such that enumerating all the devices directly would give the page exclusive ownership of all of them, unless you do funky tricks in the back-end. If this is a concern, I strongly recommend making the decision to "use" a port explicit, via something like what @jussi-kalliokoski suggests (adding a .open() method to the port). If it can be synchronous, great; if not, just return a Promise for the opened port.

cwilso commented 11 years ago

It needs to be trivial to iterate through the attached MIDI devices. That "list" may change over time quite dynamically, yes (devices are frequently plugged in and unplugged) - although the same set of connected devices tends to always show up in the same order in a given OS.

You need to be able to enumerate ports, because you need to offer the user the ability to select which MIDI device they want to communicate with. You need to fill a drop-down box, or whatever. It would suck, in my opinion, to use ID strings as the selector, in particular as there's another issue filed that those IDs should be reset whenever cookies are cleared - although if MapLike would let me do a for...in and get back MIDIInputs/MIDIOutputs, great. As a developer, I don't really want an array of its keys, I want an array of its actual objects. "MIDIPort" is really not a great concept in WebMIDI, either - I'm hoping to remove it, except as an abstract class - because you can't do anything useful directly with it.

I'm still not certain that we have an actual issue with only being able to obtain exclusive access - I need to look into it, but I don't have the bandwidth right now to dig up a Windows machine and do some development to check. If anyone else can get to that before I can, I'd appreciate it. If it is a problem, we'll need an open/close method pair, I expect.

On Mon, Jun 10, 2013 at 8:38 AM, tabatkins notifications@github.com wrote:

It sounds like, from the replies in this thread, that the port index isn't stable across devices (the [0] port could be an audio device on one machine, and a lighting device on another). It also sounds like this may not even be stable across a single page's lifetime, if devices are disconnected then reconnected. Is this correct?

What's the use-case for enumerating ports? It sounds like there is one, or else you wouldn't worry about exposing sequences in the first place, but I'm not sure. If there's a use-case for enumerating, but indexes are always (potentially) unstable, it sounds like what you really want is a Map type, not a sequence. You can enumerate a Map by specifying whether you want an array of its keys (in this case, id strings), its values (in this case, ports), or tuples of both. This gives you enumeration, without accidentally smuggling in the ability to use unstable indexes.

I have a thread in public-script-coord about adding Map-likes to WebIDL, which would make this very easy to specify. It would look something like:

partial interface MIDIAccess { readonly attribute MapLike<DOMString, MIDIPort> inputs; readonly attribute MapLike<DOMString, MIDIPort> outputs;}


@cwilso https://github.com/cwilso and @jussi-kalliokoskihttps://github.com/jussi-kalliokoskibring up another important issue, though. It appears that some OSes might only be able to grant exclusive access, such that enumerating all the devices directly would give the page exclusive ownership of all of them, unless you do funky tricks in the back-end. If this is a concern, I strongly recommend making the decision to "use" a port explicit, via something like what @jussi-kalliokoskihttps://github.com/jussi-kalliokoskisuggests (adding a .open() method to the port). If it can be synchronous, great; if not, just return a Promise for the opened port.

— Reply to this email directly or view it on GitHubhttps://github.com/WebAudio/web-midi-api/issues/2#issuecomment-19206839 .

tabatkins commented 11 years ago

Yeah, MapLike just means "has all the same methods as Map, but might do magic things underneath".

To easily enumerate, you can do inputs.keys() to get an iterator of ids, inputs.values() to get an iterator of the MIDIPorts, or inputs.items() to get an iterator of [id, MIDIPort] arrays.

Basically, using a MapLike doesn't add or subtract from your abilities; it just gives you a correct API shape. You get enumeration and id-addressing, without accidentally exposing a meaningless index.

jussi-kalliokoski commented 11 years ago

Is this correct?

Yes.

it sounds like what you really want is a Map type

This is true, actually, a map would be a better fit for the use case.

If anyone else can get to that before I can, I'd appreciate it.

At least based on my tests a few months ago with winmm on Win7 this was true. I also haven't been able to get two DAWs to get input from one device at the same time, so I have a strong feeling this is not just because I'm not used to Windows' MIDI APIs.

we'll need an open/close method pair

I think that a close method shouldn't be necessary. When the open pages on the UA no longer have references to a given MIDI{Input,Output}, the port should be automatically closed. It would be annoying if a part of a program received a MIDI{Input,Output} and then some other part of the program would suddenly close the port while the other is using it.

jussi-kalliokoski commented 11 years ago

I'm not sure how that change to the spec represents the consensus on this issue at all.

jussi-kalliokoski commented 11 years ago

That is, I didn't think we had reached consensus yet, and most certainly not one that those changes represent.

cwilso commented 11 years ago

I did not think we had a firm consensus yet, no, or I would have been suggesting resolution of the bug. I don't consider this bug closed.

It appears that Maplike is closest to what we want; however, it also appears that Maplike isn't quite ready yet. getInput/getInputs is clearly a poor pattern. I would have liked to have inputs[]/outputs[] as an attribute, but I'm not positive how that is workable in IDL and implementation; at any rate, we wanted to get out an experimental implementation, and wanted to have it reflect something closer to what we thought we'd eventually have. An implementation that used getInput/getInputs seemed like it would do more damage than good.

I'm happy to hear suggestions on how to move this issue forward; I do think this is closer to the pattern we want, but if you think it's a backward change, then please elaborate.

On Sat, Jul 6, 2013 at 1:51 AM, Jussi Kalliokoski notifications@github.comwrote:

That is, I didn't think we had reached consensus yet, and most certainly not one that those changes represent.

— Reply to this email directly or view it on GitHubhttps://github.com/WebAudio/web-midi-api/issues/2#issuecomment-20551366 .

jussi-kalliokoski commented 11 years ago

but if you think it's a backward change, then please elaborate.

I most certainly think that it's a backward change to name a method as if it were an attribute. inputs/outputs make sense if they are attribute names but as method names why would they not be getInputs()/getOutputs()?

however, it also appears that Maplike isn't quite ready yet

Neither is our API.

we wanted to get out an experimental implementation, and wanted to have it reflect something closer to what we thought we'd eventually have

Sure, that's all good.

but I'm not positive how that is workable in IDL and implementation

Yeah, IDL I'm not sure, but what implementations can do is that whenever the maplike is accessed, check whether the list of devices has been refreshed on the current stable snapshot and refresh it if not (obviously we should leave it an implementation detail how often the list is actually refreshed, as long as it stable within a stable snapshot). It induces a performance cost, obviously, but I think under all circumstances this code path will be very cold, and this way we can keep the symmetry of midiAccess.inputs === midiAccess.inputs, but keep the list fresh and stable at all times.

cwilso commented 11 years ago

On Sun, Jul 7, 2013 at 5:14 AM, Jussi Kalliokoski notifications@github.comwrote:

but if you think it's a backward change, then please elaborate.

I most certainly think that it's a backward change to name a method as if it were an attribute. inputs/outputs make sense if they are attribute names but as method names why would they not be getInputs()/getOutputs.

As above: was intending to make them attributes, but couldn't quite figure out a good way to name them that way yet.

however, it also appears that Maplike isn't quite ready yet

Neither is our API.

I did not mean this to be "so we can't use Maplike in Web MIDI" - my point was that it wasn't clear how to redraft it TODAY to make it look like what it would look like with Maplike.

Actually, on closer reflection, I think I CAN define this:

[MapClass(DOMString, MIDIInput)] interface MIDIInputMap { // has .entries, .keys, .values, .size and .forEach };

interface MIDIAccess : EventTarget { attribute MIDIInputMap inputs; ... };

But there are some issues with this: mostly, it pretty much requires reliance on the unique IDs that we will have to cook up for each port (and, according to issue #, will need to get reset/rebuilt when cache is cleared). I'm still unsure that this is better (and it's certainly more complex) than effectively an Array.

Note that I did, in fact, leave this change on the cwilso-edit branch.

jussi-kalliokoski commented 11 years ago

Note that I did, in fact, leave this change on the cwilso-edit branch.

Whoops, my bad, sorry.

Actually, on closer reflection, I think I CAN define this

I think that is indeed something closer to what we want.

will need to get reset/rebuilt when cache is cleared

To reiterate, that is an edge-case and people shouldn't be surprised that when they clear their data, their data is cleared so I don't think it's a very healthy argument against the IDs.

it pretty much requires reliance on the unique IDs that we will have to cook up for each port

Correction: Best guess at unique IDs. An algorithm for associating an almost unique ID to a device will probably be the easiest part of implementing this API and it makes sure that the easiest way for developers to do things is the one that results in the best user experience. To me the decision to have that seems like a no-brainer.

cwilso commented 11 years ago

Actually, on closer reflection, I think I CAN define this

I think that is indeed something closer to what we want.

The trouble I'm having is that I'm feeling like I'm seriously making this up as I go along, as half of the Map/MapClass bits (http://dev.w3.org/2006/webapi/WebIDL/#MapClass) in the WebIDL spec are lacking detail if not examples.

I cooked up how I think this will look with Maplike class vs. Array:

Maplike: // to tell how many entries there are: var numberOfMIDIInputts = inputs.size; // to populate a selection box or whatever inputs.values( function( port ) { // add "port" to box here });

vs (Array style)

// to tell how many entries there are: var numberOfMIDIInputts = inputs.length // to populate a selection box or whatever for (var i=0; i<inputs.length; i++) { // add inputs[i] to box here }

In the Maplike case, MIDIAccess.inputs would be an object that looked like this (pardon the inevitable errors, and this presumes we use a DOMString for a key, which may not be appropriate):

inputs = {
  readonly int size;

  // iterators
  function keys( void function ( DOMString ) );
  function entries( void function ( Array ) );  // Array would contain [ MIDIKeyType, MIDIInput ]
  function values( void function ( MIDIInput ) );

  // getter/tester
  MIDIKeyType get( DOMString key );
  boolean has( DOMString key );
}

The MIDIAccess would look like:

interface MIDIAccess : EventTarget {
  readonly attribute MapLike<DOMString, MIDIPort> inputs;
  readonly attribute MapLike<DOMString, MIDIPort> outputs;
  attribute EventHandler onconnect;
  attribute EventHandler ondisconnect;
}

I still think there are some oddities that will take a bit of getting used to with Maplike (like "size" instead of "length", and lack of indexed access), but I expect it will be okay.

To reiterate, that is an edge-case and people shouldn't be surprised that when they clear their data, their data is cleared so I don't think it's a very healthy argument against the IDs.

Hmm. I remain unconvinced, because I think "the setup of my MIDI ports" is going to seem like it should be immutable. But I get the privacy/tracking concern, and I'll go along with this for now, at least. :)

it pretty much requires reliance on the unique IDs that we will have to cook up for each port Correction: Best guess at unique IDs. An algorithm for associating an almost unique ID to a device will probably be the easiest part of implementing this API and it makes sure that the easiest way for developers to do things is the one that results in the best user experience. To me the decision to have that seems like a no-brainer.

I'm less concerned by how that cooking up is done than by what guidance we give to developers on how they should code for the most important scenarios: -access all ports for selection (e.g. presenting a dropdown box) -finding a "preferred" port (e.g. I look for a DJ device) -remembering and re-accessing a stored port on that machine

From the shape I've listed above, if this is how Maplike should look, I think this will work okay. Thoughts?

jussi-kalliokoski commented 11 years ago

what guidance we give to developers on how they should code for the most important scenarios

I made a small gist to illustrate how one might approach the scenarios you listed. The gist application shows the user a dropdown list to pick the port and assigns an event listener to the port the user selects (and removes it from the previously selected one), remembering the user's preference by storing the port id in local storage and the next time the application is opened the port is automatically selected.

The gist is not a real world application or anything but I think it shows well how with IDs the only inconvenience of remembering user preferred devices in a most-of-the-time reliable manner is storing the preference, which is unavoidable. Having it this simple (45 LOC being simple is debatable of course) to ask the user which port to use and remembering the user preference should also help discourage authors from just picking the first port in the array, although it's of course still possible to do so.

I'm feeling like I'm seriously making this up as I go along

That's probably OK, I think at this stage there won't be much damage done if we have to make small adjustments when the WebIDL spec becomes clearer about this. FWIW I think the IDL you presented looks solid, aside from the separate concern of getting rid of MIDIPort. ;)

From the shape I've listed above, if this is how Maplike should look, I think this will work okay.

I think so too.

cwilso commented 10 years ago

Fixed by https://github.com/WebAudio/web-midi-api/commit/997c2adfdda083b59339e66a43babaa09f53fb93.