how to dispose of something that is created by a factory method?

[pixelzoom]

This problem was encountered in https://github.com/phetsims/vegas/issues/59#issuecomment-378430997 ...

LevelSelectioNode constructor looks like this:

/**
 * @param {Node} icon Scenery node that appears on the button above the score display...
 * @param {Node} scoreDisplay - displays the score
 * @param {Object} [options]
 * @constructor
 */
function LevelSelectionItemNode( icon, scoreDisplay, options ) {

In general, scoreDisplay can be any Node, and the client is responsible for managing that Node.

There are 3 common types of scoreDisplay, so (as a convenience) LevelSelectioNode has static factory methods for creating a LevelSelectionNode with a specific type of scoreDisplay. Here's one example; the other 2 are similar:

/**
 * Convenience function to create a LevelSelectionItemNode with a ScoreDisplayDiscreteStars.
 * @param {Node} icon
 * @param {Property.<number>} scoreProperty
 * @param {Object} [options] - see LevelSelectionItemNode and ScoreDisplayDiscreteStars
 * @public
 * @static
 */
createWithScoreDisplayDiscreteStars: function( icon, scoreProperty, options ) {
  options = _.extend( { ... }, options );
  var scoreDisplay = new ScoreDisplayDiscreteStars( scoreProperty, .. );
  return new LevelSelectionItemNode( icon, scoreDisplay, ... ) );
},

In this case, the factory function creates ScoreDisplayDiscreteStars then passes it to LevelSelectionItemNode. When the LevelSelectionItemNode is disposed of, the ScoreDisplayDiscreteStars must be disposed too, since it links to scoreProperty. But nothing knows about the ScoreDisplayDiscreteStars; and LevelSelectionItemNode doesn't own it, so shouldn't dispose of it.

One thought is to do something like this:

createWithScoreDisplayDiscreteStars: function( icon, scoreProperty, options ) {
  options = _.extend( { ... }, options );
  var scoreDisplay = new ScoreDisplayDiscreteStars( scoreProperty, ... );
  var levelSelectionItemNode = new LevelSelectionItemNode( icon, scoreDisplay, ... ); 
+ levelSelectionItemNode.disposeEmitter.addListener( function() {
+   scoreDisplay.dispose();
+ } );
  return levelSelectionItemNode;
}

... but there's currently no such thing as disposeEmitter in the Node type hierarchy.

This seems like a general problem. Suggestions on how to address this?

[jonathanolson]

there's currently no such thing as disposeEmitter in the Node type hierarchy.

I'm fine with adding that, since having LevelSelectionItemNode dispose the scoreDisplay directly isn't great (since it could be a reused node).

I'm not personally a fan (at least if it forces another file), but you could turn the factor method into its own subtype with an inherit call (override dispose). Small inline types wouldn't be the worst thing if it solves this type of problem.

[jessegreenberg]

Could LevelSelectionItemNode directly dispose of scoreDisplay if that function has been implemented?

this.disposeLevelSelectionItemNode = function() {
  scoreDisplay.dispose && scoreDisplay.dispose();
}

[pixelzoom]

@jessegreenberg asked:

Could LevelSelectionItemNode directly dispose of scoreDisplay if that function has been implemented? ...

No, because scoreDisplay is created by the client, and may be reused by the client. In general, if you didn't create it, you don't dispose of it.

[pixelzoom]

@jonathanolson suggested:

... Small inline types wouldn't be the worst thing if it solves this type of problem.

I'd rather not monkey with adding disposeEmitter. So perhaps I'll investigate inline types, if there's agreement that that is a good general solution.

[pixelzoom]

Here's what createWithScoreDisplayDiscreteStars might look like with an inline type definition:

    createWithScoreDisplayDiscreteStars: function( icon, scoreProperty, options ) {

      options = _.extend( {
        listener: null, // {function|null} called when the button is pressed
        scoreDisplayOptions: null // see ScoreDisplayDiscreteStars options
      }, options );

      function WrapperType( icon, scoreProperty, options ) {
        // @private
        this.scoreDisplay = new ScoreDisplayDiscreteStars( scoreProperty, options.scoreDisplayOptions );
        LevelSelectionItemNode.call( this, icon, scoreDisplay, _.omit( options, [ 'scoreDisplayOptions' ] ) )
      }

      inherit( LevelSelectionItemNode, WrapperType, {
        dispose: function() {
          this.scoreDisplay.dispose();
          LevelSelectionItemNode.prototype.dispose.call( this );
        }
      } );

      return new WrapperType( icon, scoreDisplay, options );
    },

[jonathanolson]

I was expecting no factory function, you would directly create an instance, e.g.:

