new sims are using old-style formatting patterns

[pixelzoom]

Noted during Wave Interference code review phetsims/wave-interference#259.

Wave Interference, a new sim, is necessarily using old-style (numeric placeholders) formatting patterns:

  "cmValue": {
    "value": "{0} cm"
  },
  "nmValue": {
    "value": "{0} nm"
  },

New-style (named placeholders) would be:

  "cmValue": {
    "value": "{{value}} cm"
  },
  "nmValue": {
    "value": "{{value}} nm"
  },

I say "necessarily using" because we have common components (NumberControl, NumberDisplay,...) that use this format.

Creating more strings that use the old-style seems like something that we want to avoid.

Options, in order of increasing cost/complexity:

(1) Tolerate this and do nothing.

(2) Make client code adapter new-style patterns to UI components that use old-style components. E.g.

const oldPattern = '{0} cm';
const newPattern = StringUtils.format( oldPattern, '{{value}}' );
const string = StringUtils.fillIn( newPattern, { value: 10 } );

(3) Provide a utility for doing (2).

(4) Change the UI components to recognize both old-style and new-style.

(5) Change the UI components to use new-style, and change all existing translations.

[jonathanolson]

I could be remembering incorrectly, but I thought we would use the new style ('{{value}} cm') and then StringUtils.fillIn( str, { value: '{0}' } ) when we had to provide things to a component that required numbered values. It seems like option (2) in practice, but is not very verbose.

An example is in masses-and-springs for use with NumberControl:

        valuePattern: StringUtils.fillIn( gravityValueString, {
          gravity: '{0}'
        } ),

with the string:

  "gravityValue": {
    "value": "{{gravity}} m/s<sup>2</sup>"
  },

I also see this pattern being used a lot when searching for '{0}'. I'd propose that this adapter pattern is fine for now, particularly since trying to update everything to named strings would break a lot of currently-translated strings.

[pixelzoom]

Raising priority since this is blocking phetsims/wave-interference#266.

[pixelzoom]

My recollection is the same as what @jonathanolson said in #446 (comment). But I thought we should revisit since (a) Wave Interference isn't using that pattern, and (b) it's not a great longterm solution.

[pixelzoom]

I volunteered for this at today's status meeting 12/20/18. The plan is to make NumberDisplay support both numbered and named placeholders, with the constrain that they be {0} and {{value}} respectively.

[pixelzoom]

Something I ran into immediately when using named placeholders with NumberDisplay... StringFormat.fillIn fails an assertion if all placeholders are not filled in.

Here's a typical example:

const valueUnitsString = requirejs( ... ); // "{{value}} {{units}}"
...
const numberDisplay = new NumberDisplay( ..., {
  format: StringUtils.fillIn( valueUnitsString, {
    units: 'nm'
  } )
} );

The above will fail because {{value}} is left unfilled, intended to be filled in by NumberDisplay.

Does anyone object to changing the behavior of StringFormat.fillIn, so that it's OK to leave placeholders unfilled? @samreid @jonathanolson?

[pixelzoom]

#446 (comment) got a 👍 on Slack.

[pixelzoom]

These repose are currently using the workaround to covert {{value}} to {0}, search for value: '{0}'.

• gas-properties
• projectile-motion

[pixelzoom]

Summary of changes:

• NumberDisplay (and therefore NumberControl) now support both named and numbered
• placeholders. NumberDisplay uses StringUtils.fillIn internally.
• Sims that were converting {{value}} to {0} have had that workaround removed.

Since this issue was motivated by phetsims/wave-interference#266, @samreid would you please review?

[samreid]

Unit test phetcommon/phetcommon-tests.html?ea is failing, can you please take a look before I review?

[pixelzoom]

Will do. I didn't realize that phetcommon had unit tests.

[pixelzoom]

StringUtils.fillIn no longer requires that all parameters be filled in. And the hack of converting {{value}} to {0} is not longer needed for NumberControl and NumerDisplay.

So this:

valuePattern: StringUtils.fillIn( pattern0Value1UnitsString, { value: '{0}', units: unitString } ),

should be changed to this:

valuePattern: StringUtils.fillIn( pattern0Value1UnitsString, { units: unitString } ),

[pixelzoom]

• In addition to inverse-square-law ISLCObjectControlPanel....

There are other sims that are doing some wonky replacements of '{0}'.

• cck-common: 2 replacements with '{0}' that could have been avoided by using '{{value}}' in strings. Should at least be changed to '{{value}}'.

• masses-and-springs: 2 replacements with '{0}' that could have been avoided by using '{{value}}' in strings. Should at least be changed to '{{value}}'.

• pendulum-lab: 4 replacements with '{0}' that could have been avoided by using '{{value}}' in strings. Should at least be changed to '{{value}}'.

• trig-tour: CoordinatesRow has a really messed up use of StringUtils.format that doesn't involve any translated strings. It should be converted to StringUtils.fillIn, or (better yet) use string concat or an ES6 string template.

[pixelzoom]

I'll create sim-specific issues for all of the above.

[pixelzoom]

To prevent a proliferation of '{{value}}' and '{0}' string literals, I've added NAMED_PLACEHOLDER and NUMBERED_PLACEHOLDER static constants to NumberDisplay.

[pixelzoom]

Sim-specific issues have been created for sims that are doing unnecessary replacement or should be using constants instead of string literals.

Back to @samreid to continue review.

[samreid]

I skimmed the commits and didn't see anything that requires further work.

I added @deprecated annotation and converted to block comment so IDEA (and maybe other tools) can indicate usages as deprecated:

Back to @pixelzoom about this last point--close if all is well.