How should PhET-iO customize text in components like ComboBoxes?

[samreid]

From https://github.com/phetsims/sun/issues/405. One request from the design https://github.com/phetsims/sun/issues/405#issuecomment-455431908 was

change the text name of elements, like "mystery object" or "object 1"

It was noted in the design meeting:

We can write up a proposal for how to set initial or default values for when the sim is starting up. @kathy-phet said we can put that as a maybe for future work, when a client needs it.

[samreid]

@pixelzoom said:

Re:

• change the text name of elements, like "mystery object" or "object 1"

It's important to note that this operation is generally not specific to, or performed on, ComboBox. ComboBox displays ComboBoxItems, with include the visual representations (Nodes) that corresponds to values. In the case where those visual representations contain text, we want to change the text at the source - NOT after the Nodes have been assembled, and most definitely NOT after they have been provided to the ComboBox.

[samreid]

We previously experimented with a PhET-iO mode where objects were customized as they were created, but it was ultimately removed it because it was incompatible with subtyping. We could open a new investigation for this, or look at using query parameters (or another upstream mechanism) to accomplish this.

@jonathanolson previously promoted making our UI components more mutable, and we have done that in some cases, such as changing colors. @pixelzoom said often it is simpler to dispose an entire UI component and create a new one, but @jonathanolson pointed out that can damage the state of interaction with an in-use component.

we want to change the text at the source - NOT after the Nodes have been assembled, and most definitely NOT after they have been provided to the ComboBox.

@pixelzoom also pointed out that it may be possible to change text after UI component construction as long as we correctly set maxWidth on individual components so the layout isn't disturbed.

we want to change the text at the source

One way to change the values at the source and have the changes reflected downstream would be to model them as axon Properties. This is what we typically do for other cases.

[samreid]

@pixelzoom continued:

But again, since I'm not sure everyone understands... Regardless of whether UI components are mutable, text strings should not be changed at the UI component. We use MVC, so in general there may be multiple views of one text string. So the correct place to make the change is at that one text string, not in each UI component.

@pixelzoom also pointed out that it may be possible to change text after UI component construction as long as we correctly set maxWidth on individual components so the layout isn't disturbed.

