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.
MIT License
8 stars 6 forks source link

Add linked elements for global Properties in PreferencesModel #840

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

While @pixelzoom and I were talking about https://github.com/phetsims/joist/issues/837, we realized it would be awesome to add a new field to customPreferences over in https://github.com/phetsims/joist/issues/835 where you could provide PhET-iO instrumented Proeprties to be linked to under the Preferences model of the provided customPreference.

@jessegreenberg, I'm so excited about this!

pixelzoom commented 1 year ago

For the benefit of designers (@arouinfar @amanda-phet @kathy-phet)... This means adding linked elements under general.model.preferencesModel. For example, for geometric-optics:

screenshot_1822
zepumph commented 1 year ago

Let's add the localeProperty here too while we are at it, and perhaps we will have a couple more from the general Preferences as we went. (see https://github.com/phetsims/joist/issues/847 for implementation)

zepumph commented 1 year ago

Alright. localeProperty and colorProfileProperty were both accessed by the view globally, so I moved them into the PreferencesModel. I also created a general way to pass in Properties from custom preferences that you want linked. By default, they take the tandemName of the Property they are linking to, but you can provide a custom tandemName if you'd like.

I only implemented this for geometric-optics, and I thought I'd ask @pixelzoom to confirm the design before I pass through the rest of them throughout sim-specific custom Preferences.

image

samreid commented 1 year ago

Now that https://github.com/phetsims/chipper/issues/1313 has been addressed, we can use a better type for LinkedModelProperties.property, I'll commit momentarily.

pixelzoom commented 1 year ago

Looks nice -- I verified in geometric-optics, and it presents the way that I expected in Studio.

The API is a little verbose, for example in GOSim.ts:

            linkedModelProperties: [
              { property: GOPreferences.focalLengthModelTypeProperty },
              { property: GOPreferences.add2FPointsCheckboxProperty },
              { property: GOPreferences.cueingArrowsEnabledProperty }
            ]

But I understand that linkedModelProperties is LinkedModelProperties[] so that we can optionally specify tandems for each linked element.

The one nit I have is that the names linkedModelProperties and LinkedModelProperties are a bit misleading, makes it sound like this is an array of Property, when in fact it's more complicated.

zepumph commented 1 year ago

Yeah, I couldn't think of anything better while implementing, but I agree with you.

modelPropertyLinkages
propertiesToLink
supplementalPreferencesProperties
linkToModelPropertiesData
supplementalPropertyData
linkToMeProperties
dependantLinkables
modelLinkables
modelPropertyLinkables

I doesn't have to be Properties technically, but there is likely no reason to have any other PhetioObjects linked here, so perhaps we should with modelLinkables or modelPropertyLinkables. Thoughts?

pixelzoom commented 1 year ago

type ModelPropertyLinkable and modelLinkables: ModelPropertyLinkable[] would be an improvement. I can't think of better names.

zepumph commented 1 year ago

Sounds great thanks! I like it much more.

pixelzoom commented 1 year ago

Reopening - did you mean to close this before changing the names? I still see linkedModelProperties?: LinkedModelProperties[] in PreferencesModel.ts and call sites.

zepumph commented 1 year ago

I'm having a real pushing problem this week. Thanks for keeping me honest.