Merge into trunk : ghostDeploy : Code : juju-gui

Status:	Merged
Merged at revision:	1050
Proposed branch:	lp://staging/~gary/juju-gui/ghostDeploy
Merge into:	lp://staging/juju-gui/experimental
Diff against target:	167 lines (+42/-15) 6 files modified app/store/env/sandbox.js (+11/-3) app/templates/service-constraints-viewlet.handlebars (+1/-1) app/templates/service-relations-viewlet.handlebars (+1/-0) app/views/ghost-inspector.js (+5/-0) app/views/topology/service.js (+20/-5) app/views/viewlets/inspector-header.js (+4/-6)
To merge this branch:	bzr merge lp://staging/~gary/juju-gui/ghostDeploy
Related bugs:	Link a bug report

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

Description of the change

Inspector shows up after ghost inspector

- Per UX, after ghost inspector finishes, real inspector starts
- Changed new service creation to not bounce the box around
- Made a couple of other small tweaks for sandbox edge cases and capitalization

https://codereview.appspot.com/13246050/

Revision history for this message

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

#

Download full text (8.0 KiB)

Reviewers: mp+185933_code.launchpad.net,

Message:
Please take a look.

Description:
Inspector shows up after ghost inspector

- Per UX, after ghost inspector finishes, real inspector starts
- Changed new service creation to not bounce the box around
- Made a couple of other small tweaks for sandbox edge cases and
capitalization

https://code.launchpad.net/~gary/juju-gui/ghostDeploy/+merge/185933

(do not edit description out of merge proposal)

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

Affected files (+44, -15 lines):
   A [revision details]
   M app/store/env/sandbox.js
   M app/templates/service-constraints-viewlet.handlebars
   M app/templates/service-relations-viewlet.handlebars
   M app/views/ghost-inspector.js
   M app/views/topology/service.js
   M app/views/viewlets/inspector-header.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: app/templates/service-constraints-viewlet.handlebars
=== modified file 'app/templates/service-constraints-viewlet.handlebars'
--- app/templates/service-constraints-viewlet.handlebars 2013-09-16
07:36:24 +0000
+++ app/templates/service-constraints-viewlet.handlebars 2013-09-16
20:09:02 +0000
@@ -1,6 +1,6 @@
  <div class="settings-constraints">
    <div class="view-container">
- <h2>Constraints for New Units</h2>
+ <h2>Constraints for new units</h2>
      <div class="view-content">
        {{> service-constraints-viewlet}}
      </div>

Index: app/templates/service-relations-viewlet.handlebars
=== modified file 'app/templates/service-relations-viewlet.handlebars'
--- app/templates/service-relations-viewlet.handlebars 2013-09-07 14:53:02
+0000
+++ app/templates/service-relations-viewlet.handlebars 2013-09-16 20:09:02
+0000
@@ -1,3 +1,4 @@
  <div class="view-container settings-config">
+ <h2>Relations</h2>
    <div data-bind="aggregateRelations"></div>
  </div>

Index: app/views/ghost-inspector.js
=== modified file 'app/views/ghost-inspector.js'
--- app/views/ghost-inspector.js 2013-09-16 07:00:29 +0000
+++ app/views/ghost-inspector.js 2013-09-16 21:09:30 +0000
@@ -300,6 +300,11 @@
});

this.closeInspector();
+ // This flag is used twice in the service topology module as a marker
+ // to know that it should not move the service or the canvas around
+ // (as opposed to services received from the environment).
+ ghostService.set('localCreation', true);
+ this.options.environment.createServiceInspector(ghostService);
}

};

