Merge into trunk : fixSuccessiveConflicts : Code : juju-gui

Status:	Merged
Merged at revision:	1031
Proposed branch:	lp://staging/~gary/juju-gui/fixSuccessiveConflicts
Merge into:	lp://staging/juju-gui/experimental
Diff against target:	164 lines (+63/-26) 3 files modified app/views/databinding.js (+5/-0) app/views/inspector.js (+34/-26) test/test_inspector_constraints.js (+24/-0)
To merge this branch:	bzr merge lp://staging/~gary/juju-gui/fixSuccessiveConflicts
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-09-11	Pending
Review via email: mp+185100@code.staging.launchpad.net

Description of the change

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://codereview.appspot.com/13663043/

Revision history for this message

Gary Poster (gary) wrote on 2013-09-11:

#

Download full text (6.5 KiB)

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);
...

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: gary.poster@canonical.com-20130911143029-wvdbkc6i64piulz7
+New revision: gary.poster@canonical.com-20130911154424-1h5ylou6783ddjfd

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');

Revision history for this message

Richard Harding (rharding) wrote on 2013-09-11:

#

Per irc, tests pass in IE10. Will review as a follow up.

https://codereview.appspot.com/13663043/

Revision history for this message

Richard Harding (rharding) wrote on 2013-09-11:

#

LGTM

https://codereview.appspot.com/13663043/

Revision history for this message

Gary Poster (gary) wrote on 2013-09-11:

#

*** Submitted:

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.

R=rharding
CC=
https://codereview.appspot.com/13663043

https://codereview.appspot.com/13663043/

juju-gui

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

Commit message

Description of the change

Preview Diff

Subscribers