dropper.soluteProperty is missing valid values

[Nancy-Salpepi]

Test device
Macbook air (m1 chip)

Operating System
13.1

Browser
safari

Problem description
For phetsims/qa#872 and phetsims/qa#873, the solutes should have set values (radio buttons instead of get/set value buttons).

[arouinfar]

Thanks @Nancy-Salpepi. I think a better user experience in Studio would be to select between the possible solutes using radio buttons.

The get/setValue interface works, but it's a bit quirky. If the phetioID has a typo or there is a syntax error, Studio will catch the error and a dialog will pop up. The pH field, however, is ignored. This is true when using Set Value interface in Studio as well as when you use phetioClient.invoke in the console. For example:

1. Navigate to soluteProperty and press Get Value to populate the box.
2. Use the ComboBox to select a different solute.
3. Edit the pH in the text box, and then press Set Value.
4. The solute will switch back to whatever the solute was in step (1) and its pH will be the stock pH, not whatever nonsense was entered in (3).

I am pretty sure we asked @pixelzoom to display the pH as part of the solute value so clients have easy access to the stock pH. My guess is that the inclusion of the pH may be to blame for the strange get/setValue behavior highlighted in the example above.

[pixelzoom]

Below is a patch that sets the validValues for dropper.soluteProperty, which causes Studio to present those values as radio buttons, like this:

I didn't commit the patch because there's a lot of overhead involved, including:

• patch ph-scale 1.6
• patch ph-scale-basics 1.6
• regenerate API files for both sims
• test migration from 1.5 for both sims

Testing migration is problematic because common code is not in a stable state (as I understand it). So I'm going to wait until RC testing has been completed, and common-code changes for migration have been completed.

Patch

Subject: [PATCH] set validValues for dropper.soluteProperty, https://github.com/phetsims/ph-scale/issues/271
---
Index: js/micro/view/MicroScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/micro/view/MicroScreenView.ts b/js/micro/view/MicroScreenView.ts
--- a/js/micro/view/MicroScreenView.ts	(revision e48561d459954db133ed343cc018963f2b8b2834)
+++ b/js/micro/view/MicroScreenView.ts	(date 1674098047957)
@@ -133,7 +133,7 @@
 
     // solutes combo box
     const soluteListParent = new Node();