Index: app/store/env/sandbox.js
=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js 2013-09-13 16:55:51 +0000
+++ app/store/env/sandbox.js 2013-09-16 20:09:02 +0000
@@ -852,13 +852,21 @@
          'Series': function(attrs, self) {
            var db = self.get('state').db;
            var service = db.services.getById(attrs.service);
- var charm = db.charms.getById(service.get('charm'));
- ...

Reviewers: mp+185933_code.launchpad.net,

Message:
Please take a look.

Description:
Inspector shows up after ghost inspector

- Per UX, after ghost inspector finishes, real inspector starts
- Changed new service creation to not bounce the box around
- Made a couple of other small tweaks for sandbox edge cases and
capitalization

https://code.launchpad.net/~gary/juju-gui/ghostDeploy/+merge/185933

(do not edit description out of merge proposal)

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

Affected files (+44, -15 lines):
   A [revision details]
   M app/store/env/sandbox.js
   M app/templates/service-constraints-viewlet.handlebars
   M app/templates/service-relations-viewlet.handlebars
   M app/views/ghost-inspector.js
   M app/views/topology/service.js
   M app/views/viewlets/inspector-header.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: jeff.pihach@canonical.com-20130916141413-mc254v7v2r867d4z
+New revision: gary.poster@canonical.com-20130916211041-s65xjmewj10h7yjo

Index: app/templates/service-constraints-viewlet.handlebars
=== modified file 'app/templates/service-constraints-viewlet.handlebars'
--- app/templates/service-constraints-viewlet.handlebars	2013-09-16  
07:36:24 +0000
+++ app/templates/service-constraints-viewlet.handlebars	2013-09-16  
20:09:02 +0000
@@ -1,6 +1,6 @@
  <div class="settings-constraints">
    <div class="view-container">
-    <h2>Constraints for New Units</h2>
+    <h2>Constraints for new units</h2>
      <div class="view-content">
        {{> service-constraints-viewlet}}
      </div>

Index: app/templates/service-relations-viewlet.handlebars
=== modified file 'app/templates/service-relations-viewlet.handlebars'
--- app/templates/service-relations-viewlet.handlebars	2013-09-07 14:53:02  
+0000
+++ app/templates/service-relations-viewlet.handlebars	2013-09-16 20:09:02  
+0000
@@ -1,3 +1,4 @@
  <div class="view-container settings-config">
+  <h2>Relations</h2>
    <div data-bind="aggregateRelations"></div>
  </div>

Index: app/views/ghost-inspector.js
=== modified file 'app/views/ghost-inspector.js'
--- app/views/ghost-inspector.js	2013-09-16 07:00:29 +0000
+++ app/views/ghost-inspector.js	2013-09-16 21:09:30 +0000
@@ -300,6 +300,11 @@
        });

this.closeInspector();
+      // This flag is used twice in the service topology module as a marker
+      // to know that it should not move the service or the canvas around
+      // (as opposed to services received from the environment).
+      ghostService.set('localCreation', true);
+      this.options.environment.createServiceInspector(ghostService);
      }

};

Index: app/store/env/sandbox.js
=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js	2013-09-13 16:55:51 +0000
+++ app/store/env/sandbox.js	2013-09-16 20:09:02 +0000
@@ -852,13 +852,21 @@
          'Series': function(attrs, self) {
            var db = self.get('state').db;
            var service = db.services.getById(attrs.service);
-          var charm = db.charms.getById(service.get('charm'));
-          return charm.get('series');
+          if (service) {
+            var charm = db.charms.getById(service.get('charm'));
+            return charm.get('series');
+          } else {
+            return null; // Probably unit/service was deleted.
+          }
          },
          'CharmURL': function(attrs, self) {
            var db = self.get('state').db;
            var service = db.services.getById(attrs.service);
-          return service.get('charm');
+          if (service) {
+            return service.get('charm');
+          } else {
+            return null; // Probably unit/service was deleted.
+          }
          },
          PublicAddress: 'public_address',
          PrivateAddress: 'private_address',

Index: app/views/topology/service.js
=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-09-10 01:45:00 +0000
+++ app/views/topology/service.js	2013-09-16 21:10:41 +0000
@@ -106,9 +106,16 @@
          delete annotations['gui-y'];
          // Only update position if we're not already in a drag state (the
          // current drag supercedes any previous annotations).
+        var localCreation = d.model.get('localCreation');
+        if (localCreation) {
+          // This flag has served its purpose, at initialization time on  
the
+          // canvas.  Remove it, so future changes will have the usual
+          // behavior.
+          d.model.set('localCreation', false);
+        }
          if (!d.inDrag) {
-          self.drag.call(this, d, self, {x: x, y: y},
-              self.get('useTransitions'));
+          var useTransitions = self.get('useTransitions')  
&& !localCreation;
+          self.drag.call(this, d, self, {x: x, y: y}, useTransitions);
          }
        }});

