Improve dragline behavior.

app/views/environment.js

 'use strict';

[gary.poster, 2012/11/07 15:10:41]

Please add the module yuidoc comment.

[benji, 2012/11/08 17:25:06]

Done.

 
 YUI.add('juju-view-environment', function(Y) {
 
   var views = Y.namespace('juju.views'),
       utils = Y.namespace('juju.views.utils'),
       Templates = views.Templates,
       models = Y.namespace('juju.models');
 
   var EnvironmentView = Y.Base.create('EnvironmentView', Y.View,

[gary.poster, 2012/11/07 15:10:41]

Please add the class yuidoc comment.

[benji, 2012/11/08 17:25:06]

Done.

                                       [views.JujuBaseView], {
         events: {
           '#zoom-out-btn': {click: 'zoom_out'},
           '#zoom-in-btn': {click: 'zoom_in'},
           '.graph-list-picker .picker-button': {
             click: 'showGraphListPicker'
           },
           '.graph-list-picker .picker-expanded': {
             click: 'hideGraphListPicker'
           },
           // Menu/Controls
           '.add-relation': {
+            /** The user clicked on the "Build Relation" menu item. */

[gary.poster, 2012/11/07 15:10:41]

We had a good conversation in a hangout about my mild concerns about the fact
that this is required because of yuidoc-linter even though yuidoc ignored it;
and about the fact that you are using a /** TEXT */ syntax when I thought we
were standardizing on a multiline syntax:
/**
 * TEXT
 */

I still have those concerns, and would feel slightly happier if you changed to
always use the multiline spelling (particularly given the use of "/* */" rather
than "//") but I am OK with letting you document and advocate for your
positions, in your role as Bringer-of-YUIDoc-to-the-Masses.

[benji, 2012/11/08 17:25:06]

The multi-line format seems to overemphasize the importance of the comment, so I
have left these.  However, I have updated the style guide to include a (first
draft) policy that explains the situations in which you would use one or the
other.

             click: function() {
               var box = this.get('active_service'),
                   service = this.serviceForBox(box),
                   context = this.get('active_context');
+              this.addRelationDragStart.call(this, box, context);

[gary.poster, 2012/11/07 15:10:41]

Could you check to see if "this.addRelationDragStart(box, context);" will work? 
I think it will.

[thiago, 2012/11/07 15:21:04]

why do you need to use the 'call' method and pass 'this'?
You could simply call 'this.addRelationDragStart(box, context)', no?

[benji, 2012/11/08 17:25:06]

It does indeed work.

[benji, 2012/11/08 17:25:06]

Right.  That's fixed.

               this.service_click_actions
                         .toggleControlPanel(box, this, context);
-              this.service_click_actions
-                        .addRelationStart(box, this, context);
+              this.service_click_actions.addRelationStart(box, this, context);
             }
           },
           '.view-service': {
+            /** The user clicked on the "View" menu item. */
             click: function() {
               // Get the service element
               var box = this.get('active_service'),
                   service = this.serviceForBox(box);
               this.service_click_actions
                         .toggleControlPanel(box, this);
               this.service_click_actions
                         .show_service(service, this);
             }
           },
           '.destroy-service': {
+            /** The user clicked on the "Destroy" menu item. */
             click: function() {
               // Get the service element
               var box = this.get('active_service'),
                   service = this.serviceForBox(box);
               this.service_click_actions
                         .toggleControlPanel(box, this);
               this.service_click_actions
                         .destroyServiceConfirm(service, this);
             }
           }
@@ skipping 10 matching lines @@
               if (!self.hasSVGClass(rect, 'selectable-service')) {
                 return;
               }
 
               // Do not fire unless we're within the service box.
               var container = self.get('container'),
                   mouse_coords = d3.mouse(container.one('svg').getDOMNode());
               if (!d.containsPoint(mouse_coords, self.zoom)) {
                 return;
               }
+
+              // Do not fire if we're on the same service.
+              if (d === self.get('addRelationStart_service')) {
+                return;
+              }

[gary.poster, 2012/11/07 15:10:41]

not entirely idle thought, but one that is not really directly pertinent to your
branch: I wonder how a service is supposed to make a relationship to itself,
which IIUC is possible.

[benji, 2012/11/08 17:25:06]

That is a good question.  I tried a bit but I couldn't get the app to do it.

+
               self.set('potential_drop_point_service', d);
               self.set('potential_drop_point_rect', rect);
               self.addSVGClass(rect, 'hover');
 
               // If we have an active dragline, stop redrawing it on mousemove
               // and draw the line between the two nearest connector points of
               // the two services.
               if (self.dragline) {
                 var connectors = d.getConnectorPair(
                     self.get('addRelationStart_service')),
@@ skipping 20 matching lines @@
               }
               var rect = Y.one(this).one('.service-border');
               self.set('potential_drop_point_service', null);
               self.set('potential_drop_point_rect', null);
               self.removeSVGClass(rect, 'hover');
 
               if (self.dragline) {
                 self.dragline.attr('class',
                     'relation pending-relation dragline dragging');
               }
+            },
+            /**
+             * If the mouse moves over a service and we are adding a relation,
+             * then the dragline needs to be updated.
+             */

[gary.poster, 2012/11/07 15:10:41]

As mentioned yesterday, please add @method, @param and @return tags with the
appropriate bits and bobs.

Oh, wait: this is an event handler, so yuidoc will not document it.

:-( I find this a bit hard to catch.

[benji, 2012/11/08 17:25:06]

Yep, the plethora of event handlers makes this code hard to grok.  I am not sure
what we should do instead.  As an improvement I refactored the code so there is
a single view method that gets called when any of the mousemove events are
triggered.  That method now has full docs.

+            mousemove: function(d, self) {
+              if (self.clickAddRelation) {
+                var container = self.get('container'),
+                    node = container.one('#canvas rect:first-child');
+                self.rectMousemove.call(node.getDOMNode(), d, self);

[gary.poster, 2012/11/07 15:10:41]

So, where other uses of "call" in this file seem pointless, this one actually
seems to have a point...and it seems like a really odd pattern to me.  As we
discussed yesterday, you are following an existing pattern, so I'm not asking
for any changes from you, but could you find out from Matt or Ben why this
pattern exists, and arrange for the information to be shared with the rest of
us, either from them or you?

[thiago, 2012/11/07 15:21:04]

why to you set the scope to getDOMNode and pass self ('this') as parameter? You
could pass the domNode as parameter instead...

self.rectMousemove(node.getDOMNode(), d);

[benji, 2012/11/08 17:25:06]

This was preexisting code (that got moved around).  It is indeed insane.  I
refactored this extensively and it now doesn't do this.

+              }
             }
           },
           '.sub-rel-block': {
             mouseenter: function(d, self) {
               // Add an 'active' class to all of the subordinate relations
               // belonging to this service.
               self.subordinateRelationsForService(d)
                 .forEach(function(p) {
                     self.addSVGClass('#' + p.id, 'active');
                   });
@@ skipping 11 matching lines @@
             },
             mouseout: function(d, self) {
               d3.select(this)
                 .select('.unit-count')
                 .attr('class', 'unit-count hide-count');
             }
           },
 
           // Relation Related
           '.rel-label': {
-            click: 'relationClick'
+            /** The user clicked on the relation label. */
+            click: 'relationClick',
+            /**
+             * If the mouse moves over a relation label and we are adding a
+             * relation, then the dragline needs to be updated.
+             */

[gary.poster, 2012/11/07 15:10:41]

Add @method, @param, @return, please. ^D^D^D Nevermind, event handler.

Unnecessary extra credit if you also explain in the comment why we can't do some
kind of bubbling for this event handling, rather than duplicating the same basic
handler for multiple elements.  Maybe it's just a standard d3 thing that is
obvious to anyone who uses it, in which case nm.

[benji, 2012/11/08 17:25:06]

I don't understand event bubbling.  However I did refactor this so there is only
a single view method that handles these events and the three existing mousemove
events call it.
I have no idea.

+            mousemove: function(d, self) {
+              if (self.clickAddRelation) {
+                var container = self.get('container'),
+                    node = container.one('#canvas rect:first-child');
+                self.rectMousemove.call(node.getDOMNode(), d, self);

[thiago, 2012/11/07 15:21:04]

It seems we have mousemove more than once in this file. You could extract it to
a common function in this closure. 

[benji, 2012/11/08 17:25:06]

Good catch.  I have refactored the code so there is instead only a single view
method that all of these evens call.

[benji, 2012/11/08 17:25:06]

Yep, fixed as per earlier comment.

+              }
+            }
           },
 
-          // Canvas related
           '#canvas rect:first-child': {
+            /**
+             * If the user clicks on the background we cancel any active add
+             * relation.

[gary.poster, 2012/11/07 15:10:41]

Add @method, @param, @return please. ^D^D^D Nevermind, event handler.

Your description is the truth but not the whole truth, right?  If you can
describe what else is going on, please do.  If not, please at least clarify that
other mysterious things are going on also, so a comment reader will know to look
a bit more closely.

[benji, 2012/11/08 17:25:06]

Refactored away, see above.
It is all the truth that I am aware of. :)
I don't know that there are other mysterious things are going on.

+             */
             click: function(d, self) {
               var container = self.get('container');
               container.all('.environment-menu.active').removeClass('active');
               self.service_click_actions.toggleControlPanel(null, self);
               self.cancelRelationBuild();
+            },
+            /**
+             * If the mouse moves over the background and we are adding a
+             * relation, then the dragline needs to be updated.
+             */
+            mousemove: function(d, self) {
+              if (self.clickAddRelation) {
+                var container = self.get('container'),
+                    node = container.one('#canvas rect:first-child');
+                self.rectMousemove.call(node.getDOMNode(), d, self);
+              }
+            }
+          },
+          '.dragline': {
+            /** The user clicked while the dragline was active. */
+            click: function(d, self) {
+              // It was technically the dragline that was clicked, but the
+              // intent was to click on the background, so...
+              self.backgroundClicked.call(self);

[gary.poster, 2012/11/07 15:10:41]

Please try "self.backgroundClicked();"

[thiago, 2012/11/07 15:21:04]

You dont need to use 'call'. You can do...

self.backgroundClicked();

[benji, 2012/11/08 17:25:06]

Fixed.

             }
           }
         },
 
         d3Events: {
           '.service': {
             'mousedown.addrel': function(d, self) {
               var evt = d3.event;
               self.longClickTimer = Y.later(750, this, function(d, e) {
                 // Provide some leeway for accidental dragging.
                 if ((Math.abs(d.x - d.oldX) + Math.abs(d.y - d.oldY)) /
                     2 > 5) {
                   return;
                 }
 
-                // set a flag on the view that we're building a relation
-                self.buildingRelation = true;
-
                 // Sometimes mouseover is fired after the mousedown, so ensure
                 // we have the correct event in d3.event for d3.mouse().
                 d3.event = e;
 
-                // Flash an indicator around the center of the service block.
-                var center = d.getCenter();
-                self.vis.append('circle')
-                  .attr('cx', center[0])
-                  .attr('cy', center[1])
-                  .attr('r', 100)
-                  .attr('class', 'mouse-down-indicator')
-                  .transition()
-                  .duration(750)
-                  .ease('bounce')
-                  .attr('r', 0)
-                  .remove();
-
                 // Start the process of adding a relation
                 self.addRelationDragStart.call(self, d, this);
               }, [d, evt], false);
             },
             'mouseup.addrel': function(d, self) {
               // Cancel the long-click timer if it exists.
               if (self.longClickTimer) {
                 self.longClickTimer.cancel();
               }
             }
@@ skipping 192 matching lines @@
         serviceDblClick: function(d, self) {
           // Just show the service on double-click.
           var service = self.serviceForBox(d);
           (self.service_click_actions.show_service)(service, self);
         },
 
         relationClick: function(d, self) {
           self.removeRelationConfirm(d, this, self);
         },
 
+        /**
+         * Update the dragline to follow the mouse.
+         *
+         * @method rectMousemove
+         */
+        rectMousemove: function(d, self) {

[thiago, 2012/11/07 15:21:04]

According the the rest of the code, the scope here is domNode. If you dont
change the scope you can do (not tested... :) )...

// *************************
self.rectMousemove(node.getDOMNode());

rectMousemove: function(domNode) {
  var mouse = d3.mouse(domNode);
  d3.event.x = mouse[0];
  d3.event.y = mouse[1];
  this.addRelationDrag(this.get('addRelationStart_service'), domNode);
},
// *************************

btw, you dont use the 'd' parameter.

[benji, 2012/11/08 17:25:06]

Yep, this got refactored considerably, somewhat along the lines you outlined.
Indeed.  Unfortunately there is some sort of framework thing going on here that
foists arguments upon functions.  I documented the fact that the argument is
unused.

+          var mouse = d3.mouse(this);

[gary.poster, 2012/11/07 15:10:41]

Commenting that "this" is not what it appears to be would be appreciated. (Yuck
:-/ but I accept your explanation that this is standard in this file for some
reason, albeit with concern.)

[benji, 2012/11/08 17:25:06]

The refactoring removed the local "this" craziness.

+          d3.event.x = mouse[0];
+          d3.event.y = mouse[1];
+          self.addRelationDrag
+            .call(self, self.get('addRelationStart_service'), this);

[gary.poster, 2012/11/07 15:10:41]

This ought to work as

"self.addRelationDrag(self.get('addRelationStart_service'), this);"

Please give it a try.

[benji, 2012/11/08 17:25:06]

Yep.  I wonder why there was so much of this pattern in the code.  I have found
several other instances of FOO.method.call(FOO, ...) and fixed them.  If we had
an easy to use linting system, it would be nice to add a warning about such
constructs.

+        },
+
         /*
          * Sync view models with current db.models.
          */
         updateData: function() {
           //model data
           var vis = this.vis,
               db = this.get('db'),
               relations = db.relations.toArray(),
               services = db.services.map(views.toBoundingBox);
 
@@ skipping 600 matching lines @@
               container = this.get('container');
           if (curr_action === 'show_service') {
             this.set('currentServiceClickAction', 'addRelationStart');
           } else if (curr_action === 'addRelationStart' ||
               curr_action === 'ambiguousAddRelationCheck') {
             this.set('currentServiceClickAction', 'toggleControlPanel');
           } // Otherwise do nothing.
         },
 
         addRelationDragStart: function(d, context) {
           // Create a pending drag-line behind services.

[gary.poster, 2012/11/07 15:10:41]

Is it really "behind" now?  I think it is "in front of" because of the change to
append.  Am I right?

[benji, 2012/11/08 17:25:06]

Done.

-          var dragline = this.vis.insert('line', '.service')
+          var dragline = this.vis.append('line')
               .attr('class', 'relation pending-relation dragline dragging'),
               self = this;
 
-          // Start the line in the middle of the service.
-          var point = d.getCenter();
-          dragline.attr('x1', point[0])
-              .attr('y1', point[1])
-              .attr('x2', point[0])
-              .attr('y2', point[1]);
+          // Start the line between the cursor and the nearest connector
+          // point on the service.
+          var mouse = d3.mouse(Y.one('svg').getDOMNode());
+          self.cursorBox = views.BoundingBox();
+          self.cursorBox.pos = {x: mouse[0], y: mouse[1], w: 0, h: 0};
+          var point = self.cursorBox.getConnectorPair(d);
+          dragline.attr('x1', point[0][0])
+              .attr('y1', point[0][1])
+              .attr('x2', point[1][0])
+              .attr('y2', point[1][1]);
           self.dragline = dragline;
-          self.cursorBox = views.BoundingBox();
 
           // Start the add-relation process.
           self.service_click_actions
-          .addRelationStart(d, self, context);
+            .addRelationStart(d, self, context);
         },
 
         addRelationDrag: function(d, context) {
           // Rubberband our potential relation line if we're not currently
           // hovering over a potential drop-point.
           if (!this.get('potential_drop_point_service')) {
             // Create a BoundingBox for our cursor.
             this.cursorBox.pos = {x: d3.event.x, y: d3.event.y, w: 0, h: 0};
 
             // Draw the relation line from the connector point nearest the
@@ skipping 76 matching lines @@
               },
               this)));
         },
 
         cancelRelationBuild: function() {
           if (this.dragline) {
             // Get rid of our drag line
             this.dragline.remove();
             this.dragline = null;
           }
+          this.clickAddRelation = null;
           this.set('currentServiceClickAction', 'toggleControlPanel');
+          this.buildingRelation = false;
           this.show(this.vis.selectAll('.service'))
                   .classed('selectable-service', false);
         },
 
+        /**
+         * The user clicked on the environment view background.
+         *
+         * If we are in the middle of adding a relation, cancel the relation
+         * adding.
+         *
+         * @method backgroundClicked

[gary.poster, 2012/11/07 15:10:41]

Might as well add a "@return {undefined}" in there, as suggested by yuidoc docs.

[benji, 2012/11/08 17:25:06]

Done.

+         */
+        backgroundClicked: function() {
+          if (this.clickAddRelation) {
+            this.cancelRelationBuild();
+          }
+        },
+
+        /**
+         * An "add relation" action has been initiated by the user.
+         *
+         * @method startRelation

[gary.poster, 2012/11/07 15:10:41]

@param and @return

[benji, 2012/11/08 17:25:06]

Done.

+         */
+        startRelation: function(service) {
+          // Set flags on the view that indicate we are building a relation.
+          this.buildingRelation = true;
+          this.clickAddRelation = true;
+
+          this.show(this.vis.selectAll('.service'));
+
+          var db = this.get('db'),
+              getServiceEndpoints = this.get('getServiceEndpoints'),
+              endpoints = models.getEndpoints(
+              service, getServiceEndpoints(), db),

[gary.poster, 2012/11/07 15:10:41]

I'm guessing that's the best indentation that our linter allows?  would be nice
if it could come in by 2 or 4 spaces.

[benji, 2012/11/08 17:25:06]

Fixed.  This was bad when it was moved from another location.

+
+              /* Transform endpoints into a list of
+                 * relatable services (to the service)
+                 */

[gary.poster, 2012/11/07 15:10:41]

This has broken indentation.

Can we simply use "//" comments for this?  Automation will be able to
indent/dedent them better.  I'd really prefer for "/* */" comments to only be
used for yuidoc comments.  Tools have a much harder time interpreting what to do
within them.

[benji, 2012/11/08 17:25:06]

This comment was in this code when I moved it from elsewhere.  I converted it to
use leading // comments.  I am a little surprised that tooling would have
trouble with the C-style comments.

+              possible_relations = Y.Array.map(

[thiago, 2012/11/07 15:21:04]

The indentation in this block is really strange. Maybe you could just declare
the variable and then assign the value to it in another line.

[benji, 2012/11/08 17:25:06]

Agreed.  I didn't notice how weird it was when I moved it from somewhere else. 
I have fixed it.

+              Y.Array.flatten(Y.Object.values(

[gary.poster, 2012/11/07 15:10:41]

can we indent this 2 or 4 spaces?

[benji, 2012/11/08 17:25:06]

This was inherited from elsewhere, but the fix was easy.

+              endpoints)),
+              function(ep) {return ep.service;}),
+              invalidRelationTargets = {};
+
+          // Iterate services and invert the possibles list.
+          db.services.each(function(s) {
+            if (Y.Array.indexOf(possible_relations,
+                s.get('id')) === -1) {
+              invalidRelationTargets[s.get('id')] = true;
+            }
+          });
+
+          // Fade elements to which we can't relate.
+          // Rather than two loops this marks
+          // all services as selectable and then
+          // removes the invalid ones.
+          this.fade(this.vis.selectAll('.service')
+              .classed('selectable-service', true)
+              .filter(function(d) {
+                return (d.id in invalidRelationTargets &&
+                          d.id !== service.id);
+              }))
+              .classed('selectable-service', false);
+
+          // Store possible endpoints.
+          this.set('addRelationStart_possibleEndpoints', endpoints);
+          // Set click action.
+          this.set('currentServiceClickAction', 'ambiguousAddRelationCheck');
+        },
+
 
         /*
          * Zoom in event handler.
          */
         zoom_out: function(evt) {
           var slider = this.slider,
               val = slider.get('value');
           slider.set('value', val - 25);
         },
 
@@ skipping 245 matching lines @@
             }
             btn.set('disabled', false);
           },
 
 
           /*
            * Fired when clicking the first service in the add relation
            * flow.
            */
           addRelationStart: function(m, view, context) {
-            view.show(view.vis.selectAll('.service'));
-
-            var db = view.get('db'),
-                getServiceEndpoints = view.get('getServiceEndpoints'),
-                service = view.serviceForBox(m),
-                endpoints = models.getEndpoints(
-                    service, getServiceEndpoints(), db),
-
-                /* Transform endpoints into a list of
-                 * relatable services (to the service in m)
-                 */
-                possible_relations = Y.Array.map(
-                    Y.Array.flatten(Y.Object.values(
-                        endpoints)),
-                    function(ep) {return ep.service;}),
-                invalidRelationTargets = {};
-
-            // Iterate services and invert the possibles list.
-            db.services.each(function(s) {
-              if (Y.Array.indexOf(possible_relations,
-                  s.get('id')) === -1) {
-                invalidRelationTargets[s.get('id')] = true;
-              }
-            });
-
-            // Fade elements to which we can't relate.
-            // Rather than two loops this marks
-            // all services as selecable and then
-            // removes the invalid ones
-            view.fade(view.vis.selectAll('.service')
-              .classed('selectable-service', true)
-              .filter(function(d) {
-                  return (d.id in invalidRelationTargets &&
-                          d.id !== m.id);
-                }))
-              .classed('selectable-service', false);
-
-
-
+            var service = view.serviceForBox(m);
+            view.startRelation.call(view, service);

[gary.poster, 2012/11/07 15:10:41]

Try view.startRelation(service) please

[thiago, 2012/11/07 15:21:04]

It seems you like to use the 'call' function. :)

