juju-gui

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

  1. 1102640
  2. Merge into trunk
Proposed by Brad Crittenden
Status: Merged
Merged at revision: 339
Proposed branch: lp:~bac/juju-gui/1102640
Merge into: lp:juju-gui/experimental
Diff against target: 62 lines (+16/-10)
2 files modified
app/views/topology/relation.js (+8/-10)
app/views/topology/service.js (+8/-0)
To merge this branch: bzr merge lp:~bac/juju-gui/1102640
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+144389@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :
Download full text (3.7 KiB)

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...

Read more...

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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/

Revision history for this message
Brad Crittenden (bac) wrote :

*** 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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
420 .attr('y2', d3.event.y);420 .attr('y2', d3.event.y);
421 }421 }
422 },422 },
423
424
425 addRelationDragEnd: function() {423 addRelationDragEnd: function() {
426 // Get the line, the endpoint service, and the target <rect>.424 // Get the line, the endpoint service, and the target <rect>.
427 var self = this;425 var self = this;
@@ -606,14 +604,14 @@
606604
607 if (endpoints && endpoints.length === 1) {605 if (endpoints && endpoints.length === 1) {
608 // Create a relation with the only available endpoint.606 // Create a relation with the only available endpoint.
609 var ep = endpoints[0],607 var ep = endpoints[0];
610 endpoints_item = [608 var endpoints_item = [
611 [ep[0].service, {609 [ep[0].service,
612 name: ep[0].name,610 { name: ep[0].name,
613 role: 'server' }],611 role: 'server' }],
614 [ep[1].service, {612 [ep[1].service,
615 name: ep[1].name,613 { name: ep[1].name,
616 role: 'client' }]];614 role: 'client' }]];
617 view.addRelationEnd(endpoints_item, view, context);615 view.addRelationEnd(endpoints_item, view, context);
618 return;616 return;
619 }617 }
620618
=== 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 @@
89 if (!d.containsPoint(mouse_coords, topo.zoom)) {89 if (!d.containsPoint(mouse_coords, topo.zoom)) {
90 return;90 return;
91 }91 }
92 // serviceClick is being called after dragend is processed. In those
93 // cases the current click action should not be invoked.
94 if (topo.ignoreServiceClick) {
95 topo.ignoreServiceClick = false;
96 return;
97 }
92 // Get the current click action98 // Get the current click action
93 var curr_click_action = context.get('currentServiceClickAction');99 var curr_click_action = context.get('currentServiceClickAction');
94 // Fire the action named in the following scheme:100 // Fire the action named in the following scheme:
@@ -282,7 +288,9 @@
282 dragend: function(d, self) {288 dragend: function(d, self) {
283 var topo = self.get('component');289 var topo = self.get('component');
284 if (topo.buildingRelation) {290 if (topo.buildingRelation) {
291 topo.ignoreServiceClick = true;
285 topo.fire('addRelationDragEnd');292 topo.fire('addRelationDragEnd');
293 //d3.event.sourceEvent.preventDefault();
286 }294 }
287 else {295 else {
288 if (!d.inDrag ||296 if (!d.inDrag ||

Subscribers

People subscribed via source and target branches