LevelSelectionNode.DiscreteStarSelectionNode = function( icon, scoreProperty, options ) {

  options = _.extend( {
    listener: null, // {function|null} called when the button is pressed
    scoreDisplayOptions: null // see ScoreDisplayDiscreteStars options
  }, options );

  // @private
  this.scoreDisplay = new ScoreDisplayDiscreteStars( scoreProperty, options.scoreDisplayOptions );
  LevelSelectionItemNode.call( this, icon, scoreDisplay, _.omit( options, [ 'scoreDisplayOptions' ] ) )
};

inherit( LevelSelectionNode, LevelSelectionNode.DiscreteStarSelectionNode, {
  dispose: function() {
    this.scoreDisplay.dispose();
    LevelSelectionItemNode.prototype.dispose.call( this );
  }
} );

and to create:

new LevelSelectionNode.DiscreteStarSelectionNode( ... );

[pixelzoom]

4/5/18 dev meeting notes: Go with https://github.com/phetsims/vegas/issues/62#issuecomment-379039398

[pixelzoom]

@andrea-phet Let's apply the approach in https://github.com/phetsims/vegas/issues/62#issuecomment-379039398 to LevelSelectionItemNode.

[pixelzoom]

@jonathanolson Rather than having 3 different convenience types for the 3 different score display types, what do you think of the approach shown below? This approach relies on the fact that all 3 score display types have the same constructor signature.

  var ScoreDisplayDiscreteStars = require( 'VEGAS/ScoreDisplayDiscreteStars' );
  var ScoreDisplayNumberAndStar = require( 'VEGAS/ScoreDisplayNumberAndStar' );
  var ScoreDisplayTextAndNumber = require( 'VEGAS/ScoreDisplayTextAndNumber' );

  var VALID_SCORE_DISPLAY_CONSTRUCTORS = [
    // all constructors must have the same signature!
    ScoreDisplayDiscreteStars, ScoreDisplayNumberAndStar, ScoreDisplayTextAndNumber
  ];
  /**
   * Convenience constructor for creating a LevelSelectionItemNode with a specific type of scoreDisplay.
   * Instantiation and disposal of the scoreDisplay instance is opaque to the client.
   * @param {Node} icon - appears on the button above the score display, scaled to fit
   * @param {Property.<number>} scoreProperty
   * @param {Object} [options]
   * @constructor
   */
  LevelSelectionItemNode.ScoreDisplayCreator = function( icon, scoreProperty, options ) {

    options = _.extend( {
      listener: null, // {function|null} called when the button is pressed
      scoreDisplayConstructor: ScoreDisplayDiscreteStars, // {constructor} for creating scoreDisplay
      scoreDisplayOptions: null // see scoreDisplay's constructor
    }, options );

    assert && assert( _.includes( VALID_SCORE_DISPLAY_CONSTRUCTORS, options.ScoreDisplayConstructor,
      'invalid ScoreDisplayConstructor: ' + options.ScoreDisplayConstructor ) );

    // @private all constructors must have the same signature!
    this.scoreDisplay = new options.scoreDisplayConstructor( scoreProperty, options.scoreDisplayOptions );

    LevelSelectionItemNode.call( this, icon, this.scoreDisplay, _.omit( options, [ 'scoreDisplayOptions' ] ) );
  };

  vegas.register( 'LevelSelectionItemNode.ScoreDisplayCreator', LevelSelectionItemNode.ScoreDisplayCreator );

  inherit( LevelSelectionItemNode, LevelSelectionItemNode.ScoreDisplayCreator, {

    // @public
    dispose: function() {
      this.scoreDisplay.dispose();
      LevelSelectionItemNode.prototype.dispose.call( this );
    }
  } );

[jonathanolson]

I'm curious about whether a factory method (instead of passing in a constructor and options) would work?

Also, are those the only score displays supported (since it checks for those)? What if I want to have a score display specific to a sim?

That said I'm fine with it as written.

[pixelzoom]

@jonathanolson asked:

I'm curious about whether a factory method (instead of passing in a constructor and options) would work?

Factory methods could create the score display, but don't support disposing of the score display. That's how we ended up with a type - so we could override dispose.

Also, are those the only score displays supported (since it checks for those)? What if I want to have a score display specific to a sim?

If you're using the LevelSelectionButton constructor directly, then the parameter is {Node} scoreDisplay. You can pass in anything you want, but you're responsible for managing it.

If you're using the convenience constructor, then you're limited to the 3 common-code ScoreDisplay* types.

[jonathanolson]

If you're using the convenience constructor, then you're limited to the 3 common-code ScoreDisplay* types.

Sounds fine to me.