Merge lp://staging/~bcsaller/juju-gui/viewmodel-improvements into lp://staging/juju-gui/experimental

Reviewers: mp+145755_code.launchpad.net,

Message:
Please take a look.

Description:
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)

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

Affected files:
   A [revision details]
   M app/views/service.js
   M app/views/topology/service.js
   M app/views/utils.js
   M test/test_environment_view.js
   M test/test_service_module.js
   M test/test_topology.js
   M undocumented

Hey Ben. I was just skimming this and admiring it in a pre-review kind of way when I noticed that there's a conflict in test/test_service_module.js. Could you resolve it before anyone digs in?

Thanks

Gary

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#newcode102
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#newcode902
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#newcode846
test/test_environment_view.js:846: // update using pos
s/pos/position/ ?

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

https://codereview.appspot.com/7231067/

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.

https://codereview.appspot.com/7231067/

Please take a look.

https://codereview.appspot.com/7231067/

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#newcode102
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#newcode201
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#newcode242
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#newcode243
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#newcode245
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.)

https://codereview.appspot.com/7231067/

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.

https://codereview.appspot.com/7231067/

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.

https://codereview.appspot.com/7231067/

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.

https://codereview.appspot.com/7231067/

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#newcode779
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#newcode256
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#newcode910
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.

https://codereview.appspot.com/7231067/

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#newcode243
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#newcode245
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.

https://codereview.appspot.com/7231067/

*** 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

https://codereview.appspot.com/7231067/