Merge into trunk : bug1167967 : Code : juju-gui

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

Revision history for this message

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

#

Reviewers: mp+158468_code.launchpad.net,

Message:
Please take a look.

Description:
Fix bug 1167967 (hopefully)

This branch attempts to fix the referenced bug. I was unable to
duplicate the symptoms locally exactly, but duplicated them closely
enough that I am optimistic that these changes address the issue.
Regardless, they seem like reasonable changes.

I also fixed a couple of landscape issues I saw with newly created
services without annotations.

The aspect of this branch I wonder most if reviewers (particularly Ben)
will like is the change to the route url algorithm. If it's not clear
why I chose it, I'm happy to discuss my rationale and other options.

Thank you.

https://code.launchpad.net/~gary/juju-gui/bug1167967/+merge/158468

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/app.js
   M app/assets/javascripts/ns-routing-app-extension.js
   M app/views/charm.js
   M app/views/landscape.js
   M app/views/service.js
   M app/views/utils.js
   M test/test_app_hotkeys.js
   M test/test_charm_view.js
   M test/test_landscape.js
   M test/test_routing.js
   M test/test_service_view.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/app.js
=== modified file 'app/app.js'
--- app/app.js 2013-04-09 20:50:46 +0000
+++ app/app.js 2013-04-11 19:46:11 +0000
@@ -184,7 +184,7 @@

        Y.detachAll('window-alt-E-pressed');
        Y.on('window-alt-E-pressed', function(data) {
- this.fire('navigateTo', { url: '/' });
+ this.fire('navigateTo', { url: '/:gui:/' });
          data.preventDefault = true;
        }, this);

