phetsims / concentration

"Concentration" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/concentration
GNU General Public License v3.0
1 stars 7 forks source link

string-to-value coercion for non-builtin types #38

Closed pixelzoom closed 9 years ago

pixelzoom commented 9 years ago

Notes from discussion with @samreid and @jbphet about alternatives to the "2-way binding" that appears in ConcentrationModel (see soluteAPINameProperty) and Solute.

For together instrumentation, a couple of string parameters were added to Solute:

@param {string} apiName - A non-internationalized unique identifier that can be used to access the solute
* @param {string} listItemComponentID - componentID for combo box list items

which results in this kind of instantiation:

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

These new strings don't fit with the componentID pattern used elsewhere, and (in the case of listItemComponentID) are specific to a UI component (combo box). Replace with the options.componentID pattern used elsewhere. Eg:

function Solute( ..., options ) {
  options = _.extend( {
    componentID: null
  }, options );
  this.componentID = options.componentID;
  ...
  if ( options.componentID ) {
    together && together.addComponent( this );
  }
}

Solute.DRINK_MIX = new Solute( ..., { 
  componentID: 'concentrationScreen.solute.drinkMix' 
} );

Then we want query parameter to look like this:

?concentrationScreen.solute=concentrationScreen.solute.drinkMix

So we need to coerce string 'concentrationScreen.solute.drinkMix' to a Solute.

In concentration.api, we'll need to add a Solute type, entries for the Solute instances, and change the type of 'concentrationScreen.solute':

  var Solute = {

    name: 'Solute',
    superType: Object,

    toTogetherString: function( solute ) {
      return solute.componentID;
    },

    fromTogetherString: function( str ) {
      return together.getComponent( str );
    }
  };

    'concentrationScreen.solute': { type: property( Solute ) },

    'concentrationScreen.solute.drinkMix': { type: Solute },
    'concentrationScreen.solute.cobaltIINitrate': { type: Solute },
    'concentrationScreen.solute.cobaltChloride': { type: Solute },
    //etc for other Solute instance

Perhaps the toTogetherString and fromTogetherString implementations shown above for Solute should be the defaults?

We'll need something similar for other non-builtin types, e.g. Vector2, so we can do:

?concentrationScreen.concentrationMeter.probeLocation=200,300

Non-working hacks were done in branch 'together-values', see https://github.com/phetsims/beers-law-lab/issues/92.

samreid commented 9 years ago

This pattern has been implemented and seems like it is working well. Great suggestion, @pixelzoom. We still need to implement more fundamental type coercions, but the pattern seems good.

samreid commented 9 years ago

I'm planning to add other type coercions as I need/test them, so no other work planned on this at the moment. @pixelzoom would you like to review/discuss?

pixelzoom commented 9 years ago

Looking good, I like this much better.

In concentration-api.js, I see:

  var Solute = {
    ...
    formatForArch: function( solute ) {
      return solute.componentID;
    },
   ...
  };

https://github.com/phetsims/together/issues/30 indicates that "arch" is going away, e.g. arch.start/end will be changed to together.startEvent/endEven. Should formatForArch be formatForTogether, or something else?

samreid commented 9 years ago

perhaps it will become formatForEventStream

samreid commented 9 years ago

Renamed above, anything else to do here?

pixelzoom commented 9 years ago

Looks good, closing.