Prototype of data binding conflict resolution

Prototype of data binding conflict resolution

This branch changes the way we implement data binding by
moving the bind keys into a data-attribute on the elements.
This makes it easy to have dynamic templates bound to
dynamic attributes on a model.

When changes come in from Juju the databinding engine checks
for dirty fields, a field becomes dirty if the YUI valueChange
event would fire on it. If it finds that a change has come in
for a dirty field it calls the viewlets conflict method which
handles the conflict resolution UI. After the user has resolved
the conflict the field it then returns to a clean state and can
be updated automatically by the databinding engine. 

Please QA

Steps to QA:
Load the application with the flag `serviceInspector`
Deploy the `glance` charm.
Click the service and `view` to open the new inspector
Click `Config` to display the configuration properties
Open the console, paste and run::

g = app.db.services.item(0);gc = g.get('config');gc['db-user'] = 'asdf';g.set('config', gc);

You'll notice that the db-user field is updated with the new value
Edit the db-user field and run the script again
You'll now see that the field is dirty
Resolve the conflict
change the value to set in the above script and then run again
You'll see that the field changes as intended.

https://code.launchpad.net/~hatch/juju-gui/conflict-resolution/+merge/170204

(do not edit description out of merge proposal)

[jeff.pihach, 2013-06-18 21:15:56]

Please take a look.

[benjamin.saller, 2013-06-19 20:01:35]

LGTM

There is still a lot I think we can improve, but given this is behind the
feature flag I think its reasonable to move forward with it. Some of these
comments should be addressed now, but most are for the next iteration which
hopefully we can pair on again.

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

https://codereview.appspot.com/10397045/diff/1/app/models/models.js#newcode74
app/models/models.js:74: list.fire('change');
Thanks

https://codereview.appspot.com/10397045/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10397045/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:10: style="display: inline-block;
margin: 0 10px; cursor: pointer;">Config</div>
We should move the styles out of the template, can happen later.

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

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#oldco...
app/views/databinding.js:183: if (this.model) {this.unbind();}
We have an issue with cleanup where closing an inspector seems to leave bound
events, we'll have to fix that in the next iteration as well.

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

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:70: return
(modelChangeKeys.indexOf(binding.name.split('.')[0]) > -1);
This needs a card or to go on a list for tracking.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:201: }, viewlet);
I think removing explicit bindings isn't the right direction, we should infer
bindings when attrs are present in the DOM and not in the viewlet, but we want
the ability to add overrides to the handling of specific bindings at the viewlet
layer.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:205: 'valueChange', this._storeChanged, this,
viewlet));
I think we can forgo the valueChange for the HTML5 'change' event which forms
now fire.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:259: BindingEngine.prototype._storeChanged =
function(e, viewlet) {
We want to keep the old/new version in JS here at the bind engine layer. I'm not
sure if getData is using its own index on the node (which is ok) or doing
something like HTML5 dataset which in our case isn't great as we'll repeatedly
touch the DOM and it will get slow.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:309: // this shows up on a profile.
wrt my comment here we can bind a HTML5 MutationObserver to the viewlets DOM and
recompute the explicit/implicit bindings when its mutated outside of this update
cycle.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:322: binding.viewlet.conflict(
Conflict/resolve is reasonable for now though I might call conflict, 'merge'.
The current impl binds the data handling and the UI more than I like but its
fine for now.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:358: }
resolve worked out pretty well :)

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

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1076
app/views/service.js:1076: handler.detach();
handler's var def should be moved before its first usage.

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1079
app/views/service.js:1079: resolve(node, viewletName, newVal);
These should be add/removeClass calls

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1088
app/views/service.js:1088: newVal =
model.get('config')[node.getData('bind').split('.')[1]];
as noted before this pattern could be improved.

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

https://codereview.appspot.com/10397045/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1333: });
This shouldn't be needed, we use service.units as its custom model list now,
what am I missing?

[teknico, 2013-06-20 15:53:46]

