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

Reviewers: mp+184407_code.launchpad.net,

Message:
Please take a look.

Description:
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

This is a prerequisite to updating the inspector save UX.

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

(do not edit description out of merge proposal)

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

Affected files (+267, -97 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/viewlet-manager.js
   M test/test_databinding.js
   M test/test_inspector_constraints.js

Reviewers: mp+184407_code.launchpad.net,

Message:
Please take a look.

Description:
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

This is a prerequisite to updating the inspector save UX.

https://codereview.appspot.com/13608043

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

(do not edit description out of merge proposal)

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

Affected files (+268, -99 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/viewlet-manager.js
   M test/test_databinding.js
   M test/test_inspector_constraints.js

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#oldcode69
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#newcode38
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#newcode51
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#newcode272
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#newcode678
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#newcode841
app/views/databinding.js:841: delete viewlet.changedValues[key];
Switching the changedValues to a hash just seemed to make so much code
simpler.

https://codereview.appspot.com/13385045/

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#newcode62
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#newcode688
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#newcode875
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#newcode26
test/test_databinding.js:26: 'model', 'model-list',
'node-event-simulate'];
this needs the event-valueChange since you are binding with that event
below.

https://codereview.appspot.com/13385045/

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.

https://codereview.appspot.com/13385045/

Hi Rick. Thank you very much for the review. I have a few points I'd like to negotiate on. Please see below.

On Sep 7, 2013, at 7:36 AM, Richard Harding <email address hidden> 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#newcode62
> 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?

Yeah, I agree we need something like that. I'll think on it and do something like this before landing.

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

Yes, but my argument is that, in that case, we have a pre-existing problem, because we should be explicitly running an addBinding for every binding we have, as I read the code. Therefore I decided to go for the "explicit error we can debug more easily rather than a silent error we'll never know about" route. Do you agree with this, but request that I add a comment to this effect; or disagree, and argue for more defensive code here?

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

This module subscribes to the valueChanged event. I think the dependency rightly belongs here, because of that. Perhaps it would be the right thing to do to remove the dependency in the inspector module, given my argument. WDYT?

>
> 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#newcode26
> test/test_databinding.js:26: 'model', 'model-list',
> 'node-event-simulate'];
> this needs the event-valueChange since you are binding with that event
> below.

Not if the module I test (databinding) itself depends on this, IMO. I only agree with you if I revert the event-valuechange dependency in databinding, but I think that the dependency arrangement I have in my branch is better than the one in trunk in this regard. Agree?

Thanks again!

Gary

>
> https://codereview.appspot.com/13385045/
>
> --
> https://code.launchpad.net/~gary/juju-gui/databindingChangedValues/+merge/184407
> You are the owner of lp:~gary/juju-gui/databindingChangedValues.

On Sat, 07 Sep 2013, Gary Poster wrote:

> >
> > https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode688
> > app/views/databinding.js:688: if (nodeHandler.eq(e.target,
> > binding.get(model))) {
> > will this blow up if no binding was found matching key?
>
> Yes, but my argument is that, in that case, we have a pre-existing
> problem, because we should be explicitly running an addBinding for every
> binding we have, as I read the code. Therefore I decided to go for the
> "explicit error we can debug more easily rather than a silent error we'll
> never know about" route. Do you agree with this, but request that I add
> a comment to this effect; or disagree, and argue for more defensive code
> here?

This is completely cool. I assumed as much as reading it. I merely brought
it up as "this looks like a potential boom point, are we double sure it's
safe to assume here". Thanks for giving it a solid look through.

> >
> > https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode875
> > app/views/databinding.js:875: 'event-valuechange']
> > is this required in this module? It's used in the inspector I believe,
> > but not here.
>
> This module subscribes to the valueChanged event. I think the dependency
> rightly belongs here, because of that. Perhaps it would be the right
> thing to do to remove the dependency in the inspector module, given my
> argument. WDYT?

Well, this is where I'm not sure. If you look in databinding.js valueChange
is never used once in the file. It's purely doing model/value lookups and
is not directly binding to the inputs themselves. It's getting passed the
results of those and they're bound in the inspector code.