If we're going to rely on maxWidth to solve this problem, then we should consider that we're typically using maxWidth only for translated string. If the PhET-iO client is allowed to change non-translated text (as was proposed for the formula in Beer's Law Lab), then we'll potentially need to start setting maxWidth for all text. And we'll need a way to test, since stringTest won't expose problems with non-translated strings.

[samreid]

I've identified the following strategies that might address this:

1. Model the text as an AXON/Property and make sure all clients link to it for changes. This could complicate UI components such as ComboBox, if they need to observe changes in child elements and update layout accordingly. If we set maxWidth = this.width on elements, it could lead to a staggered layout.
2. Instrument the SCENERY/Text so its value can be updated. Would not ripple throughout the simulation (say, back to the model or to other views).
3. Provide the customized value on startup so that it will be created with the correct initial value. Could be done through query string or through a new PhET-iO API layer. This is not desirable because it is different than the typical way of customizing something in a PhET-iO simulation.
4. When a dependency value changes, dispose the old ComboBox and create a new one. This is undesirable because it would be difficult to capture the interaction state of the ComboBox, such as whether it was popped open, or an element was highlighted.

[samreid]

Here is a patch that demonstrates one way to accomplish strategy (1) above. It uses maxWidth on combo box elements to prevent things from going out of bounds.

``` Index: js/molarity/model/Solute.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/molarity/model/Solute.js (revision 68c2b89ee039ea157e807651e44e7b3fbcf46775) +++ js/molarity/model/Solute.js (date 1548037271000) @@ -15,6 +15,8 @@ var molarity = require( 'MOLARITY/molarity' ); var PhetioObject = require( 'TANDEM/PhetioObject' ); var SoluteIO = require( 'MOLARITY/molarity/model/SoluteIO' ); + var StringProperty = require( 'AXON/StringProperty' ); + var Tandem = require( 'TANDEM/Tandem' ); /** * @param {string} name @@ -29,11 +31,14 @@ options = _.extend( { particleColor: maxColor, // the solute's color as a particle - phetioType: SoluteIO + phetioType: SoluteIO, + tandem: Tandem.optional }, options ); // @public - this.name = name; + this.nameProperty = new StringProperty( name, { + tandem: options.tandem.createTandem( 'nameProperty' ) + } ); this.formula = formula; this.saturatedConcentration = saturatedConcentration; this.minColor = minColor; Index: js/molarity/view/SoluteComboBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/molarity/view/SoluteComboBox.js (revision 68c2b89ee039ea157e807651e44e7b3fbcf46775) +++ js/molarity/view/SoluteComboBox.js (date 1548037803000) @@ -22,7 +22,7 @@ const pattern0LabelString = require( 'string!MOLARITY/pattern.0label' ); const soluteString = require( 'string!MOLARITY/solute' ); - class SoluteComboBox extends ComboBox { + class SoluteComboBox extends ComboBox { /** * @param {Solute[]} solutes * @param {Property.} selectedSoluteProperty @@ -50,6 +50,8 @@ // {ComboBoxItem[]} const items = solutes.map( createItem ); + const maxWidth = _.maxBy( items, item => item.node.width ).node.width; + items.forEach( item => item.node.setMaxWidth( maxWidth ) ); super( items, selectedSoluteProperty, listParent, options ); } @@ -69,9 +71,10 @@ stroke: solute.maxColor.darkerColor() } ); - const textNode = new Text( solute.name, { + const textNode = new Text( solute.nameProperty.value, { font: new PhetFont( 20 ) } ); + solute.nameProperty.link( name => textNode.setText( name ) ); const hBox = new HBox( { spacing: 5, ```

It looks like this:

I'd like to get initial feedback on this strategy before going further. @zepumph or @pixelzoom can you please comment?

[pixelzoom]

5. Tell clients that they can't change the text.

... which we should do because of the cost of 1-4, but won't do because we can't seem to say no.

So regarding the other proposed strategies...

2 might be OK in specific cases, but it's not at all generally appropriate for MVC.

3 won't handle the case where the client wants to change text after the sim has started. For example, they start with "Mystery Liquid", then change to "Drink Mix" after the student has solved a problem.

4 is a colossal pain, and swapping out UI components creates all kinds of other problems for PhET-iO. E.g. if a ComboBox has been customized, and one of it's items changes, creating a new ComboBox requires recreating the state of the original ComboBox.

So if we have to make the text changeable, then 1 is the most general approach. We'll have to convert every string that the client might want to change to a StringProperty (not a Property as stated above). And we'll have to set (and somehow test) maxWidth for every such string in the sim, whether it's translated or not. All of that will add more cost to instrumenting (and testing) sims, and generally make writing sims less fun.

[pixelzoom]

@samreid said:

I'd like to get initial feedback on this strategy before going further.

I'm not sure what "going further" entails. But if you're thinking of instrumenting strings in Beer's Law Lab, please don't. Beer's Law Lab is in a bad enough state now, and (imo) it's inappropriate to do further work on PhET-iO instrumentation until the PhET-iO redesign has begun, see https://github.com/phetsims/beers-law-lab/issues/230.

[samreid]

@pixelzoom closed this 14 hours ago

Did you intend to close this issue?

I'm not sure what "going further" entails.

Exploring other strategies.

which we should do because of the cost of 1-4, but won't do because we can't seem to say no.

It seems appropriate to enumerate strategies and estimate their costs. Is it a foregone conclusion that the cost will be prohibitive?

[pixelzoom]

Did you intend to close this issue?

No, thanks for reopening.

It seems appropriate to enumerate strategies and estimate their costs. Is it a foregone conclusion that the cost will be prohibitive?

Agree, it's appropriate to enumerate and estimate their cost. And my estimate was that strategies 2-4 are inappropriate, and strategy 1 will be costly. Whether the cost is prohibitive isn't my call.

[zepumph]

2. Instrument the SCENERY/Text so its value can be updated. Would not ripple throughout the simulation (say, back to the model or to other views).

What about adding support for Property.<string> to RichText (and maybe Text too). This could help us in a variety of ways for PhET-iO I think.

UPDATE:

var textProperty = new Property( 'drink mix');
var text = new RichText( textProperty);

 // provide text to combobox

Then changing the textProperty would change the view too. I know this solution doesn't get to the true root of the model.

[samreid]

As far as I know, some scenery nodes support Color|Property.<Color>--perhaps we should evaluate whether that has been working well and whether we want to expand that pattern to text and other things.

[zepumph]

I have no idea how to continue on this issue, marking for PhET-iO meeting. It would be good to decide on a convention moving forward. Especially since recently @pixelzoom instrumented a couple of sims by controlling text like this with Properties in the model, and having the view listen to all changes (similar to (1) in https://github.com/phetsims/sun/issues/458#issuecomment-455927317), but @jbphet ended up creating Properties in just one case in the view to support this in SOM, see

https://github.com/phetsims/states-of-matter/blob/1e888729c8fa5a6b26d12c3dac0603533665f0f6/js/phase-changes/view/PhaseChangesMoleculesControlPanel.js#L111-L131

[zepumph]

This is another problem (similar to https://github.com/phetsims/studio/issues/190) that will be fixed by https://github.com/phetsims/studio/issues/192. Unmarking for phet-io meeting. We will check in on this after the other one is completed.