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

[Gary Poster]

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/

[Gary Poster (gary)]

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

[Gary Poster (gary)]

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/

[Madison Scott-Clary (makyo)]

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/

[Madison Scott-Clary (makyo)]

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/

[Gary Poster (gary)]

*** 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/

=== 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 21:15:45 +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',

=== 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 21:15:45 +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>

=== 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 21:15:45 +0000
@@ -1,3 +1,4 @@
 <div class="view-container settings-config">
+  <h2>Relations</h2>
   <div data-bind="aggregateRelations"></div>
 </div>

=== 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:15:45 +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);
     }
 
   };

=== 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:15:45 +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')) {

=== 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 21:15:45 +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.