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

Rename simSoundEnabledProperty to audioEnabledProperty #864

Closed samreid closed 1 year ago

samreid commented 1 year ago

From https://github.com/phetsims/joist/issues/744, @zepumph said:

simSoundEnabledProperty is a weird name, and a weird linkedElement pointing back to the audioEnabledProperty, let's rename it throughout the preferences model to audioEnabledProperty.

samreid commented 1 year ago

When I started on this, I had 2 questions:

After this change, it would be

export type AudioModel = BaseModelType & {
  audioEnabledProperty: Property<boolean>;
  soundEnabledProperty: Property<boolean>;

Are the semantics for audio vs sound sufficiently well described? Is it confusing to have audioEnabled right next to soundEnabled?

Second, I saw numerous occurrences of simSoundEnabledProperty in the sim Quadrilateral and wasn't sure if those should also be renamed.

@jessegreenberg can you please comment on these points?

jessegreenberg commented 1 year ago

Yes, this rename makes sense to me and matches PhET's definition of "audio" and "sound". Distinctions between each audio feature are in audioManager.ts. Id like to flesh out the documentation for these model types.

Second, I saw numerous occurrences of simSoundEnabledProperty in the sim Quadrilateral

Thanks, I renamed all of those to a more sim-specific name.

jessegreenberg commented 1 year ago

I added some documentation for the AudioModel. Back to @samreid for the rename and the other PhET-iO changes described, but feel free to reassign if you would like me to do it.

samreid commented 1 year ago

Fixed, tested in studio. All seems well, closing.

samreid commented 1 year ago

Discovered in https://github.com/phetsims/chipper/issues/946, there are TODOs remaining in the code. Reopening.

jessegreenberg commented 1 year ago

Thanks, the TODO was

TODO: To be renamed to audioEnabledProperty, see https://github.com/phetsims/joist/issues/864

simply the pointer to the work of this issue which is now done. Removed, closing.