Abstraction for adding listeners to observables of the paper land model

[jessegreenberg]

One of the more complicated parts of program code right now is creating "observer" program code that listens for changes to "observable" program code in another program. The "observer" and "observable" could be detected in any order and that needs to be handled well. Currently this requires complicated listeners in program code. This should be abstracted in board.

Brainstorming notes so far:

/**
 * @param componentName {string} - name of the model component you expect to be in the model
 * @param handleComponent {function} - function that will be called if the component exists and when it is added to the model
 */
paperLand.addModelObserver( componentName: string, handleComponent: ( component ) => {} )
    handleComponentExists():
        - call handleComponent( component )
        - Add a listener that will call handleComponentDoesNotExist() when component is removed (with self removal)
    handleComponentDoesNotExist():
        - Add a listener that will call handleComponentExists() when the component is added to the model (with self removal)

    - if model component exists, handleComponentExists()
    - if model component does not exist, handleComponentDoesNotExist()

paperLand.removeModelObserver( componentName: string, handleComponent: ( component ) => {} )
    - if model component exists
        call handleComponent( component )

    - Remove any listeners on the global model waiting for the component to be added/removed

Usage could look like this:

Usage might look like this:

// in onProgramAdded
scratchpad.altitudeListener = altitude => {
  console.log( altitude );
}
scratchpad.handleAltitudePropertyAdded = altitudeProperty => {
  altitudeProperty.link( scratchpad.altitudeListener );
}
scratchpad.handleAltitudePropertyRemoved = altitudeProperty => {
  altitudeProperty.unlink( scratchpad.altitudeListener );
}

phet.paperLand.addModelObserver( 'altitudeProperty', scratchpad.altitudeListener );

// then in onProgramRemoved
phet.paperLand.removeModelObserver( 'altitudeProperty', altitude.handleAltitudePropertyRemoved );
delete scratchpad.handleAltitudePropertyAdded;
delete scratchpad.handleAltitudePropertyRemoved;
delete scratchpad.altitudeListener;

One downside of this is that we need handleAltitudePropertyAdded and handleAltitudePropertyRemoved. These handle the link and unlink. If that was done in the addModelObserver they wouldn't be necessary. But I wanted this to support any type of model component (maybe it uses addListener instead of link) or maybe even a variable that isn't observable and doesn't change.

[jessegreenberg]

As part of this, we should change the global modelProperty to be a map. We have Emitters that let us know when something is added and removed now.

[jessegreenberg]

We could have the proposal in https://github.com/phetsims/paper-land/issues/61#issue-1648038297as a general solution, and we could ALSO have a version of it that uses link directly.

[jessegreenberg]

Seems to be working well! But needs a lot more testing. These could be disruptive changes and I didn't get to a commit point but here is progress:

