Public API for `isSettingStateProperty` should be read-only. #311

pixelzoom · 2024-05-10T20:51:52Z

Noted while investigating phetsims/gas-properties#240 (comment), yet another problem that involves isSettingPhetioStateProperty.

I find myself having to check isSettingPhetioStateProperty frequently when instrumenting sims, to short-circuit code that should not be called when restoring state. So I was surprised to find that this rather important (global!) Property allows me to set it, with no complains from tsc. For example:

    // When the number of particles in the container changes ...
    this.particleSystem.numberOfParticlesProperty.link( numberOfParticles => {
      if ( !isSettingPhetioStateProperty.value ) {
        isSettingPhetioStateProperty.value = true;

My understanding is that the only place that isSettingPhetioStateProperty should be set (and therefore the only place this should be settable) is in PhetioStateEngine.ts, specifically:

  public setState( state: FullPhetioState, scopeTandem: Tandem ): void {
    this.isSettingStateProperty.value = true;
    ....
    this.isSettingStateProperty.value = false;
  }

Let's tighten this up so that isSettingStateProperty is read-only.

The text was updated successfully, but these errors were encountered:

samreid · 2024-05-10T21:49:58Z

Here is a working implementation of the proposal above. I used a patch instead of branches:

Subject: [PATCH] Rename "Case Index" to "Case index", see https://github.com/phetsims/projectile-data-lab/issues/251
---
Index: phet-io/js/PhetioStateEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts
--- a/phet-io/js/PhetioStateEngine.ts	(revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795)
+++ b/phet-io/js/PhetioStateEngine.ts	(date 1715377244737)
@@ -16,7 +16,6 @@
 
 import Emitter from '../../axon/js/Emitter.js';
 import TEmitter, { TReadOnlyEmitter } from '../../axon/js/TEmitter.js';
-import TProperty from '../../axon/js/TProperty.js';
 import PropertyStateHandler from '../../axon/js/PropertyStateHandler.js';
 import propertyStateHandlerSingleton from '../../axon/js/propertyStateHandlerSingleton.js';
 import merge from '../../phet-core/js/merge.js';
@@ -31,7 +30,7 @@
 import dataStream from './dataStream.js';
 import phetio from './phetio.js';
 import { TPhetioEngine } from './phetioEngine.js';
-import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js';
+import { writableIsSettingPhetioStateProperty } from '../../tandem/js/isSettingPhetioStateProperty.js';
 import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js';
 import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js';
 import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js';
@@ -74,7 +73,7 @@
   // to indicate whether state is being set or not. In general when using for a sim. TANDEM/isSettingPhetioStateProperty
   // should be preferred to this Property because it will exist for all brands (where this is only available in
   // phet-io brand).
-  public isSettingStateProperty: TProperty<boolean> = isSettingPhetioStateProperty;
+  public isSettingStateProperty = writableIsSettingPhetioStateProperty;
 
   // a list of setStateHelpers. These are sim specific functions that help set state to avoid
   // "Impossible set state errors".
Index: projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts
--- a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts	(revision b00659aac770fd068b6c23828495f3b1ec3305a0)
+++ b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts	(date 1715377375059)
@@ -187,7 +187,7 @@
       }
     };
 
-    isSettingPhetioStateProperty.addListener( updateHistogram );
+    isSettingPhetioStateProperty.lazyLink( updateHistogram );
 
     // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes.
     fields.forEach( field => {
Index: projectile-data-lab/js/common/view/HistogramNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/projectile-data-lab/js/common/view/HistogramNode.ts b/projectile-data-lab/js/common/view/HistogramNode.ts
--- a/projectile-data-lab/js/common/view/HistogramNode.ts	(revision b00659aac770fd068b6c23828495f3b1ec3305a0)
+++ b/projectile-data-lab/js/common/view/HistogramNode.ts	(date 1715377366180)
@@ -258,7 +258,7 @@
       }
     };
 
-    isSettingPhetioStateProperty.addListener( () => updateHistogram( true ) );
+    isSettingPhetioStateProperty.lazyLink( () => updateHistogram( true ) );
 
     // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes.
     fields.forEach( field => {
Index: tandem/js/isSettingPhetioStateProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts
--- a/tandem/js/isSettingPhetioStateProperty.ts	(revision a444e3f516f4c52eea41f71d87e4051450fa1c1c)
+++ b/tandem/js/isSettingPhetioStateProperty.ts	(date 1715377505170)
@@ -2,15 +2,22 @@
 
 /**
  * Property that is set to true when the PhET-iO State Engine is setting the state of a simulation.
+ *
  * @author Sam Reid (PhET Interactive Simulations)
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
 
 import tandemNamespace from './tandemNamespace.js';
 import TinyProperty from '../../axon/js/TinyProperty.js';
+import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
 
-const isSettingPhetioStateProperty = new TinyProperty( false );
+// This one is for specialized usage in the PhetioStateEngine, which changes the value
+const writableIsSettingPhetioStateProperty = new TinyProperty( false );
+
+// Simulations can use this one to observe the value
+const isSettingPhetioStateProperty: TReadOnlyProperty<boolean> = writableIsSettingPhetioStateProperty;
 
 tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty );
 
-export default isSettingPhetioStateProperty;
\ No newline at end of file
+export default isSettingPhetioStateProperty;
+export { writableIsSettingPhetioStateProperty };
\ No newline at end of file

This is also passing all precommit hooks (took 1m45s). @pixelzoom or @zepumph please review and feel free to commit if it passes review.

pixelzoom · 2024-05-10T22:02:42Z

The patch is marginally better, but still provides a public API for modifying writableIsSettingPhetioStateProperty.

Since PhetioStateEngine is the sole place where isSettingPhetioStateProperty should be set, a better implementation would be:

Define and instantiate const isSettingPhetioStateProperty in PhetioStateEngine.
Make a readonly reference available in PhetioStateEngine via public static readonly isSettingPhetioStateProperty.
Provide isSettingPhetioStateProperty.ts as a convenient shortcut that is equivalent to PhetioStateEngine. isSettingPhetioStateProperty, i.e.:

const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty;

tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty );

export default isSettingPhetioStateProperty;

pixelzoom · 2024-05-10T22:14:38Z

Below is a patch to demonstrate the implementation I described in #311 (comment). It's equivalent to the current implementation in that isSettingPhetioStateProperty is still settable. To make it readonly, add a type specification to public static readonly isSettingPhetioStateProperty. But I'm not sure what that type specification is, since I'm not familiar with the TinyProperty hierarchy. And the readonly type needs to support addListener, which is used (exclusively) by PDL.

patch

Subject: [PATCH] add isSettingPhetioStateProperty check, https://github.com/phetsims/gas-properties/issues/238
---
Index: phet-io/js/PhetioStateEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts
--- a/phet-io/js/PhetioStateEngine.ts	(revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795)
+++ b/phet-io/js/PhetioStateEngine.ts	(date 1715379079440)
@@ -31,10 +31,10 @@
 import dataStream from './dataStream.js';
 import phetio from './phetio.js';
 import { TPhetioEngine } from './phetioEngine.js';
-import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js';
 import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js';
 import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js';
 import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js';
+import TinyProperty from '../../axon/js/TinyProperty.js';
 
 // constants
 const DELETED_VALUE = 'DELETED'; // this is copied in documentation for PhetioEngineIO.getChangedState(), and used in Studio.
@@ -48,6 +48,8 @@
   alreadyCalledForThisStateSet?: boolean;
 };
 
+const isSettingPhetioStateProperty = new TinyProperty( false );
+
 class PhetioStateEngine implements TPhetioStateEngine {
 
   // emits before state the state is set. Emits with the state object that will be set. Use the
@@ -86,6 +88,8 @@
     hasListenerOrderDependencies: true // We rely on Properties undeferring value first (since everything else just notifies).
   } );
 
+  public static readonly isSettingPhetioStateProperty = isSettingPhetioStateProperty;
+
   public constructor( private readonly phetioEngine: TPhetioEngine, providedOptions?: PhetioStateEngineOptions ) {
 
     const options = optionize<PhetioStateEngineOptions>()( {
Index: tandem/js/isSettingPhetioStateProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts
--- a/tandem/js/isSettingPhetioStateProperty.ts	(revision a444e3f516f4c52eea41f71d87e4051450fa1c1c)
+++ b/tandem/js/isSettingPhetioStateProperty.ts	(date 1715379079447)
@@ -7,9 +7,9 @@
  */
 
 import tandemNamespace from './tandemNamespace.js';
-import TinyProperty from '../../axon/js/TinyProperty.js';
+import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js';
 
-const isSettingPhetioStateProperty = new TinyProperty( false );
+const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty;
 
 tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty );

pixelzoom · 2024-05-10T22:20:57Z

But I'm not sure what that type specification is, since I'm not familiar with the TinyProperty hierarchy. And the readonly type needs to support addListener, which is used (exclusively) by PDL.

Below is a patch that I believe addresses the problem.

patch

Subject: [PATCH] add isSettingPhetioStateProperty check, https://github.com/phetsims/gas-properties/issues/238
---
Index: phet-io/js/PhetioStateEngine.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/phet-io/js/PhetioStateEngine.ts b/phet-io/js/PhetioStateEngine.ts
--- a/phet-io/js/PhetioStateEngine.ts	(revision f2f620fb6f958ac3e7c504355b7c7d1dd2652795)
+++ b/phet-io/js/PhetioStateEngine.ts	(date 1715379455639)
@@ -31,10 +31,11 @@
 import dataStream from './dataStream.js';
 import phetio from './phetio.js';
 import { TPhetioEngine } from './phetioEngine.js';
-import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js';
 import isClearingPhetioDynamicElementsProperty from '../../tandem/js/isClearingPhetioDynamicElementsProperty.js';
 import isPhetioStateEngineManagingPropertyValuesProperty from '../../tandem/js/isPhetioStateEngineManagingPropertyValuesProperty.js';
 import { TPhetioStateEngine } from '../../tandem/js/TPhetioStateEngine.js';
+import TinyProperty from '../../axon/js/TinyProperty.js';
+import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
 
 // constants
 const DELETED_VALUE = 'DELETED'; // this is copied in documentation for PhetioEngineIO.getChangedState(), and used in Studio.
@@ -48,6 +49,8 @@
   alreadyCalledForThisStateSet?: boolean;
 };
 
+const isSettingPhetioStateProperty = new TinyProperty( false );
+
 class PhetioStateEngine implements TPhetioStateEngine {
 
   // emits before state the state is set. Emits with the state object that will be set. Use the
@@ -86,6 +89,8 @@
     hasListenerOrderDependencies: true // We rely on Properties undeferring value first (since everything else just notifies).
   } );
 
