Merge lp:~makyo/juju-gui/bundle-zoomtofit into lp:juju-gui/experimental

[Madison Scott-Clary]

Bundle topo preview fixes

Center bundle topo properly and size SVG to parent container. This incidentally fixes what appeared to be a bug of non-promulgated charms not being displayed (was off the canvas).

https://codereview.appspot.com/14930052/

[Madison Scott-Clary (makyo)]

Reviewers: mp+192363_code.launchpad.net,

Message:
Please take a look.

Description:
Bundle topo preview fixes

Center bundle topo properly and size SVG to parent container. This
incidentally fixes what appeared to be a bug of non-promulgated charms
not being displayed (was off the canvas).

https://code.launchpad.net/~makyo/juju-gui/bundle-zoomtofit/+merge/192363

(do not edit description out of merge proposal)

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

Affected files (+15, -17 lines):
   A [revision details]
   M app/subapps/browser/views/bundle.js
   M app/views/topology/bundle.js
   M app/views/topology/utils.js
   M test/test_bundle_module.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: test/test_bundle_module.js
=== modified file 'test/test_bundle_module.js'
--- test/test_bundle_module.js 2013-10-10 00:05:02 +0000
+++ test/test_bundle_module.js 2013-10-22 10:52:27 +0000
@@ -113,11 +113,11 @@
            // that we've scaled the canvas as expected. In the model.
            assert.equal(
                parseFloat(bundle.topology.get('scale'), 10).toFixed(2),
- 0.48);
+ 0.44);
            // and on the canvas.
            var scaleAttr = svg.select('g').attr('transform');
            var match = /scale\(([\d\.]+)\)/.exec(scaleAttr);
- assert.equal(parseFloat(match[1]).toFixed(2), 0.48);
+ assert.equal(parseFloat(match[1]).toFixed(2), 0.44);
            done();
          }).then(undefined, done);
    });

Index: app/views/topology/bundle.js
=== modified file 'app/views/topology/bundle.js'
--- app/views/topology/bundle.js 2013-10-18 16:47:34 +0000
+++ app/views/topology/bundle.js 2013-10-22 10:20:48 +0000
@@ -381,6 +381,9 @@
      var topo = this.topology;
      var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
      var centroid = topoUtils.centroid(vertices);
+ var scale = topo.get('scale');
+ centroid[0] += 96 * scale;
+ centroid[1] += 96 * scale;
      this.topology.modules.PanZoomModule.panToPoint({point: centroid});
    };