```patch Subject: [PATCH] Use Checkbox directly, remove superclass, see https://github.com/phetsims/gravity-force-lab-basics/issues/324" --- Index: client/board/entry.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/board/entry.js b/client/board/entry.js --- a/client/board/entry.js (revision 6c2425f66b8c5bf9fcaf39f864d82106277bc4dc) +++ b/client/board/entry.js (date 1680217001281) @@ -10,9 +10,9 @@ import React from 'react'; import ReactDOM from 'react-dom'; import styles from './BoardMain.css'; -import paperLand from './paperLand.js'; import PaperLandControls from './PaperLandControls.js'; import SceneryDisplay from './SceneryDisplay.js'; +require( './boardModel.js' ); // constants const DISPLAY_SIZE = new phet.dot.Dimension2( 640, 480 ); @@ -145,63 +145,64 @@ console.warn( 'createAndLoadWrappedAudioBuffer not defined' ); } -// Emits events when model components are added or removed, to be used in program code. Emits with -// {string} - name of the model component -// {*} - Reference to the component being added or removed -paperLand.modelComponentAddedEmitter = new window.phet.axon.Emitter(); -paperLand.modelComponentRemovedEmitter = new window.phet.axon.Emitter(); - -/** - * Adds a model component to the model Object with the provided name. Emits events so client code can observe - * changes to the model. - * @param {string} componentName - * @param {*} componentObject - any model component (Property, or object with multiple Properties and values) - */ -paperLand.addModelComponent = ( componentName, componentObject ) => { - const existingModel = modelProperty.value; - if ( existingModel[ componentName ] === undefined ) { - - // Update the model Property, which is also our map for name -> component - modelProperty.value = { - [ componentName ]: componentObject, - - // spread operator copies existing model into a new object - ...existingModel - }; - - paperLand.modelComponentAddedEmitter.emit( componentName, componentObject ); - } - else { - console.warn( `Model already has component with name ${componentName}` ); - } -}; - -/** - * Remove a component with the provided name from the model. Updates the global modelProperty which is our map - * of all model components and also emits a separate Emitter. - * @param {string} componentName - */ -paperLand.removeModelComponent = componentName => { - const existingModel = modelProperty.value; - const componentObject = existingModel[ componentName ]; - - if ( componentObject === undefined ) { - console.warn( `Model does not have component with name ${componentName}` ); - } - else { - - // delete the object from the global model and then reassign to trigger a Property change - const objectCopy = { ...existingModel }; - delete objectCopy[ componentName ]; - modelProperty.value = objectCopy; +// +// // Emits events when model components are added or removed, to be used in program code. Emits with +// // {string} - name of the model component +// // {*} - Reference to the component being added or removed +// paperLand.modelComponentAddedEmitter = new window.phet.axon.Emitter(); +// paperLand.modelComponentRemovedEmitter = new window.phet.axon.Emitter(); - // emit events, passing the componentObject through so that client can dispose of various objects - paperLand.modelComponentRemovedEmitter.emit( componentName, componentObject ); - - // dispose the component when we are done with it, if supported - componentObject.dispose && componentObject.dispose(); - } -}; +// /** +// * Adds a model component to the model Object with the provided name. Emits events so client code can observe +// * changes to the model. +// * @param {string} componentName +// * @param {*} componentObject - any model component (Property, or object with multiple Properties and values) +// */ +// paperLand.addModelComponent = ( componentName, componentObject ) => { +// const existingModel = modelProperty.value; +// if ( existingModel[ componentName ] === undefined ) { +// +// // Update the model Property, which is also our map for name -> component +// modelProperty.value = { +// [ componentName ]: componentObject, +// +// // spread operator copies existing model into a new object +// ...existingModel +// }; +// +// paperLand.modelComponentAddedEmitter.emit( componentName, componentObject ); +// } +// else { +// console.warn( `Model already has component with name ${componentName}` ); +// } +// }; +// +// /** +// * Remove a component with the provided name from the model. Updates the global modelProperty which is our map +// * of all model components and also emits a separate Emitter. +// * @param {string} componentName +// */ +// paperLand.removeModelComponent = componentName => { +// const existingModel = modelProperty.value; +// const componentObject = existingModel[ componentName ]; +// +// if ( componentObject === undefined ) { +// console.warn( `Model does not have component with name ${componentName}` ); +// } +// else { +// +// // delete the object from the global model and then reassign to trigger a Property change +// const objectCopy = { ...existingModel }; +// delete objectCopy[ componentName ]; +// modelProperty.value = objectCopy; +// +// // emit events, passing the componentObject through so that client can dispose of various objects +// paperLand.modelComponentRemovedEmitter.emit( componentName, componentObject ); +// +// // dispose the component when we are done with it, if supported +// componentObject.dispose && componentObject.dispose(); +// } +// }; // Update the sim design board based on changes to the paper programs. const updateBoard = presentPaperProgramInfo => { Index: client/board/boardModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/board/boardModel.js b/client/board/boardModel.js new file mode 100644 --- /dev/null (date 1680217961467) +++ b/client/board/boardModel.js (date 1680217961467) @@ -0,0 +1,164 @@ +import paperLand from './paperLand.js'; + +// Map - keys are the name of the model component, values are any kind of model component +const boardModel = new Map(); + +const componentNameToAddedListenerMap = new Map(); +const componentNameToRemovedListenerMap = new Map(); + +// Emits events when model components are added or removed, to be used in program code. Emits with two args +// {string} - name of the model component +// {*} - Reference to the component being added or removed +paperLand.modelComponentAddedEmitter = new window.phet.axon.Emitter(); +paperLand.modelComponentRemovedEmitter = new window.phet.axon.Emitter(); + +/** + * Adds a model component to the model Object with the provided name. Emits events so client code can observe + * changes to the model. + * @param {string} componentName + * @param {*} componentObject - any model component (Property, or object with multiple Properties and values) + */ +paperLand.addModelComponent = ( componentName, componentObject ) => { + if ( boardModel.get( componentName ) === undefined ) { + boardModel.set( componentName, componentObject ); + paperLand.modelComponentAddedEmitter.emit( componentName, componentObject ); + } + else { + console.warn( `Model already has component with name ${componentName}` ); + } +}; + +/** + * Remove a component with the provided name from the model. Updates the global modelProperty which is our map + * of all model components and also emits a separate Emitter. + * @param {string} componentName + */ +paperLand.removeModelComponent = componentName => { + const componentObject = boardModel.get( componentName ); + + if ( componentObject === undefined ) { + console.warn( `Model does not have component with name ${componentName}` ); + } + else { + + // delete the object from the global model and then reassign to trigger a Property change + boardModel.delete( componentName ); + + // emit events, passing the componentObject through so that client can dispose of various objects + paperLand.modelComponentRemovedEmitter.emit( componentName, componentObject ); + + // dispose the component when we are done with it, if supported + componentObject.dispose && componentObject.dispose(); + } +}; + +const addListenerToModelChangeEmitter = ( componentName, emitter, listener, listenerMap ) => { + emitter.addListener( listener ); + + if ( !listenerMap.has( componentName ) ) { + listenerMap.set( componentName, [] ); + } + listenerMap.get( componentName ).push( listener ); +}; + +const removeListenerFromModelChangeEmitter = ( componentName, emitter, listener, listenerMap ) => { + emitter.removeListener( listener ); + + if ( !listenerMap.has( componentName ) ) { + throw new Error( 'listenerMap does not have provided listener.' ); + } + else { + const listeners = listenerMap.get( componentName ); + const index = listeners.indexOf( listener ); + if ( index > -1 ) { + listeners.splice( index, 1 ); + + if ( listeners.length === 0 ) { + listenerMap.delete( componentName ); + } + } + else { + throw new Error( 'listener was not in the array for component' ); + } + } +}; + +paperLand.addModelObserver = ( componentName, handleComponentAttach, handleComponentDetach ) => { + + // Component exists in the model - do whatever work is needed on attach and add listeners to watch for when + // the component is removed again. + const handleComponentExists = component => { + handleComponentAttach( component ); + + const componentRemovedListener = removedComponentName => { + if ( componentName === removedComponentName ) { + handleComponentDoesNotExist( component ); + removeListenerFromModelChangeEmitter( componentName, paperLand.modelComponentRemovedEmitter, componentRemovedListener, componentNameToRemovedListenerMap ); + } + }; + addListenerToModelChangeEmitter( componentName, paperLand.modelComponentRemovedEmitter, componentRemovedListener, componentNameToRemovedListenerMap ); + }; + const handleComponentDoesNotExist = component => { + if ( component !== undefined ) { + handleComponentDetach( component ); + } + + const componentAddedListener = ( addedComponentName, addedComponent ) => { + if ( componentName === addedComponentName ) { + handleComponentExists( addedComponent ); + removeListenerFromModelChangeEmitter( componentName, paperLand.modelComponentAddedEmitter, componentAddedListener, componentNameToAddedListenerMap ); + } + }; + addListenerToModelChangeEmitter( componentName, paperLand.modelComponentAddedEmitter, componentAddedListener, componentNameToAddedListenerMap ); + }; + + if ( boardModel.has( componentName ) ) { + + // component already exists in the model, handle it and wait for removal + handleComponentExists( boardModel.get( componentName ) ); + } + else { + + // component does not exist yet, wait for it to be added + handleComponentDoesNotExist(); + } +}; + +paperLand.removeModelObserver = ( componentName, handleComponentDetach ) => { + if ( boardModel.has( componentName ) ) { + handleComponentDetach( boardModel.get( componentName ) ); + } + + // remove any modelComponentAdded/modelComponentRemoved emitter listeners that were added on + // addModelObserver... + const componentAddedListeners = componentNameToAddedListenerMap.get( componentName ); + if ( componentAddedListeners ) { + while ( componentAddedListeners.length > 0 ) { + paperLand.modelComponentAddedEmitter.removeListener( componentAddedListeners.pop() ); + } + } + + const componentRemovedListeners = componentNameToRemovedListenerMap.get( componentName ); + if ( componentRemovedListeners ) { + while ( componentRemovedListeners.length > 0 ) { + paperLand.modelComponentRemovedEmitter.removeListener( componentRemovedListeners.pop() ); + } + } +}; + +paperLand.addModelPropertyLink = ( componentName, listener ) => { + paperLand.addModelObserver( + componentName, + component => component.link( listener ), + component => component.unlink( listener ) + ); +}; + +paperLand.removeModelPropertyLink = ( componentName, listener ) => { + paperLand.removeModelObserver( + componentName, + component => component.unlink( listener ) + ); +}; + +export default boardModel; \ No newline at end of file ```

