should the tracer tool be draggable by its probe?

[pixelzoom]

Discovered while making the screenshot shown in #135 (comment).

I tried to drag the tracer tool by it's probe, which I think is a natural thing to do if you want to put the probe on a specific location. But I discovered that you can only move it by dragging the body of the tool.

In case I have the terminology wrong, this is what the code refers to as the tracer tool:

[arouinfar]

@pixelzoom I really like this suggestion! It does seem like a natural place to drag the tool, so please go ahead and make this change @andrea-phet.

[pixelzoom]

@andrea-phet somewhat related to this, in TracerNode:

111    // @public Should be added as a listener by our parent when the time is right
112    this.movableDragHandler = new MovableDragHandler( tracer.positionProperty, {
...
123    // When dragging, move the tracer tool
124    rectangle.addInputListener( this.movableDragHandler );

The comment on line 111 seems incorrect. The listener is added on line 124, and movableDragHandler is only public so that events can be forwarded to it by ToolboxPanel.

[pixelzoom]

Here's the patch to implement this. Deltas are indicated by + and - in the leftmost column.

Index: js/common/view/TracerNode.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/TracerNode.js	(revision 7af73b937d2e1caf814bed69f9ab6cb0d2ddeeab)
+++ js/common/view/TracerNode.js	(revision )
@@ -73,6 +73,11 @@
    * @constructor
    */
   function TracerNode( tracer, transformProperty, screenView, options ) {
+
+    options = _.extend( {
+      cursor: 'pointer'
+    }, options );
+
     var self = this;
     
     // @public is this being handled by user?
@@ -92,9 +97,7 @@
         fill: OPAQUE_BLUE,
         stroke: 'gray',
         lineWidth: 4,
-        opacity: 0.8,
-        pickable: true,
-        cursor: 'pointer'
+        opacity: 0.8
       }
     );
 
@@ -120,9 +123,6 @@
       }
     } );
 
-    // When dragging, move the tracer tool
-    rectangle.addInputListener( this.movableDragHandler );
-
     // crosshair view
     var crosshairShape = new Shape()
       .moveTo( -CIRCLE_AROUND_CROSSHAIR_RADIUS, 0 )
@@ -224,7 +224,6 @@
       self.tracer.updateData();
     } );
 
-    options = options || {};
     assert && assert( !options.children, 'this type sets its own children' );
 
     // Rendering order
@@ -239,6 +238,9 @@
 
     Node.call( this, options );
 
+    // When dragging, move the tracer tool
+    this.addInputListener( this.movableDragHandler );
+
     // visibility of the tracer
     tracer.isActiveProperty.link( function( active ) {
       self.visible = active;

[andrealin]

Oh, thanks @pixelzoom I didn't see your comment until after the commit above.

[andrealin]

Assigning to @arouinfar to review.

[arouinfar]

👍

[pixelzoom]

Reopening.

Works great, but the implementation is a little non-standard. Since the entire TracerNode is now draggable, it would be preferable to (1) add the drag handler to the entire node (after Node.call), instead of circle and rectangle separately, (2) set cursor: 'pointer' on the TracerNode (via options passed to Node.call), rather than its components.

[pixelzoom]

It's also unnecessary to set pickable: true for rectangle. Calling addInputListener automatically makes a Node pickable.

[andrealin]

There's just one thing: the halo that highlights the dot that the tracer tool is on is a child of TracerNode. I don't want that draggable, which is why I only pick circle and rectangle to drag.

[pixelzoom]

If you want to be totally safe and independent of rendering order, set pickable: false explicitly on haloNode. But with the current rendering order, haloNode is always behind circle, which is filled with TRANSPARENT_WHITE. So haloNode will not receive events.

[andrealin]

Oh yeah, good point.

[pixelzoom]

Looks great. I restored the !options.children assertion in the above commit, you probably deleted that unintentionally. Feel free to close after reviewing.

[andrealin]

Okay, closing.