@@ -394,21 +397,15 @@
    BundleTopology.prototype.zoomToFit = function() {
      var topo = this.topology;
      var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
- var bb = topoUtils.getBoundingBox(vertices);
+ var bb = topoUtils.getBoundingBox(vertices, 96, 96);
      var width = topo.get('width'),
          height = topo.get('height');

      // Zoom to Fit
- // We are really only interested in scale down
- // here when the bundle is too large to
- // render in the space provided.
- var maxScale = 1.0;
- if (bb.w > width || bb.h > height) {
- maxScale = Math.min(bb.w / width, bb.h / height);
- maxScale -= 0.05; // Margin
- }
+ var maxScale = Math.min(width / bb.w, height / bb.h);
+ maxScale -= 0.05; // Margin
      // Clamp Scale
- maxScale = Math.max(0.25 , Math.min(1...

[Gary Poster (gary)]

LGTM with trivials. Would be nice to have a test of the getBoundingBox
changes. Doing QA now. Thank you!

https://codereview.appspot.com/14930052/diff/1/app/subapps/browser/views/bundle.js
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14930052/diff/1/app/subapps/browser/views/bundle.js#newcode175
app/subapps/browser/views/bundle.js:175: var options = {size: [720,
360]};
As we discussed, please check with Luca to see if he thinks we can
increase the height a bit.

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

https://codereview.appspot.com/14930052/diff/1/app/views/topology/bundle.js#newcode386
app/views/topology/bundle.js:386: centroid[1] += 96 * scale;
It would be lovely to deduce 96. Failing that, I think we ought to at
least document what it is supposed to be (perhaps by using a well-named
"constant" variable).

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

https://codereview.appspot.com/14930052/diff/1/app/views/topology/utils.js#newcode174
app/views/topology/utils.js:174: translateX: minX - boxWidth,
translateY: minY - boxHeight,
You said we don't use this, but we also agreed that boxHeight and
boxWidth should each be divided by 2. Delete or fix. :-)

https://codereview.appspot.com/14930052/

[Gary Poster (gary)]

On 2013/10/23 17:10:13, gary.poster wrote:
> LGTM with trivials. Would be nice to have a test of the
getBoundingBox changes.
> Doing QA now. Thank you!

https://codereview.appspot.com/14930052/diff/1/app/subapps/browser/views/bundle.js
> File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14930052/diff/1/app/subapps/browser/views/bundle.js#newcode175
> app/subapps/browser/views/bundle.js:175: var options = {size: [720,
360]};
> As we discussed, please check with Luca to see if he thinks we can
increase the
> height a bit.

https://codereview.appspot.com/14930052/diff/1/app/views/topology/bundle.js
> File app/views/topology/bundle.js (right):

https://codereview.appspot.com/14930052/diff/1/app/views/topology/bundle.js#newcode386
> app/views/topology/bundle.js:386: centroid[1] += 96 * scale;
> It would be lovely to deduce 96. Failing that, I think we ought to at
least
> document what it is supposed to be (perhaps by using a well-named
"constant"
> variable).

https://codereview.appspot.com/14930052/diff/1/app/views/topology/utils.js
> File app/views/topology/utils.js (right):

https://codereview.appspot.com/14930052/diff/1/app/views/topology/utils.js#newcode174
> app/views/topology/utils.js:174: translateX: minX - boxWidth,
translateY: minY -
> boxHeight,
> You said we don't use this, but we also agreed that boxHeight and
boxWidth
> should each be divided by 2. Delete or fix. :-)

QA good. Thank you! This is a huge improvement, and we could launch
with it if needed, I think. I look forward to proper centering in the
future, if we can.

Gary

https://codereview.appspot.com/14930052/

[lp:~makyo/juju-gui/bundle-zoomtofit]

1150. By Madison Scott-Clary on 2013-10-23

Better fitting at the cost of better centering; next iteration.

1151. By Madison Scott-Clary on 2013-10-23

Test change for new math

[Madison Scott-Clary (makyo)]

Please take a look.

https://codereview.appspot.com/14930052/

[Gary Poster (gary)]

On 2013/10/23 21:31:09, matthew.scott wrote:
> Please take a look.

Looks even better, thank you.

If you could get this landed before tomorrow morning's demo that would
be nice, but if not, no worries. :-)

https://codereview.appspot.com/14930052/

[Madison Scott-Clary (makyo)]

*** Submitted:

Bundle topo preview fixes

Center bundle topo properly and size SVG to parent container. This
incidentally fixes what appeared to be a bug of non-promulgated charms
not being displayed (was off the canvas).

R=gary.poster
CC=
https://codereview.appspot.com/14930052

https://codereview.appspot.com/14930052/

=== modified file 'app/subapps/browser/views/bundle.js'
--- app/subapps/browser/views/bundle.js	2013-10-17 16:55:19 +0000
+++ app/subapps/browser/views/bundle.js	2013-10-23 21:28:47 +0000
@@ -172,7 +172,7 @@
       var content = this.template(attrs);
       var node = this.get('container').setHTML(content);
       var renderTo = this.get('renderTo');
-      var options = {size: [480, 360]};
+      var options = {size: [720, 500]};
       this.hideIndicator(renderTo);
 
       var showTopo = true;

