consolidate code for forwarding events

[jbphet]

We've been using a pattern for a while where a node in some sort of toolbox creates a new node and then forwards subsequent input events to the created node. There was recently a need to make touch snag work for this, and the code was implemented - and essentially duplicated - in CCK and Expression Exchange. This should be consolidated into a new object to avoid code duplication. The relevant code in CCK is in CoinTermCreatorNode, and currently looks like this:

    this.addInputListener( {

      down: function( event ) {

        // ignore this if already dragging
        if ( event.pointer.dragging ) { return; }

        // don't try to start drags with a right mouse button or an attached pointer
        if ( !event.canStartPress() ) { return; }

        // Determine the origin position of the new element based on where the creator node is.  This is done so that
        // the position to which this element will return when it is "put away" will match the position of this creator
        // node.
        var originPosition = expressionManipulationView.globalToLocalPoint( self.localToGlobalPoint( Vector2.ZERO ) );

        // Determine the initial position where this element should move to after it's created based on the location of
        // the pointer event.
        var initialPosition = expressionManipulationView.globalToLocalPoint( event.pointer.point );

        // create and add the new coin term to the model, which result in a node being created in the view
        var createdCoinTerm = coinTermCreatorFunction( typeID, {
          initialPosition: originPosition,
          initialCount: options.createdCoinTermInitialCount,
          decomposable: options.createdCoinTermDecomposable,
          initiallyOnCard: options.onCard
        } );
        createdCoinTerm.setPositionAndDestination( initialPosition );
        expressionManipulationModel.addCoinTerm( createdCoinTerm );

        // get the view node that should have appeared in the view so that events can be forwarded to its drag handler
        var createdCoinTermView = expressionManipulationView.getViewForCoinTerm( createdCoinTerm );
        assert && assert( createdCoinTermView, 'unable to find coin term view' );

        if ( createdCoinTermView ) {

          // forward the event to the view node's drag handler
          createdCoinTermView.dragHandler.startDrag( event );
        }
      },

      // touch enters this node
      touchenter: function( event ) {
        this.down( event );
      },

      // touch moves over this node
      touchmove: function( event ) {
        this.down( event );
      }

    } )

@samreid - can you note where this pattern was used in CCK, and then we can decide who will consolidate.

[jbphet]

Here is the related issue from Expression Exchange: phetsims/expression-exchange#121

[jonathanolson]

How about:

node.addInputListener( SimpleDragHandler.createForwardingListener( {
  down: function( event ) {
    // no need to check event.pointer.dragging or canStartPress here
  },
  touchSnag: {boolean}
  // no need to do touchenter/touchmove here
} ) );

[samreid]

@samreid - can you note where this pattern was used in CCK, and then we can decide who will consolidate.

CircuitElementToolbox.js and SensorToolbox.js (the 2 toolboxes in circuit-construction-kit-common) have this pattern, you can search for touchenter to see it in action.

SimpleDragHandler.createForwardingListener

I'm not sure how that would work exactly--does it return a plain object or a SimpleDragHandler? If the former, then it seems like it would work OK but would be a bit odd that SimpleDragHandler.createSomething doesn't return a SimpleDragHandler. And we discussed a few days ago that SimpleDragHandler doesn't support multi-interaction.

[jonathanolson]

I'm not sure how that would work exactly

createForwardingListener: function( listener ) {
  return {
    down: function( event ) {
      if ( !event.pointer.dragging && event.canStartPress() ) {
        listener.down( event );
      }
    },
    touchenter: function( event ) {
      listener.touchSnag && listener.down( event );
    },
    touchmove: function( event ) {
      listener.touchSnag && listener.down( event );
    }
  };
}

would be a bit odd that SimpleDragHandler.createSomething doesn't return a SimpleDragHandler.

It's totally SimpleDragHandler-specific with the check for dragging. Would a better function name help?

And we discussed a few days ago that SimpleDragHandler doesn't support multi-interaction.

Yup, it supports preventing it!

[samreid]

Understood, thanks for clarifying, that sounds great!

[samreid]

I tried the solution proposed above and I'm getting double drags:

I thought the event.pointer.pressed would have prevented that, but something's wrong.

[samreid]

Touchenter and touchmove should be:

        touchenter: function( event ) {
          dragHandler.touchSnag && this.down( event );
        },
        touchmove: function( event ) {
          dragHandler.touchSnag && this.down( event );
        }

So they can short circuit.

[samreid]

I added SimpleDragHandler.createForwardingListener in the preceding commit. I also separated out the touchSnag into the options and renamed as allowTouchSnag for consistency with SimpleDragHandler. @jonathanolson can you please review? @jbphet can you please try this out for your sim?

[jonathanolson]

Don't see it pushed to master (checked https://github.com/phetsims/scenery/blob/master/js/input/SimpleDragHandler.js). I only see the CCK commit.

[jonathanolson]

I thought the event.pointer.pressed would have prevented that, but something's wrong.

Sorry about the bug in my implementation!

[samreid]

No problem, it should be pushed now.

[pixelzoom]

I'm unclear on how the addition of SimpleDragHandler.createForwardingListener affects the general creator pattern that we started with in phetsims/scenery-phet#214. Does it change this bit? If so, how?

        if ( createdCoinTermView ) {

          // forward the event to the view node's drag handler
          createdCoinTermView.dragHandler.startDrag( event );
        }

[samreid]

The pattern is changed. The old pattern was:

 this.addInputListener( {

      down: function( event ) {

        // ignore this if already dragging
        if ( event.pointer.dragging ) { return; }

        // don't try to start drags with a right mouse button or an attached pointer
        if ( !event.canStartPress() ) { return; }

        createSomething();

        // forward the event to the view node's drag handler
        {{something}}.dragHandler.startDrag( event );
      },

      // touch enters this node
      touchenter: function( event ) {
        this.down( event );
      },

      // touch moves over this node
      touchmove: function( event ) {
        this.down( event );
      }

    } )