Index: test/test_app_hotkeys.js
=== modified file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js 2013-04-08 18:23:28 +0000
+++ test/test_app_hotkeys.js 2013-04-11 19:46:11 +0000
@@ -50,7 +50,7 @@
    it('should listen for alt-E events', function() {
      var altEtriggered = false;
      app.on('navigateTo', function(ev) {
- if (ev && ev.url === '/') {
+ if (ev && ev.url === '/:gui:/') {
          altEtriggered = true;
        }
        // Avoid URL change performed by additional listeners.

Index: test/test_charm_view.js
=== modified file 'test/test_charm_view.js'
--- test/test_charm_view.js 2013-02-15 11:58:05 +0000
+++ test/test_charm_view.js 2013-04-11 19:46:11 +0000
@@ -88,7 +88,7 @@
          env: env});
        var redirected = false;
        charmView.on('navigateTo', function(e) {
- assert.equal('/', e.url);
+ assert.equal('/:gui:/', e.url);
          redirected = true;
        });
        var deployInput = charmView.get('container').one('#charm-deploy');

Index: app/views/charm.js
=== modified file 'app/views/charm.js'
--- app/views/charm.js 2013-03-19 21:58:04 +0000
+++ app/views/charm.js 2013-04-11 19:46:11 +0000
@@ -97,7 +97,7 @@
// The deploy call generates an event ...

Reviewers: mp+158468_code.launchpad.net,

Message:
Please take a look.

Description:
Fix bug 1167967 (hopefully)

This branch attempts to fix the referenced bug.  I was unable to
duplicate the symptoms locally exactly, but duplicated them closely
enough that I am optimistic that these changes address the issue.
Regardless, they seem like reasonable changes.

I also fixed a couple of landscape issues I saw with newly created
services without annotations.

The aspect of this branch I wonder most if reviewers (particularly Ben)
will like is the change to the route url algorithm.  If it's not clear
why I chose it, I'm happy to discuss my rationale and other options.

Thank you.

https://code.launchpad.net/~gary/juju-gui/bug1167967/+merge/158468

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/app.js
   M app/assets/javascripts/ns-routing-app-extension.js
   M app/views/charm.js
   M app/views/landscape.js
   M app/views/service.js
   M app/views/utils.js
   M test/test_app_hotkeys.js
   M test/test_charm_view.js
   M test/test_landscape.js
   M test/test_routing.js
   M test/test_service_view.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: jcsackett@canonical.com-20130411192455-n7g2b3vkap16care
+New revision: gary.poster@canonical.com-20130411194611-8zdbxdps88v4py03

Index: app/app.js
=== modified file 'app/app.js'
--- app/app.js	2013-04-09 20:50:46 +0000
+++ app/app.js	2013-04-11 19:46:11 +0000
@@ -184,7 +184,7 @@

Y.detachAll('window-alt-E-pressed');
        Y.on('window-alt-E-pressed', function(data) {
-        this.fire('navigateTo', { url: '/' });
+        this.fire('navigateTo', { url: '/:gui:/' });
          data.preventDefault = true;
        }, this);

Index: test/test_app_hotkeys.js
=== modified file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js	2013-04-08 18:23:28 +0000
+++ test/test_app_hotkeys.js	2013-04-11 19:46:11 +0000
@@ -50,7 +50,7 @@
    it('should listen for alt-E events', function() {
      var altEtriggered = false;
      app.on('navigateTo', function(ev) {
-      if (ev && ev.url === '/') {
+      if (ev && ev.url === '/:gui:/') {
          altEtriggered = true;
        }
        // Avoid URL change performed by additional listeners.

Index: test/test_charm_view.js
=== modified file 'test/test_charm_view.js'
--- test/test_charm_view.js	2013-02-15 11:58:05 +0000
+++ test/test_charm_view.js	2013-04-11 19:46:11 +0000
@@ -88,7 +88,7 @@
          env: env});
        var redirected = false;
        charmView.on('navigateTo', function(e) {
-        assert.equal('/', e.url);
+        assert.equal('/:gui:/', e.url);
          redirected = true;
        });
        var deployInput = charmView.get('container').one('#charm-deploy');

Index: app/views/charm.js
=== modified file 'app/views/charm.js'
--- app/views/charm.js	2013-03-19 21:58:04 +0000
+++ app/views/charm.js	2013-04-11 19:46:11 +0000
@@ -97,7 +97,7 @@
        // The deploy call generates an event chain leading to a call to
        // `app.on_database_changed()`, which re-dispatches the current view.
        // For this reason we need to redirect to the root page right now.
-      this.fire('navigateTo', {url: '/'});
+      this.fire('navigateTo', {url: '/:gui:/'});
        env.deploy(charmUrl, serviceName, config,
            Y.bind(this._deployCallback, this)
        );

Index: app/assets/javascripts/ns-routing-app-extension.js
=== modified file 'app/assets/javascripts/ns-routing-app-extension.js'
--- app/assets/javascripts/ns-routing-app-extension.js	2013-03-21 15:32:06  
+0000
+++ app/assets/javascripts/ns-routing-app-extension.js	2013-04-11 19:46:11  
+0000
@@ -167,7 +167,9 @@
        var keys = Y.Object.keys(base).sort();
        Y.each(keys, function(ns) {
          url = slash(url);
-        url += ':' + ns + ':' + base[ns];
+        if (base[ns] !== '/') {
+          url += ':' + ns + ':' + base[ns];
+        }
        });

url = slash(url);

Index: test/test_landscape.js
=== modified file 'test/test_landscape.js'
--- test/test_landscape.js	2013-03-26 21:20:28 +0000
+++ test/test_landscape.js	2013-04-11 19:46:11 +0000
@@ -158,6 +158,14 @@
      node.one('.updates-control').getStyle('display').should.equal('none');
      node.one('.restart-control').getStyle('display').should.equal('block');

+    // We handle missing annotations on a service.
+    mysql.set('annotations', {});
+    landscape.update();
+    views.utils.updateLandscapeBottomBar(landscape, env, mysql, node,
+        'service');
+    node.one('.machine-control').getStyle('display').should.equal('none');
+
+    // We handle normal unit annotations.
      unit.annotations = {'landscape-computer': '+unit:mysql-0'};
      landscape.update();

@@ -171,6 +179,13 @@
      node.one('.updates-control').getStyle('display').should.equal('none');
      node.one('.restart-control').getStyle('display').should.equal('none');

+    // We handle completely missing annotations on a unit.
+    delete unit.annotations;
+    landscape.update();
+    views.utils.updateLandscapeBottomBar(landscape, env, unit, node,
+        'unit');
+    node.one('.machine-control').getStyle('display').should.equal('none');
+
      node.remove();
    });

Index: app/views/landscape.js
=== modified file 'app/views/landscape.js'
--- app/views/landscape.js	2013-04-09 20:22:01 +0000
+++ app/views/landscape.js	2013-04-11 19:46:11 +0000
@@ -128,7 +128,8 @@
          }
          url += slash(modelAnnotation);
        } else if (model.name === 'serviceUnit') {
-        modelAnnotation = model.annotations['landscape-computer'];
+        modelAnnotation = (
+            model.annotations && model.annotations['landscape-computer']);
          if (!modelAnnotation) {
            console.warn('Unit missing the landscape-computer annotation!');
            return undefined;

Index: test/test_routing.js
=== modified file 'test/test_routing.js'
--- test/test_routing.js	2013-04-08 18:23:28 +0000
+++ test/test_routing.js	2013-04-11 19:46:11 +0000
@@ -46,6 +46,12 @@
      var u = router.url(match);
      assert(u === '/charms/precise/mediawiki/:inspector:/services/mysql/');

+    // Root keys are implicit.
+    u = router.url({foo: '/'});
+    assert.strictEqual(u, '/');
+    u = router.url({foo: '/shazam', bar: '/'});
+    assert.strictEqual(u, '/:foo:/shazam/');
+
      // Sorted keys.
      u = router.url({charmstore: '/', gamma: 'g', a: 'alpha', b: 'beta'});
      assert(u === '/:a:alpha/:b:beta/:gamma:g/');

Index: app/views/service.js
=== modified file 'app/views/service.js'
--- app/views/service.js	2013-04-04 07:22:16 +0000
+++ app/views/service.js	2013-04-11 19:46:11 +0000
@@ -225,7 +225,8 @@
              ));
          this.panel.hide();
          this.panel.destroy();
-        this.fire('navigateTo', {url: '/'});
+        this.fire('navigateTo', {url: '/:gui:/'});
+        db.fire('update');
        }
      }
    };

