Preferences Properties should not be instrumented in sims where they are irrelevant.

phetsims / faradays-electromagnetic-lab

"Faraday's Electromagnetic Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.

GNU General Public License v3.0

1 stars 0 forks source link

Preferences Properties should not be instrumented in sims where they are irrelevant. #160

Closed pixelzoom closed 7 months ago

pixelzoom commented 7 months ago

Noticed while working on #136 ...

Currently, all Preferences Properties are instrumented and featured in all sims in the FEL suite. These elements should not be instrumented in sims where they are not relevant:

global.model.preferences.addEarthCheckboxProperty
global.model.preferences.currentFlowProperty
global.model.preferences.earthHemisphereProperty

@arouinfar FYI.

pixelzoom commented 7 months ago

Hmm... I'm not sure how I'm going to do this when these Properties are all on global. The Properties are created as singletons, there's no way to pass anything to them, and this is the current PhET-iO convention. I'll probably need to discuss with @arouinfar.

pixelzoom commented 7 months ago

This was a big change, touching 47 files. In order to conditionally instrument Preferences Properties, I had to convert FELPreferences to a class (instead of a collection of globals), instantiate an instance of FELPreferences for each sim, and pass that instances (or specific Properties) through constructors.

@arouinfar please review. For all sims in the suite:

Look under global.model.preferences in Studio, verify that only relevant Properties appear.
Look in the Preferences > Simulation, verify that only relevant controls appear, and that their initial state is correct.

arouinfar commented 7 months ago

This was a big change, touching 47 files.

A big change indeed!!

@arouinfar please review.

I reviewed as requested, and all sims have the appropriate properties visible under global.model.preferences and have the correct controls under Preferences > SImulation with the desired defaults.

zepumph commented 7 months ago

This comment may have come just a bit late, but In Buoyancy, we used the packageJSON.name to determine what preferences were instrumented statically. I'm going to reopen so @pixelzoom can see this comment, but feel free to close with no work.

https://github.com/phetsims/density-buoyancy-common/blob/4d5609c071c4a270755bbecf60f5a74532eb3c0f/js/common/model/DensityBuoyancyCommonPreferences.ts#L16

https://github.com/phetsims/molecule-shapes/blob/f393695d2ff5ab3d0eaf96964ea7beb9ed4af837/js/common/MoleculeShapesGlobals.js#L21

pixelzoom commented 7 months ago

Thanks, I see how using packageJSON.name would’ve saved me some time. But I’m not wild about the pattern we’ve been using for Preferences model/view. Or using packageJSON.name to configure what perhaps should not be globals in the first place. But I don’t feel strongly that you shouldn’t use that pattern.