=== modified file 'app/views/topology/bundle.js'
--- app/views/topology/bundle.js	2013-10-23 16:03:22 +0000
+++ app/views/topology/bundle.js	2013-10-23 21:28:47 +0000
@@ -378,6 +378,12 @@
     var topo = this.topology;
     var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
     var centroid = topoUtils.centroid(vertices);
+    var scale = topo.get('scale'),
+        width = topo.get('width'),
+        height = topo.get('height');
+    var bb = topo.get('bundleBoundingBox');
+    centroid[0] += Math.abs(width - bb.w) * scale;
+    centroid[1] += Math.abs(height - bb.h) * scale;
     this.topology.modules.PanZoomModule.panToPoint({point: centroid});
   };
 
@@ -391,21 +397,16 @@
   BundleTopology.prototype.zoomToFit = function() {
     var topo = this.topology;
     var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
-    var bb = topoUtils.getBoundingBox(vertices);
+    var bb = topoUtils.getBoundingBox(vertices, 96, 96);
+    topo.set('bundleBoundingBox', bb);
     var width = topo.get('width'),
         height = topo.get('height');
 
     // Zoom to Fit
-    // We are really only interested in scale down
-    // here when the bundle is too large to
-    // render in the space provided.
-    var maxScale = 1.0;
-    if (bb.w > width || bb.h > height) {
-      maxScale = Math.min(bb.w / width, bb.h / height);
-      maxScale -= 0.05; // Margin
-    }
+    var maxScale = Math.min(width / bb.w, height / bb.h);
+    maxScale -= 0.1; // Margin
     // Clamp Scale
-    maxScale = Math.max(0.25 , Math.min(1.0, maxScale));
+    maxScale = Math.min(1.0, maxScale);
     this.centerViewport(maxScale);
   };
 

=== modified file 'app/views/topology/topology.js'
--- app/views/topology/topology.js	2013-10-18 16:47:34 +0000
+++ app/views/topology/topology.js	2013-10-23 21:28:47 +0000
@@ -235,6 +235,11 @@
   }, {
     ATTRS: {
       /**
+       * @property {Object} boundingBox
+       * A bounding-box for all of the services
+       */
+      boundingBox: {},
+      /**
        * @property {models.Database} db
        */
       db: {},

=== modified file 'app/views/topology/utils.js'
--- app/views/topology/utils.js	2013-10-03 04:47:21 +0000
+++ app/views/topology/utils.js	2013-10-23 21:28:47 +0000
@@ -160,7 +160,7 @@
     return centroid;
   };
 
-  utils.getBoundingBox = function(vertices) {
+  utils.getBoundingBox = function(vertices, boxWidth, boxHeight) {
     var minX = Infinity, maxX = -Infinity,
         minY = Infinity, maxY = -Infinity;
 
@@ -171,9 +171,8 @@
       if (v[1] > maxY) { maxY = v[1]; }
     });
     return {
-      translateX: minX, translateY: minY,
-      w: maxX - minX, h: maxY - minY};
-
+      translateX: minX - boxWidth / 2, translateY: minY - boxHeight / 2,
+      w: maxX - minX + boxWidth, h: maxY - minY + boxHeight};
   };
 
 }, '0.1.0', {

=== modified file 'test/test_bundle_module.js'
--- test/test_bundle_module.js	2013-10-23 16:03:22 +0000
+++ test/test_bundle_module.js	2013-10-23 21:28:47 +0000
@@ -111,11 +111,11 @@
           // that we've scaled the canvas as expected.  In the model.
           assert.equal(
               parseFloat(bundle.topology.get('scale'), 10).toFixed(2),
-              0.48);
+              0.39);
           // and on the canvas.
           var scaleAttr = svg.select('g').attr('transform');
           var match = /scale\(([\d\.]+)\)/.exec(scaleAttr);
-          assert.equal(parseFloat(match[1]).toFixed(2), 0.48);
+          assert.equal(parseFloat(match[1]).toFixed(2), 0.39);
           done();
         }).then(undefined, done);
   });