Position new services more intelligently.

Position new services more intelligently.

New services are positioned more intelligently than by just running the pack layout on them by padding them beside the existing services.  They are currently only padded to the right because a) the topology always starts at (0, 0) and anything to the left might be hidden and b) monitor screens are generally wider than they are tall and there's a higher chance of the new service being on the screen in that case.  A future branch will center the viewport on the new service intelligently with the charm browser.

https://code.launchpad.net/~makyo/juju-gui/new-svc-placement/+merge/163386

(do not edit description out of merge proposal)

[matthew.scott, 2013-05-10 23:55:16]

Please take a look.

[gary.poster, 2013-05-11 03:04:06]

LGTM.  Thank you!

Gary

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js#newcode29
app/config-debug.js:29: simulateEvents: false,
I think I agree that this should be the default.  We could actually put a
handler in the flags namespace to turn on simulation, so that you go to
:flags:/simulate in order to start simulation...

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:616: if (new_services.length === 1 &&
new_services[0].model.get('pending')) {
Maybe add a comment clarifying what's going on?

// If we are adding a new pending ("ghost") service, put it
// to the right of the existing services.

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/topology.js#...
app/views/topology/topology.js:162: return Y.Lang.isNumber(box.x);
When would box.x not be a number?  Maybe add a comment?

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:8: Find a point outside of a given list of vertices,
used for placing
...vertices.  This is used...

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:44: switch (vertices.length) {
Add a comment?

// d3.geom.hull, used by _exteriorToHull, requires at least three points.  We
can handle other cases very simply.

[benji, 2013-05-11 13:52:55]

Looks good.  Thanks.

LGTM-ly, y'rs

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js#newcode29
app/config-debug.js:29: simulateEvents: false,
On 2013/05/11 03:04:06, gary.poster wrote:
> I think I agree that this should be the default.

+1

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:22: @param {number} padding An integer to use in
padding.
Maybe "The padding around existing vertices in pixels." would convey a bit more
information.

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:33: Math.pow(vertex[1], 2));
All hail Pythagoras.

[matthew.scott, 2013-05-14 14:46:05]

Thanks for the reviews!

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js#newcode29
app/config-debug.js:29: simulateEvents: false,
On 2013/05/11 03:04:06, gary.poster wrote:
> I think I agree that this should be the default.  We could actually put a
> handler in the flags namespace to turn on simulation, so that you go to
> :flags:/simulate in order to start simulation...

Done.

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:616: if (new_services.length === 1 &&
new_services[0].model.get('pending')) {
On 2013/05/11 03:04:06, gary.poster wrote:
> Maybe add a comment clarifying what's going on?
> 
> // If we are adding a new pending ("ghost") service, put it
> // to the right of the existing services.

Done.

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/topology.js#...
app/views/topology/topology.js:162: return Y.Lang.isNumber(box.x);
On 2013/05/11 03:04:06, gary.poster wrote:
> When would box.x not be a number?  Maybe add a comment?

We just need a truthy value, but 0 evaluates to false.  Since the only other
possibility should be undefined, I'll switch to testing that.

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

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:8: Find a point outside of a given list of vertices,
used for placing
On 2013/05/11 03:04:06, gary.poster wrote:
> ...vertices.  This is used...

Done.

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:22: @param {number} padding An integer to use in
padding.
On 2013/05/11 13:52:55, benji wrote:
> Maybe "The padding around existing vertices in pixels." would convey a bit
more
> information.

Done.

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:33: Math.pow(vertex[1], 2));
On 2013/05/11 13:52:55, benji wrote:
> All hail Pythagoras.

Done.

https://codereview.appspot.com/9162048/diff/1/app/views/topology/utils.js#new...
app/views/topology/utils.js:44: switch (vertices.length) {
On 2013/05/11 03:04:06, gary.poster wrote:
> Add a comment?
> 
> // d3.geom.hull, used by _exteriorToHull, requires at least three points.  We
> can handle other cases very simply.

Done.

[matthew.scott, 2013-05-14 15:04:14]

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/9162048/diff/1/app/config-debug.js#newcode29
app/config-debug.js:29: simulateEvents: false,
On 2013/05/14 14:46:05, matthew.scott wrote:
> On 2013/05/11 03:04:06, gary.poster wrote:
> > I think I agree that this should be the default.  We could actually put a
> > handler in the flags namespace to turn on simulation, so that you go to
> > :flags:/simulate in order to start simulation...
> 
> Done.

Oops, on testing, this doesn't work because the app hasn't built the feature
flags yet by the time the simulator is built and started.  I'm going to revert
this for this branch, then make a slack task to do this elsewhere.  This should
build and start the simulator only if the flag is set, so that code should be
pulled out and moved to somewhere it can be called, perhaps in a declarative
fashion:

// In a flags module...
_route: function() {
  Y.each(Y.Object.keys(window.flags), function(flag) {
    (this[flag])(window.flags[flag]);
  });
},

simulate: function(flagValue) { /* simulator code */ }

[matthew.scott, 2013-05-14 15:23:11]

*** Submitted:

Position new services more intelligently.

New services are positioned more intelligently than by just running the pack
layout on them by padding them beside the existing services.  They are currently
only padded to the right because a) the topology always starts at (0, 0) and
anything to the left might be hidden and b) monitor screens are generally wider
than they are tall and there's a higher chance of the new service being on the
screen in that case.  A future branch will center the viewport on the new
service intelligently with the charm browser.

R=gary.poster, benji
CC=
https://codereview.appspot.com/9162048