phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

Remove parts of PreferencesModel that are confusing because of Tandem.OPT_OUT. #938

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

This is an example of why it's potentially dangerous to simply add migration rules without understanding/documenting why they are being added.

While working on https://github.com/phetsims/phet-io/issues/1963#event-10350028993, we wondered about these rules that appear in many sims:

    new TandemFragmentDelete( `${simName}.general.model.preferencesModel.visualModel.colorProfileProperty` ),
    new TandemFragmentDelete( `${simName}.general.model.preferencesModel.localizationModel.localeProperty` ),

And there are similar sim-specific rules, like these for beers-law-lab:

    new TandemFragmentDelete( `${simName}.general.model.preferencesModel.simulationModel.showSoluteAmountProperty` ),
    new TandemFragmentDelete( `${simName}.general.model.preferencesModel.simulationModel.showSolutionVolumeProperty` ),

Sims have been using the modelLinkables option to specify Properties that should appear under ${simName}.general.model.preferencesModel.simulationModel. For example, in BLLSim.ts:

      preferencesModel: new PreferencesModel( {
        simulationOptions: {
...
            modelLinkables: [
              { property: BLLPreferences.showSoluteAmountProperty },
              { property: BLLPreferences.showSolutionVolumeProperty }
            ]
          } ]
        }
      } ),

That feature was apparently short-circuited by work in https://github.com/phetsims/phet-io/issues/1913. Those link are no longer being created because PreferencesModel was changed to opt-out of ALL instrumentation. In PreferencesModel.ts:

        tandem: Tandem.OPT_OUT, // Uninstrumented for now, but keep the file's instrumentation just in case, see https://github.com/phetsims/phet-io/issues/1913

We also noticed that occurrences of modelLinkables are now incomplete -- for example, beers-law-lab is now missing 2 preferences Properties (beakerUnitsProperty and concentrationMeterUnitsProperty). And newer sims have been ignoring modelLinkables altogether (it's only used in 5 sims).

We concluded that we should: (1) Remove usages of modelLinkables (2) remove modelLinkables from the API, because keeping dead/unexercised code around is confusing, and (in this case) not likely to provide much benefit. (3) Clearly/consistently document the rules related to this in all migration.ts files.

And one additional task that I discovered: (4) Since PreferencesModel is opting out of ALL instrumentation, are there other parts of the API that are like modelLinkables, and should also be removed?

I'll handle (1) above. Assigning to @zepumph and @samreid to handle (2), (3), and (4).

pixelzoom commented 1 year ago

In the above commits, I removed usages of modelLinkables.

I also added this temporary assertion to PreferencesModel addPhetioLinkedElementsForModel, to prevent further uses of modelLinkables before this GitHub issue is completed:

      //TODO https://github.com/phetsims/joist/issues/938 remove the modelLinkables options
      assert && assert( !customPreference.modelLinkables, 'modelLinkables is not supported, see https://github.com/phetsims/joist/issues/938' );
samreid commented 1 year ago

@zepumph there are 6 occurrences of modelLinkables in 1 file. Should I remove them? Will any migration rules be necessary here? I thought not, but wasn't sure why it was mentioned above.

zepumph commented 1 year ago

Sounds good. I'm still not too interested in pulling out all the addLinkedElement logic from PreferencesModel, but please do tell me if you want me to do it and I will gladly.

samreid commented 1 year ago

@jbphet @zepumph and I discussed this today and feel it is in good shape to be closed.