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

Reviewers: mp+185100_code.launchpad.net,

Message:
Please take a look.

Description:
fix successive conflict UX calls

if you set a model value and then set a new one, the newest one will be
lost. This branch fixes that problem, and adds a test for simpler
conflict UX.

https://code.launchpad.net/~gary/juju-gui/fixSuccessiveConflicts/+merge/185100

(do not edit description out of merge proposal)

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

Affected files (+65, -26 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/inspector.js
   M test/test_inspector_constraints.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_inspector_constraints.js
=== modified file 'test/test_inspector_constraints.js'
--- test/test_inspector_constraints.js 2013-09-11 00:22:45 +0000
+++ test/test_inspector_constraints.js 2013-09-11 15:40:06 +0000
@@ -247,4 +247,28 @@
      assert.isTrue(controls.hasClass('closed'));
    });

+ it('handles conflicts correctly', function() {
+ var viewlet = getViewlet(inspector);
+ changeForm(viewlet, 'arch', 'i386');
+ inspector.model.set('constraints', {arch: 'lcars'});
+ var node =
viewlet.container.one('input[data-bind="constraints.arch"]');
+ node.simulate('click');
+ var wrapper = node.ancestor('.settings-wrapper');
+ var option = wrapper.one('.resolver .config-field');
+ assert.equal(option.getHTML(), 'lcars');
+ });
+
+ it('handles successive conflicts correctly', function() {
+ var viewlet = getViewlet(inspector);
+ changeForm(viewlet, 'arch', 'i386');
+ inspector.model.set('constraints', {arch: 'lcars'});
+ // This is the successive change.
+ inspector.model.set('constraints', {arch: 'arm64'});
+ var node =
viewlet.container.one('input[data-bind="constraints.arch"]');
+ node.simulate('click');
+ var wrapper = node.ancestor('.settings-wrapper');
+ var option = wrapper.one('.resolver .config-field');
+ assert.equal(option.getHTML(), 'arm64');
+ });
+
  });

Index: app/views/databinding.js
=== modified file 'app/views/databinding.js'
--- app/views/databinding.js 2013-09-11 14:22:16 +0000
+++ app/views/databinding.js 2013-09-11 15:34:27 +0000
@@ -300,6 +300,8 @@

         * name: A string that is the binding name. It references a
(possibly
                 nested) attribute of the associated viewlet's model.
+ * annotations: A hash on which viewlets can scribble whatever they
want
+ to.
         * viewlet: (optional) The viewlet that is matched with this binding.
         * target: (optional) Associated DOM node. If this exists, then it
is
                   unique: no other binding in this engine shares it.
@@ -350,6 +352,9 @@
        }

        binding.viewlet = viewlet;
+ if (binding.annotations === undefined) {
+ binding.annotations = {};
+ }
        this._bindings.push(binding);
        return binding;
      };

Index: app/views/inspector.js
=== modified file 'app/views/inspector.js'
--- app/views/inspector.js 2013-09-11 03:13:28 +0000
+++ app/views/inspector.js 2013-09-11 15:44:24 +0000
@@ -1204,6 +1204,7 @@
         Calls the databinding resolve method
         @method sendResolve
        */
+ var viewlet = this;
        var key = node.getData('bind');
        var modelValue = model.get(key);
        var field = binding.field;
@@ -1212,41 +1213,51 @@
        var option = resolver.one('.config-field');
        var handlers = [];

+ if (binding.annotations.conflict) {
+ binding.annotations.conflict.cancel();
+ }
+
+ binding.annotations.conflict = {
+ /**
+ Cancel this conflict handling UX.
+
+ @method cancel
+ */
+ cancel: function() {
+ handlers.forEach(function(h) { h.detach();});
+ viewlet._clearModified(node);
+ viewlet._clearConflictPending(node);
+ viewlet._clearConflict(node);
+ resolver.addClass('hidden');
+ delete binding.annotations.conflict;
+ }
+ };
        /**
- User selects one of the two conflicting values.
- @method sendResolve
+ User selects one of the two conflicting values.
+
+ @method sendResolve
         */
        function sendResolve(e) {
          e.halt(true);
- var formValue = field.get(node);
- handlers.forEach(function(h) { h.detach();});
-
- /* jshint -W040 */
- // Ignore 'possible strict violation'
- this._clearModified(node);
- this._clearConflict(node);
-
- resolver.addClass('hidden');
-
+ binding.annotations.conflict.cancel();
          if (e.currentTarget.hasClass('conflicted-env')) {
            resolve(modelValue);
          } else {
+ var formValue = field.get(node);
            resolve(formValue);
          }
        }

        /**
- User selects a conflicting field, show the resolution UI
+ User selects a conflicting field, show the resolution UI

- @method setupResolver
+ @method setupResolver
        */
        function setupResolver(e) {
          e.halt(true);
- /* jshint -W040 */
- // Ignore 'possible strict violation'
- this._clearConflictPending(node);
- this._makeConflict(node);
- this._makeConflict(option);
+ viewlet._clearConflictPending(node);
+ viewlet._makeConflict(node);
+ viewlet._makeConflict(option);
          option.setStyle('width', node.get('offsetWidth'));
          option.setHTML(modelValue);
          resolver.removeClass('hidden');
@@ -1257,14 +1268,10 @@
        this._makeConflictPending(node);

        handlers.push(wrapper.delegate(
- 'click',
- setupResolver,
- '.conflict-pending',
- this));
+ 'click', setupResolver, '.conflict-pending'));
+ handlers.push(wrapper.delegate('click', sendResolve, '.conflict'));
+ },

- handlers.push(wrapper.delegate('click', sendResolve,
- '.conflict', this));
- },
      'unsyncedFields': function() {
        var node = this.container.one('.controls .confirm');
        if (!node.getData('originalText')) {
@@ -1272,6 +1279,7 @@
        }
        node.setHTML('Overwrite');
      },
+
      'syncedFields': function() {
        var controls = this.container.one('.controls');
        var node = controls.one('.confirm');