encapsulate the format of phetioID

[pixelzoom]

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.

[pixelzoom]

In #25 (comment), @zepumph said:

sonificationUISoundsSampled.js is client wrapper code, so we don't have access to the Tandem instance there. We only have the string literal (phetioID).

[pixelzoom]

In #25 (comment), I said:

sonificationUISoundsSampled.js is client wrapper code, so we don't have access to the Tandem instance there. We only have the string literal (phetioID).

#41 (comment): "then factor out the id-related functions so that they can be by both Tandem and cases like the above"

[pixelzoom]

In #25 (comment) @zepumph said:
We struggle with this, because we don't have a library that that is used on the sim side (with requirejs) or with wrapper code (just a global, see WrapperUtils.js).

[pixelzoom]

What's the barrier to having a library that encapsulates tandem id and is usable by both sims and wrappers?

[pixelzoom]

And for the record... The lack of encapsulation is not limited to wrappers. I see 9 occurrences of '.' in the phet-io repository that qualify.

[pixelzoom]

Looks like not all occurrences of '.' in the phet-io repository are related to tandem identifier. But my point is some of them likely are, and it's not easy to tell which ones.

[zepumph]

I see two of the 9 (10 for me) that apply to this problem:

var format = function( phetioID ) {
      if ( phetioID.indexOf( '.' ) > 0 ) {
        var lastIndex = phetioID.lastIndexOf( '.' );
        return phetioID.substring( 0, lastIndex + 1 ) + '%c' + phetioID.substring( lastIndex + 1, phetioID.length );
      }

These should use use a Tandem static method to parse so that we can factor out the usage of '.' into a singular "tandemDelimiterChar" in Tandem.js. Is this the type of thing you are trying to centralize?

[pixelzoom]

The delimiter is only part of the encapsulation problem. The functions for picking apart a tandem id are the other part. E.g the above example is essentially duplicating Tandem tail and parentTandem.

[samreid]

phetioEvents.js should change to take a Tandem as an argument rather than a phetioID. Other places I see phetioID.:

sonification: these look like they are doing string matching and probably OK
build-an-atom: testing strings, perhaps OK
create-instance-proxy-div:

  /**
   * Get the display name from a lower case, dot separated string.
   * @param {string} phetioID
   * @returns {*}
   */
  var getFriendlyName = function( phetioID ) {
    var array = phetioID.split( '.' );
    var tail = array[ array.length - 1 ];
    return toWhitespaced( tail );
  };

instance-proxies.js does prefix matching, maybe can be deleted.

I'm not convinced that factoring out

    var array = phetioID.split( '.' );
    var tail = array[ array.length - 1 ];

into an auxiliary library file that is only used from one location. In fact https://stackoverflow.com/questions/573145/get-everything-after-the-dash-in-a-string-in-javascript pointed out that a simpler implementation is phetioID.split( '.' ).pop() which also seems like it doesn't need to be factored out.

[samreid]

@pixelzoom thoughts?

[pixelzoom]

sonification: these look like they are doing string matching and probably OK
build-an-atom: testing strings, perhaps OK

Not clear what specifically you're referring to, so I can't comment on whether I think that are OK.

Things are properly encapsulated if:
(1) you can change the separator from '.' to something else in one place
(2) code for parsing a tandem (e.g. tail, parentTandem) is not duplicated

... and that is currently not the case.

[pixelzoom]

Related issue: phetsims/molecules-and-light#165, phetio ids should not be formed using string concatenation

[samreid]

Based on the remarks in this issue and in phetsims/molecules-and-light#165 I want to make sure we are on the same page about usage of phetioIDs and how they are intended to be used. Let's look for instance at phet-io-wrappers/build-an-atom-game/build-an-atom-game.html which has this code:

simIFrameClient.invokeSequence([
  { phetioID: 'buildAnAtom.gameScreen.model.provideFeedbackProperty', method: 'setValue', args: [ false ] },
  { phetioID: 'buildAnAtom.gameScreen.view.scoreboard.scoreText', method: 'setVisible', args: [ false ] },
  { phetioID: 'buildAnAtom.gameScreen.view.scoreboard.startOverButton', method: 'setVisible', args: [ false ] }
]);

The first phetioID is buildAnAtom.gameScreen.model.provideFeedbackProperty which is a term that a PhET-iO client would observe in the Instance Proxies wrapper or Documentation wrapper which lists all of the phetioIDs.

In order to factor out the . are you picturing something like this?

var Tandem = require('SOME WAY OF LOADING TANDEM.JS IN A WRAPPER HTML FILE');
// ...
var tandem = Tandem.createRootTandem('buildAnAtom'); // Note: root tandem is defined for sims, not for wrappers.  The wrapper could access the sim Tandem.rootTandem but it would be asynchronous
simIFrameClient.invokeSequence([
    { phetioID: tandem.createTandem('gameScreen').createTandem('model').createTandem('provideFeedbackProperty', method: 'setValue', args: [ false ] },
 // ...

If this is what you are picturing, then please describe how the benefits outweigh the costs. If this is not what you are picturing, then please clarify or describe an alternative.

[pixelzoom]

I'm OK with usages like:

{ phetioID: 'buildAnAtom.gameScreen.model.provideFeedbackProperty', method: 'setValue', args: [ false ] },

... where you're using a complete id that was formed elsewhere. If you were forming this id using string concatenation, or somehow manipulating this id (e.g. phetioID.split( '.' ).pop()) that's not OK.

[zepumph]

phetioID: 'buildAnAtom.gameScreen.model.provideFeedbackProperty'
vs
If you were forming this id using string concatenation, or somehow manipulating this id (e.g. phetioID.split( '.' ).pop()) that's not OK.

To me, string manipulation and using intact phetioIDs (both in the wrapper code) use the same assumptions about id structure and separator key. @pixelzoom would you explain how you find these different? Please note that I am only talking about the wrapper side, which most often means that we aren't using requirejs, but rather plain ol' javascript.

UPDATE: formatting (@samreid)

[samreid]

@pixelzoom I factored out PhetioIDUtils and used it in Tandem.js and createInstanceProxyDiv.js, can you please review?

[pixelzoom]

Looks good to me. Anything else to do?

[samreid]

That's all for now, closing.