Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(693)

Issue 8680043: Fix bug 1167967 (hopefully)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by gary.poster
Modified:
10 years, 11 months ago
Reviewers:
bcsaller, mp+158468, matthew.scott
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 5

Patch Set 2 : Fix bug 1167967 (hopefully) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -26 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M app/app.js View 1 chunk +1 line, -1 line 0 comments Download
M app/assets/javascripts/ns-routing-app-extension.js View 1 2 chunks +5 lines, -2 lines 0 comments Download
M app/views/charm.js View 1 chunk +1 line, -1 line 0 comments Download
M app/views/landscape.js View 1 chunk +2 lines, -1 line 0 comments Download
M app/views/service.js View 1 chunk +2 lines, -1 line 0 comments Download
M app/views/utils.js View 1 chunk +16 lines, -14 lines 0 comments Download
M test/test_app_hotkeys.js View 1 chunk +1 line, -1 line 0 comments Download
M test/test_charm_view.js View 1 chunk +1 line, -1 line 0 comments Download
M test/test_landscape.js View 2 chunks +15 lines, -0 lines 0 comments Download
M test/test_routing.js View 1 1 chunk +12 lines, -0 lines 0 comments Download
M test/test_service_view.js View 1 chunk +7 lines, -4 lines 0 comments Download

Messages

Total messages: 7
gary.poster
Please take a look.
10 years, 11 months ago (2013-04-11 19:55:30 UTC) #1
bcsaller
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 ...
10 years, 11 months ago (2013-04-11 20:10:37 UTC) #2
matthew.scott
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: ...
10 years, 11 months ago (2013-04-11 22:45:15 UTC) #3
gary.poster
On 2013/04/11 20:10:37, bcsaller wrote: > LGTM. > > Some suggestions around the url handling ...
10 years, 11 months ago (2013-04-11 23:16:13 UTC) #4
gary.poster
On 2013/04/11 22:45:15, matthew.scott wrote: > LGTM with Ben's comments at least replied to. Thanks! ...
10 years, 11 months ago (2013-04-11 23:16:43 UTC) #5
gary.poster
*** Submitted: Fix bug 1167967 (hopefully) This branch attempts to fix the referenced bug. I ...
10 years, 11 months ago (2013-04-11 23:20:42 UTC) #6
gary.poster
10 years, 11 months ago (2013-04-11 23:21:26 UTC) #7
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b