Code review comment for lp://staging/~gary/juju-gui/bug1250547

Revision history for this message
Gary Poster (gary) wrote :

Reviewers: mp+194932_code.launchpad.net,

Message:
Please take a look.

Description:
Simplify bundle vis centering

It turns out that using a polygon centroid isn't quite what our eyes
expected, and also caused a bug when elements were away from the center
mass of services. Using the bounding box gives more expected results.

I didn't add a test, because I argued to myself that this was a visual
thing, but I'm really lying to myself, and if you want to call me out on
it, please feel free to appeal to my better self. :-)

To QA, look at a lot of bundles, like those from "hatch", "jorge", and
"gary". Then also look at the problematic one,
http://localhost:8888/sidebar/search/bundle/~makyo/mediawiki-scalable/5/mediawiki-scalable/
. Compare it with the rendering in trunk (like on comingsoon) to see
the difference.

https://code.launchpad.net/~gary/juju-gui/bug1250547/+merge/194932

(do not edit description out of merge proposal)

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

Affected files (+6, -11 lines):
   A [revision details]
   M app/views/topology/bundle.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/bundle.js
=== modified file 'app/views/topology/bundle.js'
--- app/views/topology/bundle.js 2013-11-05 18:10:05 +0000
+++ app/views/topology/bundle.js 2013-11-12 19:54:35 +0000
@@ -426,18 +426,11 @@
    */
    BundleTopology.prototype.panToCenter = function() {
      var topo = this.topology;
- var vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
- var centroid = topoUtils.centroid(vertices);
- var width = topo.get('width'),
- height = topo.get('height');
+ // We don't want a polygon centroid for our services: we simply want
+ // the center of the bounding box.
      var bb = topo.get('bundleBoundingBox');
- // Shift the centroid by the size of a service block.
- // Note: if the size was clamped on the Y axis, we should only clamp by
- // half the size of a service on the X axis or we risk clipping.
- var clampedVertically = Math.abs(width - bb.w) < Math.abs(height -
bb.w);
- centroid[0] += SERVICE_SIZE * (clampedVertically ? 1 : 0.5);
- centroid[1] += SERVICE_SIZE * 0.5;
- this.topology.modules.PanZoomModule.panToPoint({point: centroid});
+ var center = [bb.translateX + bb.w / 2, bb.translateY + bb.h / 2];
+ this.topology.modules.PanZoomModule.panToPoint({point: center});
    };

« Back to merge proposal