View model improvements

View model improvements

Change BoundingBoxes to retain reference to module. This allows box models
to directly resolve the db.Model backing them. This also allow access to the 
DOMNode in the canvas directly. Certain features of box directly depend on
methods being available in the passed in module, mocking this is still
possible and shown in tests.

https://code.launchpad.net/~bcsaller/juju-gui/viewmodel-improvements/+merge/145755

(do not edit description out of merge proposal)

[bcsaller, 2013-01-31 00:34:30]

Please take a look.

[teknico, 2013-01-31 10:44:24]

Land with changes.

This looks like a nice refactoring. Thanks for regenerating the "undocumented"
file. Please have a look at the comments below.

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

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:102: serviceDblClick: function(d, self) {
A more descriptive name than "d" on these methods would be nice, some day.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:902: destroyServiceConfirm: function(m, view) {
Ditto for "m". Can we do away with these one-letter names, anyway? They hamper
readability.

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

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode611
app/views/utils.js:611: * Utility class that encapsulates Y.Models and keeps
their position
It is not a class anymore, is it? Can this comment be converted to a YUI comment
block somehow?

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode861
app/views/utils.js:861: * @param {ServiceModule} Module holding box canvas and
context.
s/Module/module/

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode865
app/views/utils.js:865: **/
Add a "@method toBoundingBoxes" directive, and remove one asterisk from the
block closing mark.

https://codereview.appspot.com/7231067/diff/1/app/views/utils.js#newcode889
app/views/utils.js:889: * relation.getAttrs() plus "source", "target", and other
convenience data.
Indent the text, to show that it continues from the line above.

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:846: // update using pos
s/pos/position/ ?

https://codereview.appspot.com/7231067/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:887: boxes.wordpress.id.should.equal('wordpress');
Nicer. :-)

[teknico, 2013-01-31 12:06:08]

Uhm, I should mention that while "make test-prod" completes successfully, in
"make test-debug" mocha times out after setup, right before running the tests.
"make test-debug" does complete successfully in trunk, here.

[bcsaller, 2013-01-31 16:57:58]

Please take a look.

[gary.poster, 2013-01-31 17:30:12]

I was in the middle of a review--with a long hiatus for various calls and other
responsibilities--when you made the new version.  These comments are from the
previous revision, so they may or may not still be pertinent.  I'm sending them
off and will start a new review after lunch.

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

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:102: serviceDblClick: function(d, self) {
On 2013/01/31 10:44:24, teknico wrote:
> A more descriptive name than "d" on these methods would be nice, some day.

+1

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:201: var service = box;
Shouldn't this be box.model?  If so, why does it still work?  If it is really ok
for this to be the box, why should we call it the service?

Please address this one way or another so it is clear to the reader what is
going on--and, if necessary, tested that the method is doing what we expect.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:242: views.toBoundingBoxes(this, db.services,
this.service_boxes);
So, this mutates something or other?  I need to go read it...

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:243: this.services =
Y.Object.values(this.service_boxes);
Why do we have to stash this?

If we really want this on the module, I'd like it initialized and explained in
the initializer, the way you do with service_boxes.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:245: // XXX: containment breaking alias, do we
need this?
If we do, then we should only store service_boxes on topo, and not locally on
"this," right?  (No need to change this now, just a fly-by thought.)

[matthew.scott, 2013-01-31 17:53:55]

Reviewing this in light of the possible changes to my current branch.  Overall I
think it's really good stuff (I like the new box object much better, as it makes
things a little more readable, to me), and will help me out in SOME respects,
but only a few.  Thanks for the work.  I'll defer to the others for landability,
as they've got good comments.

[matthew.scott, 2013-01-31 19:51:03]

Increasing the number of units for a service is not reflected in the environment
view (increased WP from 1 to 10; service isn't resized, health graph remains the
same, unit count still says 1).  Additionally, I receive some errors when trying
to create relationships between services.  Sometimes, it'll fail out when trying
to find available endpoints, and sometimes it dies on creating a relation.  I
was trying with haproxy.  After doing so, several things seemed to fail in a
cascade.

[bcsaller, 2013-02-01 00:59:09]

On 2013/01/31 19:51:03, matthew.scott wrote:
> Increasing the number of units for a service is not reflected in the
environment
> view (increased WP from 1 to 10; service isn't resized, health graph remains
the
> same, unit count still says 1).  Additionally, I receive some errors when
trying
> to create relationships between services.  Sometimes, it'll fail out when
trying
> to find available endpoints, and sometimes it dies on creating a relation.  I
> was trying with haproxy.  After doing so, several things seemed to fail in a
> cascade.

You found a pre-existing bug but the fix is a small delta. Trying to only
incrementally render the env view lead to calling update() and rendered()
on dbchange but we were only doing this when env view was the current view. This
has been changed in the branch to always update the env view on db change as
well as any other actions that need to be taken.

[gary.poster, 2013-02-01 02:26:05]

Hi Ben.  Thank you for this very nice branch.

Land with changes, as documented in this review and my previous one.

I asked Matt to look at this as well if he has time.  I didn't have time to look
at it during normal hours, but already had some comments waiting in the wings,
so decided to complete my review.

Gary

https://codereview.appspot.com/7231067/diff/10/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/7231067/diff/10/app/views/topology/relation.js...
app/views/topology/relation.js:779: return (relation.source.modelId ===
service.modelId ||
These sorts of changes--change calls to attrs--are throughout the branch, and
nice.

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

https://codereview.appspot.com/7231067/diff/10/app/views/topology/service.js#...
app/views/topology/service.js:256: views.toBoundingBoxes(this, db.services,
this.service_boxes);
From comment on previous partial review: I now see that this.service_boxes is
mutated.  Cool.

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js#newcode618
app/views/utils.js:618: function positionProp(name) {
Nice.

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js#newcode790
app/views/utils.js:790: getNearestConnector: {
Now that it is a property, wouldn't this be better as a noun,
"nearestConnector"?

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js#newcode817
app/views/utils.js:817: getConnectorPair: {
Likewise: "connectorPair"?

https://codereview.appspot.com/7231067/diff/10/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7231067/diff/10/test/test_environment_view.js#...
test/test_environment_view.js:910: it('must be able to update boxes with new
model data',
Nice test.

https://codereview.appspot.com/7231067/diff/10/undocumented
File undocumented (right):

https://codereview.appspot.com/7231067/diff/10/undocumented#newcode247
undocumented:247: app/models/charm.js:50 "initializer"
We went from 234 to 247 undocumented functions.  It's our policy to try to keep
that number stable or lower even when we regenerate the "undocumented" file.  I
reviewed why the number went up, and I see that it is largely or exclusively
because of the property getter and setter functions in the utils work you did. 
You documented the properties quite nicely, so, without a counterargument to
convince me otherwise, this case seems to be one in which our yuidoc linter
needs to be improved.  Therefore, I'd like to honor our policy, but I'd ask you
to do it by simply filing a bug against our yuidoc linter describing the
situation, and how the linter could be improved to no longer complain about this
situation.  If you'd also like to document 13 random functions arbitrarily to
keep our count down, I won't stop you, but I do not ask for that and in fact
would prefer if that task did not slow landing the branch down.

[bcsaller, 2013-02-01 15:35:29]

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

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:243: this.services =
Y.Object.values(this.service_boxes);
On 2013/01/31 17:30:12, gary.poster wrote:
> Why do we have to stash this?
> 
> If we really want this on the module, I'd like it initialized and explained in
> the initializer, the way you do with service_boxes.

removed.

https://codereview.appspot.com/7231067/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:245: // XXX: containment breaking alias, do we
need this?
On 2013/01/31 17:30:12, gary.poster wrote:
> If we do, then we should only store service_boxes on topo, and not locally on
> "this," right?  (No need to change this now, just a fly-by thought.)
Moved to topo.

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7231067/diff/10/app/views/utils.js#newcode790
app/views/utils.js:790: getNearestConnector: {
On 2013/02/01 02:26:05, gary.poster wrote:
> Now that it is a property, wouldn't this be better as a noun,
> "nearestConnector"?

these two methods take arguments and are not normal properties so they retain
the 'get' prefix.

[bcsaller, 2013-02-01 15:44:03]

*** Submitted:

View model improvements

Change BoundingBoxes to retain reference to module. This allows box models
to directly resolve the db.Model backing them. This also allow access to the 
DOMNode in the canvas directly. Certain features of box directly depend on
methods being available in the passed in module, mocking this is still
possible and shown in tests.

R=teknico, gary.poster, matthew.scott
CC=
https://codereview.appspot.com/7231067