improve databinding changedValues accuracy

improve databinding changedValues accuracy

- more accurately keep track of changed values
- switch changedValues to being an Object
- Make changedValues public
- added eq to databinding fields

https://code.launchpad.net/~gary/juju-gui/databindingChangedValues/+merge/184407

(do not edit description out of merge proposal)

[gary.poster, 2013-09-07 01:00:39]

Please take a look.

[gary.poster, 2013-09-07 01:08:55]

Comments for reviewers.

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

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#oldco...
app/views/databinding.js:69: function _getNodeHandler(node) {
I moved this down into the class, because I needed it for tests and it seemed
reasonable and easy there.

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

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:38: 'input[type=checkbox]': Object.create({
Using Object.create is an easy way to let the other functions refer to each
other.

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:51: 'eq': function(node, value) {
I added 'eq' to all of the field handlers so that the _storeChanged method
(below) could use it.

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:272: @method getFieldHandler
This should be "getNodeHandler".  I adjusted it in my branch.  Same for similar
bits in the docstring.

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:678: BindingEngine.prototype._storeChanged =
function(e, viewlet) {
These changes are the real heart of my branch.

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:841: delete viewlet.changedValues[key];
Switching the changedValues to a hash just seemed to make so much code simpler.

[rharding, 2013-09-07 06:33:20]

LGTM with notes below. Will QA in a sec.

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

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:62: return (normalizedValue === currentValue);
should we refactor this out into any _stringEq or something since it's duped in
the three eq methods?

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:688: if (nodeHandler.eq(e.target, binding.get(model)))
{
will this blow up if no binding was found matching key?

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:875: 'event-valuechange']
is this required in this module? It's used in the inspector I believe, but not
here.

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

https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js#newco...
test/test_databinding.js:26: 'model', 'model-list', 'node-event-simulate'];
this needs the event-valueChange since you are binding with that event below.

[rharding, 2013-09-07 06:43:37]

QA ok. Looks like this is just what's needed to prevent the conflict indicators
from coming up when the value changed matches the backend changed value. Thanks
for updating it.

[gary.poster, 2013-09-09 12:38:43]

Please take a look.

[gary.poster, 2013-09-09 12:57:55]

On 2013/09/07 06:33:20, rharding wrote:
> LGTM with notes below. Will QA in a sec.
> 
> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js
> File app/views/databinding.js (right):
> 
>
https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
> app/views/databinding.js:62: return (normalizedValue === currentValue);
> should we refactor this out into any _stringEq or something since it's duped
in
> the three eq methods?

We discussed this and the other points via email (I thought Rietveld would get
the comments).  Done.

> 
>
https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
> app/views/databinding.js:688: if (nodeHandler.eq(e.target,
binding.get(model)))
> {
> will this blow up if no binding was found matching key?

In sum, this was intentional, because it should never blow up if everything is
working properly before this point in the code.  I changed the code to make this
more explicit.

> 
>
https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newco...
> app/views/databinding.js:875: 'event-valuechange']
> is this required in this module? It's used in the inspector I believe, but not
> here.

On further discussion, I pointed out that the valueChange subscription is in
databinding, so Rick agreed with me that what I did was fine.

> 
> https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js
> File test/test_databinding.js (right):
> 
>
https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js#newco...
> test/test_databinding.js:26: 'model', 'model-list', 'node-event-simulate'];
> this needs the event-valueChange since you are binding with that event below.

As above.

Thank you, Rick!

[benji, 2013-09-09 13:30:01]

This branch looks good, yet I found ways to complain anyway.

LGTM

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

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
app/views/databinding.js:37: var _textEq = function(node, value) {
The leading underscore strikes me as odd.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
app/views/databinding.js:56: return !! value;
This is just a nitpick: I don't think we normally have a space between the
double negation and the variable name.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
app/views/databinding.js:78: var _indexBindings = function(bindings, keyfunc,
multiple) {
Another leading underscore.  Maybe there a pattern I don't recognize.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
app/views/databinding.js:679: binding = b;
Tricky.  Maybe too tricky.

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#ne...
test/test_databinding.js:596: done();
Darn.  I guess this is the best we can do.

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#ne...
test/test_databinding.js:677: // of full. The system should allow for this
As a reward for making this bad comment better I will complain that it isn't
perfect:

- It's unnecessary (and slightly confusing) to have "dep" instead of (I assume)
"dependency".

- The first sentence is good enough for a period, but the second... no way!

[gary.poster, 2013-09-09 14:01:40]

On 2013/09/09 13:30:01, benji wrote:
> This branch looks good, yet I found ways to complain anyway.

:-) Thank you for the review.  Comments below.

> 
> LGTM
> 
> https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js
> File app/views/databinding.js (right):
> 
>
https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
> app/views/databinding.js:37: var _textEq = function(node, value) {
> The leading underscore strikes me as odd.

Yeah, on reflection, I agree.

>
https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
> app/views/databinding.js:56: return !! value;
> This is just a nitpick: I don't think we normally have a space between the
> double negation and the variable name.

Yeah, this was conscious because I think the "!!" is an odd "cast to bool"
operator but prefer it to an explicit Boolean().  I thought the space might call
it out a bit better, but yeah, on reflection agree that my thought is not
particularly compelling and I should stick to precedent.

> 
>
https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
> app/views/databinding.js:78: var _indexBindings = function(bindings, keyfunc,
> multiple) {
> Another leading underscore.  Maybe there a pattern I don't recognize.

<shrug>.  As before, agreed with you and removed.

> 
>
https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#ne...
> app/views/databinding.js:679: binding = b;
> Tricky.  Maybe too tricky.

Heh.  I dunno.  I thought about and decided to keep it: it seemed like it is or
will soon become an idiomatic pattern, once "some" is in a greater percentage of
existing browsers.  In deference to your reasonable concern, though, I did add a
comment that might help a reader in the future to know what is going on.  Maybe.

> 
> https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js
> File test/test_databinding.js (right):
> 
>
https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#ne...
> test/test_databinding.js:596: done();
> Darn.  I guess this is the best we can do.

Yeah, apparently. :-/  I could have tried to write a narrower test instead, but
stuck with the pre-existing, more integrated approach.

> 
>
https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#ne...
> test/test_databinding.js:677: // of full. The system should allow for this
> As a reward for making this bad comment better I will complain that it isn't
> perfect:
> 
> - It's unnecessary (and slightly confusing) to have "dep" instead of (I
assume)
> "dependency".
> 
> - The first sentence is good enough for a period, but the second... no way!

:-P OK, OK, I made these changes and a couple more I thought helped. :-)

Thanks again!

[gary.poster, 2013-09-09 14:01:55]

*** Submitted:

improve databinding changedValues accuracy

- more accurately keep track of changed values
- switch changedValues to being an Object
- Make changedValues public
- added eq to databinding fields

R=rharding, benji
CC=
https://codereview.appspot.com/13385045