Refactor and rename BindingEngine._storeChanged

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.

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

(do not edit description out of merge proposal)

[gary.poster, 2013-09-11 01:04:33]

Please take a look.

[benjamin.saller, 2013-09-11 01:37:51]

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#newco...
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#newco...
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#newco...
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.

[gary.poster, 2013-09-11 01:53:41]

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#newco...
> 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#newco...
> 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#newco...
> 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!

[gary.poster, 2013-09-11 02:15:55]

*** 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