Similar to #18, we should move TandemSimpleDragHandler code to SimpleDragHandler and delete TandemSimpleDragHandler.

/**
 * Tandem is a general instance registry that can be used to track creation/disposal of instances in PhET Simulations.
 * It is used for phetio.js instrumentation for PhET-iO support.
 *
 * @author Sam Reid (PhET Interactive Simulations)
 */

This is inaccurate. There is no registry implemented here. This is just a thin wrapper that forwards registration requests to "instance listeners". The registry appears to live in pheti-io.phetio.js.

From review of IOObject in #36 (comment):

• IOObject and ObjectIO in the same file, potentially confusing.
  A Type always refers to its IO Type (formerly known as TType). For instance Vector2 refers to Vector2IO. IOObject has the IO prefix because it is specific to PhET-iO. Not sure how to clarify this.

Yes, I understand the semantics, and I understand the roles of IOObject and ObjectIO. I'm just predicting that they will be inadvertently confused because of the similarity of their names.

Tandem should be using (instead of duplicating) joist.packageJSON. If a circular dependency is preventing this, then packageJSON.js should be moved from joist to phet-core.

The constructor in TandemRectangle.js calls out to Rectangle like this,

 function TandemRectangle( x, y, width, height, options ) {
    Tandem.validateOptions( options ); // The tandem is required when brand==='phet-io'
    Rectangle.call( this, x, y, width, height, options );
//...
}

which is the signature for the Dot version. The Scenery constructor is this:

  function Rectangle( x, y, width, height, cornerXRadius, cornerYRadius, options ) 

I'll fix it.

Today @jbphet and I added an assert to Tandem.createTandem that checks that the tandem exists.

However, this is failing a bunch of PhET-branded sims:

assert && assert( typeof id === 'string' && id.length > 0, 'id must be defined' );

