Merge lp:~bac/juju-gui/1102640 into lp:juju-gui/experimental

[Brad Crittenden]

When adding a relation, the service menu is shown.

If adding a relation using the 'Build Relation' action from the service menu
on the originating service, the click on the target service produces two
events. The dragend event is handled properly and sets up the relation. Then
the click event calls serviceClick.

Attempts to use the stopPropagation, stopEvent, and preventDefault methods on
the d3.event did not produce the desired results.

This work-around is pretty hacky but it does work. A more general solution
should be found.

Also, attempts to change the test 'must be able to add a relation from the
service menu' to simulate the click events and ensure the service menu was not
left displayed did not work.

This branch is submitted as is in an attempt to finish the last card blocking
the charm announcement.

https://codereview.appspot.com/7179049/

[Brad Crittenden (bac)]

Reviewers: mp+144389_code.launchpad.net,

Message:
Please take a look.

Description:
When adding a relation, the service menu is shown.

If adding a relation using the 'Build Relation' action from the service
menu
on the originating service, the click on the target service produces two
events. The dragend event is handled properly and sets up the relation.
  Then
the click event calls serviceClick.

Attempts to use the stopPropagation, stopEvent, and preventDefault
methods on
the d3.event did not produce the desired results.

This work-around is pretty hacky but it does work. A more general
solution
should be found.

Also, attempts to change the test 'must be able to add a relation from
the
service menu' to simulate the click events and ensure the service menu
was not
left displayed did not work.

This branch is submitted as is in an attempt to finish the last card
blocking
the charm announcement.

https://code.launchpad.net/~bac/juju-gui/1102640/+merge/144389

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7179049/

Affected files:
   A [revision details]
   M app/views/topology/relation.js
   M app/views/topology/service.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/views/topology/relation.js
=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js 2013-01-22 14:09:00 +0000
+++ app/views/topology/relation.js 2013-01-22 20:25:32 +0000
@@ -420,8 +420,6 @@
                .attr('y2', d3.event.y);
        }
      },
-
-
      addRelationDragEnd: function() {
        // Get the line, the endpoint service, and the target <rect>.
        var self = this;
@@ -606,14 +604,14 @@

        if (endpoints && endpoints.length === 1) {
          // Create a relation with the only available endpoint.
- var ep = endpoints[0],
- endpoints_item = [
- [ep[0].service, {
- name: ep[0].name,
- role: 'server' }],
- [ep[1].service, {
- name: ep[1].name,
- role: 'client' }]];
+ var ep = endpoints[0];
+ var endpoints_item = [
+ [ep[0].service,
+ { name: ep[0].name,
+ role: 'server' }],
+ [ep[1].service,
+ { name: ep[1].name,
+ role: 'client' }]];
          view.addRelationEnd(endpoints_item, view, context);
          return;
        }

Index: app/views/topology/service.js
=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js 2013-01-22 13:38:47 +0000
+++ app/views/topology/service.js 2013-01-22 16:24:49 +0000
@@ -89,6 +89,12 @@
        if (!d.containsPoint(mouse_coords, topo.zoom)) {
          return;
        }
+ // serviceClick is being called after dragend is processed. In those
+ // cases the current click action should not be invoked.
+ if (topo.ign...

[Benjamin Saller (bcsaller)]

LGTM, an unfortunate but needed work-around for the time being. Thanks.
Could you also add a card about (or just add now) a test ensuring the
behavior.

https://codereview.appspot.com/7179049/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7179049/diff/1/app/views/topology/service.js#newcode95
app/views/topology/service.js:95: topo.ignoreServiceClick = false;
If the state isn't shared it should be encapsulated at the module level,
meaning this.ignoreServiceClick. Nothing else need know about or check
this state.

https://codereview.appspot.com/7179049/diff/1/app/views/topology/service.js#newcode293
app/views/topology/service.js:293:
//d3.event.sourceEvent.preventDefault();
Prune.

https://codereview.appspot.com/7179049/

[Brad Crittenden (bac)]

*** Submitted:

When adding a relation, the service menu is shown.

If adding a relation using the 'Build Relation' action from the service
menu
on the originating service, the click on the target service produces two
events. The dragend event is handled properly and sets up the relation.
  Then
the click event calls serviceClick.

Attempts to use the stopPropagation, stopEvent, and preventDefault
methods on
the d3.event did not produce the desired results.

This work-around is pretty hacky but it does work. A more general
solution
should be found.

Also, attempts to change the test 'must be able to add a relation from
the
service menu' to simulate the click events and ensure the service menu
was not
left displayed did not work.

This branch is submitted as is in an attempt to finish the last card
blocking
the charm announcement.

R=bcsaller
CC=
https://codereview.appspot.com/7179049

https://codereview.appspot.com/7179049/

=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js	2013-01-22 14:09:00 +0000
+++ app/views/topology/relation.js	2013-01-22 20:32:22 +0000
@@ -420,8 +420,6 @@
               .attr('y2', d3.event.y);
       }
     },
-
-
     addRelationDragEnd: function() {
       // Get the line, the endpoint service, and the target <rect>.
       var self = this;
@@ -606,14 +604,14 @@
 
       if (endpoints && endpoints.length === 1) {
         // Create a relation with the only available endpoint.
-        var ep = endpoints[0],
-                endpoints_item = [
-                  [ep[0].service, {
-                    name: ep[0].name,
-                    role: 'server' }],
-                  [ep[1].service, {
-                    name: ep[1].name,
-                    role: 'client' }]];
+        var ep = endpoints[0];
+        var endpoints_item = [
+          [ep[0].service,
+           { name: ep[0].name,
+             role: 'server' }],
+          [ep[1].service,
+           { name: ep[1].name,
+             role: 'client' }]];
         view.addRelationEnd(endpoints_item, view, context);
         return;
       }

=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-01-22 13:38:47 +0000
+++ app/views/topology/service.js	2013-01-22 20:32:22 +0000
@@ -89,6 +89,12 @@
       if (!d.containsPoint(mouse_coords, topo.zoom)) {
         return;
       }
+      // serviceClick is being called after dragend is processed.  In those
+      // cases the current click action should not be invoked.
+      if (topo.ignoreServiceClick) {
+        topo.ignoreServiceClick = false;
+        return;
+      }
       // Get the current click action
       var curr_click_action = context.get('currentServiceClickAction');
       // Fire the action named in the following scheme:
@@ -282,7 +288,9 @@
     dragend: function(d,  self) {
       var topo = self.get('component');
       if (topo.buildingRelation) {
+        topo.ignoreServiceClick = true;
         topo.fire('addRelationDragEnd');
+        //d3.event.sourceEvent.preventDefault();
       }
       else {
         if (!d.inDrag ||