Merge lp://staging/~gary/juju-gui/bug1250547 into lp://staging/juju-gui/experimental

Proposed by Gary Poster
Status: Merged
Merged at revision: 1194
Proposed branch: lp://staging/~gary/juju-gui/bug1250547
Merge into: lp://staging/juju-gui/experimental
Diff against target: 24 lines (+4/-11)
1 file modified
app/views/topology/bundle.js (+4/-11)
To merge this branch: bzr merge lp://staging/~gary/juju-gui/bug1250547
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+194932@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/25460043/

To post a comment you must log in.
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});
    };

Revision history for this message
Madison Scott-Clary (makyo) wrote :
Revision history for this message
Gary Poster (gary) wrote :

*** Submitted:

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.

R=matthew.scott
CC=
https://codereview.appspot.com/25460043

https://codereview.appspot.com/25460043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches