Eliminate unnecessary conflicts

Eliminate unnecessary conflicts

Fixes the following issues:
 * If you change one model parent.child value (e.g. constraints.arch) in the inspector and then set a model parent.child value (e.g., constaints.cpu-cores) in the db, you will get an unnecessary conflict on the arch.
 * resetDOMToModel could not clear conflicts.

I also added a lot of comments, and a couple of tests for pre-existing functionality that I broke during my refactoring.

https://code.launchpad.net/~gary/juju-gui/eliminateUnnecessaryConflicts/+merge/185256

(do not edit description out of merge proposal)

[gary.poster, 2013-09-12 13:00:31]

Please take a look.

[gary.poster, 2013-09-12 13:05:42]

Please take a look.

[gary.poster, 2013-09-12 13:14:04]

Please take a look.

[rharding, 2013-09-12 13:42:24]

code looks good. One note about resolver being able to be null and that should
come through in the branch I just landed.

Will QA.

https://codereview.appspot.com/13340047/diff/13001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13340047/diff/13001/app/views/inspector.js#new...
app/views/inspector.js:1212: var resolver = wrapper.one('.resolver');
resolver can be null.

https://codereview.appspot.com/13340047/diff/13001/app/views/inspector.js#new...
app/views/inspector.js:1221: resolver.addClass('hidden');
and so needs to be checked here.

https://codereview.appspot.com/13340047/diff/13001/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13340047/diff/13001/test/test_databinding.js#n...
test/test_databinding.js:726: });
ok, now you've gone and blown my mind.

[rharding, 2013-09-12 13:47:54]

QA not good:


- Using the qa instructions, when I have the back end change the arch, the value
for cpu cores is emptied.

In testing, each time I set a value in the backend the other value will empty
out.

[rharding, 2013-09-12 15:14:12]

Noted where the change is happening.

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

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:1000: binding.field.set(node, value);
this is what's clearing. Changing the arch causes the cpu cores to also get
processed and since it's not edited, it falls into here and gets set to value
which is "".

[rharding, 2013-09-12 15:23:33]

Final note on where I think the issue is and I'm not sure I follow how it's
supposed to work.

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

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:906: var value = binding.values[binding.name] =
binding.get(model);
So this seems to be the issue. The value comes out as undefined because
binding.get(model) is undefined. The old value isn't used and then all the
comparisons are thrown off.

[rharding, 2013-09-12 17:54:50]

LGTM qa-ok. I was missing the .set() call was not touching one field in the
constraints, but the entire contraints object.

[gary.poster, 2013-09-12 18:01:50]

On 2013/09/12 17:54:50, rharding wrote:
> LGTM qa-ok. I was missing the .set() call was not touching one field in the
> constraints, but the entire contraints object.

Thank you, Rick.

I have changed the code to handle the case of the resolver potentially being
missing, as you mentioned in your code review.

As we discussed, I'd like to divide up the _updateGUI functionality into smaller
methods in a separate branch.

[gary.poster, 2013-09-12 18:04:24]

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

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:906: var value = binding.values[binding.name] =
binding.get(model);
On 2013/09/12 15:23:34, rharding wrote:
> So this seems to be the issue. The value comes out as undefined because
> binding.get(model) is undefined. The old value isn't used and then all the
> comparisons are thrown off.

Rick and I agree that this was from a qa misunderstanding.  It's ok.

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:1000: binding.field.set(node, value);
On 2013/09/12 15:14:13, rharding wrote:
> this is what's clearing. Changing the arch causes the cpu cores to also get
> processed and since it's not edited, it falls into here and gets set to value
> which is "".

Again, part of the QA misunderstanding.  All is good.

https://codereview.appspot.com/13340047/diff/13001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13340047/diff/13001/app/views/inspector.js#new...
app/views/inspector.js:1221: resolver.addClass('hidden');
On 2013/09/12 13:42:24, rharding wrote:
> and so needs to be checked here.