Index: app/views/utils.js
=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-04-11 12:47:10 +0000
+++ app/views/utils.js	2013-04-11 19:46:11 +0000
@@ -422,20 +422,22 @@

if (envAnnotations['landscape-url']) {
        controls.show();
-      machine.show();
-      machine.one('a').setAttribute('href',
-          landscape.getLandscapeURL(model));
-
-      if (annotations['landscape-security-upgrades']) {
-        updates.show();
-        updates.one('a').setAttribute('href',
-            landscape.getLandscapeURL(model, 'security'));
-      }
-
-      if (annotations['landscape-needs-reboot']) {
-        restart.show();
-        restart.one('a').setAttribute('href',
-            landscape.getLandscapeURL(model, 'reboot'));
+      var baseLandscapeURL = landscape.getLandscapeURL(model);
+      if (baseLandscapeURL) {
+        machine.show();
+        machine.one('a').setAttribute('href', baseLandscapeURL);
+
+        if (annotations['landscape-security-upgrades']) {
+          updates.show();
+          updates.one('a').setAttribute('href',
+              landscape.getLandscapeURL(model, 'security'));
+        }
+
+        if (annotations['landscape-needs-reboot']) {
+          restart.show();
+          restart.one('a').setAttribute('href',
+              landscape.getLandscapeURL(model, 'reboot'));
+        }
        }
      }
    };

Index: test/test_service_view.js
=== modified file 'test/test_service_view.js'
--- test/test_service_view.js	2013-03-21 15:32:06 +0000
+++ test/test_service_view.js	2013-04-11 19:46:11 +0000
@@ -349,20 +349,23 @@
           destroy.simulate('click');
           var called = false;
           view.on('navigateTo', function(ev) {
-           assert.equal('/', ev.url);
+           assert.equal('/:gui:/', ev.url);
             called = true;
           });
           var callbacks = Y.Object.values(env._txn_callbacks);
           callbacks.length.should.equal(1);
-         // Since we don't have an app to listen to this event and tell the
-         // view to re-render, we need to do it ourselves.
-         db.on('update', view.render, view);
+         var dbUpdated = false;
+         db.on('update', function() {
+           dbUpdated = true;
+         });
           callbacks[0]({result: true});
           var _ =  
expect(db.services.getById(service.get('id'))).to.not.exist;
           db.relations.map(function(u) {return u.get('id');})
          .should.eql(['relation-0000000001']);
           // Catch show environment event.
           called.should.equal(true);
+         // The db should be updated.
+         dbUpdated.should.equal(true);
         });

