phetsims / center-and-variability

"Center and Variability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 2 forks source link

Replace variabilityModelResetInProgressProperty with isResettingProperty #612

Open pixelzoom opened 9 months ago

pixelzoom commented 9 months ago

I ran into this while looking for "sound" exemplars for FEL.

Why is VariabilityModel variabilityModelResetInProgressProperty needed? Is seems redundant, given that you have global isResettingProperty. It looks like it was added by @samreid for https://github.com/phetsims/center-and-variability/issues/236 (sounds) in https://github.com/phetsims/center-and-variability/commit/c7bcb7e85bb90ecb032a7bccfda89502366b9f86. Recommended to replace it with isResettingProperty.

Relevant code...

isResettingProperty.ts:

const isResettingProperty = new BooleanProperty( false );
export default isResettingProperty;

CAVScreenView.ts:

    this.resetAllButton = new ResetAllButton( {
      listener: () => {
        assert && assert( !isResettingProperty.value, 'cannot reset while already resetting' );

        isResettingProperty.value = true;
        this.interruptSubtreeInput(); // cancel interactions that may be in progress
        model.reset();
        isResettingProperty.value = false;
      },

VariabilityScreenView.ts:

export default class VariabilityScreenView extends CAVScreenView {
...
  public constructor( model: VariabilityModel, providedOptions: VariabilityScreenViewOptions ) {
...
    this.intervalToolNode = new IntervalToolNode( model.intervalToolModel, model.variabilityModelResetInProgressProperty,

VariabilityModel.ts:

  // Whether the variability model is currently in the process of resetting. Used to handle intermediate states.
  public readonly variabilityModelResetInProgressProperty = new BooleanProperty( false );

  public override reset(): void {
    this.variabilityModelResetInProgressProperty.value = true;
...
    this.variabilityModelResetInProgressProperty.value = false;
  }
samreid commented 9 months ago

Here is a patch that addresses the recommendation above:

```diff Subject: [PATCH] Update assertion message, see https://github.com/phetsims/projectile-data-lab/issues/141 --- Index: js/variability/view/VariabilityScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/variability/view/VariabilityScreenView.ts b/js/variability/view/VariabilityScreenView.ts --- a/js/variability/view/VariabilityScreenView.ts (revision e4ecf93986d47c3c4e9a893b1ff1b64f215d3e12) +++ b/js/variability/view/VariabilityScreenView.ts (date 1707882805957) @@ -91,8 +91,7 @@ variabilityRadioButtonGroupWrapper.top = accordionBoxWrapper.top + 8; } ); - this.intervalToolNode = new IntervalToolNode( model.intervalToolModel, model.variabilityModelResetInProgressProperty, - this.modelViewTransform, new DerivedProperty( [ variabilityAccordionBox.boundsProperty ], bounds => bounds.top ), + this.intervalToolNode = new IntervalToolNode( model.intervalToolModel, this.modelViewTransform, new DerivedProperty( [ variabilityAccordionBox.boundsProperty ], bounds => bounds.top ), options.tandem.createTandem( 'intervalToolNode' ) ); // To avoid a cycle during startup, we must create the AccordionBox and IntervalToolNode, then propagate the IntervalToolNode Index: js/variability/model/VariabilityModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/variability/model/VariabilityModel.ts b/js/variability/model/VariabilityModel.ts --- a/js/variability/model/VariabilityModel.ts (revision e4ecf93986d47c3c4e9a893b1ff1b64f215d3e12) +++ b/js/variability/model/VariabilityModel.ts (date 1707882849921) @@ -52,9 +52,6 @@ // Whether the pointer is currently being dragged by the keyboard. Used to avoid conflicts between keyboard and mouse/touch interaction. public readonly isPointerKeyboardDraggingProperty: Property; - // Whether the variability model is currently in the process of resetting. Used to handle intermediate states. - public readonly variabilityModelResetInProgressProperty = new BooleanProperty( false ); - // Emitter that is fired on reset public readonly resetEmitter = new Emitter(); @@ -163,7 +160,6 @@ } public override reset(): void { - this.variabilityModelResetInProgressProperty.value = true; super.reset(); this.selectedVariabilityMeasureProperty.reset(); @@ -177,8 +173,6 @@ this.isPointerVisibleProperty.reset(); this.resetEmitter.emit(); - - this.variabilityModelResetInProgressProperty.value = false; } } Index: js/variability/view/IntervalToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/variability/view/IntervalToolNode.ts b/js/variability/view/IntervalToolNode.ts --- a/js/variability/view/IntervalToolNode.ts (revision e4ecf93986d47c3c4e9a893b1ff1b64f215d3e12) +++ b/js/variability/view/IntervalToolNode.ts (date 1707882849915) @@ -31,6 +31,7 @@ import intervalToolLoop_wav from '../../../sounds/intervalToolLoop_wav.js'; import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import isResettingProperty from '../../../../soccer-common/js/model/isResettingProperty.js'; export default class IntervalToolNode extends Node { @@ -39,7 +40,7 @@ private readonly continuousPropertySoundGenerator: ContinuousPropertySoundGenerator; - public constructor( model: IntervalToolModel, resetInProgressProperty: BooleanProperty, modelViewTransform: ModelViewTransform2, accordionBoxTopProperty: TReadOnlyProperty, tandem: Tandem ) { + public constructor( model: IntervalToolModel, modelViewTransform: ModelViewTransform2, accordionBoxTopProperty: TReadOnlyProperty, tandem: Tandem ) { const handle1 = new IntervalToolPredictionSlider( model.handle1ValueProperty, modelViewTransform, CAVConstants.VARIABILITY_DRAG_RANGE, model.isHandle1BeingMouseDraggedProperty, model.isHandle1BeingKeyboardDraggedProperty, { @@ -155,7 +156,7 @@ initialOutputLevel: 0.25, playbackRateCenterOffset: 0, - resetInProgressProperty: resetInProgressProperty, + resetInProgressProperty: isResettingProperty, trimSilence: false, // a very precise sound file is used, so make sure it doesn't get changed fadeTime: 0.3, delayBeforeStop: 0.25, ```

@marlitas can you please review? Should we commit this? How should we test it?

marlitas commented 3 months ago

This sim is no longer actively being worked on. Unassigning.