Merge lp:~makyo/juju-gui/adding-subordinates into lp:juju-gui/experimental

[Madison Scott-Clary]

Fix initial creation of subordinates.

When creating a subordinate service initially, it was not being laid out properly by the pack layout, nor was it seeming to have drag events attached. Pack layout was expecting a value accessor function to return some number greater than zero when constructing its radius, which informs the x/y coordinates. Returning zero results in NaN for all three values, thus positioning the service at (0,0) and causing drag events to fail when updating the service's coordinates with dx/dy + NaN. This branch fixes this by ensuring that zero is never returned as the node's value. This fix is quick and intended to make the UI function with the current layout algorithm. Future layout algorithms, if they're designed by hand, will need to keep this in mind as well.

https://codereview.appspot.com/6856067/

[Madison Scott-Clary (makyo)]

Reviewers: mp+135022_code.launchpad.net,

Message:
Please take a look.

Description:
Fix initial creation of subordinates.

When creating a subordinate service initially, it was not being laid out
properly by the pack layout, nor was it seeming to have drag events
attached. Pack layout was expecting a value accessor function to return
some number greater than zero when constructing its radius, which
informs the x/y coordinates. Returning zero results in NaN for all
three values, thus positioning the service at (0,0) and causing drag
events to fail when updating the service's coordinates with dx/dy + NaN.
  This branch fixes this by ensuring that zero is never returned as the
node's value. This fix is quick and intended to make the UI function
with the current layout algorithm. Future layout algorithms, if they're
designed by hand, will need to keep this in mind as well.

https://code.launchpad.net/~makyo/juju-gui/adding-subordinates/+merge/135022

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/views/environment.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/environment.js
=== modified file 'app/views/environment.js'
--- app/views/environment.js 2012-11-19 19:52:43 +0000
+++ app/views/environment.js 2012-11-20 00:06:25 +0000
@@ -318,7 +318,9 @@
            this.vis = vis;
            this.tree = d3.layout.pack()
                  .size([width, height])
- .value(function(d) {return d.unit_count;})
+ .value(function(d) {
+ return d.unit_count === 0 ? 1 : d.unit_count;
+ })
                  .padding(300);

            this.updateCanvas();

[Benjamin Saller (bcsaller)]

The diff looks fine, but there is behavior we expect on subordinates
that isn't be tested, I think we can at minimum check to see if subs get
x/y position assignment? What do you think?

https://codereview.appspot.com/6856067/

[Brad Crittenden (bac)]

Looks good Matt. Thanks.

https://codereview.appspot.com/6856067/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6856067/diff/1/app/views/environment.js#newcode323
app/views/environment.js:323: })
maybe this would be more readable:

min(1, d.unit_count)

https://codereview.appspot.com/6856067/

[Madison Scott-Clary (makyo)]

A test was added as well, but rather than just testing whether or not
subordinates had been placed properly, it tests that all services have
been placed properly.

https://codereview.appspot.com/6856067/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6856067/diff/1/app/views/environment.js#newcode323
app/views/environment.js:323: })
On 2012/11/20 14:50:48, bac wrote:
> maybe this would be more readable:

> min(1, d.unit_count)

Thanks! Did Math.max(d.unit_count, 1); (since we want 1 or higher)

https://codereview.appspot.com/6856067/

[lp:~makyo/juju-gui/adding-subordinates]

254. By Madison Scott-Clary on 2012-11-20

Fixing initial creation of subordinate services

[Madison Scott-Clary (makyo)]

Please take a look.

https://codereview.appspot.com/6856067/

[Brad Crittenden (bac)]

The new test makes sense to me Matt.

https://codereview.appspot.com/6856067/

[Madison Scott-Clary (makyo)]

*** Submitted:

Fix initial creation of subordinates.

When creating a subordinate service initially, it was not being laid out
properly by the pack layout, nor was it seeming to have drag events
attached. Pack layout was expecting a value accessor function to return
some number greater than zero when constructing its radius, which
informs the x/y coordinates. Returning zero results in NaN for all
three values, thus positioning the service at (0,0) and causing drag
events to fail when updating the service's coordinates with dx/dy + NaN.
  This branch fixes this by ensuring that zero is never returned as the
node's value. This fix is quick and intended to make the UI function
with the current layout algorithm. Future layout algorithms, if they're
designed by hand, will need to keep this in mind as well.

R=benjamin.saller, bac
CC=
https://codereview.appspot.com/6856067

https://codereview.appspot.com/6856067/

=== modified file 'app/views/environment.js'
--- app/views/environment.js	2012-11-19 19:52:43 +0000
+++ app/views/environment.js	2012-11-20 16:28:23 +0000
@@ -318,7 +318,9 @@
           this.vis = vis;
           this.tree = d3.layout.pack()
                 .size([width, height])
-                .value(function(d) {return d.unit_count;})
+                .value(function(d) {
+                return Math.max(d.unit_count, 1);
+              })
                 .padding(300);
 
           this.updateCanvas();

=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2012-11-15 20:48:30 +0000
+++ test/test_environment_view.js	2012-11-20 16:28:23 +0000
@@ -166,6 +166,53 @@
         }
     );
 
+    it('must be able to place new services properly', function() {
+      var view = new views.environment({
+        container: container,
+        db: db,
+        env: env
+      }),
+          tmp_data = {
+            result: [
+              ['service', 'add', {
+                'subordinate': true,
+                'charm': 'cs:precise/puppet-2',
+                'id': 'puppet2'
+              }],
+              ['service', 'add', {
+                'charm': 'cs:precise/mysql-6',
+                'id': 'mysql2'
+              }],
+              ['unit', 'add', {
+                'machine': 0,
+                'agent-state': 'started',
+                'public-address': '192.168.122.222',
+                'id': 'mysql2/0'
+              }]
+            ],
+            op: 'delta'
+          },
+          properTransform = /translate\(\d+\.?\d*,\d+\.?\d*\)/;
+      view.render();
+
+      container.all('.service').each(function(serviceNode) {
+        // Ensure that all initial service nodes' transform attributes are
+        // properly formated (i.e.: no NaN values).
+        properTransform.test(serviceNode.getAttribute('transform'))
+          .should.equal(true);
+      });
+
+      db.on_delta({ data: tmp_data });
+      view.render();
+
+      container.all('.service').each(function(serviceNode) {
+        // Ensure that all new service nodes' transform attributes are properly
+        // formated as well (i.e.: no NaN values).
+        properTransform.test(serviceNode.getAttribute('transform'))
+          .should.equal(true);
+      });
+    });
+
     it('must be able to render subordinate relation indicators',
        function() {
          var view = new views.environment({