Tandem.js line 47:

    // @private - options (even subtype options) must be stored on the instance so they can be passed through the super
    // type inheritance chain. Note: Make sure that added options here are also added to options for inheritance and/or
    // for composition (createTandem) as they make sense.
    this.options = _.extend( {

this.options is @private and unused outside of the constructor. So there's no reason to make it an instance field.

Now that we are passing types as Property options, the PropertySet constructor is getting a bit more complex:

    // Options 1
    PropertySet.call( this, {
      voltage: voltage,
      polarity: CLBConstants.POLARITY.POSITIVE
    }, {
      tandemSet: tandem ? {
        voltage: tandem.createTandem( 'voltageProperty' ),
        polarity: tandem.createTandem( 'polarityProperty' )
      } : {},
      typeSet: {
        voltage: TNumber && TNumber( 'volts' ),
        polarity: TString
      }
    } );

@andrewadare and I discussed restructuring this like so:

    // Options 2
    TandemPropertySet.call( this, {
      voltage: {
        value: voltage,
        tandem: tandem && tandem.createTandem( 'voltageProperty' ),
        type: TNumber && TNumber( 'volts' )
      },
      polarity: {
        value: CLBConstants.POLARITY.POSITIVE,
        tandem: tandem && tandem.createTandem( 'polarityProperty' ),
        type: TString
      }
    } );

The main benefit is that it clusters related metadata together.

Implementing this in tandem is one way to accomplish creation of the new API without breaking backward compatibility.

This breaks with the pattern of other Tandem* classes keeping the same API, but still helps us restructure the data in a better way.

See #8. If we are going to centralize tandem subclasses in tandem, then TandemDragHandler should be there too.

See phetsims/axon#148

This will help prevent accidental overrides, like we did for prefixDispose instance methods.

Currently, if you do

tandem.createPoolElementTandem('electron');
//then
tandem.createPoolElementTandem('neutron');

you will receive:
electron_0
neutron_1

It should be neutron_0, so we need to change this implementation. The use case will be something like:

var electronPoolTandem = tandem.createPoolTandem('electron');
var electron0Tandem = electronPoolTandem.createPoolElementTandem();

PoolTandem would be a subclass that adds the index.

From phetsims/sun#192

createSupertypeTandem would use the same id but provide no-op for addInstance().

@pixelzoom suggested instead of putting if (brand==='phet-io') in several methods, we may want to make a Tandem interface with a no-op implementation.

This may be more important now because we are considering making tandems non-null in common code.

I find myself often needing to cover my bases with the following commands:

options.tandem.isLegalAndUsable() && options.tandem.addInstance( this, TTooMuchFunTypingBoilderPlate ).

Is there a way around this? Marking for phet-io meeting.

On Thursday we discussed centralizing the logic for add/removeInstance, perhaps like it is done for scenery Node subclasses. Here are my raw notes:

pass in the phetioType in options or on prototype.....

then tandem.addinstance

maybe need a PhETObject core type that everything extends.

Only one tandem per object.

OK to put things on the prototype but not on the instance.

If we go with this.subtypeTandem, then we have to iterate through and get rid of this.tandem because they will get stomped on.

See https://github.com/phetsims/phet-io/issues/436

This is a temporary branch until the work is completed and merged into master or we identify a reason this strategy cannot work and the branch is abandoned/deleted.

This will help us identify the following issues:
(a) cases where an event triggers the an event on the same instance
(b) cases where an object is disposed while triggering an event.

From #46

What is the correct term for referring to the id field of a Tandem instance? I see no less than 4 different names for this thing in code and issues:

id
phetioID
tandem id
wrapper id

... and it's confusing. Highly recommended that you standardize on a term, one that makes sense for the public API.

There is apparently nothing to prevent tandem ids from being shared between static and non-static.

Example, with molecule-polarity loaded in the browser:

> var rootTandem = phet.tandem.Tandem.createRootTandem()
> var myTandem = rootTandem.createTandem( 'foo' )
> myTandem.id
"molecule-polarity.foo"
> myTandem.static
false
> var staticTandem = phet.tandem.Tandem.createStaticTandem( 'foo' )
> staticTandem.id
"molecule-polarity.foo"
> staticTandem.static
true

This seems like a potential problem.

[NOTE: Example above was edited, I copied some things incorrectly from my console.]

In states of matter and a few others, it looks like BooleanRectangularStickyToggleButton is never calling PhetioObject.initializePhetioObject.

TypeError: Cannot read property 'id' of undefined
    at BooleanRectangularStickyToggleButton.startEvent (/tandem/js/PhetioObject.js?bust=1514317781124:99)
    at StickyToggleButtonModel.toggle (/sun/js/buttons/StickyToggleButtonModel.js?bust=1514317781124:114)
    at Array.downListener (/sun/js/buttons/StickyToggleButtonModel.js?bust=1514317781124:61)
    at Emitter.emit2 (/axon/js/Emitter.js?bust=1514317781124:179)
    at Property._notifyListeners (/axon/js/Property.js?bust=1514317781124:180)
    at Property._setAndNotifyListeners (/axon/js/Property.js?bust=1514317781124:167)
    at Property.set (/axon/js/Property.js?bust=1514317781124:114)
    at Object.down (/sun/js/buttons/ButtonListener.js?bust=1514317781124:42)
    at ButtonListener.buttonDown (/scenery/js/input/DownUpListener.js?bust=1514317781124:103)
    at ButtonListener.down (/scenery/js/input/DownUpListener.js?bust=1514317781124:139)

Similar to #18, we should move TandemButtonListener code to ButtonListener and delete TandemButtonListener.

In beers-law-lab, ConcentrationMeterNode was given {Property.<string>} readoutProperty to provide access to exactly what the user is seeing on the meter. This was at request of a customer. This was not a straightforward instrumentation, and required a relatively expensive change to the sim code. Presumably any displayed value (in any sim) is fair game for this feature. So we need a better way of doing this, preferably something that lets us make any displayed value available via the PhET-iO API, with minimal changes to the sim code.

See https://github.com/phetsims/phet-io/issues/489

I emailed the developer list:

PhET Developers,

I'm about to create branches in 6 repos to support the work-in-progress for moving phet-io types to simulations as part of https://github.com/phetsims/phet-io/issues/489

This approach seems feasible, so please keep in mind that changes to these repos will eventually need to merge.

Currently the repos are:
axon
faradays-law
joist
phet-io
sun
tandem

I'll use the branch name "phet-io-types" and create issues in each repo. This list of repos will likely grow as I expand support for other sims. I'll have to decide at what point to cut over to master, but I'd like to get most or all of the phet-io tests working first and address as many corner cases as possible (or at least understand their constraints).

Best Regards,
Sam

In concentration, Solutes are being created statically--that is, at module load time. If they are going to register themselves at module load time, then Tandem listeners should be registered before they run.

One possibility would be to move Tandem to preload.

Another possibility would be for Tandem to buffer the addInstance calls until all listeners are wired up (but how to know if there will be more listeners later?). Or for Tandem to just keep a reference to everything forever?

Or a way to make sure Tandem loads first during requirejs initialization?

In Tandem.js:

33 // increment names of required tandems to avoid collisions for uninstrumented sims with phetioValidateTandems=false
34 var uninstrumentCodeIndex = 0;

Looks to me like this var should be named uninstrumentedCodeIndex.

We would like to integrate tandem into all core classes, including Emitter. It may not be possible for Emitter, since Emitter may need to include child Emitters as part of its definition. We'll have to see.

See https://github.com/phetsims/phet-io/issues/926

I noticed as part of From #34 that we use the package.JSON's name of a sim to get the root tandem. Is there a better way of doing this?

From #34 (comment):

I have never noticed this before, but it seems weird to me that Tandem.js is reaching in to packageJSON for a rootTandem. At the very least perhaps it should fall back to something. I'll create a new issue to discuss.

@samreid should Tandem know about package.json for the name of the root tandem? Maybe it would be better to have that passed in from joist/more sim specific code.

In phetsims/beers-law-lab#99, it was decided that beers-law-lab should be fully instrumented (a) because it's partially done to support concentration, and (b) to give me an opportunity to instrument a sim while the API is still in flux.

@samreid suggested in phetsims/beers-law-lab#99 (comment):

Before @pixelzoom starts on this, it would be good for @jbphet & @samreid to provide some documentation/instruction/support that @pixelzoom or other instrumentors would be able to use.

It was agreed, and @jbphet was assigned to provide the documentation.

Why do TandemSimpleDragHandler and TandemEmitter use options for passing instrumentation information?

While instrumenting Molarity (phetsims/molarity#31) @samreid said that it's clearer to pass tandem as a required constructor arg. Why doesn't that apply here? We certainly won't be using these types in non-instrumented sims, so the instrumentation information isn't at all optional.

I'm playing devil's advocate to a certain extent here, because I'm not a fan of tandem (or other phet-io information) being a constructor parameter.

See phetsims/tasks#378

Tandem.js line 249:

return this.supplied || (this.optional && !this.supplied);

Tandem instances do not have an optional field. Tandem does have a static optional field.

Tandem.js:

    get tail() {
      assert && assert( this.id.indexOf( '.' ) >= 0, 'tandem ID does not have a tail' );

      var lastIndexOfDot = this.id.lastIndexOf( '.' );
      var tail = this.id.substring( lastIndexOfDot + 1 );
      assert && assert( tail.length > 0, 'tandem ID did not have a tail' );
      return tail;
    },

    /**
     * Returns a Tandem for everything except the tail.
     * @returns {Tandem}
     * @public
     */
    get parentTandem() {
      assert && assert( this.id.indexOf( '.' ) >= 0, 'tandem ID does not have a tail' );

      var lastIndexOfDot = this.id.lastIndexOf( '.' );
      var headID = this.id.substring( 0, lastIndexOfDot );

      return new Tandem( headID, {
        required: this.required,
        supplied: this.supplied,
        enabled: this.enabled
      } );
    },

In both cases you're duplicating the logic used to determine whether the tandem has a tail.

Should be:

    get tail() {
      var lastIndexOfDot = this.id.lastIndexOf( '.' );
      assert && assert( lastIndexOfDot !== -1, 'tandem ID does not have a tail' );
      var tail = this.id.substring( lastIndexOfDot + 1 );
      assert && assert( tail.length > 0, 'tandem ID did not have a tail' );
      return tail;
    },

    /**
     * Returns a Tandem for everything except the tail.
     * @returns {Tandem}
     * @public
     */
    get parentTandem() {

      var lastIndexOfDot = this.id.lastIndexOf( '.' );
      assert && assert( lastIndexOfDot !== -1, 'tandem ID does not have a tail' );
      var headID = this.id.substring( 0, lastIndexOfDot );

      return new Tandem( headID, {
        required: this.required,
        supplied: this.supplied,
        enabled: this.enabled
      } );
    },

You might also consider adding the tandem id to each of these assertion messages, to make the messages more useful.

I see '.' duplicated 6 times. The tandem id separator should most definitely be factored out into a constant. And made public via a static constant if you're using it outside of Tandem.

TANDEM was added to beers-law-lab-config.js, but tandem doesn't appear in package.json.

This causes 2 problems:

(1) dependencies.json is incomplete, it is missing tandem shas. So for interoperable sims published to date, we have no way to reproduce their source code snapshot.

(2) "Quick Start" instructions in README.md are incomplete. This is causing problems for non-PhET developers, as reported in https://groups.google.com/forum/#!topic/developing-interactive-simulations-in-html5/NnEOWam9sYQ

This is a problem for all sims that have been instrumented with tandem. i.e.:

beaker
beers-law-lab
color-vision
concentration
energy-skate-park-basics
faradays-law
molecules-and-light

See https://github.com/phetsims/phet-io/issues/1227

In Tandem.removeInstance I don't see a check that no-ops if the tandem is !required && !supplied like I do in addInstance. That seems problematic, since then Tandem is calling remove listeners on an object that hasn't been registered with phet-io before. How have we not run into trouble before? @samreid tag so you are aware.

I suggest removeInstance look like:

    /**
     * Removes an instance from the
     * @param {Object} instance - the instance to remove
     * @public
     */
    removeInstance: function( instance ) {
        if ( !this.required && !this.supplied ) {
             return;
        }

      // Only active when running as phet-io
      if ( PHET_IO_ENABLED && this.enabled ) {
        for ( var i = 0; i < instanceListeners.length; i++ ) {
          instanceListeners[ i ].removeInstance( this.id, instance );
        }
      }
    },

Relocating this from #25 (comment), since it's an issue of it's own. Also related to #40.

There is currently no encapsulation of the tandem id format. The separator ('.') appears as a string literal in numerous places (see #40). And client code is doing things like this:

// sonificationUISoundsSampled.js
163 var lastIdElement = messageObject.phetioID.split('.').pop();

If it's not appropriate to use Tandem in cases like the above, then factor out the id-related functions so that they can be by both Tandem and cases like the above. With the current approach, you currently have zero encapsulation of the id format.

Agreed upon after discussing with @samreid. I implemented it below and fuzz testing for both brands is working.

Missing visibility annotations, missing @static for some static functions, inadequate documentation (e.g. launched), ...

Notice in commit 8da7c97 while https://github.com/phetsims/phet-io/issues/1281.

Questions about PhetioObject.js:

(1) Why is the return value of startEvent documented as @returns {number|boolean}? When would it be number vs boolean? And how can it possibly be anything but boolean when the return statement is a boolean expression? I.e.:

94 return this.phetioObjectTandem.isLegalAndUsable() && phetioEvents.start( eventType, id, this.phetioType, event, args );

(2) Similarly for endEvent, why is the id parameter documented as @param {number|boolean} id. When is it appropriate to use a number vs a boolean?

The separator character ('.') should not be allowed in the id argument to createTandem, createGroupTandem and similar functions.

Example, with molecule-polarity loaded in the browser:

> var rootTandem = phet.tandem.Tandem.createRootTandem( 'mySim' )
> var someTandem = rootTandem.createTandem( 'foo.bar' );
> someTandem.id
"moleculePolarity.foo.bar"
> someTandem.parentTandem
Tandem {options: Object, id: "moleculePolarity.foo", required: true, supplied: true, static: false…}

This is returning a Tandem ("moleculePolarity.foo") that has never existed and is being created as a side effect.

This lack of validation is also being exploited as a lazy shortcut in many places. E.g. in Sim.js:

173 tandem: tandem.createTandem( 'sim.showHomeScreenProperty' ),
179 tandem: tandem.createTandem( 'sim.screenIndexProperty' ),
189 tandem: tandem.createTandem( 'sim.activeProperty' ),
532 tandem: tandem.createTandem( 'sim.barrierRectangle' )

Tandem uses the listener pattern for notifying when an instance is added and removed. The documentation for addInstance includes the specification for an instance listener:

    /**
     * Adds a listener that will be notified when items are registered/deregistered
     * Listeners have the form
     * {
     *   addInstance(id,instance),
     *   removeInstance(id,instance)
     * }
     * where id is of type {string} and instance is of type {Object}
     *
     * @param {Object} instanceListener - described above
     * @public
     * @static
     */
    addInstanceListener: function( instanceListener ) {
      instanceListeners.push( instanceListener );
    },

In this case, the function names don't conform to the typical listener pattern. The current names, addInstance and removeInstance, make it sound like they are doing the adding and removing. The names should be something like instanceAdded and instanceRemoved respectively. Highly recommended to rename these.

From #44 (comment) @jonathanolson said:

It has my approval (just reviewed the above commit), just fix the parentheses-space formatting that got reverted.

I'm guessing that @jonathanolson is referring to this new space:

This space is added by WebStorm IDEA PhET Formatting. Is it undesriable?

From review of IOObject in #36 (comment):

I said:

To clarify... It looks like you applied this mostly (solely?) to view types. View types (subtypes of scenery nodes) continue to use the old approach.

@samreid replied:

Yes, scenery currently has an incompatible implementation of PhET-iO support, but the APIs are being designed to match up. For instance, the IOObject options match the Node options.

I said:

Then why not use IOObject in scenery? Or factor out the options to be used in both IOObject and Node? The need to keep Node and IOObject options in-sync creates a longterm maintenance issue.

See https://github.com/phetsims/phet-io/issues/436

From #31 @zepumph and I implemented a new type called IOObject.js which is intended to be used as a base type for instrumented PhET-iO types. It attempts to standardize a number of new options that were being handled with an odd strategy in phet-io.js.

IOObject.js is checked in to master for review, and we have purposefully avoided using it until review is complete.

Here's a visual diff of how it would be used, for instance, in PrecipitateParticle.js (assuming that SoluteParticle.js inherited from IOObject).

Please note in particular:

• this pattern uses tandem in the options
• automatically takes care of tandem.addInstance
• automatically takes care of tandem.removeInstance in the inherited dispose call
• the phetioType is passed as an option
• this will prevent us from needing to put this.{{SUBTYPE_NAME}}Tandem = tandem anywhere
• this will allow us to eliminate createSupertypeTandem
• We will be able to eliminate the PhET-iO options from the top of phetio.js

The two TODOs marked in IOObject can be investigated once this preliminary review is complete.

Discussion about whether this should be a base type or mixin are archived here--my recommendation is base type: #31 (comment)

@pixelzoom can you please take look?

I see no safeguards in Tandem that prevent the same instance from being added/removed twice, or for removing an instance that hadn't been added.

Here's an example, where nothing fails with assertions enabled:

> var lengthTandem = new phet.tandem.Tandem( 'lengthProperty' )
> var widthTandem = new phet.tandem.Tandem( 'widthProperty' )
> var lengthProperty = new phet.axon.Property( 0 )
> var widthProperty = new phet.axon.Property( 0 )

> lengthTandem.addInstance( lengthProperty )

// should fail
> lengthTandem.addInstance( lengthProperty )

> lengthTandem.removeInstance( lengthProperty )

// should fail
> lengthTandem.removeInstance( lengthProperty )

// should fail
lengthTandem.removeInstance( widthProperty )