SoluteIO 'name' does not update when locale is changed.

[pixelzoom]

After adding support for dynamic locale in #239, we have a problem with SoluteIO. It is currently defined like this:

  /**
   * SoluteIO handles PhET-iO serialization of Solute. Since all Solutes are static instances, it implements
   * 'Reference type serialization', as described in the Serialization section of
   * https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-technical-guide.md#serialization
   * But because we want 'name' and 'pH' fields to appear in Studio, we cannot simply subclass ReferenceIO, We
   * must also provide stateSchema and toStateObject. See https://github.com/phetsims/ph-scale/issues/205.
   */
  public static readonly SoluteIO = new IOType( 'SoluteIO', {
    valueType: Solute,
    supertype: ReferenceIO( IOType.ObjectIO ),
    stateSchema: {
      name: StringIO,
      pH: NumberIO
    },
    toStateObject: solute => {
      const soluteReference = ReferenceIO( IOType.ObjectIO ).toStateObject( solute );
      soluteReference.name = solute.nameProperty.value;
      soluteReference.pH = solute.pH;
      return soluteReference;
    }
  } );

And the value shown in Studio displays 'phetioID', 'name', and 'pH' fields, for example:

The problem is that the name field is now dynamic (changes with localeProperty) and it does not update where SoluteIO is displayed in Studio.

@zepumph @samreid thoughts?

[pixelzoom]

The easiest thing to do here would be to reimplement SoluteIO like this:

  public static readonly SoluteIO = new IOType( 'SoluteIO', {
    valueType: Solute,
    supertype: ReferenceIO( IOType.ObjectIO ),
} );

I'm not really convinced that showing Solute name and pH in Studio has much value. The instructional designer can always consult the running sim. @arouinfar your thoughts?

[samreid]

I'll self-unassign until we hear from @arouinfar, but I agree if information is in the sim, it does not need to be restated in studio.

[arouinfar]

The problem is that the name field is now dynamic (changes with localeProperty) and it does not update where SoluteIO is displayed in Studio.

I'm not really convinced that showing Solute name and pH in Studio has much value.

Can we drop the name field and instead just include the phetioID and pH? We include the pH so users know the undiluted value.

[pixelzoom]

I can certainly drop the 'name' field for SoluteIO.

But what's the general plan for when a dynamic string needs to appear in an IOType in Studio. @zepumph @samreid? Should I create a general issue, and in which repo?

[zepumph]

#243 (comment) seems fine to me. I see 2 other problems though:

1. It would be ideal if we could go to phScale.global.model.solutes.soda and see its initialState. It has serialization potential, but since it is phetioState:false, we don't get serialization data in the wrapper.
2. It seems like a weird bug in studio that I cannot explain that phScale.global.model.solutes.soda says its IOType is ObjectIO instead of SoluteIO.

If (1) was not the case, then #243 (comment) would be particularly ideal because we would be able to go to that in studio and see the state of it. @samreid, yet another time we wished we differentiated between allowing to have a serialized value VS. being included in state setting.

[pixelzoom]

1. It would be ideal if we could go to phScale.global.model.solutes.soda and see its initialState. It has serialization potential, but since it is phetioState:false, we don't get serialization data in the wrapper.

How/why does it have any serialization potential other than "reference"? It has no Properties that it owns, and nameProperty is a references to a localized string Property.

2. It seems like a weird bug in studio that I cannot explain that phScale.global.model.solutes.soda says its IOType is ObjectIO instead of SoluteIO.

That does seem buggy.

I see the same problem in BLL with static solutes under beersLawLab.global.model.solutes. Static solutions under beersLawLab.beersLawScreen.model.solutions look OK.

[pixelzoom]

@arouinfar please review. SoluteIO 'name' field was removed in the above commit. Here's how the value of a soluteProperty looks in Studio:

[samreid]

It's unclear if I need to comment on any parts of this, I'll self-unassign for now but please reinvite me if necessary.

[pixelzoom]

Yes @samreid -- I would still like you or @zepumph to reply to #243 (comment):

But what's the general plan for when a dynamic string needs to appear in an IOType in Studio. @zepumph @samreid? Should I create a general issue, and in which repo?

[samreid]

But what's the general plan for when a dynamic string needs to appear in an IOType in Studio.

Dynamic, localized strings should not appear in IOType documentation or method names. If we need to link one PhetioObject to a LocalizedString, perhaps that could be done via a linked element?

[pixelzoom]

Dynamic, localized strings should not appear in IOType documentation or method names.

It's not appearing in IOType documentation or method names. It's in the state object.

If we need to link one PhetioObject to a LocalizedString, perhaps that could be done via a linked element?

That is the problem that resulted in phetsims/axon#414. LocalizedStrings are TReadOnlyProperty<string> in Strings.ts files, which is not linkable.

[samreid]

In phetsims/axon#414 we will make the localized strings linkable.

Copying the current value of one instrumented property so it will show up in the state of another PhetioObject doesn't sound like a good idea to me.

But what's the general plan for when a dynamic string needs to appear in an IOType in Studio. @zepumph @samreid? Should I create a general issue, and in which repo?

This sounds like a job for linked elements, if I understand correctly.

[pixelzoom]

This sounds like a job for linked elements, if I understand correctly.

You understand correctly, and I agree. So I'll unassign @samreid and @zepumph.

I'll create a new ph-scale issue to link Solute to its nameProperty when phetsims/axon#414 has been addressed.

Remaining work here is review by @arouinfar, see #243 (comment).

[arouinfar]

Looks good, thanks @pixelzoom.