Rollup of Databinding and View Container work

Rollup of Databinding and View Container work

This branch adds the View Container, Databinding Engine,
Viewlet, and View Controller into the source tree under
the flag `serviceInspector` to show the start of a  service 
inspector prototype. 

The easiest way to use is to use the flag `serviceInspector`.
Click on a service then click 'view', an inspector panel
will open (it allows multiple inspectors open at once) with
two tabs, one for displaying the title and the other the units.
If you continue to watch the units page you will see it change
as the simulator modifies the service.

https://code.launchpad.net/~hatch/juju-gui/service_inspector/+merge/168826

(do not edit description out of merge proposal)

[jeff.pihach, 2013-06-11 22:38:40]

Please take a look.

[rharding, 2013-06-11 23:20:51]

Questions below. Sorry if I'm being dense playing 'code review catchup'.

https://codereview.appspot.com/10019050/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10019050/diff/1/app/models/models.js#newcode34
app/models/models.js:34: var _process_delta = function(list, action,
change_data, change_base) {
underscores? the horror!

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

https://codereview.appspot.com/10019050/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:46: 'get': function(node) { return node.get('text');},
you have an open space but not a close one?

https://codereview.appspot.com/10019050/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:108: * @param {Object} config A bindings Object, see
description in `bind`.
add viewlet param

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

https://codereview.appspot.com/10019050/diff/1/app/views/environment.js#newco...
app/views/environment.js:87: var existing = this._inspectors[name];
couldn't this just be existing.destroy() and it would not need to worry about
doing these extra steps in this code but the inspector itself worries about it?
It sucks that the View has to worry about proper inspector cleanup.

https://codereview.appspot.com/10019050/diff/1/app/views/environment.js#newco...
app/views/environment.js:92: delete this._inspectors[name];
same here, seems like if remove is there we'd run destroy. why does remove not
remove the container and events?

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

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1065
app/views/service.js:1065: };
add a space please

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1069
app/views/service.js:1069: @method ServiceInspector
no @params defined

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1089
app/views/service.js:1089: true, undefined, 0, true);
this is a little hard to read. Mixing 0, true, undefined?

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1096
app/views/service.js:1096: // XXX: to debug
what about using a feature flag to detect if it should be on the window object
for debugging? Or a config setting? How will we know to go back and remove this
XXX down the road?

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1117
app/views/service.js:1117: views.ServiceInspector = ServiceInspector;
:(

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

https://codereview.appspot.com/10019050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1272: serviceInspector.inspector.after('destroy',
function(e) {
see here we've got an event based on destroy, but in the previous code it wasn't
run.

https://codereview.appspot.com/10019050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1273: setInspector(e.currentTarget, true);
is this a Node instance? In looking at the previous code I'd expected a string
since it was used as an index.

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:89: Viewlet to, usually one nested in the model
passed to the View Container.
This reads confusingly. "specify another model to bind...usually one nested in
the model..." There's a model inside a model?

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:128: ViewContainer class for rendering a parent view
container which manages the
Would this just be better named ViewletManager then?

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:199: getName: function() {
so the name of the container is the model id? If there are multiple viewlets can
there be multiple models?

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:222: var viewletContainer =
container.one(this.viewletContainer);
if this is a selector can we call it selector? viewletContainer makes it seem
like it'd be a Node instance or something.

[gary.poster, 2013-06-12 02:58:53]

Just looked at the doc so far.  These are the thoughts I had for it.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst
File docs/viewlet-data-binding.rst (right):

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:5: The Juju GUI inspector panels are build using a
light-weight custom viewlet
built

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:18: Uppon instantiation the View Container creates
new classes for all of the
Upon instantiation, the

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:33: with the exception that it takes an event
object as a configuration value on
takes an "events" object

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:39: - viewletConfig: Configuration for the
`Viewlets`_.
From your example, it looks like this is not "viewletConfig" but "viewlets." 
Which is it?

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:94: - rebind: A function that returns a model or
model list in which to bind this
rename to getBindTarget or similar?

and suggest rewrite:"to which this viewlet should be bound"

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:103: `container` property with the compiled
output.
Am I right in assuming that this method is only called once for a given
model/viewlet combination, and afterwards only data changes (for models) or
update (for model lists) are used?  If so, I suggest clarifying that.

[jeff.pihach, 2013-06-12 04:21:30]

Thanks for the reviews, comments are below and updates, where mentioned, will be
coming shortly.

https://codereview.appspot.com/10019050/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10019050/diff/1/app/models/models.js#newcode34
app/models/models.js:34: var _process_delta = function(list, action,
change_data, change_base) {
lol

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

https://codereview.appspot.com/10019050/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:108: * @param {Object} config A bindings Object, see
description in `bind`.
On 2013/06/11 23:20:51, rharding wrote:
> add viewlet param

Done.

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

https://codereview.appspot.com/10019050/diff/1/app/views/environment.js#newco...
app/views/environment.js:87: var existing = this._inspectors[name];
I agree this isn't ideal - this tear down will likely be moved into the
Controller which is still under prototype - see the topology/service.js for the
comments on the destroy()

https://codereview.appspot.com/10019050/diff/1/app/views/environment.js#newco...
app/views/environment.js:92: delete this._inspectors[name];
See above and comment in topology/service.js The delete is here to make sure
that there isn't any hanging around references to destroyed instances to aid in
GC.

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

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1069
app/views/service.js:1069: @method ServiceInspector
This is still a prototype and will likely change extensively over the next few
days, sorry I didn't include that in the comments.

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

https://codereview.appspot.com/10019050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1272: serviceInspector.inspector.after('destroy',
function(e) {
the inspector calls it's own destroy method when the user clicks the X which
then emits the destroy method automatically. 

We decided we would try and keep the collection of inspectors on the environment
because they can be instantiated from many different places in the app. I agree
that this implementation is not ideal and will be looking at a better way to
accomplish this cross class setup.

https://codereview.appspot.com/10019050/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1273: setInspector(e.currentTarget, true);
because this is after a destroy call e.currentTarget refers to the class being
destroyed.

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js
File app/views/view-container.js (right):

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:89: Viewlet to, usually one nested in the model
passed to the View Container.
model inception! I agree it's a little confusing on face value but it will make
sense once the service inspector is completed.

The service model contains reference to other models, like the unit modellist,
so the viewlets can be rebound to other models, like these nested ones.

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:128: ViewContainer class for rendering a parent view
container which manages the
Hmm... I like that, will strongly consider it.

https://codereview.appspot.com/10019050/diff/1/app/views/view-container.js#ne...
app/views/view-container.js:199: getName: function() {
Every service has it's own model and this uses the primary model passed in as
the ID. You can only have a single instance per service/model so this saves us
from having to keep some third party id's in sync.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst
File docs/viewlet-data-binding.rst (right):

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:5: The Juju GUI inspector panels are build using a
light-weight custom viewlet
On 2013/06/12 02:58:54, gary.poster wrote:
> built

Done.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:18: Uppon instantiation the View Container creates
new classes for all of the
On 2013/06/12 02:58:54, gary.poster wrote:
> Upon instantiation, the

Done.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:33: with the exception that it takes an event
object as a configuration value on
On 2013/06/12 02:58:54, gary.poster wrote:
> takes an "events" object

Done.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:39: - viewletConfig: Configuration for the
`Viewlets`_.
viewlets, sorry I used the internal property name instead of the config key.
changed.

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:94: - rebind: A function that returns a model or
model list in which to bind this
Ben and I were trying to come up with a better name for this. maybe
changeBindTarget ?

https://codereview.appspot.com/10019050/diff/1/docs/viewlet-data-binding.rst#...
docs/viewlet-data-binding.rst:103: `container` property with the compiled
output.
Yes you are correct, however in our prototyping we did find it useful to call
the render method from update sometimes. I have updated the docs to reflect
that.

[jeff.pihach, 2013-06-12 04:23:57]

Please take a look.

[gary.poster, 2013-06-12 14:08:27]

LGTM with comments.  Nice progress.  Thanks!

Gary

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:264: // Apply this action for this instance to alll service
models as well.
all

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:265: // In the future we can transition from using db.units
to always
Do you mean that we can get rid of db.units entirely?  I know you mentioned that
over another channel, but I want to make sure I understand what you mean here.

Can we get rid of db.units as part of this larger effort (not this branch!), if
we really need to change the architecture?  I don't like the duplication.

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:266: // looking at db.serviices[serviceId].units
services

https://codereview.appspot.com/10019050/diff/12001/app/templates/view-contain...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10019050/diff/12001/app/templates/view-contain...
app/templates/view-container.handlebars:2: <div class="close" style="float:
right; border: 1px solid black; padding: 3px;">X</div>
Maybe we should put an XXX around the inline style bits?  I assume we don't want
this after we are in prototype mode

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js#n...
app/views/databinding.js:109: * #param {Object} viewlet A reference to the
viewlet being bound.
@param?

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js#n...
app/views/databinding.js:189: }
 else {
   viewletModel = model;
}


right?

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1067: Constructor for View Container Constroller
Controller

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1095: this.inspector.showViewlet('overview');
I like this code a lot.  Very nice top-down use of components, avoiding
inversion of control.  Clear to read, and clear to see what does what.  I can
imagine small changes in the future to streamline this (like combining the
instantiation of the binding engine and the bind call, maybe, into a helper
function that does both?) but this seems like a great direction to me.  Thanks!

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1096: // XXX: to debug
I wonder if this indicates that it should be easier to get the
ServiceInspector(s) from the top level--some path from the app object?

https://codereview.appspot.com/10019050/diff/12001/docs/viewlet-data-binding.rst
File docs/viewlet-data-binding.rst (right):

https://codereview.appspot.com/10019050/diff/12001/docs/viewlet-data-binding....
docs/viewlet-data-binding.rst:6: data-binding micro framework with four discreet
components `View Container`_,
discrete

[gary.poster, 2013-06-12 14:12:32]

A few comments because I was curious about what you all were saying about this
code I liked. :-)

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

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1069
app/views/service.js:1069: @method ServiceInspector
On 2013/06/12 04:21:30, jeff.pihach wrote:
> This is still a prototype and will likely change extensively over the next few
> days, sorry I didn't include that in the comments.

I still like it. :-)

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1089
app/views/service.js:1089: true, undefined, 0, true);
On 2013/06/11 23:20:51, rharding wrote:
> this is a little hard to read. Mixing 0, true, undefined? 

+1, simply because Y.mix is not a clear API IMO.  Not sure what would be better.

https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1117
app/views/service.js:1117: views.ServiceInspector = ServiceInspector;
On 2013/06/11 23:20:51, rharding wrote:
> :(

Sad face because this could be done at definition time, above, I assume?  If so,
+1, though I don't feel sad about it. :-)

[rharding, 2013-06-12 14:15:48]

LGTM so hopefully some useful feedback for the future. I understand things are
still in flux.

[rharding, 2013-06-12 14:16:57]

On 2013/06/12 14:12:32, gary.poster wrote:
>
https://codereview.appspot.com/10019050/diff/1/app/views/service.js#newcode1117
> app/views/service.js:1117: views.ServiceInspector = ServiceInspector;
> On 2013/06/11 23:20:51, rharding wrote:
> > :(
> 
> Sad face because this could be done at definition time, above, I assume?  If
so,
> +1, though I don't feel sad about it. :-)

Yea, that's just me being a pita about the convention of putting a definition
down at the bottom of the file. I understand it's convention sometimes, but it's
not like we follow all conventions out there :)

[jeff.pihach, 2013-06-12 15:02:11]

Thanks for the reviews, changes have been made and are incoming. I'll also be
XXXing the areas which we know will be changing in the prototype.

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:264: // Apply this action for this instance to alll service
models as well.
On 2013/06/12 14:08:27, gary.poster wrote:
> all

Done.

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:265: // In the future we can transition from using db.units
to always
as far as we understand it it should be fairly low impact to remove db.units
however we didn't spend time investigating. So I'd like to do it in a followup
after the stable version of this code lands.

https://codereview.appspot.com/10019050/diff/12001/app/models/models.js#newco...
app/models/models.js:266: // looking at db.serviices[serviceId].units
On 2013/06/12 14:08:27, gary.poster wrote:
> services

Done.

https://codereview.appspot.com/10019050/diff/12001/app/templates/view-contain...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10019050/diff/12001/app/templates/view-contain...
app/templates/view-container.handlebars:2: <div class="close" style="float:
right; border: 1px solid black; padding: 3px;">X</div>
I fully expect the contents of this file to be completely gone as soon as we
start on the real layout. Will add XXX

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js#n...
app/views/databinding.js:109: * #param {Object} viewlet A reference to the
viewlet being bound.
On 2013/06/12 14:08:27, gary.poster wrote:
> @param?

Done.

https://codereview.appspot.com/10019050/diff/12001/app/views/databinding.js#n...
app/views/databinding.js:189: }
viewletModel is init'd with `model` (see 178)

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1067: Constructor for View Container Constroller
On 2013/06/12 14:08:27, gary.poster wrote:
> Controller

Done.

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1095: this.inspector.showViewlet('overview');
I was thinking that it would be difficult to follow maybe that was unfounded. :)

https://codereview.appspot.com/10019050/diff/12001/app/views/service.js#newco...
app/views/service.js:1096: // XXX: to debug
Yeah there are some issues with regards to data access across, in this case, the
environment, the topology service view and this service view. that we haven't
fully ironed out yet.

https://codereview.appspot.com/10019050/diff/12001/docs/viewlet-data-binding.rst
File docs/viewlet-data-binding.rst (right):

https://codereview.appspot.com/10019050/diff/12001/docs/viewlet-data-binding....
docs/viewlet-data-binding.rst:6: data-binding micro framework with four discreet
components `View Container`_,
On 2013/06/12 14:08:27, gary.poster wrote:
> discrete

Done.

[jeff.pihach, 2013-06-12 15:47:38]

*** Submitted:

Rollup of Databinding and View Container work

This branch adds the View Container, Databinding Engine,
Viewlet, and View Controller into the source tree under
the flag `serviceInspector` to show the start of a  service 
inspector prototype. 

The easiest way to use is to use the flag `serviceInspector`.
Click on a service then click 'view', an inspector panel
will open (it allows multiple inspectors open at once) with
two tabs, one for displaying the title and the other the units.
If you continue to watch the units page you will see it change
as the simulator modifies the service.

R=rharding, gary.poster
CC=
https://codereview.appspot.com/10019050