Add together API support

[samreid]

Concentration has been selected as a simulation to use for API/arch/metacog/together integration. This issue will track commits done in the concentration repo and in the beers-law-lab repo, so they can be reviewed when complete. Work will be done in a branch api until it can be reviewed by the primary sim developer, @pixelzoom.

[samreid]

@pixelzoom please let me know when we can review the changes above together. I'd like to move to master sooner rather than later, but only after we have had a chance to discuss and evaluate the changes.

[pixelzoom]

I'll have some time on Monday 3/9.

[samreid]

I added a way to test save/load dynamically at http://localhost:8080/together/examples/mirror.html (needs to have URLs updated to match your configuration). This loads from one iframe and plays back immediately to another iframe so we can easily visualize what is getting saved/restored from the state.

[samreid]

(2) Move together type descriptions (currently in joist.TogetherTypes) closer to the actual JavaScript types that they are describing. The current (centralized) approach creates circular dependencies on joist in common code repos, and makes it likely that we'll forget to change something. For example, if I'm working on scenery-phet.FaucetNode, I'd like to make changes there, rather than remember to look for type 'Faucet' in joist.TogetherTypes. This would also facilitate keeping enumeration of legal values (e.g., values: [ 'liquid', 'solid' ]) in one place.

Everyone please be aware this means Scenery, Dot, Axon, etc will be getting these type definitions.

[samreid]

(2) Move together type descriptions (currently in joist.TogetherTypes) closer to the actual JavaScript types that they are describing.

I have defined the following type in TogetherTypes.js

  var Button = {
    name: 'Button',
    superType: Node,
    events: {
      fired: 'fired'
    }
  };

Note that this does not apply to a single Button definition in the code (in Sun, for instance) but pertains to several different actual implementations. In my opinion, it is essential for us to simplify our complex type hierarchy before presenting to 3rd parties/researchers--but where should the declaration of Button live? And how will we handle similar cases where the implementation Type doesn't match what we want to expose as the public API type?

[samreid]

Also, the way the require statements are looking now is like this:

  var Object = require( 'PHET_CORE/together/Object' ).TogetherType;
  var Node = require( 'SCENERY/nodes/Node' ).TogetherType;

However, this is unconventional style of imports but works well to avoid name collisions between Node (the scenery type) and Node (the together type). The API declaration is supposed to be human readable, so its important that the types used in the file are the names we want and not some awkward renaming to avoid collisions.

[samreid]

It would be best to discuss the above issues before I put a lot of work into this. Feel free to respond asynchronously (if the above comments are sufficiently clear) or let me know when you are available for discussion @pixelzoom.

[samreid]

Reassigning to @pixelzoom since I am ready for discussion.

[pixelzoom]

After this morning's discussion with @samreid...

For type definitions, I'm leaning towards either: (1) one TogetherTypes file per repo, or (2) everything in a single together.TogetherTypes file. For (2), my main concern would be how large that file might get.

We also discussed the possibility of having all of the *-together-api.js files live in the together repo, see https://github.com/phetsims/together/issues/11#issuecomment-85088984.

[samreid]

@pixelzoom what do you think about starting with (2) and if/when the file gets too large then going to (1) one file per repo, but keeping all of those files in the together repo. For instance: together/js/api/Joist.js etc.

[pixelzoom]

@samreid That sounds fine.

[pixelzoom]

@samreid asked via Skype:

what else left to do before moving beers-law-lab together branch to master?

Push stuff in BLLDropperNode into scenery-phet.EyeDropperNode, as described in https://github.com/phetsims/arch/issues/14#issuecomment-85135586. Or are you avoiding that because calling together.addComponent before the BLLDropperNode is fully constructed will cause problems? If so, then that would be a huge general problem.

[samreid]

The current customization scheme doesn't handle subclassing well, but that shouldn't be solved at the moment. I moved componentID and together.addComponent to scenery-phet in the above two commits. Next I'll merge beers-law-lab together to master. Note that no merge is necessary for concentration because it no longer has any code (aside from added entries to the html file).

[samreid]

