phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
54 stars 12 forks source link

Support phetioDocumentation for input listeners #1166

Open pixelzoom opened 3 years ago

pixelzoom commented 3 years ago

In molecule-polarity, TriatomicMoleculeNode has 5 DragListeners, each of which manipulates some aspect of a complicated molecule. Because DragListener is not a PhetioObject, it's currently not possible to add phetioDocumentation to describe these listeners. So there can be no description of these listeners displayed in Studio.

Here's what the TriatomicMoleculeNode looks like. All of the atoms and bonds are individually draggable.

screenshot_884

And here's the documentation that I added, before discovering this issue:

    const atomBDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging atom B rotates the molecule',
      tandem: options.tandem.createTandem( 'atomBDragListener' )
    } );
    atomBNode.addInputListener( atomBDragListener );

    const bondABDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging the bond that connects atoms A and C rotates the molecule',
      tandem: options.tandem.createTandem( 'bondABDragListener' )
    } );
    bondABNode.addInputListener( bondABDragListener );

    const bondBCDragListener = new MoleculeAngleDragListener( molecule, this, {
      phetioDocumentation: 'dragging the bond that connects atoms B and C rotates the molecule',
      tandem: options.tandem.createTandem( 'bondBCDragListener' )
    } );
    bondBCNode.addInputListener( bondBCDragListener );

    const atomADragListener = new BondAngleDragListener( molecule, molecule.bondAngleAProperty, atomANode, {
      phetioDocumentation: 'dragging atom A changes the angle of the bond that connects atoms A and B',
      tandem: options.tandem.createTandem( 'atomADragListener' )
    } );
    atomANode.addInputListener( atomADragListener );

    const atomCDragListener = new BondAngleDragListener( molecule, molecule.bondAngleCProperty, atomCNode, {
      phetioDocumentation: 'dragging atom C changes the angle of the bond that connects atoms B and C',
      tandem: options.tandem.createTandem( 'atomCDragListener' )
    } );
    atomCNode.addInputListener( atomCDragListener );

Should it be possible to add phetioDocumentation to input listeners? If not, how is the instructional designer supposed to understand what they do?

This is probably a good topic to discuss with designers, so assigning @arouinfar.

samreid commented 3 years ago

Some options:

(a) Make PressListener extend PhetioObject so it and its subclasses can be instrumented. This would mean instrumenting something only for the sake of phetioDocumentation, and may be confusing why it is documented. (b) Pass through phetioDocumentation and possibly other options to the children. Or add a phetioDocumentationPattern that is used to fill in child documentation. (c) Add pressActionOptions, releaseActionOptions etc using the nested options pattern. This would end up duplicating data between the different actions.

(a) seems preferable, but I'd like to hear @zepumph's recommendation and whether we have other options.

zepumph commented 1 year ago

We also have run into this in https://github.com/phetsims/axon/issues/412. @pixelzoom do we still wish there was phetioDocumentation for PressListeners?

zepumph commented 1 year ago

If we need to, then EnabledComponent will need to extend PhetioObject since it is unfortunately no longer a mixin, which will have great consequences for ButtonModel. Likely we will need to pick up the patch in https://github.com/phetsims/axon/issues/412#issuecomment-1271884484, and I'm unsure how much effort that is worth. Unless this is a huge bummer to not have phetioDocumentation, I recommend closing this issue.

That said, I'm on the side of PhET-iO work where instrumenting an object just for phetioDocumentation will probably never feel very fruitful (developer bias). Let me know what you need

pixelzoom commented 1 year ago

As I said in https://github.com/phetsims/scenery/issues/1166#issue-820574667:

This is probably a good topic to discuss with designers, so assigning @arouinfar.

I don't see any evidence that this has been discussed with a designer. And @arouinfar said in https://github.com/phetsims/axon/issues/412#issuecomment-1244604623:

I don't think listeners always need documentation, but there are definitely cases where it is useful, such as molecule-polarity. It seems like optional documentation would be helpful here.

... where "such as molecule-polarity" is what resulted in this issue and https://github.com/phetsims/axon/issues/412. So again... please discuss with a designer.

That said... The phetioDocumentation for listeners shown in https://github.com/phetsims/scenery/issues/1166#issue-820574667 has been in place for 2+ years. It's not hypothetical, it was requested. But I guess I should rip it out, since it's not passed to anything, and it's not displaying in Studio. @arouinfar FYI, I'm going to remove it.