[jessegreenberg]

This was pushed in the above commit. I tested out in basic programs and also added some unit tests for this functionality in particular. It seemed worthwhile because of the listener complexity needed here. "listener soup" is an appropriate description.

As part of this we changed modelProperty to be a Map. That impacts all existing program code, we need to update them.

[jessegreenberg]

Oops, this isn't working for adding mutliple model observers to the same observable. Not sure why yet, but the implementation should handle that.

EDIT: I added unit tests for this condition and they are passing, so maybe there is something wrong with the program code.

EDIT2: Ah, here is the order of operations that breaks things.

addModelComponent
addModelPropertyLink (first observer)
addModelPropertyLink (secondObserver)

removeModelPropertyLink (firstObserver) ---> incorrectly removes `modelComponentRemovedEmitter` listeners for BOTH observers.

EDIT3: A solution was added and I added unit tests for it. I couldn't think of a good way to support it without complicating program code a little. addModelPropertyLink now returns a unique ID that has to be passed to the removeModelPropertyLink. The ID is then used internally by board to add/remove the internal listeners in a pair of addModelPropertyLink/removeModelPropertyLink calls.

Now to update programs again...

[jessegreenberg]

While porting the IDRC programs I am finding it would be helpful to have similar functions for controllers too so you can modify a Property only when it exists in the model. It could be easy, Ill add it now.

[jessegreenberg]

That was added with unit tests for it. I am sure we will continue to iterate on this and create new abstractions as we identify pain points, but this is working well enough to close.