I merged together to master and deleted together and arch branches. @pixelzoom I'm ready to close this issue if you are satisfied.

[pixelzoom]

I reviewed the merge. A couple of questions...

(1) beers-law-lab.Dropper contains this dead code, can it be deleted?

55 //together && together.addComponent( 'concentrationScreen.dropper.location', thisDropper.locationProperty );

(2) beers-law-lab.SoluteComboBox contains this line:

49 listNodeVisiblePropertyID: 'concentrationScreen.soluteComboBox.listVisible'

ComboBox has no such option, and 'PropertyID' doesn't match the 'ComponentID' naming convention. I found it by accident because buttonNodeComponentID is on the line above it. Is this vestigial?

[pixelzoom]

I like how the together-specific API and html files live in the together repo.

[pixelzoom]

@samreid concentration still has a 'together' branch.

[samreid]

I addressed the dead and unused code issues raised in #34 (comment)

The together branch of concentration just contains the additional scripts in the _en.html file that are required to launch. I think we shall keep that branch for all sims that can are instrumented for together.

@pixelzoom I'm ready to close this issue if you are satisfied.

[pixelzoom]

Reopening, I discovered some things that I didn't find when searching for 'together' and 'componentID'. In ConcentrationModel:

    // The Public API for this simulation provides a simplified name for the selected solute
    // Here, we perform 2-way binding between the solute name and the solute instance
    // This means we can get the value of the solute by name, and set it back by name,
    // which enables save/load/record/playback/configuration through query parameters/etc.
    var soluteAPINameProperty = new Property( thisModel.solute.value.apiName, { componentID: 'concentrationScreen.solute' } );
    soluteAPINameProperty.link( function( soluteAPIName ) {
      for ( var i = 0; i < thisModel.solutes.length; i++ ) {
        var solute = thisModel.solutes[ i ];
        if ( solute.apiName === soluteAPIName ) {
          thisModel.solute.value = solute;
        }
      }
    } );
    thisModel.solute.link( function( solute ) {
      soluteAPINameProperty.value = solute.apiName;
    } );

(1) Describing this as "The Public API" tells the reader nothing. The public API for a JavaScript type is all of the properties that are publicly accessible. Describe this a "the together API" or something specific.

(2) I'm a little concerned about having to set up this 2-way binding. This was not in the "4 steps" that you've enumerated many times. And it's something that will need to be done whenever there is selection. Let's discuss. Or at least make other developers aware of this additional instrumentation requirement.

[pixelzoom]

The above observer that handles the 2-way binding is also not robust. If solute.apiName !== soluteAPIName for all solutes, that is a programming error and it's silently ignore.

[pixelzoom]

I have the same criticisms about this in Solute's constructor:

@param {string} apiName - A non-internationalized unique identifier that can be used to access the solute

Together is not the only API, this needs to say something about together specifically.

[pixelzoom]

I also do not like the listItemComponentID (which is missing from jsdoc, btw) in Solute constructor. Example:

Solute.DRINK_MIX = new Solute(...,
  'drinkMix',
 'concentrationScreen.soluteComboBox.drinkMixButton',
...
);

The responsibility for naming combo box list items should live with ComboBox.  Also consider having ComboBox set up the 2-way binding between items and their componentIDs.

[pixelzoom]

Trying to write JSdoc for Solute's @param listItemComponentID shows how out-of-place it is. Eg:

@param {string} listItemComponentID - componentID for combo box list items

Why would anything about combo box be in the model? What if some other UI component were used to select the solute? What if there was >1 way to select the solute? This does not belong here.

[samreid]

This was not in the "4 steps" that you've enumerated many times.

The 4-step plan assumes the simulation is already implemented using idiomatic axon & PhET patterns. This will not be the case in many places and hence will require a significant amount of labor.

[pixelzoom]

Discussed alternatives to 2-way coupling and Solute.apiName/listItemComponentID with @samreid and @jbphet. Created "together-values" branch of beers-law-lab to investigate further, see phetsims/beers-law-lab#92. Notes about how to address above concerns appear in #38.

Closing.