LGTM, even if it doesn't mean much yet. :-) Will QA later.

[jeff.pihach, 2013-06-20 16:43:01]

Thanks for the reviews guys, I have created a number of cards on the board to
address the things we discussed Ben.

https://codereview.appspot.com/10397045/diff/1/app/templates/view-container.h...
File app/templates/view-container.handlebars (right):

https://codereview.appspot.com/10397045/diff/1/app/templates/view-container.h...
app/templates/view-container.handlebars:10: style="display: inline-block;
margin: 0 10px; cursor: pointer;">Config</div>
Agreed, this is just ugly prototype code so it's easy to `ctrl+a del` when we
write the real layout :)

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

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#oldco...
app/views/databinding.js:183: if (this.model) {this.unbind();}
On 2013/06/19 20:01:35, benjamin.saller wrote:
> We have an issue with cleanup where closing an inspector seems to leave bound
> events, we'll have to fix that in the next iteration as well.

Done.

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

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:70: return
(modelChangeKeys.indexOf(binding.name.split('.')[0]) > -1);
On 2013/06/19 20:01:35, benjamin.saller wrote:
> This needs a card or to go on a list for tracking.

Done.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:201: }, viewlet);
Agreed

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:205: 'valueChange', this._storeChanged, this,
viewlet));
That would be nice - will have to see if it's worth the additional work to write
the callback and manage the handlers

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:259: BindingEngine.prototype._storeChanged =
function(e, viewlet) {
This method is only called once the user interacts with the element. A reference
to the element is stored in the event facade so it isn't touching the DOM at
all.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:309: // this shows up on a profile.
Good idea

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:322: binding.viewlet.conflict(
On 2013/06/19 20:01:35, benjamin.saller wrote:
> Conflict/resolve is reasonable for now though I might call conflict, 'merge'.
> The current impl binds the data handling and the UI more than I like but its
> fine for now.

Done.

https://codereview.appspot.com/10397045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:358: }
:)

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

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1076
app/views/service.js:1076: handler.detach();
I agree however the linter wouldn't pass without me pre-hoisting the function.

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1079
app/views/service.js:1079: resolve(node, viewletName, newVal);
Agree, this is prototype code, as this will certainly be different come designs
from UX

https://codereview.appspot.com/10397045/diff/1/app/views/service.js#newcode1088
app/views/service.js:1088: newVal =
model.get('config')[node.getData('bind').split('.')[1]];
Agreed, card created

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

https://codereview.appspot.com/10397045/diff/1/app/views/topology/service.js#...
app/views/topology/service.js:1333: });
To display the charm config properties descriptions we need access to the
charms. This could definitely be tightened up so we only pass in the data we
need but I wasn't sure what we would need after all of the views were done.

[jeff.pihach, 2013-06-20 17:16:12]

*** Submitted:

Prototype of data binding conflict resolution

This branch changes the way we implement data binding by
moving the bind keys into a data-attribute on the elements.
This makes it easy to have dynamic templates bound to
dynamic attributes on a model.

When changes come in from Juju the databinding engine checks
for dirty fields, a field becomes dirty if the YUI valueChange
event would fire on it. If it finds that a change has come in
for a dirty field it calls the viewlets conflict method which
handles the conflict resolution UI. After the user has resolved
the conflict the field it then returns to a clean state and can
be updated automatically by the databinding engine. 

Please QA

Steps to QA:
Load the application with the flag `serviceInspector`
Deploy the `glance` charm.
Click the service and `view` to open the new inspector
Click `Config` to display the configuration properties
Open the console, paste and run::

g = app.db.services.item(0);gc = g.get('config');gc['db-user'] =
'asdf';g.set('config', gc);

You'll notice that the db-user field is updated with the new value
Edit the db-user field and run the script again
You'll now see that the field is dirty
Resolve the conflict
change the value to set in the above script and then run again
You'll see that the field changes as intended.

R=benjamin.saller, teknico
CC=
https://codereview.appspot.com/10397045