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

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