I'd rewrite your statement from:

This module subscribes to the valueChanged event.

- to -

The databinding code is subscribed, by its users, to ui inputs which
occasionally use the valueChanged event.

Anyway, the reason I brought it up was purely that if you grep the file for
valueChange it won't occur and that seemed strange that it would depend on
a module it's not in fact using. I'm definitely ok landing as is, but
wanted to bring up the point/discuss it.

> > 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#newcode26
> > test/test_databinding.js:26: 'model', 'model-list',
> > 'node-event-simulate'];
> > this needs the event-valueChange since you are binding with that event
> > below.
>
> Not if the module I test (databinding) itself depends on this, IMO. I
> only agree with you if I revert the event-valuechange dependency in
> databinding, but I think that the dependency arrangement I have in my
> branch is better than the one in trunk in this regard. Agree?

As you say, this basically is fallout of the real decision in the above
notes.

Rick

On 09/07/2013 08:39 PM, Richard Harding wrote:
> On Sat, 07 Sep 2013, Gary Poster wrote:
>
>>>
>>> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode688
>>> app/views/databinding.js:688: if (nodeHandler.eq(e.target,
>>> binding.get(model))) {
>>> will this blow up if no binding was found matching key?
>>
>> Yes, but my argument is that, in that case, we have a pre-existing
>> problem, because we should be explicitly running an addBinding for every
>> binding we have, as I read the code. Therefore I decided to go for the
>> "explicit error we can debug more easily rather than a silent error we'll
>> never know about" route. Do you agree with this, but request that I add
>> a comment to this effect; or disagree, and argue for more defensive code
>> here?
>
> This is completely cool. I assumed as much as reading it. I merely brought
> it up as "this looks like a potential boom point, are we double sure it's
> safe to assume here". Thanks for giving it a solid look through.

Great.

>
>>>
>>> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode875
>>> app/views/databinding.js:875: 'event-valuechange']
>>> is this required in this module? It's used in the inspector I believe,
>>> but not here.
>>
>> This module subscribes to the valueChanged event. I think the dependency
>> rightly belongs here, because of that. Perhaps it would be the right
>> thing to do to remove the dependency in the inspector module, given my
>> argument. WDYT?
>
> Well, this is where I'm not sure. If you look in databinding.js valueChange
> is never used once in the file. It's purely doing model/value lookups and
> is not directly binding to the inputs themselves. It's getting passed the
> results of those and they're bound in the inspector code.
>
> I'd rewrite your statement from:
>
> This module subscribes to the valueChanged event.
>
> - to -
>
> The databinding code is subscribed, by its users, to ui inputs which
> occasionally use the valueChanged event.
>
> Anyway, the reason I brought it up was purely that if you grep the file for
> valueChange it won't occur and that seemed strange that it would depend on
> a module it's not in fact using. I'm definitely ok landing as is, but
> wanted to bring up the point/discuss it.

This is what I am seeing in databinding.js, and the core of my argument:

          viewlet._eventHandles.push(
              node.on('valueChange', this._storeChanged, this, viewlet));

That's in BindingEngine.prototype._bind, at line 445 in my branch and
line 422 in trunk. Is that convincing, or am I misunderstanding something?

Thanks

Gary

>
>>> 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#newcode26
>>> test/test_databinding.js:26: 'model', 'model-list',
>>> 'node-event-simulate'];
>>> this needs the event-valueChange since you are binding with that event
>>> below.
>>
>> Not if the module I test (databinding) itself depends on this, IMO. I
>> only agree with you if I revert the event-valuechange dependency in
>> databinding, ...

Please take a look.

https://codereview.appspot.com/13385045/

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#newcode62
> 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#newcode688
> 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#newcode875
> 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#newcode26
> 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!

https://codereview.appspot.com/13385045/

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#newcode37
> 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#newcode56
> 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#newcode78
> 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#newcode679
> 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#newcode596
> 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#newcode677
> 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!

https://codereview.appspot.com/13385045/

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

https://codereview.appspot.com/13385045/