it('should send an expose RPC call when exposeService is invoked',

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2013-04-11:

#

LGTM.

Some suggestions around the url handling but take it or leave it.

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' + ns
+ ':' + base[ns];
Maybe a flag to the method to control this behavior so we can use this
method to generate all such links?

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
In cases like this I previously recommended that we use nsRouter.url to
create the URL. That no longer works in this case as '/' becomes
implicit in the namespace.

I like the abstraction of something that can build and assemble proper
URLs for the developer. Maybe a flag to nsRouter.url that forces
inclusion. I'll make a note in the router code as well but its up to you
if you agree or not.

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
app/views/landscape.js:132: if (!modelAnnotation) {
oops. While improv doesn't add these annotations to new nodes it was my
understanding that landscape will? Still, I'm sure there is going to be
some latency in that process and these checks are valid.

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js
File test/test_routing.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
The reason I originally went the other way was I though presence in the
URL could indicate state, such that :inspector:/ might indicate an open
panel. I think you're right though that there are other ways to model it
and making '/' implicit is fine.

https://codereview.appspot.com/8680043/

LGTM.

Some suggestions around the url handling but take it or leave it.

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' + ns
+ ':' + base[ns];
Maybe a flag to the method to control this behavior so we can use this
method to generate all such links?

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
In cases like this I previously recommended that we use nsRouter.url to
create the URL. That no longer works in this case as '/' becomes
implicit in the namespace.

I like the abstraction of something that can build and assemble proper
URLs for the developer. Maybe a flag to nsRouter.url that forces
inclusion. I'll make a note in the router code as well but its up to you
if you agree or not.

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
app/views/landscape.js:132: if (!modelAnnotation) {
oops. While improv doesn't add these annotations to new nodes it was my
understanding that landscape will? Still, I'm sure there is going to be
some latency in that process and these checks are valid.

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js
File test/test_routing.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
The reason I originally went the other way was I though presence in the
URL could indicate state, such that :inspector:/ might indicate an open
panel. I think you're right though that there are other ways to model it
and making '/' implicit is fine.

https://codereview.appspot.com/8680043/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-04-11:

#

LGTM with Ben's comments at least replied to. Thanks!

https://codereview.appspot.com/8680043/diff/1/test/test_service_view.js
File test/test_service_view.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_service_view.js#newcode359
test/test_service_view.js:359: dbUpdated = true;
+1 :)

https://codereview.appspot.com/8680043/

Revision history for this message

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

#

On 2013/04/11 20:10:37, bcsaller wrote:
> LGTM.

> Some suggestions around the url handling but take it or leave it.

Thank you for the thoughtful review.

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
> File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
> app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' +
ns + ':' +
> base[ns];
> Maybe a flag to the method to control this behavior so we can use this
method to
> generate all such links?

I agonized about this comment a bit. First I made the new behavior
optional, and wrote tests. Then I decided that I thought the new
behavior ought to be the easy, new one, because I think it is what we
usually want, and undid all the option work. Then I decided to make an
option to have the old behavior...but I still kept the non-calculated
root strings ('/:gui:/') because I thought they were clear enough and
short enough. I don't know. I'm still a bit conflicted, but I feel
like removing root paths is the right thing to do, and the right thing
to make easy.

> https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
> File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
> app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
> In cases like this I previously recommended that we use nsRouter.url
to create
> the URL. That no longer works in this case as '/' becomes implicit in
the
> namespace.

For this case, right. I could use the flag I added, but I currently
think that the string is fine for a root path.

> I like the abstraction of something that can build and assemble proper
URLs for
> the developer. Maybe a flag to nsRouter.url that forces inclusion.
I'll make a
> note in the router code as well but its up to you if you agree or not.

Yeah, I agree but it conflicts with other desires I have for this.

> https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
> File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
> app/views/landscape.js:132: if (!modelAnnotation) {
> oops. While improv doesn't add these annotations to new nodes it was
my
> understanding that landscape will? Still, I'm sure there is going to
be some
> latency in that process and these checks are valid.

Yeah, that's what I thought.

> https://codereview.appspot.com/8680043/diff/1/test/test_routing.js
> File test/test_routing.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
> test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
> The reason I originally went the other way was I though presence in
the URL
> could indicate state, such that :inspector:/ might indicate an open
panel. I
> think you're right though that there are other ways to model it and
making '/'
> implicit is fine.

Cool, yeah.

https://codereview.appspot.com/8680043/

On 2013/04/11 20:10:37, bcsaller wrote:
> LGTM.

> Some suggestions around the url handling but take it or leave it.

Thank you for the thoughtful review.

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
> File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode171
> app/assets/javascripts/ns-routing-app-extension.js:171: url += ':' +
ns + ':' +
> base[ns];
> Maybe a flag to the method to control this behavior so we can use this
method to
> generate all such links?

I agonized about this comment a bit.  First I made the new behavior
optional, and wrote tests.  Then I decided that I thought the new
behavior ought to be the easy, new one, because I think it is what we
usually want, and undid all the option work.  Then I decided to make an
option to have the old behavior...but I still kept the non-calculated
root strings ('/:gui:/') because I thought they were clear enough and
short enough.  I don't know.  I'm still a bit conflicted, but I feel
like removing root paths is the right thing to do, and the right thing
to make easy.

> https://codereview.appspot.com/8680043/diff/1/app/views/charm.js
> File app/views/charm.js (right):

https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
> app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
> In cases like this I previously recommended that we use nsRouter.url
to create
> the URL. That no longer works in this case as '/' becomes implicit in
the
> namespace.

For this case, right.  I could use the flag I added, but I currently
think that the string is fine for a root path.

> I like the abstraction of something that can build and assemble proper
URLs for
> the developer. Maybe a flag to nsRouter.url that forces inclusion.
I'll make a
> note in the router code as well but its up to you if you agree or not.

Yeah, I agree but it conflicts with other desires I have for this.

> https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js
> File app/views/landscape.js (left):

https://codereview.appspot.com/8680043/diff/1/app/views/landscape.js#oldcode132
> app/views/landscape.js:132: if (!modelAnnotation) {
> oops. While improv doesn't add these annotations to new nodes it was
my
> understanding that landscape will? Still, I'm sure there is going to
be some
> latency in that process and these checks are valid.

Yeah, that's what I thought.

> https://codereview.appspot.com/8680043/diff/1/test/test_routing.js
> File test/test_routing.js (right):

https://codereview.appspot.com/8680043/diff/1/test/test_routing.js#newcode53
> test/test_routing.js:53: assert.strictEqual(u, '/:foo:/shazam/');
> The reason I originally went the other way was I though presence in
the URL
> could indicate state, such that :inspector:/ might indicate an open
panel. I
> think you're right though that there are other ways to model it and
making '/'
> implicit is fine.

Cool, yeah.

https://codereview.appspot.com/8680043/

Revision history for this message

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

#

On 2013/04/11 22:45:15, matthew.scott wrote:
> LGTM with Ben's comments at least replied to. Thanks!

Cool, thank you for the review.

https://codereview.appspot.com/8680043/

Revision history for this message

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

#

*** Submitted:

Fix bug 1167967 (hopefully)

This branch attempts to fix the referenced bug. I was unable to
duplicate the symptoms locally exactly, but duplicated them closely
enough that I am optimistic that these changes address the issue.
Regardless, they seem like reasonable changes.

I also fixed a couple of landscape issues I saw with newly created
services without annotations.

The aspect of this branch I wonder most if reviewers (particularly Ben)
will like is the change to the route url algorithm. If it's not clear
why I chose it, I'm happy to discuss my rationale and other options.

Thank you.

R=bcsaller, matthew.scott
CC=
https://codereview.appspot.com/8680043

https://codereview.appspot.com/8680043/

Revision history for this message

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

#

On 04/11/2013 07:16 PM, <email address hidden> wrote:
>
> https://codereview.appspot.com/8680043/diff/1/app/views/charm.js#newcode100
>> app/views/charm.js:100: this.fire('navigateTo', {url: '/:gui:/'});
>> In cases like this I previously recommended that we use nsRouter.url
> to create
>> the URL. That no longer works in this case as '/' becomes implicit in
> the
>> namespace.
>
> For this case, right. I could use the flag I added, but I currently
> think that the string is fine for a root path.

Eh. I'm submitting this and I still don't know. Maybe what I did
first, post-review--"combine" excluded root paths by default and "url"
included them by default--was right. But that feels wrong too. Ugh.

If we determine that "url"'s utility has really been reduced by my
changes, then I would like to look at this again.

Thank you.

Gary

juju-gui

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

Commit message

Description of the change

Preview Diff

Subscribers