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

A better API to supply sim-specific preferences into any Preferences tab #835

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Currently we have createSimControls which is a single node that goes onto the general tab, but on Friday @jessegreenberg and I discussed the potentential need to have sim-specfic settings that don't map into general, but rather other tabs. I wanted to prototype with an option to preferencesConfiguration like:

{
  tab: PreferencesTab.INPUT
  createNode: ()=> new MyContent()
{

And update the PreferencesDialog to append these into whatever tab is needed. This will immediately be helpful over in https://github.com/phetsims/ratio-and-proportion/issues/498. And likely more in the future as well.

zepumph commented 1 year ago

This is blocking publication of RAP I believe.

zepumph commented 1 year ago

Also good to tag https://github.com/phetsims/joist/issues/837

zepumph commented 1 year ago

@jessegreenberg and I like:

{
  customPreferences: [
    {
      tab: PreferencesTabs.INPUT,
      createContent: ( tandem: Tandem ) => new MyContent(),
    }
  ]
}
jessegreenberg commented 1 year ago
Progress Patch ```patch Index: joist/js/preferences/PreferencesPanels.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/PreferencesPanels.ts b/joist/js/preferences/PreferencesPanels.ts --- a/joist/js/preferences/PreferencesPanels.ts (revision 39fb5a6c859382701b1b74fc2bbd134f32fabf1e) +++ b/joist/js/preferences/PreferencesPanels.ts (date 1661208473740) @@ -67,7 +67,9 @@ let visualPreferencesPanel: Node | null = null; if ( supportedTabs.includes( PreferencesType.VISUAL ) ) { - visualPreferencesPanel = new VisualPreferencesPanel( preferencesModel.visualModel ); + visualPreferencesPanel = new VisualPreferencesPanel( preferencesModel.visualModel, { + tandem: options.tandem.createTandem( 'visualPreferencesPanel' ) + } ); const visualBox = panelAlignGroup.createBox( visualPreferencesPanel ); this.addChild( visualBox ); this.content.push( new PreferencesPanelContainer( visualPreferencesPanel, PreferencesType.VISUAL ) ); @@ -75,7 +77,9 @@ let audioPreferencesPanel: Node | null = null; if ( supportedTabs.includes( PreferencesType.AUDIO ) ) { - audioPreferencesPanel = new AudioPreferencesPanel( preferencesModel.audioModel ); + audioPreferencesPanel = new AudioPreferencesPanel( preferencesModel.audioModel, { + tandem: options.tandem.createTandem( 'audioPreferencesPanel' ) + } ); const audioBox = panelAlignGroup.createBox( audioPreferencesPanel ); this.addChild( audioBox ); this.content.push( new PreferencesPanelContainer( audioPreferencesPanel, PreferencesType.AUDIO ) ); @@ -83,7 +87,9 @@ let inputPreferencesPanel: Node | null = null; if ( supportedTabs.includes( PreferencesType.INPUT ) ) { - inputPreferencesPanel = new InputPreferencesPanel( preferencesModel.inputModel ); + inputPreferencesPanel = new InputPreferencesPanel( preferencesModel.inputModel, { + tandem: options.tandem.createTandem( 'inputPreferencesPanel' ) + } ); const inputBox = panelAlignGroup.createBox( inputPreferencesPanel ); this.addChild( inputBox ); this.content.push( new PreferencesPanelContainer( inputPreferencesPanel, PreferencesType.INPUT ) ); @@ -91,7 +97,9 @@ let localizationPreferencesPanel: Node | null = null; if ( supportedTabs.includes( PreferencesType.LOCALIZATION ) ) { - localizationPreferencesPanel = new LocalizationPreferencesPanel( preferencesModel.localizationModel ); + localizationPreferencesPanel = new LocalizationPreferencesPanel( preferencesModel.localizationModel, { + tandem: options.tandem.createTandem( 'localizationPreferencesPanel' ) + } ); const localizationBox = panelAlignGroup.createBox( localizationPreferencesPanel ); this.addChild( localizationBox ); this.content.push( new PreferencesPanelContainer( localizationPreferencesPanel, PreferencesType.LOCALIZATION ) ); Index: joist/js/preferences/GeneralPreferencesPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/GeneralPreferencesPanel.ts b/joist/js/preferences/GeneralPreferencesPanel.ts --- a/joist/js/preferences/GeneralPreferencesPanel.ts (revision 39fb5a6c859382701b1b74fc2bbd134f32fabf1e) +++ b/joist/js/preferences/GeneralPreferencesPanel.ts (date 1661205824403) @@ -11,6 +11,7 @@ import { Node, VBox, VBoxOptions, VoicingRichText } from '../../../scenery/js/imports.js'; import HSeparator from '../../../sun/js/HSeparator.js'; import Tandem from '../../../tandem/js/Tandem.js'; +import tandem from '../../../tandem/js/Tandem.js'; import joist from '../joist.js'; import joistStrings from '../joistStrings.js'; import PreferencesDialog from './PreferencesDialog.js'; @@ -72,6 +73,14 @@ let simControls: Node | null = null; let simControlsPanelSection: Node | null = null; + if ( generalModel.customPreferences.length > 0 ) { + generalModel.customPreferences.forEach( customPreference => { + providedChildren.push( + new PreferencesPanelSection( { contentNode: customPreference.createContent( options.tandem ) } ) + ); + } ); + } + if ( generalModel.createSimControls ) { simControls = generalModel.createSimControls( options.tandem ); simControlsPanelSection = new SimControlsPanelSection( simControls ); Index: joist/js/preferences/VisualPreferencesPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/VisualPreferencesPanel.ts b/joist/js/preferences/VisualPreferencesPanel.ts --- a/joist/js/preferences/VisualPreferencesPanel.ts (revision 39fb5a6c859382701b1b74fc2bbd134f32fabf1e) +++ b/joist/js/preferences/VisualPreferencesPanel.ts (date 1661208718190) @@ -18,6 +18,7 @@ import { VisualModel } from './PreferencesModel.js'; import optionize, { EmptySelfOptions } from '../../../phet-core/js/optionize.js'; import ProjectorModeToggleSwitch from './ProjectorModeToggleSwitch.js'; +import Tandem from '../../../tandem/js/Tandem.js'; // constants const interactiveHighlightsString = joistStrings.preferences.tabs.visual.interactiveHighlights; @@ -32,6 +33,7 @@ public constructor( visualModel: VisualModel, providedOptions?: NodeOptions ) { const options = optionize()( { + tandem: Tandem.OPTIONAL, // pdom tagName: 'div', @@ -39,6 +41,8 @@ labelContent: 'Visual' }, providedOptions ); + const tandem = options.tandem; + options.tandem = Tandem.OPTIONAL; super( options ); const contentNode = new VBox( { @@ -70,6 +74,14 @@ contentNode.addChild( interactiveHighlightsEnabledSwitch ); } + if ( visualModel.customPreferences.length > 0 ) { + visualModel.customPreferences.forEach( customPreference => { + contentNode.addChild( new Node( { + children: [ customPreference.createContent( options.tandem ) ] + } ) ); + } ); + } + const panelSection = new PreferencesPanelSection( { contentNode: contentNode, Index: ratio-and-proportion/js/ratio-and-proportion-main.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/ratio-and-proportion/js/ratio-and-proportion-main.ts b/ratio-and-proportion/js/ratio-and-proportion-main.ts --- a/ratio-and-proportion/js/ratio-and-proportion-main.ts (revision 0f018c73ffa7ce5090a15a08942e0ca079a3c9d2) +++ b/ratio-and-proportion/js/ratio-and-proportion-main.ts (date 1661207767091) @@ -14,6 +14,7 @@ import DiscoverScreen from './discover/DiscoverScreen.js'; import ratioAndProportionStrings from './ratioAndProportionStrings.js'; import SimControlsNode from './common/view/SimControlsNode.js'; +import { RichText } from '../../scenery/js/imports.js'; const ratioAndProportionTitleString = ratioAndProportionStrings[ 'ratio-and-proportion' ].title; @@ -27,8 +28,8 @@ }, hasKeyboardHelpContent: true, preferencesModel: new PreferencesModel( { - generalOptions: { - createSimControls: () => new SimControlsNode() + audioOptions: { + customPreferences: [ { createContent: () => new RichText( 'Hello there!' ) }, { createContent: () => new SimControlsNode() } ] } } ) }; Index: joist/js/preferences/PreferencesModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/PreferencesModel.ts b/joist/js/preferences/PreferencesModel.ts --- a/joist/js/preferences/PreferencesModel.ts (revision 39fb5a6c859382701b1b74fc2bbd134f32fabf1e) +++ b/joist/js/preferences/PreferencesModel.ts (date 1661205631717) @@ -22,11 +22,19 @@ import Tandem from '../../../tandem/js/Tandem.js'; import localeProperty from '../localeProperty.js'; +type CustomPreference = { + createContent: ( parentTandem: Tandem ) => Node; +}; + +type CustomPreferencesOptions = { + customPreferences?: CustomPreference[]; +}; + type GeneralPreferencesOptions = { // Creates any Node you would like on the "General" tab. createSimControls?: ( ( tandem: Tandem ) => Node ) | null; -}; +} & CustomPreferencesOptions; type VisualPreferencesOptions = { @@ -36,7 +44,7 @@ // whether the sim supports the "Interactive Highlights" feature, and checkbox to enable in the // Preferences Dialog supportsInteractiveHighlights?: boolean; -}; +} & CustomPreferencesOptions; type AudioPreferencesOptions = { @@ -49,13 +57,13 @@ // included if supportsSound is also true. supportsSound?: boolean; supportsExtraSound?: boolean; -}; +} & CustomPreferencesOptions; type InputPreferencesOptions = { // Whether to include "gesture" controls supportsGestureControl?: boolean; -}; +} & CustomPreferencesOptions; type LocalizationPreferencesOptions = { @@ -67,7 +75,7 @@ // provided, the Localization tab will include a UI component to swap out pieces of artwork to match the selected // region and culture. RegionAndCultureDescriptors contains information for the UI component to describe each choice. regionAndCultureDescriptors?: RegionAndCultureDescriptor[]; -}; +} & CustomPreferencesOptions; type PreferencesModelSelfOptions = { @@ -163,23 +171,28 @@ phetioReadOnly: true }, providedOptions ) ), generalOptions: optionize()( { - createSimControls: null + createSimControls: null, + customPreferences: [] }, providedOptions.generalOptions ), visualOptions: optionize()( { supportsProjectorMode: false, - supportsInteractiveHighlights: phetFeatures.supportsInteractiveHighlights + supportsInteractiveHighlights: phetFeatures.supportsInteractiveHighlights, + customPreferences: [] }, providedOptions.visualOptions ), audioOptions: optionize()( { supportsVoicing: phetFeatures.supportsVoicing, supportsSound: phetFeatures.supportsSound, - supportsExtraSound: phetFeatures.supportsExtraSound + supportsExtraSound: phetFeatures.supportsExtraSound, + customPreferences: [] }, providedOptions.audioOptions ), inputOptions: optionize()( { - supportsGestureControl: phetFeatures.supportsGestureControl + supportsGestureControl: phetFeatures.supportsGestureControl, + customPreferences: [] }, providedOptions.inputOptions ), localizationOptions: optionize()( { supportsMultipleLocales: !!localeProperty.validValues && localeProperty.validValues.length > 1, - regionAndCultureDescriptors: [] + regionAndCultureDescriptors: [], + customPreferences: [] }, providedOptions.localizationOptions ) }; @@ -195,7 +208,8 @@ tandem: visualTandem.createTandem( 'interactiveHighlightsEnabledProperty' ), phetioState: false, phetioReadOnly: true - } ) + } ), + customPreferences: options.visualOptions.customPreferences }; // For now, the Voicing feature is only available when we are running in the english locale, accessibility @@ -225,7 +239,9 @@ voiceRateProperty: voicingManager.voiceRateProperty, voiceProperty: voicingManager.voiceProperty, - toolbarEnabledProperty: new BooleanProperty( true ) + toolbarEnabledProperty: new BooleanProperty( true ), + + customPreferences: options.audioOptions.customPreferences }; const inputTandem = options.tandem.createTandem( 'inputModel' ); @@ -235,14 +251,17 @@ tandem: inputTandem.createTandem( 'gestureControlsEnabledProperty' ), phetioState: false, phetioReadOnly: true - } ) + } ), + customPreferences: options.inputOptions.customPreferences }; this.localizationModel = { supportsMultipleLocales: options.localizationOptions.supportsMultipleLocales, regionAndCultureProperty: localizationManager.regionAndCultureProperty, - regionAndCultureDescriptors: options.localizationOptions.regionAndCultureDescriptors + regionAndCultureDescriptors: options.localizationOptions.regionAndCultureDescriptors, + + customPreferences: options.localizationOptions.customPreferences }; Index: joist/js/preferences/AudioPreferencesPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/joist/js/preferences/AudioPreferencesPanel.ts b/joist/js/preferences/AudioPreferencesPanel.ts --- a/joist/js/preferences/AudioPreferencesPanel.ts (revision 39fb5a6c859382701b1b74fc2bbd134f32fabf1e) +++ b/joist/js/preferences/AudioPreferencesPanel.ts (date 1661208409594) @@ -6,7 +6,7 @@ * @author Jesse Greenberg (PhET Interactive Simulations) */ -import { HBox, Node, Text, VBox, VBoxOptions } from '../../../scenery/js/imports.js'; +import { HBox, Text, VBox, VBoxOptions } from '../../../scenery/js/imports.js'; import joist from '../joist.js'; import joistStrings from '../joistStrings.js'; import PreferencesDialog from './PreferencesDialog.js'; @@ -14,6 +14,7 @@ import PreferencesToggleSwitch from './PreferencesToggleSwitch.js'; import SoundPanelSection from './SoundPanelSection.js'; import VoicingPanelSection from './VoicingPanelSection.js'; +import PreferencesPanelSection from './PreferencesPanelSection.js'; // constants const audioFeaturesString = joistStrings.preferences.tabs.audio.audioFeatures.title; @@ -27,10 +28,12 @@ */ public constructor( audioModel: AudioModel, providedOptions?: VBoxOptions ) { - const panelChildren: Node[] = []; + const contentOptions: VBoxOptions = { align: 'left', spacing: PreferencesPanelSection.DEFAULT_ITEM_SPACING }; + const leftContent = new VBox( contentOptions ); + const rightContent = new VBox( contentOptions ); if ( audioModel.supportsVoicing ) { - panelChildren.push( new VoicingPanelSection( audioModel ) ); + leftContent.addChild( new VoicingPanelSection( audioModel ) ); } if ( audioModel.supportsSound ) { @@ -40,14 +43,21 @@ // through the "Audio Features" toggle only. const hideSoundToggle = audioModel.supportsVoicing !== audioModel.supportsSound; - panelChildren.push( new SoundPanelSection( audioModel, { + rightContent.addChild( new SoundPanelSection( audioModel, { includeTitleToggleSwitch: !hideSoundToggle } ) ); } const sections = new HBox( { align: 'top', - children: panelChildren + children: [ leftContent, rightContent ] + } ); + + audioModel.customPreferences.forEach( ( customPreference, i ) => { + const container = i % 2 === 0 ? leftContent : rightContent; + container.addChild( + new PreferencesPanelSection( { contentNode: customPreference.createContent( ), contentLeftMargin: 0 } ) + ); } ); const allAudioSwitch = new PreferencesToggleSwitch( audioModel.simSoundEnabledProperty, false, true, { ```
zepumph commented 1 year ago

Lots of good progress here!

Each preferences type now has its own customPreferences to add. We are using it in RAP to provides input preferences, and ESP and Quad for general controls.

I think we are ready to unblock https://github.com/phetsims/joist/issues/837

zepumph commented 1 year ago

@jessegreenberg, can you please review and recommend next steps?

jessegreenberg commented 1 year ago

Great, thanks. I think this will support us well for now. I noticed disposal missing for a couple of components in tabs and I added them for completion though I don't think they were critical since these tabs don't appear in state (to my understanding). I also found a layout issue in the localization tab where the "Region and Culture" combobox wasn't in the right spot when it was alone in the dialog. Fixed in https://github.com/phetsims/joist/commit/b1b4e78437be9051a5801ee16dc2b46a6589bba6.

Closing this one.