Fix bug 1167967 (hopefully)

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)

[gary.poster, 2013-04-11 19:55:30]

Please take a look.

[bcsaller, 2013-04-11 20:10:37]

LGTM.

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

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

https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routi...
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.

[matthew.scott, 2013-04-11 22:45:15]

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#newco...
test/test_service_view.js:359: dbUpdated = true;
+1 :)

[gary.poster, 2013-04-11 23:16:13]

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-routi...
> File app/assets/javascripts/ns-routing-app-extension.js (right):
> 
>
https://codereview.appspot.com/8680043/diff/1/app/assets/javascripts/ns-routi...
> 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.

[gary.poster, 2013-04-11 23:16:43]

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.

[gary.poster, 2013-04-11 23:20:42]

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

[gary.poster, 2013-04-11 23:21:26]

On 04/11/2013 07:16 PM, gary.poster@canonical.com 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