[benji, 2012/11/08 17:25:06]

The code I inherited likes the call method.  I have fixed this and all other
instances of this anti-pattern.

             // Store start service in attrs.
             view.set('addRelationStart_service', m);
-            // Store possible endpoints.
-            view.set('addRelationStart_possibleEndpoints', endpoints);
-            // Set click action.
-            view.set('currentServiceClickAction', 'ambiguousAddRelationCheck');
           },
 
           /*
            * Test if the pending relation is ambiguous.  Display a menu if so,
            * create the relation if not.
            */
           ambiguousAddRelationCheck: function(m, view, context) {
             var endpoints = view
                   .get('addRelationStart_possibleEndpoints')[m.id],
                 container = view.get('container');
@@ skipping 11 matching lines @@
               view.service_click_actions
                 .addRelationEnd(endpoints_item, view, context);
               return;
             }
 
             // Sort the endpoints alphabetically by relation name.
             endpoints = endpoints.sort(function(a, b) {
               return a[0].name + a[1].name < b[0].name + b[1].name;
             });
 
-            // Create a pending line if it doesn't exist, as in the case of
-            // clicking to add relation.
-            if (!view.dragline) {
-              var dragline = view.vis.insert('line', '.service')
-                  .attr('class', 'relation pending-relation dragline'),
-                  points = m.getConnectorPair(
-                      view.get('addRelationStart_service'));
-              dragline.attr('x1', points[0][0])
-                .attr('y1', points[0][1])
-                .attr('x2', points[1][0])
-                .attr('y2', points[1][1]);
-              view.dragline = dragline;
-            }
+            // Stop rubberbanding on mousemove.
+            view.clickAddRelation = null;
 
             // Display menu with available endpoints.
             var menu = container.one('#ambiguous-relation-menu');
             if (menu.one('.menu')) {
               menu.one('.menu').remove(true);
             }
 
             menu.append(Templates
                 .ambiguousRelationList({endpoints: endpoints}));
 
@@ skipping 131 matching lines @@
     'juju-models',
     'd3',
     'base-build',
     'handlebars-base',
     'node',
     'svg-layouts',
     'event-resize',
     'slider',
     'view']
 });