-    const soluteComboBox = new SoluteComboBox( model.dropper.soluteProperty, model.solutes, soluteListParent, {
+    const soluteComboBox = new SoluteComboBox( model.dropper.soluteProperty, soluteListParent, {
       maxWidth: 400,
       tandem: options.tandem.createTandem( 'soluteComboBox' )
     } );
Index: js/macro/view/MacroScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/macro/view/MacroScreenView.ts b/js/macro/view/MacroScreenView.ts
--- a/js/macro/view/MacroScreenView.ts	(revision e48561d459954db133ed343cc018963f2b8b2834)
+++ b/js/macro/view/MacroScreenView.ts	(date 1674097981159)
@@ -105,7 +105,7 @@
 
     // solutes combo box
     const soluteListParent = new Node();
-    const soluteComboBox = new SoluteComboBox( model.dropper.soluteProperty, model.solutes, soluteListParent, {
+    const soluteComboBox = new SoluteComboBox( model.dropper.soluteProperty, soluteListParent, {
       maxWidth: 400,
       tandem: options.tandem.createTandem( 'soluteComboBox' )
     } );
Index: js/common/model/PHModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/PHModel.ts b/js/common/model/PHModel.ts
--- a/js/common/model/PHModel.ts	(revision e48561d459954db133ed343cc018963f2b8b2834)
+++ b/js/common/model/PHModel.ts	(date 1674097981171)
@@ -100,7 +100,7 @@
     this.beaker = new Beaker( PHScaleConstants.BEAKER_POSITION );
 
     const yDropper = this.beaker.position.y - this.beaker.size.height - 15;
-    this.dropper = new Dropper( Solute.WATER,
+    this.dropper = new Dropper( Solute.WATER, this.solutes,
       new Vector2( this.beaker.position.x - 50, yDropper ),
       new Bounds2( this.beaker.left + 40, yDropper, this.beaker.right - 200, yDropper ), {
         tandem: options.tandem.createTandem( 'dropper' )
Index: js/common/model/Dropper.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Dropper.ts b/js/common/model/Dropper.ts
--- a/js/common/model/Dropper.ts	(revision e48561d459954db133ed343cc018963f2b8b2834)
+++ b/js/common/model/Dropper.ts	(date 1674097981164)
@@ -38,7 +38,7 @@
   // See https://github.com/phetsims/ph-scale/issues/178
   public readonly visibleProperty: Property<boolean>;
 
-  public constructor( solute: Solute, position: Vector2, dragBounds: Bounds2, providedOptions: DropperOptions ) {
+  public constructor( solute: Solute, solutes: Solute[], position: Vector2, dragBounds: Bounds2, providedOptions: DropperOptions ) {
 
     const options = optionize<DropperOptions, SelfOptions, PHMovableOptions>()( {
 
@@ -59,6 +59,7 @@
     super( position, dragBounds, options );
 
     this.soluteProperty = new Property( solute, {
+      validValues: solutes,
       tandem: options.tandem.createTandem( 'soluteProperty' ),
       phetioValueType: Solute.SoluteIO,
       phetioDocumentation: 'the solute dispensed by the dropper'
Index: js/common/view/SoluteComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/SoluteComboBox.ts b/js/common/view/SoluteComboBox.ts
--- a/js/common/view/SoluteComboBox.ts	(revision e48561d459954db133ed343cc018963f2b8b2834)
+++ b/js/common/view/SoluteComboBox.ts	(date 1674097981150)
@@ -25,7 +25,7 @@
 export default class SoluteComboBox extends ComboBox<Solute> {
 
   public constructor( selectedSoluteProperty: Property<Solute>,
-                      solutes: Solute[], soluteListParent: Node,
+                      soluteListParent: Node,
                       providedOptions: SoluteComboBoxOptions ) {
 
     const options = optionize<SoluteComboBoxOptions, SelfOptions, ComboBoxOptions>()( {
@@ -41,6 +41,9 @@
 
     const items: ComboBoxItem<Solute>[] = [];
 
+    const solutes = selectedSoluteProperty.validValues!;
+    assert && assert( solutes );
+
     // Create items for the listbox
     solutes.forEach( solute => {

[zepumph]

Common migration work shouldn't block this. Please feel free to add rules and test this with 1.5.

[samreid]

I agree, this seems safe to commit to master.

[pixelzoom]

In the above commits, this issue was addressed in master for ph-scale and ph-scale-basics.

[pixelzoom]

In the above commits, this issue was patched in ph-scale 1.6 and its dependencies.

[pixelzoom]

In the above commits, this issue was patched in ph-scale-basics 1.6 and its dependencies.

[pixelzoom]

So much for "safe to proceed". Migration from ph-scale 1.5 => 1.6 works OK. But migration from ph-scale-basics 1.5. => 1.6 fails (in master!) with:

PhetioStateEngine.ts:325 Uncaught Error: Assertion failed: must support phetioState to be set
at window.assertions.assertFunction (assert.js:28:13)
at PhetioStateEngine.setStateForPhetioObject (PhetioStateEngine.ts:406:15)
at PhetioStateEngine.ts:310:14
at Array.forEach ()
at PhetioStateEngine.iterate (PhetioStateEngine.ts:295:22)
at PhetioStateEngine.setState (PhetioStateEngine.ts:253:31)
at PhetioStateEngine.setFullState (PhetioStateEngine.ts:269:10)
at PhetioEngine.implementation (phetioEngine.ts:1035:65)
at PhetioCommandProcessor.processCommand (phetioCommandProcessor.ts:417:51)
at getReturn (phetioCommandProcessor.ts:295:36)

So I'm blocked. @zepumph @samreid please advise.

[pixelzoom]

PhetioStateEngine.ts:325 Uncaught Error: Assertion failed: must support phetioState to be set

This is issue #268. On hold until that issue is addressed.

[pixelzoom]

Assertion was resolved by adding missing rules to ph-scale-basics-migration-rules.ts, see phetsims/ph-scale-basics#56.

This is ready for testing in the next RC.

[pixelzoom]

This is not ready for RC. Need to cherry-pick changes from phetsims/ph-scale-basics#56.

[zepumph]

@samreid and I will not cherry pick anything into branches for this issue, but will make sure that once other, blocking issues have their content cherry picked, that this is working correctly.

[pixelzoom]

Just to clarify... Changes to ph-scale and API files have already been cherry-picked. What has not been cherry-picked is changes to migration rules. I think that's what @zepumph is saying that they "will not cherry pick" in the preceding comment.

[samreid]

@zepumph and I cherry-picked the related changes, and regenerated the API files. It is working correctly locally. Would be good for @Nancy-Salpepi to confirm in RC.2.

[zepumph]

I confirmed in ph-scale and basics 1.6.0-rc.2. Over to QA.

[Nancy-Salpepi]

This looks good in rc.3 for pH Scale and pH Scale Basics. Closing!