@@ -1049,6 +1056,7 @@
        // nodes. This has the side effect that service blocks can overlap
        // and will be fixed later.
        var vertices;
+      var localCreation = false;
        var new_services = Y.Object.values(topo.service_boxes)
        .filter(function(boundingBox) {
              return !Y.Lang.isNumber(boundingBox.x);
@@ -1087,6 +1095,10 @@
            vertices = [];
          }
          Y.each(new_services, function(box) {
+          if (box.model.get('localCreation')) {
+            // box.model.set('localCreation', false);
+            localCreation = true;
+          }
            var existing = box.model.get('annotations') || {};
            if (!existing && !existing['gui-x']) {
              topo.get('env').update_annotations(
@@ -1111,7 +1123,7 @@
          if (!vertices) {
            vertices = topoUtils.serviceBoxesToVertices(topo.service_boxes);
          }
-        this.findAndSetCentroid(vertices);
+        this.findAndSetCentroid(vertices, localCreation);
        }
        // enter
        node
@@ -1157,14 +1169,16 @@
      @param {array} vertices A list of vertices in the form [x, y].
      @return {undefined} Side effects only.
      */
-    findAndSetCentroid: function(vertices) {
+    findAndSetCentroid: function(vertices, preventPan) {
        var topo = this.get('component'),
                centroid = topoUtils.centroid(vertices);
        // The centroid is set on the topology object due to the fact that  
it is
        // used as a sigil to tell whether or not to pan to the point after  
the
        // first delta.
        topo.centroid = centroid;
-      topo.fire('panToPoint', {point: topo.centroid});
+      if (!preventPan) {
+        topo.fire('panToPoint', {point: topo.centroid});
+      }
      },

/**
@@ -1374,6 +1388,7 @@
        // The view option should not be used with the inspector.
        if (flags.serviceInspector) {
          serviceMenu.one('.view-service').hide();
+        serviceMenu.one('.destroy-service').hide();
        }

if (box && !serviceMenu.hasClass('active')) {

Index: app/views/viewlets/inspector-header.js
=== modified file 'app/views/viewlets/inspector-header.js'
--- app/views/viewlets/inspector-header.js	2013-08-27 12:42:30 +0000
+++ app/views/viewlets/inspector-header.js	2013-09-16 20:09:02 +0000
@@ -32,15 +32,13 @@
      'render': function(model, viewContainerAttrs) {
        this.container = Y.Node.create(this.templateWrapper);
        var pojoModel = model.getAttrs();
-      if (pojoModel.scheme) {
+      if (model instanceof models.BrowserCharm) {
          pojoModel.ghost = true;
-      }
-      // If this is a service, the id is the service.charm.
-      if (pojoModel.charm) {
+        pojoModel.charmUrl = pojoModel.id;
+      } else if (model instanceof models.Service) {
          pojoModel.charmUrl = pojoModel.charm;
        } else {
-        // If this is a charm model, just use the id.
-        pojoModel.charmUrl = pojoModel.id;
+        throw 'Programmer error: unknown model type';
        }
        // Manually add the icon url for the charm since we don't have  
access to
        // the browser handlebars helper at this location.

Revision history for this message

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

#

95% fly-bys. Sorry.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode859
app/store/env/sandbox.js:859: return null; // Probably unit/service was
deleted.
This was a flyby: when deleting a service then sometimes this would
trigger. I'll remove if a test is needed; it is just for exploratory
testing AFAIK.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode868
app/store/env/sandbox.js:868: return null; // Probably unit/service was
deleted.
As above.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars
File app/templates/service-constraints-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars#newcode3
app/templates/service-constraints-viewlet.handlebars:3: <h2>Constraints
for new units</h2>
Fly-by. Every other title is sentence cased. I prefer title cased, but
this is easier for now, and I'm not sure if this is actually something
that UX cares about.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars
File app/templates/service-relations-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars#newcode2
app/templates/service-relations-viewlet.handlebars:2: <h2>Relations</h2>
Every other non-intro tab has a header. It needs one. Sorry, another
fly-by.

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode306
app/views/ghost-inspector.js:306: ghostService.set('localCreation',
true);
This line is about...well, see the comment.

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode307
app/views/ghost-inspector.js:307:
this.options.environment.createServiceInspector(ghostService);
This was the only change that this branch actually needed to accomplish
the stated task. Sorry.

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js#newcode1391
app/views/topology/service.js:1391:
serviceMenu.one('.destroy-service').hide();
Another fly-by. We should actually make this menu show and hide with
inspector, but I was amazingly able to hold myself back from
implementing that. (It was because I couldn't see how to do it quickly
:-P).

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js
File app/views/viewlets/inspector-header.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js#newcode35
app/views/viewlets/inspector-header.js:35: if (model instanceof
models.BrowserCharm) {
Another flyby. Jeff pointed out the .scheme conditional, which seemed
unnecessarily obscure to me. This seems nice and clear to me.

https://codereview.appspot.com/13246050/

95% fly-bys.  Sorry.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode859
app/store/env/sandbox.js:859: return null; // Probably unit/service was
deleted.
This was a flyby: when deleting a service then sometimes this would
trigger.  I'll remove if a test is needed; it is just for exploratory
testing AFAIK.

https://codereview.appspot.com/13246050/diff/1/app/store/env/sandbox.js#newcode868
app/store/env/sandbox.js:868: return null; // Probably unit/service was
deleted.
As above.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars
File app/templates/service-constraints-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-constraints-viewlet.handlebars#newcode3
app/templates/service-constraints-viewlet.handlebars:3: <h2>Constraints
for new units</h2>
Fly-by.  Every other title is sentence cased.  I prefer title cased, but
this is easier for now, and I'm not sure if this is actually something
that UX cares about.

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars
File app/templates/service-relations-viewlet.handlebars (right):

https://codereview.appspot.com/13246050/diff/1/app/templates/service-relations-viewlet.handlebars#newcode2
app/templates/service-relations-viewlet.handlebars:2: <h2>Relations</h2>
Every other non-intro tab has a header.  It needs one.  Sorry, another
fly-by.

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode306
app/views/ghost-inspector.js:306: ghostService.set('localCreation',
true);
This line is about...well, see the comment.

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode307
app/views/ghost-inspector.js:307:
this.options.environment.createServiceInspector(ghostService);
This was the only change that this branch actually needed to accomplish
the stated task.  Sorry.

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js#newcode1391
app/views/topology/service.js:1391:
serviceMenu.one('.destroy-service').hide();
Another fly-by.  We should actually make this menu show and hide with
inspector, but I was amazingly able to hold myself back from
implementing that.  (It was because I couldn't see how to do it quickly
:-P).

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js
File app/views/viewlets/inspector-header.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/viewlets/inspector-header.js#newcode35
app/views/viewlets/inspector-header.js:35: if (model instanceof
models.BrowserCharm) {
Another flyby.  Jeff pointed out the .scheme conditional, which seemed
unnecessarily obscure to me.  This seems nice and clear to me.

https://codereview.appspot.com/13246050/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-09-16:

#

LGTM, QA okay in Chrome/Go Env (starting up IE now, will comment if
anything goes wrong)

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/ghost-inspector.js#newcode306
app/views/ghost-inspector.js:306: ghostService.set('localCreation',
true);
On 2013/09/16 21:22:16, gary.poster wrote:
> This line is about...well, see the comment.

Would like to see this defined in the Service model's ATTRs with a
comment and default to false, just for consistency's sake.

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13246050/diff/1/app/views/topology/service.js#newcode1099
app/views/topology/service.js:1099: // box.model.set('localCreation',
false);
Remove (assuming flag gets unset later?)

https://codereview.appspot.com/13246050/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-09-16:

#

IE QA okay - services dragged onto the canvas don't retain drop
coordinates, but that's true in trunk as well. Will hunt or file bug.

https://codereview.appspot.com/13246050/

Revision history for this message

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

#

*** Submitted:

Inspector shows up after ghost inspector

- Per UX, after ghost inspector finishes, real inspector starts
- Changed new service creation to not bounce the box around
- Made a couple of other small tweaks for sandbox edge cases and
capitalization

R=matthew.scott
CC=
https://codereview.appspot.com/13246050

https://codereview.appspot.com/13246050/

juju-gui

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

Commit message

Description of the change

Preview Diff

Subscribers