Add emitters to SimpleDragHandler

[zepumph]

This will take over the events from SimpleDragHandlerIO, and we can then delete that type.

We can then uninstrument SimpleDragHandler and its sub emitters can be children of the draggable node.

[zepumph]

This doesn't need to be done for FL stable because FL will be converted to use DragListener in phetsims/faradays-law#117

[samreid]

I tried this and there's a problem. SimpleDragHandler currently has:

        self.phetioStartEvent( 'dragged', {
          x: event.pointer.point.x,
          y: event.pointer.point.y
        }, HIGH_FREQUENCY_OPTIONS );

And if we convert to Emitter we will lose the ability to indicate

  var HIGH_FREQUENCY_OPTIONS = {
    highFrequency: true
  };

How should we address this? Would the high frequency options become an Emitter phetio option? Assigned to @zepumph for discussion, but feel free to escalate to a meeting label if you think that's appropriate.

[zepumph]

It seems like these options need to be promoted to a phetio prefixed option, i,e: phetioHighFrequencyEvents: true or something.

Then it should be passed to the phetioStartEvent calls in Emitter.emit() (because that will be the only method soon.)

Before we were worried about boxing us into "one type always either fully opts in or has to be completely out." Maybe we can convince ourselves that for Emitter that is ok, since Emitters only ever should be emitting for one single task, i.e. one specific phet-io event.

[samreid]

It looks like the other option key in PhetioObject.phetioStartEvent is phetioEventType, should it also be moved to the PhetioObject constructor options?

[zepumph]

It already is!

[samreid]

Sounds good.

It seems like these options need to be promoted to a phetio prefixed option

Also, we can add that to the instanceMetadata as we did for phetioEventType. I'll work on this soon.

[samreid]

Keep in mind the drag event should be high frequency.

[zepumph]

Graphing Quadratics should not go out until we convert this to use the new emitter pattern. Marking high priority.

[samreid]

I think the "move everything to the Emitter" as exemplified in Input.js seems the cleanest (and maybe even removing the adapter methods altogether, maybe not), but I'm somewhat concerned that other developers may not like that pattern at all. Maybe we should discuss it with @jonathanolson and @pixelzoom? What do you think @zepumph?

[zepumph]

We are pretty heavily invested in this approach at this point, and it doesn't seem to be causing issues atm. I think we are good to proceed. Could we either (a) move the scenery logging out of the emitters in SimpleDragHandler, or (b) move logging into the emitters in PressListener.

I think I prefer the first option, what do you think?

[pixelzoom]

@samreid said:

... Maybe we should discuss it with @jonathanolson and @pixelzoom? ...

I am so out of the loop on this (I don't even know why we're adding Emitters to SImpleDragHandler) that I have nothing no feedback.

[samreid]

I would like to move the entire body of press() to the _pressedEmitter, but unfortunately, it has a return value which is not supported by Emitter. However, I looked through all usages of .press( event and didn't see any places where the return value is used, so maybe clients that really need that feature can call this.canPress( event ) on their own? Also, I'd like to eliminate the awkward onPress adapter, and just put the entire function body directly in _pressedEmitter: listener. Can we chat on these topics before moving forward?

[zepumph]

so maybe clients that really need that feature can call this.canPress( event ) on their own?

That is a question for @jonathanolson, I feel like part of that has to do with subtyping. Note how children also have that capability. In FireListener, the value is also returned, though in DragListener it isn't used.

This may not be related, but Mouse.js has listeners that return values too. Perhaps this is a pattern used in scenery for some input related reasons? But so far my investigation didn't yield much, see Input.dispatchToListeners for where I think those functions are called, and nothing is being done with their return values.

@jonathanolson why does PressListener.press return a value?

EDIT:

Can we chat on these topics before moving forward?

Sure thing!

[jonathanolson]

@jonathanolson why does PressListener.press return a value?

Since the subtype "overridden" press can see whether the press succeeded or not before continuing with other behavior. Its usage is:

var success = PressListener.prototype.press.call( this, event, targetNode, function() { /*...*/ } );

[zepumph]

That makes sense to me, assigning to @samreid for discussion.

[samreid]

@samreid said:

I'm somewhat concerned that other developers may not like that pattern at all.

@zepumph said:

We are pretty heavily invested in this approach at this point, and it doesn't seem to be causing issues atm. I think we are good to proceed.

@pixelzoom said:

I am so out of the loop on this (I don't even know why we're adding Emitters to SImpleDragHandler) that I have nothing no feedback.

I'll create a separate issue to review our patterns for inlining/instrumenting Emitters and see if the development team has questions or feedback.

Since the subtype "overridden" press can see whether the press succeeded or not before continuing with other behavior.

@jonathanolson are you aware of current usages of this, or is this mainly planning for potential future usage?

[pixelzoom]

@samreid said:

I'm somewhat concerned that other developers may not like that pattern at all.

I fall heavily in that category. See my feedback in #928 (comment).

[zepumph]

I think we have a clear step forward for this issue outlined in #928 (comment).

In addition it sounds like we need to keep the functionality of press's return value, so we should keep the content of pressedEmitter's callback (listener?) the way it is.

[samreid]

SimpleDragHandler is using Emitters in the style that we discussed in depth at today's PhET-iO meeting. In #928 (comment) we will update the pattern to use before instead of listener.

PressListener cannot put all of the logic in the Emitters since it requires return values, so the pattern it is using now seems reasonable. Technically, we could move the logic to listener for release but then it wouldn't match style for press so it is a wash.

Therefore I recommend we close this issue and continue in #928

@zepumph sound OK?

[zepumph]

Sounds good to me