Ack, changed locally, thank you.

https://codereview.appspot.com/13340047/diff/13001/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13340047/diff/13001/test/test_databinding.js#n...
test/test_databinding.js:726: });
On 2013/09/12 13:42:24, rharding wrote:
> ok, now you've gone and blown my mind.

:-)

[benjamin.saller, 2013-09-12 18:30:35]

LGTM, but I do think we need to verify the format case is as expected. Described
below.

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

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:914: });
Nice case to catch.

https://codereview.appspot.com/13340047/diff/13001/app/views/databinding.js#n...
app/views/databinding.js:987: if (binding.format) {
Do we run the risk here that a formatted value is what goes into the node and it
will never compare well to the model value, for example a date time string vs
the timestamp in the model. We might need to apply formatting before we do the
confict's eq check. I think we need a conflict test with a format method on the
binding.

[gary.poster, 2013-09-13 03:58:06]

Please take a look.

[gary.poster, 2013-09-13 03:59:12]

Please take a look.

[gary.poster, 2013-09-13 04:26:46]

Thank you to both Rick and Ben for their reviews.  I responded to both of them,
which meant a decent amount of changes for Ben's good point.  I then merged
trunk and realized that I had made a decision that entirely broke Rick's
checkbox conflict branch.  I talked it over with Jeff, and we agreed that Rick's
approach, or a variant of it (as seen in my test change) was better than what I
had done.  That required some really fundamental changes to my branch.

I've commented up the changes, and would appreciate a re-review from Rick in
particular, since it changes some work he just did.

Thanks again,

Gary

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

https://codereview.appspot.com/13340047/diff/21001/app/views/databinding.js#n...
app/views/databinding.js:781: formattedValue = binding.format.call(binding,
value);
This was part of my first cut at addressing Ben's concerns.

https://codereview.appspot.com/13340047/diff/21001/app/views/databinding.js#n...
app/views/databinding.js:1066: BindingEngine.prototype.clearChangedValues =
function(viewletName) {
As mentioned above, I deleted this because it seemed unnecessary. 
resetDOMToModel accomplishes all the use cases we have for now.

https://codereview.appspot.com/13340047/diff/21001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13340047/diff/21001/app/views/inspector.js#new...
app/views/inspector.js:1275:
this.container.all('.modified').removeClass('modified');
We don't need to do this (or the similar checkbox bit from trunk) because
resetDOMToModel will ask the changed method to clear things up properly.

https://codereview.appspot.com/13340047/diff/21001/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13340047/diff/21001/test/test_databinding.js#n...
test/test_databinding.js:910: model.set('a', 'KUMQUAT');
This is another example of the behavior I explicitly turned off.

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

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:305: * formattedValue: The formatted version of the
model value, as of the
This value helps me address the concern Ben raised.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:669: BindingEngine.prototype.resetDOMToModel =
function(name) {
This now performs the job of clearChangedValues.  That method is gone.  The
inspector calls this instead, after setting the appropriate model attribute.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:680: this._updateDOM(delta, true);
_updateDOM will now update the changedValues set, binding by binding.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:785: if (nodeHandler.eq(node, binding.formattedValue))
{
See the semantics of formattedValue above.

The reason why I made this change is that my previous approach made it
impossible for a viewlet to give a user an indication that a change had happened
to the model that synced the user's value with the model's.  I was going to not
worry about this, but Rick had a branch that very much worried about it in
regards to checkboxes, so I had to rethink.  Now, formattedValue only changes
when the edited value is not dirty.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:932: // We have reached this part of the code because
we are supposed to
The logic can be a lot simpler than my previous iteration, because we no longer
want to automatically resolve conflicts ever (unless a new one comes along to
replace the old one).  I still add a huge number of comments, though.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:951: binding.resolve(nodeValue, true);
The "true" means that formattedValue stays untouched.  That gives some of the
behaviors I was after.

https://codereview.appspot.com/13340047/diff/7001/app/views/databinding.js#ne...
app/views/databinding.js:992: // The user has edited the field, and the value
has changed.  This
This block (the "edited" block) used to come first.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:591: onCheckboxUpdate: function(ev) {
This is from a merge with trunk.  I didn't do this.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:785: Y.bind(this._setConfigCallback, this, container,
newVals)
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:808: _setConfigCallback: function(container, config, e) {
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:824: bindingEngine.resetDOMToModel('config');
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:850: this._saveConstraintsCallback, this, container,
constraints);
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:864: _saveConstraintsCallback: function(container,
constraints, ev) {
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:881: bindingEngine.resetDOMToModel('constraints');
This is part of the switch to using resetDOMToModel.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1107: var n = node.ancestor('.toggle').one('.modified');
This is from trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1128: node.ancestor('.toggle').one('label').append(
from trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1148: var n =
node.ancestor('.toggle').one('.conflict-pending');
from trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1169: node.get('parentNode').prepend(
from trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1207: // Not all nodes need to show the conflict ux. This
is true when
trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1224: 'conflict': function(node, nodeValue, modelValue,
resolve, binding) {
I changed the signature, and did everything else to the function except the
'skipconflictux' block immediately below: that's from trunk.

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1241: if (resolver) {
from review (thank you, Rick).

https://codereview.appspot.com/13340047/diff/7001/app/views/inspector.js#newc...
app/views/inspector.js:1293: handlers.push(wrapper.delegate(
The behavior that the test described did not work for me (that is, it was broken
for me when I switched the value on the model back and forth and did similar
exploratory testing; and I also didn't agree with what it said as a UX goal). 
This is a small tweak: you click on the conflict-pending image to get rid of it.

https://codereview.appspot.com/13340047/diff/7001/test/test_conflict_ux.js
File test/test_conflict_ux.js (right):

https://codereview.appspot.com/13340047/diff/7001/test/test_conflict_ux.js#ne...
test/test_conflict_ux.js:215: service.set('config', {debug: true});
If you switch it to false, you are changing a false value to false.  This should
not trigger a conflict theoretically IMO, and practically it means that a lot of
false positives will be triggered.  Talk to me for details.  Anyway, with
checkboxes, the only way to trigger a conflict now is to first switch the value
to the same value that the user has selected.

https://codereview.appspot.com/13340047/diff/7001/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13340047/diff/7001/test/test_databinding.js#ne...
test/test_databinding.js:841: assert.strictEqual(viewlet.conflictArgs[1],
'kumquat');
Arguments changed a bit, as mentioned.

https://codereview.appspot.com/13340047/diff/7001/test/test_databinding.js#ne...
test/test_databinding.js:868: it('does not auto-resolve conflict when model
matches', function() {
This was the critical behavioral change I made to earlier versions of my branch
to make checkbox conflicts happen under *any* circumstance.

[gary.poster, 2013-09-13 04:29:11]

Rick, I should add that I suggest looking at the incremental diff by comparing
revision set 4 on the left and 6 on the right.  That combination is where you'll
find my comments.

[rharding, 2013-09-13 12:20:38]

LGTM thanks for update and the walk through on hangouts. I appreciate that you
thought this through so thoroughly.

[gary.poster, 2013-09-13 12:27:12]

*** Submitted:

Eliminate unnecessary conflicts

Fixes the following issues:
 * If you change one model parent.child value (e.g. constraints.arch) in the
inspector and then set a model parent.child value (e.g., constaints.cpu-cores)
in the db, you will get an unnecessary conflict on the arch.
 * resetDOMToModel could not clear conflicts.

I also added a lot of comments, and a couple of tests for pre-existing
functionality that I broke during my refactoring.

R=rharding, benjamin.saller
CC=
https://codereview.appspot.com/13340047