Rename Action, instances of {Action}, and Action.emit()

[zepumph]

From #222 and its review, we need a few renames. @pixelzoom wrote:

@zepumph said:

• Rename Action.js

+1 if you come up with a better name. PhetioAction?

• Rename Action.emit() and perhaps separate that from Emitter.emit().

+1. Something like "perform" or "do" seems more appropriate.

• Rename Action instances that are named like this.mouseUpEmitter = new Action(. . .;

+1. Defer until Action name stabilizes.

I think that a Phetio prefix may be superfluous, but I'm open to suggestions. This will effect the public phet-io api, marking as blocks publication. @samreid @chrisklus what do you think they should be named. Should we bring it to dev meeting?

[samreid]

Action is described as:

An action that can be executed and sent to the PhET-iO data stream, and optionally recorded for playback. This type will also validate the argument types passed to the action function.

I think it would be redundant and inconsistent to name as PhetioAction. After all, Property and Circle are fully instrumented for PhET-iO, but we do not call them PhetioProperty and PhetioCircle.

[zepumph]

At developer meeting we decided on Action and, as a method, execute.

[zepumph]

• I should make sure to cover handlePlaybackEvent too.

[zepumph]

Discussed yesterday during phet-io team meeting. The next step is to take the "composition" approach for a test drive. I would like to have @samreid along when exploring this.

[zepumph]

Discussed in dev meeting today, @pixelzoom wants to publish ph-scale before too long. We decided this doesn't need to block publication currently, but once we do the work to convert from inheritance to composition it should block publication until things get sorted out.

We will let @pixelzoom know when we want to work on this, and perhaps hold off on this until he has taken rc shas.

[pixelzoom]

@zepumph said:

... We will let @pixelzoom know when we want to work on this, and perhaps hold off on this until he has taken rc shas

I have published 1.3.0-rc.1 for ph-scale and ph-scale-basics using master shas, so all clear to proceed.

[samreid]

After spending a few minutes on this, it's unclear to me whether composition will be an improvement. For instance, Emitter extends PhetioObject and contains Action leads to complication with respect to the options. Which phet-io options do we send to the supertype and which do we send to the Action? Will we need to support both? Why make this API more complicated by splitting out Action options from Emitter options? Maybe we should rationalize this by saying that when you execute an Emitter that is synonymous with emitting.

For the sake of argument, say we have a Mammal type which has method speak. Cat overrides speak to have console.log('meow'), and we decide to also add Cat.meow(), which calls this.speak(). What's the problem here? Having two ways to do the same thing?

Running this past @zepumph as a first line of defense before asking @pixelzoom.

UPDATE: I tried using Action via composition anyways and ran into the problem of the data stream. Action extends PhetioObject and Emitter extends PhetioObject. They cannot have the same phetioID and we do not want all emitters to show in the data stream like simulation.screen1.model.ageChangedEmitter.action executed. So it seems incorrect to use composition for this. We can either (a) leave it as it is or (b) look at factoring out the validation and phet-io even further so Emitter could use that code without having to use Action.

[zepumph]

I think I agree with most of the above sentiment. I also think that emitters should not expose execute as a function. Here having two ways to do the same thing is a maintainability concern, for example right now an emitter executing and emitting may be the same thing, but not always in the future. Also it will be harder to update actions if some execute methods pertain to a sub type.

I propose manually cutting out action related API in emitter that we deem implementation details. This will probably just look like an override with an error.

A note about your composition attempt. It was my understanding that when making this change, emitter would not extend PhetioObject, and so any phetio related options would either become nested (less ideal), or be passed to the action, and the action would be the only instrumented piece. That is also less than ideal because we would then probably want to support emitter instances calling phetio object methods.

So far I like keeping Inheritance most.

[samreid]

@pixelzoom can you review the preceding comments and recommend a course of action?

[pixelzoom]

@samreid said:

... They cannot have the same phetioID and we do not want all emitters to show in the data stream like simulation.screen1.model.ageChangedEmitter.action executed.

Why not? That seems both correct and accurate.

[samreid]

That seems to be leaking an implementation detail. Why should clients need to know or care to know what Emitters contains Actions?

[pixelzoom]

If we weren't concerned with PhET-iO, I doubt that we'd be having this discussion about inheritance vs composition. Composition is the clear choice here, and we seemed to agree on that awhile back in the comments. But now we're rationalizing ("Maybe we should rationalize this by saying...") that inheritance is the way to go. I don't think PhET-iO should weigh so heavily on our decisions about whether to use inheritance or composition, and I'm afraid that this may indicate a design flaw.

[samreid]

Composition is the clear choice here, and we seemed to agree on that awhile back in the comments

I'd love to hear a response to the Mammal argument in #243 (comment).

[pixelzoom]

If I understand the Mammal argument...

You're saying that meow calls speak and adds no new functionality, therefore having a Cat either meow or speak is correct. I.e.

meow() { this.speak(); }

And the analogy applies to Emitter because emit calls execute and adds no new functionality. And that is currently true:

emit() {
  this.execute.apply( this, arguments );
}

Problems with this analogy...

First, it has maintenance consequences. It's impossible to enforce that meow is equivalent (and remains equivalent) to speak. Ditto for emit and execute.

Second, it's bad design. Adding aliases for speak like Cat.meow, Dog.bark, etc. would be a misguided attempt at realism. It doesn't improve the implementation and has negative maintenance consequences. If you allow meow and speak to be used interchangeably, you've accomplished nothing other than creating a future problem.

Finally, the analogy doesn't hold up when we look at the class hierarchies. Yes, a Cat is a Mammal, so Cat extends Mammal is without a doubt correct. Ditto for Dog extends Mammal. And Cat and Dog make sense as sibling subclasses. Now let's look at Emitter and Action. Emitter is not clearly a type of Action. Action provides PhET-iO functionality. Emitter is not a way to specialize the role of Action, it's a way to notify listeners using Action to record that notification in the data stream. And if I have Emitter extends Action and SomethingElse extends Action, you will have a difficult time convincing me that Emitter and SomethingElse makes sense as sibling classes.

[zepumph]

Here is the way I think we should proceed:

• Have Emitter extend Action, with everything implemented as is for the most part
• Create a stub function Emitter.execute to make sure that Emitter instances don't bypass the Emitter.emit api. Emitter.execute will throw an error when called.

Rationale:

• Emitter.emit is a prolific api across the project. It is nice and people are used to it, there is no reason to change that. The implementation of Emitter.emit() currently calls over to Action for the majority of its functionality, but there is no reason to leak that into the public api.
• By keeping inheritance, we save big on the phet-io front. If we used composition, then all phet-io options would need to be passed to Action, and the Emitter wouldn't actually be instrumented. This means we would either need to create forwarding methods, or do something like Emitter.action.tandem.supplied or Emitter.action.isPhetioInstrumented. This is a deal breaker to me. One of the main reasons to split these two up is to make more modular code and responsibility for instrumented Emitters. This tradeoff is balance off of the only con I have heard so far: that it exposes Action methods that we don't want to be exposed. execute is the only public method, seems much simpler to manage that. We also haven't mentioned that Action.validators makes sense to have directly on Emitter. This was rambley, here is a summary: While it may seem like using composition is "hiding implementation detail," I argue that in fact inheritance is a better way to hide the implementation detail.

[samreid]

It seems we should have a discussion on this, marking for dev meeting--or maybe @pixelzoom @samreid and @zepumph can schedule a mini sub-meeting?

[pixelzoom]

As a counter analogy... Rectangle extends Node seems natural. Node extends Events is questionable and has been discussed more than once. Emitter extends Action seems more similar to the latter than the former.

[zepumph]

Developer meeting consensus from today (from SR):

We will keep it the way it is because it is cheap and easy, adding the override to Emitter.execute to error out when called. If this in inadequate in the future we can investigate the refactor to use composition more thoroughly.

[zepumph]

Comitted above. @samreid please review.

[samreid]

This seems to be a reasonable compromise, the change set looks good. Closing.