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