The new pattern is:

    this.addInputListener( SimpleDragHandler.createForwardingListener( {
      down: function( event ) {

        createSomething();
        {{something}}.dragHandler.startDrag(event);
      }
    }, {
      allowTouchSnag: true
    } ) );

Caveat and disclaimer: createForwardingListener is new and I don't understand it completely. But I am using it in 2 spots in CCK.

[pixelzoom]

According to @jonathanolson via a Skype call:

(1) He didn't review the implementation of createForwardingListener, and it's not correct. The first argument is definitely not a generic input listener. You must specify: a down function which has one parameter {Event} event; and optionally allowTouchSnag. So we need to decide what the function signature should be, fix it, then fix call sites in CCK.

down is required, allowTouchSnag is optional. So my feeling is that implementation should be:

/**
 * Creates an input listener that forwards events to the specified input listener
 * See https://github.com/phetsims/scenery/issues/639
 * @param {function(Event)} down - down function to be added to the input listener
 * @param {Object} [options]
 * @returns {Object} a scenery input listener
 */
createForwardingListener: function( down, options ) {

      options = _.extend( {
        allowTouchSnag: false
      }, options );

      return {
        down: function( event ) {
          if ( !event.pointer.dragging && event.canStartPress() ) {
            down( event );
          }
        },
        touchenter: function( event ) {
          options.allowTouchSnag && this.down( event );
        },
        touchmove: function( event ) {
          options.allowTouchSnag && this.down( event );
        }
      };
}

@jonathanolson (if I understood) prefers createForwardingListener( options ), where down is a "required option", because formatting is nicer at call sites. I'm not a fan of putting required arguments in options.

(2) Use of createForwardingListener makes these 2 checks unnecessary in the down function:

        // ignore this if already dragging
        if ( event.pointer.dragging ) { return; }

        // don't try to start drags with a right mouse button or an attached pointer
        if ( !event.canStartPress() ) { return; }

(3) This information needs to be folded into phetsims/scenery-phet#214. Other developers are looking to that issue for guidance about how to apply the creator pattern (phetsims/projectile-motion#20 (comment) being one of many examples).

[jonathanolson]

@jonathanolson (if I understood) prefers createForwardingListener( options ), where down is a "required option", because formatting is nicer at call sites. I'm not a fan of putting required arguments in options.

After thinking about it, I agree with not having the callback in options, and with the two parameters.

@pixelzoom's changes look great to me, any objections @samreid?

[samreid]

Those changes look great to me, thanks @pixelzoom! Since I have (the only?) code that uses createForwardingListener, I can take it for a test drive.

[samreid]

I pushed the changes in the preceding 2 commits and they are working nicely.

[samreid]

Reassigning to @pixelzoom for review.

[pixelzoom]

This specific code changes look good. But 2 remaining problems:

(1) The way in which this information was folded into the recommend pattern (see phetsims/scenery-phet#214 (comment)) is insufficient, as I've noted in that issue.

(2) This API works only if your input listener involves only down. If your input listener needs to do something on (for example) up, there's no way to specify that.

@jonathanolson your thoughts on (2) ?

[jonathanolson]

We're unlikely to need something that works on up, but could create something when that need arises.

Maybe we add a "down" qualifier to the function name?

[samreid]

@andrea-phet is reviewing phetsims/scenery-phet#214, I'm not sure what else to do for this issue. @pixelzoom can you please advise?

[pixelzoom]

Remaining issues to address:

(1) phetsims/scenery-phet#214 contains no code sample that summarizes how createForwardingListener is used in the creator pattern.

(2) It's unclear to me why createForwardingListener lives in SimpleDragHandler. It creates a generic input listener, not a SimpleDragHandler.

(3) createForwardingListener creates a limited input listener by wrapping a specific down function. A more general interface would allow you to pass in a general input listener (sans touchenter and touchmove) and return a new general input listener that wraps the down function. In #639 (comment) @jonathanolson indicates that this generalization isn't necessary, but I'm not convinced.

[jonathanolson]

(2) It's unclear to me why createForwardingListener lives in SimpleDragHandler. It creates a generic input listener, not a SimpleDragHandler.

The "event.pointer.dragging" check is actually specific to SimpleDragHandler. Sorry I hadn't thought about it before, but once #218 is finished (i.e. SimpleDragHandler uses the Pointer attach API), then the "dragging" check would be unneeded, as the canStartPress() would then work.

Thus once that's done, we CAN make the forwarding listener more general (so it would work for DragListener), and it would live in a different place (probably listeners/ForwardingListener in Scenery).

Since this would generally be beneficial before we "standardize" a pattern, I'd like to look into doing this first.

In #639 (comment) @jonathanolson indicates that this generalization isn't necessary, but I'm not convinced.

That comment looks like I'm saying we won't need an "up" variety, not referencing other input listener types that aren't SimpleDragHandlers (since usually in the down callback, you need to create the object that will have a drag listener anyways). I'd like to hear more specifics about what type of generalization you mean.

[samreid]

@pixelzoom and @jonathanolson I'll leave this up to you for now, but please invite me back if/when you need me.

[jonathanolson]

Approximately 2/3rds of simulations are erroring out when I make listeners "attach", since it then becomes an assertion failure to have the same pointer have two SimpleDragHandlers/DownUpListeners to listen to it at the same time.

I'll need to look more into it later, it won't be an easy quick fix to be able to skip checking the SimpleDragHandler "dragging" flag.

[pixelzoom]

Sounds like @jonathanolson has something in mind in #639 (comment). Feel free to assign to me and @samreid for review when you're ready.