Move all env code into single module

Move all env code into single module

This branch is a small incremental step towards a refactored env view.
While not creating many improvements itself its now simpler to begin
parallel work around factoring the MegaModule (which will be gone 
when this is all done) into proper modules, ServiceModule, RelationModule,
etc.

https://code.launchpad.net/~bcsaller/juju-gui/ultra-mega/+merge/138596

(do not edit description out of merge proposal)

[bcsaller, 2012-12-06 22:30:48]

Please take a look.

[matthew.scott, 2012-12-08 00:02:42]

Minors and suggestions, but approved.

I think this is a really good step forward on the topology stuff.  Since a lot
of this is fitting the current code into the new framework, much of the code is
the same, though I did catch a few minors in there.  I wonder if some additional
comments in mega.js might help for the future branches direction different bits
of code to different future branches?  Additionally, there are some nested
functions that may be torn up and moved about (there's a method that draws both
services and relations, for example).  These aren't necessarily for this branch,
but it might be good to have some of this in writing, even if just in code
reviews, to refer to later.

Thanks again for working on this; looking forward to better separation!

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:189: if (handler.context) {
Could these two ifs be combined to help readability?

https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handlebars
File app/templates/overview.handlebars (right):

https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handleba...
app/templates/overview.handlebars:1: <div class="topology">
<bikeshedding>With this and other changes, part of me wants to rename the
overview.handlebars file to topology.handlebars or something similar, since the
concept and use of the topology is changing.</bikeshedding>

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

https://codereview.appspot.com/6867072/diff/1/app/views/environment.js#newcode58
app/views/environment.js:58: // XXX: vomit
Not a fan, I take it?

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:538: // TODO:: figure out a clean way to update
position
I don't think this comment applies anymore, but will need to make sure.

[benji, 2012-12-10 18:00:02]

Thanks for working on this.  We are certainly headed in the right direction.  I
especially like that you are not afraid to land a branch that is an improvement
even though there is more to be done.  Iteration is a powerful thing.  I had
several questions and noted several small things that need to be fixed.

Thanks again.

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:351: * Called the first time render is
invoked. See {render}.
The standard yuidoc way, and the way all the existing yuidoc in the project is
written is with the @ directives at the end of the comment, not the beginning.

We have also been including @return and @param (@param isn't applicable here,
but I didn't want to repeat this for all the comments in the branch).

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

https://codereview.appspot.com/6867072/diff/1/app/views/environment.js#newcode19
app/views/environment.js:19: * @namespace juju.views
The yuidoc specs prescribe not including the "base" namespace name ("juju") in
@namespace directives.  I don't know why, but that also means I can't argue
against them. :)

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:3: /**
This needs an @module directive.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:13: **/
This comment is very helpful.  I especially like the direction to reviewers
about code being added here being suspect.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:165: console.log('View: Initialized: Env');
Should "Env" here be "MegaModule"?  Or, better yet, this style of logging
doesn't seem to provide much value to me so we could just do without.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:186: * Construct a persistent scene that is managed
in update.
This line and the next need to be indented by one more space.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:303: * @param {object} d Unused.
The body of this comment is indented too far.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:321: * Sync view models with current db.models.
Another indent problem.  Cut/paste bug maybe?  All the comments below show the
same symptom, so I won't repeat this for each one.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:365: tree = this.tree,
More weird indentation.  Maybe you should dedent the entire file and let the
beautifier sort out the dead.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/panzoom.js#n...
app/views/topology/panzoom.js:9: * @module topology-service
Shouldn't the module name have something to do with panning and zooming?

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/relation.js#...
app/views/topology/relation.js:9: * @module topology-service
This module name is the same as another file.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/relation.js#...
app/views/topology/relation.js:10: * @class Service
This doesn't look like the right class name.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:10: * @class Service
Shouldn't this be "ServiceModule"?

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:18: right: 0.084848},
I expect this is pre-existing code, but wow is that a very exact number.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:52: .on('dragstart', function(d) {
This is an odd way to format chained calls.

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:345: 
There are a couple of extra newlines here.

https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc
File bin/lint-yuidoc (right):

https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc#newcode160
bin/lint-yuidoc:160: main()
I don't think we want this new behavior.

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
File test/test_application_notifications.js (right):

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
test/test_application_notifications.js:198: it.skip('should show notification
for "add_relation" and "remove_relation"' +
Do we still need these skips?  If so, we should have a bug and/or card to fix
them.

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
test/test_application_notifications.js:237: env = {
I am not sure what the diff is trying to tell me for these lines.  Any ideas?

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js#newcode51
test/test_topology.js:51: '</div>');
We are using chained node operations to build the test container now.  The other
tests give good examples.

[benjamin.saller, 2012-12-11 04:32:36]

Thanks for these reviews. I went through and made changes and commented on most
everything and will re-propose soon.

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:351: * Called the first time render is
invoked. See {render}.
On 2012/12/10 18:00:02, benji wrote:
> The standard yuidoc way, and the way all the existing yuidoc in the project is
> written is with the @ directives at the end of the comment, not the beginning.
> 
> We have also been including @return and @param (@param isn't applicable here,
> but I didn't want to repeat this for all the comments in the branch).

I did the rearrangement, but I didn't add @return and @param to most of the
methods, very few are callable in a normal sense, most are either chainable
(which I've made an effort to label so) or are event handlers and return
nothing. As methods are moved to individual modules the standard for both
testing and docs will increase.

https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handlebars
File app/templates/overview.handlebars (right):

https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handleba...
app/templates/overview.handlebars:1: <div class="topology">
On 2012/12/08 00:02:42, matthew.scott wrote:
> <bikeshedding>With this and other changes, part of me wants to rename the
> overview.handlebars file to topology.handlebars or something similar, since
the
> concept and use of the topology is changing.</bikeshedding>

I do agree, but I think we'll find there is a topology and then there is the
main env view template (which includes menus and action items in its shell).
I'll keep in mind that we should factor out the core of the template for use in
other cases (like showing read only topos.

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

https://codereview.appspot.com/6867072/diff/1/app/views/environment.js#newcode19
app/views/environment.js:19: * @namespace juju.views
On 2012/12/10 18:00:02, benji wrote:
> The yuidoc specs prescribe not including the "base" namespace name ("juju") in
> @namespace directives.  I don't know why, but that also means I can't argue
> against them. :)

Done.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:365: tree = this.tree,
On 2012/12/10 18:00:02, benji wrote:
> More weird indentation.  Maybe you should dedent the entire file and let the
> beautifier sort out the dead.

Yeah, I re-ran it w/o the initial dedent and this is the kind of thing that
happened, before that there were something like 800 errors coming out of the
linter.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/panzoom.js#n...
app/views/topology/panzoom.js:9: * @module topology-service
On 2012/12/10 18:00:02, benji wrote:
> Shouldn't the module name have something to do with panning and zooming?

Yeah, these other modules are just templates really, they are dead code now, not
loaded via addModule, I could just empty them out, but I'll correct things like
this as the next branches will fill these out.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:18: right: 0.084848},
On 2012/12/10 18:00:02, benji wrote:
> I expect this is pre-existing code, but wow is that a very exact number.

Yeah, it is. There are many levels of refactoring that should occur as this
progresses, but we're on layout now, then structure, then I think details.

https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc
File bin/lint-yuidoc (right):

https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc#newcode160
bin/lint-yuidoc:160: main()
On 2012/12/10 18:00:02, benji wrote:
> I don't think we want this new behavior.

using the version from trunk now

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
File test/test_application_notifications.js (right):

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
test/test_application_notifications.js:198: it.skip('should show notification
for "add_relation" and "remove_relation"' +
On 2012/12/10 18:00:02, benji wrote:
> Do we still need these skips?  If so, we should have a bug and/or card to fix
> them.

For now, these have lines like 

views.environment.prototype.service_click_actions

and depend pretty deeply on implementation details of the 
env view. I suspect in the future we can look at the notifications database and
see if entries were added 
after actions are triggered, but we'll see, happy to add a 
card for this as the intent of the tests is valid.

https://codereview.appspot.com/6867072/diff/1/test/test_application_notificat...
test/test_application_notifications.js:237: env = {
On 2012/12/10 18:00:02, benji wrote:
> I am not sure what the diff is trying to tell me for these lines.  Any ideas?
I think they were reindented

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js#newcode51
test/test_topology.js:51: '</div>');
On 2012/12/10 18:00:02, benji wrote:
> We are using chained node operations to build the test container now.  The
other
> tests give good examples.

I didn't find a better example. Is chaining better than a template in the test
dir?

[bcsaller, 2012-12-11 04:43:32]

Please take a look.

[benji, 2012-12-11 14:22:39]

Thanks for the tweaks and answers.  I replied to some of your comments below.

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-compo...
app/assets/javascripts/d3-components.js:351: * Called the first time render is
invoked. See {render}.
On 2012/12/11 04:32:36, benjamin.saller wrote:
> On 2012/12/10 18:00:02, benji wrote:
> > The standard yuidoc way, and the way all the existing yuidoc in the project
is
> > written is with the @ directives at the end of the comment, not the
beginning.
> > 
> > We have also been including @return and @param (@param isn't applicable
here,
> > but I didn't want to repeat this for all the comments in the branch).
> 
> I did the rearrangement, but I didn't add @return and @param to most of the
> methods, very few are callable in a normal sense, most are either chainable
> (which I've made an effort to label so) or are event handlers and return
> nothing. As methods are moved to individual modules the standard for both
> testing and docs will increase.

Sounds like a plan.  Eventually we should have @return directives everywhere so
we can tell the difference between functions that don't return anything and
functions that we haven't documented.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newc...
app/views/topology/mega.js:365: tree = this.tree,
On 2012/12/11 04:32:36, benjamin.saller wrote:
> On 2012/12/10 18:00:02, benji wrote:
> > More weird indentation.  Maybe you should dedent the entire file and let the
> > beautifier sort out the dead.
> 
> Yeah, I re-ran it w/o the initial dedent and this is the kind of thing that
> happened, before that there were something like 800 errors coming out of the
> linter.

Yow.  I think we are a little ahead of the curve on JS and enforcing style.  Not
that we should change course, I still think the value is slightly greater than
the cost.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/panzoom.js#n...
app/views/topology/panzoom.js:9: * @module topology-service
On 2012/12/11 04:32:36, benjamin.saller wrote:
> On 2012/12/10 18:00:02, benji wrote:
> > Shouldn't the module name have something to do with panning and zooming?
> 
> Yeah, these other modules are just templates really, they are dead code now,
not
> loaded via addModule, I could just empty them out, but I'll correct things
like
> this as the next branches will fill these out.

Cool.

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

https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#n...
app/views/topology/service.js:18: right: 0.084848},
On 2012/12/11 04:32:36, benjamin.saller wrote:
> On 2012/12/10 18:00:02, benji wrote:
> > I expect this is pre-existing code, but wow is that a very exact number.
> 
> Yeah, it is. There are many levels of refactoring that should occur as this
> progresses, but we're on layout now, then structure, then I think details.

Sounds good.

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/6867072/diff/1/test/test_topology.js#newcode51
test/test_topology.js:51: '</div>');
On 2012/12/11 04:32:36, benjamin.saller wrote:
> On 2012/12/10 18:00:02, benji wrote:
> > We are using chained node operations to build the test container now.  The
> other
> > tests give good examples.
>
> I didn't find a better example.

Here is a translation of the HTML above into directly-created elements:

container = Y.Node.create('<div/>')
    .setStyle('visibility', 'hidden')
    .append(Y.Node.create('<button/>)
        .addClass('thing'))
    .append(Y.Node.create('<button/>)
        .addClass('target'))

> Is chaining better than a template in the test dir?

Two things would make me shy away from a template: first, the test container is
usually very simple -- often just a plain-vanilla div -- and second, it varies
from test to test.  I'd hate to see a dozen 10 byte templates littering up the
test directory.

https://codereview.appspot.com/6867072/diff/12001/app/assets/javascripts/d3-c...
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6867072/diff/12001/app/assets/javascripts/d3-c...
app/assets/javascripts/d3-components.js:400: *
I think this blank line was supposed to go above the @method.

[bcsaller, 2012-12-11 15:56:58]

*** Submitted:

Move all env code into single module

This branch is a small incremental step towards a refactored env view.
While not creating many improvements itself its now simpler to begin
parallel work around factoring the MegaModule (which will be gone 
when this is all done) into proper modules, ServiceModule, RelationModule,
etc.

R=matthew.scott, benji, benjamin.saller
CC=
https://codereview.appspot.com/6867072