Merge lp://staging/~gary/juju-gui/nodeChangeHandler into lp://staging/juju-gui/experimental

Reviewers: mp+184901_code.launchpad.net,

Message:
Please take a look.

Description:
Refactor and rename BindingEngine._storeChanged

This is a small clean-up branch in preparation to cleaning up some of
the conflict resolution edge cases. It is largely about refactoring and
renaming BindingEngine._storeChanged.

* BindingEngine._storeChanged is now called _nodeChangeHandler.
* That method simply calls out to the new method _nodeChanged. It takes
a node rather than an event with a node.
* Factored _getBinding out of _nodeChanged to make the code read better
and to test the functionality independently.
* Adjusted tests to use _nodeChanged.
* Refactored tests to reuse more code and clean up some lint.
* Added tests for _getBinding.

In addition to those related changes, I made one more peripheral change:
I removed an unused argument to unsyncedFields.

There's not much to QA here. It's just moving bits of code around.

https://code.launchpad.net/~gary/juju-gui/nodeChangeHandler/+merge/184901

(do not edit description out of merge proposal)

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

Affected files (+95, -51 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/inspector.js
   M test/test_databinding.js
   M test/test_inspector_constraints.js
   M test/test_inspector_settings.js

LGTM, minor suggestions, take it or leave it, thanks for the cleanups

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

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode668
app/views/databinding.js:668: @return {Binding} Binding reference.
This is a case, but not the only one, a single model key can have more
than one node bound and thus more than one binding. Depending on your
use case this maybe ok, but I suspect you'd always want to return an
array of all bindings for a given key.

The other option that works with the current code is to also pass a node
target, and say get the binding for (key) where binding.target ===
target. This should always be one.

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode711
app/views/databinding.js:711: if (nodeHandler.eq(node,
binding.get(model))) {
This reads nicely now, thank you. See the notes about getBinding though

https://codereview.appspot.com/13265045/diff/1/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13265045/diff/1/test/test_databinding.js#newcode35
test/test_databinding.js:35: };
nice cleanup, I'd be tempted to add 'options' to the arguments and mix
it in object create to allow add/overwrite viewlet code but you contain
a superset of what the tests need so no worries.

https://codereview.appspot.com/13265045/

On 2013/09/11 01:37:51, benjamin.saller wrote:
> LGTM, minor suggestions, take it or leave it, thanks for the cleanups

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

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode668
> app/views/databinding.js:668: @return {Binding} Binding reference.
> This is a case, but not the only one, a single model key can have more
than one
> node bound and thus more than one binding. Depending on your use case
this maybe
> ok, but I suspect you'd always want to return an array of all bindings
for a
> given key.

> The other option that works with the current code is to also pass a
node target,
> and say get the binding for (key) where binding.target === target.
This should
> always be one.

Ah! Great point, thank you. I'll take the second route. That's what
this use case needs.

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode711
> app/views/databinding.js:711: if (nodeHandler.eq(node,
binding.get(model))) {
> This reads nicely now, thank you. See the notes about getBinding
though

https://codereview.appspot.com/13265045/diff/1/test/test_databinding.js
> File test/test_databinding.js (right):

https://codereview.appspot.com/13265045/diff/1/test/test_databinding.js#newcode35
> test/test_databinding.js:35: };
> nice cleanup, I'd be tempted to add 'options' to the arguments and mix
it in
> object create to allow add/overwrite viewlet code but you contain a
superset of
> what the tests need so no worries.

Yeah, I like that idea, but I'll leave that to The Future. :-)

Thank you!

https://codereview.appspot.com/13265045/

*** Submitted:

Refactor and rename BindingEngine._storeChanged

This is a small clean-up branch in preparation to cleaning up some of
the conflict resolution edge cases. It is largely about refactoring and
renaming BindingEngine._storeChanged.

* BindingEngine._storeChanged is now called _nodeChangeHandler.
* That method simply calls out to the new method _nodeChanged. It takes
a node rather than an event with a node.
* Factored _getBinding out of _nodeChanged to make the code read better
and to test the functionality independently.
* Adjusted tests to use _nodeChanged.
* Refactored tests to reuse more code and clean up some lint.
* Added tests for _getBinding.

In addition to those related changes, I made one more peripheral change:
I removed an unused argument to unsyncedFields.

R=benjamin.saller
CC=
https://codereview.appspot.com/13265045

https://codereview.appspot.com/13265045/