+  public static readonly isSettingPhetioStateProperty: TReadOnlyProperty<boolean> = isSettingPhetioStateProperty;
+
   public constructor( private readonly phetioEngine: TPhetioEngine, providedOptions?: PhetioStateEngineOptions ) {
 
     const options = optionize<PhetioStateEngineOptions>()( {
Index: projectile-data-lab/js/common/view/HistogramNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/projectile-data-lab/js/common/view/HistogramNode.ts b/projectile-data-lab/js/common/view/HistogramNode.ts
--- a/projectile-data-lab/js/common/view/HistogramNode.ts	(revision b00659aac770fd068b6c23828495f3b1ec3305a0)
+++ b/projectile-data-lab/js/common/view/HistogramNode.ts	(date 1715379466140)
@@ -258,7 +258,7 @@
       }
     };
 
-    isSettingPhetioStateProperty.addListener( () => updateHistogram( true ) );
+    isSettingPhetioStateProperty.link( () => updateHistogram( true ) );
 
     // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes.
     fields.forEach( field => {
Index: tandem/js/isSettingPhetioStateProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tandem/js/isSettingPhetioStateProperty.ts b/tandem/js/isSettingPhetioStateProperty.ts
--- a/tandem/js/isSettingPhetioStateProperty.ts	(revision a444e3f516f4c52eea41f71d87e4051450fa1c1c)
+++ b/tandem/js/isSettingPhetioStateProperty.ts	(date 1715379079447)
@@ -7,9 +7,9 @@
  */
 
 import tandemNamespace from './tandemNamespace.js';
-import TinyProperty from '../../axon/js/TinyProperty.js';
+import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js';
 
-const isSettingPhetioStateProperty = new TinyProperty( false );
+const isSettingPhetioStateProperty = PhetioStateEngine.isSettingPhetioStateProperty;
 
 tandemNamespace.register( 'isSettingPhetioStateProperty', isSettingPhetioStateProperty );
 
Index: projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts
--- a/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts	(revision b00659aac770fd068b6c23828495f3b1ec3305a0)
+++ b/projectile-data-lab/js/sampling/view/SampleSizeThumbnailNode.ts	(date 1715379483432)
@@ -187,7 +187,7 @@
       }
     };
 
-    isSettingPhetioStateProperty.addListener( updateHistogram );
+    isSettingPhetioStateProperty.link( updateHistogram );
 
     // Similar to code in VSMScreenView that updates the angle tool node and speed tool node when the data changes.
     fields.forEach( field => {

In PhetioStateEngine:

const isSettingPhetioStateProperty = new TinyProperty( false );
...
  public static readonly isSettingPhetioStateProperty: TReadOnlyProperty<boolean> = isSettingPhetioStateProperty;

Note that this requires PDL to use link instead of addListener -- which seems highly desirable. isSettingPhetioStateProperty was formerly of type TinyProperty, which unfortunately inherits addListener from TinyEmitter.

@samreid @zepumph please review, commit if this seems OK.

samreid · 2024-05-10T22:38:33Z

This proposed line is incompatible with our constraints:

In tandem/js/isSettingPhetioStateProperty.ts

import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js';

phet-io cannot be imported from tandem, because phet-io is a private repo and not everyone (such as open source clients) can clone it.

pixelzoom · 2024-05-10T22:45:22Z

Does it also follow that sims cannot import PhetioStateEngine.ts to use PhetioStateEngine.isSettingPhetioStateProperty?

samreid · 2024-05-10T22:58:49Z

Correct, public code cannot statically import private code. Instead, we use a dynamic import in simLauncher like so: https://github.com/phetsims/joist/blob/main/js/simLauncher.ts#L23-L31

pixelzoom · 2024-05-10T23:04:53Z

Then I guess your patch in #311 (comment) is as good as it's going to get. I'm OK with that.

pixelzoom · 2024-05-13T15:51:01Z

@zepumph are you OK with @samreid's patch in #311 (comment)?

Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>

…ndem#311 Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>

zepumph · 2024-05-13T17:41:00Z

Yes perfect. Thanks for discussing. I committed with @samreid this morning.

samreid · 2024-05-13T18:33:50Z

Thanks @zepumph, @pixelzoom want to review or close?

pixelzoom · 2024-05-13T19:25:45Z

👍🏻

pixelzoom added dev:phet-io dev:typescript labels May 10, 2024

pixelzoom assigned samreid and zepumph May 10, 2024

samreid unassigned zepumph May 10, 2024

samreid assigned pixelzoom and zepumph and unassigned samreid May 10, 2024

samreid added the status:ready-for-review label May 10, 2024

pixelzoom assigned samreid and unassigned pixelzoom May 10, 2024

pixelzoom removed the status:ready-for-review label May 10, 2024

pixelzoom changed the title ~~Public API for isSettingStateProperty should be TReadOnlyProperty<boolean>.~~ Public API for isSettingStateProperty should be read-only. May 10, 2024

pixelzoom added the status:ready-for-review label May 10, 2024

samreid assigned pixelzoom and unassigned samreid May 10, 2024

pixelzoom assigned samreid May 10, 2024

pixelzoom removed the status:ready-for-review label May 10, 2024

samreid removed their assignment May 10, 2024

pixelzoom removed their assignment May 13, 2024

samreid self-assigned this May 13, 2024

zepumph added a commit that referenced this issue May 13, 2024

isSettingPhetioStateProperty is readonly for most usages, #311

27c3e2c

Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>

zepumph added a commit to phetsims/projectile-data-lab that referenced this issue May 13, 2024

isSettingPhetioStateProperty is readonly for most usages, phetsims/ta…

4d9eec0

…ndem#311 Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>

zepumph added the status:ready-for-review label May 13, 2024

zepumph removed their assignment May 13, 2024

samreid assigned pixelzoom and unassigned samreid May 13, 2024

pixelzoom closed this as completed May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public API for `isSettingStateProperty` should be read-only. #311

Public API for `isSettingStateProperty` should be read-only. #311

pixelzoom commented May 10, 2024 •

edited

Loading

samreid commented May 10, 2024

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading

samreid commented May 10, 2024

pixelzoom commented May 10, 2024

samreid commented May 10, 2024

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 13, 2024

zepumph commented May 13, 2024

samreid commented May 13, 2024

pixelzoom commented May 13, 2024

Public API for isSettingStateProperty should be read-only. #311

Public API for isSettingStateProperty should be read-only. #311

Comments

pixelzoom commented May 10, 2024 • edited Loading

samreid commented May 10, 2024

pixelzoom commented May 10, 2024 • edited Loading

pixelzoom commented May 10, 2024 • edited Loading

pixelzoom commented May 10, 2024 • edited Loading

samreid commented May 10, 2024

pixelzoom commented May 10, 2024

samreid commented May 10, 2024

pixelzoom commented May 10, 2024 • edited Loading

pixelzoom commented May 13, 2024

zepumph commented May 13, 2024

samreid commented May 13, 2024

pixelzoom commented May 13, 2024

Public API for `isSettingStateProperty` should be read-only. #311

Public API for `isSettingStateProperty` should be read-only. #311

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading

pixelzoom commented May 10, 2024 •

edited

Loading