Code review comment for lp://staging/~gary/juju-gui/databindingChangedValues

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