Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(945)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 6652055: Quicken env. redraw after relation add and remove

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 3 months ago by teknico
Modified:
13 years, 2 months ago
Reviewers:
mp+129449, hazmat
Visibility:
Public.
Quicken env. redraw after relation add and remove Do not wait for delta, but update the db and redraw the environment on RPC results, on both relation addition and removal. Also, fix a notification bug on relation removal. https://code.launchpad.net/~teknico/juju-ui/quicken-new-relation-draw/+merge/129449 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2

Patch Set 2 : Quicken env. redraw after relation add and remove #

Total comments: 1
Created: 13 years, 2 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -27 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M app/models/models.js View 1 1 chunk +3 lines, -1 line 0 comments Download
M app/store/notifications.js View 1 4 chunks +10 lines, -11 lines 0 comments Download
M app/views/environment.js View 1 7 chunks +38 lines, -10 lines 1 comment Download
M test/test_application_notifications.js View 1 2 chunks +3 lines, -4 lines 0 comments Download
M test/test_environment_view.js View 1 1 chunk +3 lines, -1 line 0 comments Download
Total messages: 7
|
teknico
Please take a look.
13 years, 3 months ago (2012年10月12日 14:54:31 UTC) #1
Please take a look.
Sign in to reply to this message.
hazmat
looks good https://codereview.appspot.com/6652055/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6652055/diff/1/app/views/environment.js#newcode1294 app/views/environment.js:1294: type: endpoints[0][1].name, not quite type != name, ...
13 years, 3 months ago (2012年10月12日 19:58:50 UTC) #2
looks good
https://codereview.appspot.com/6652055/diff/1/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6652055/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1294: type: endpoints[0][1].name,
not quite type != name, name has naught to do with type, its just the service's
name for that particular relation regardless of type. ie. wordpress calls type:
mysql relation name: db.
please also store 'scope':result.scope so we can determine if the relation is a
subordinate or not.
Sign in to reply to this message.
hazmat
I just noticed/realized we're drawing the relation name everywhere.. instead of the interface. it does ...
13 years, 3 months ago (2012年10月12日 20:11:18 UTC) #3
I just noticed/realized we're drawing the relation name everywhere.. instead of
the interface. it does look much better, but its a bit arbitrary.
Ideally we store continue to display this, but we store it as a display_name on
the rel, we should still capture the actual interface type of the rel correctly.
Sign in to reply to this message.
hazmat
the relation display_name can be either pushed to another branch/card or done here but the ...
13 years, 3 months ago (2012年10月12日 20:48:59 UTC) #4
the relation display_name can be either pushed to another branch/card or done
here but the other minors should be worked on.
https://codereview.appspot.com/6652055/diff/1/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6652055/diff/1/app/views/environment.js#newcod...
app/views/environment.js:931: env.remove_relation(
This call needs to have the qualified endpoints to be done correctly, ie. not
just service name but the 
service_name:endpoint_name for each side, else you can run into ambiguity errors
removing a relation.. like for example between mediawiki and mysql.
Sign in to reply to this message.
teknico
Please take a look.
13 years, 2 months ago (2012年10月15日 14:42:29 UTC) #5
Please take a look.
Sign in to reply to this message.
teknico
hazmat wrote [a fair number of comments :-) ]: > [snip] We believe we addressed ...
13 years, 2 months ago (2012年10月15日 14:44:24 UTC) #6
hazmat wrote [a fair number of comments :-) ]:
> [snip]
We believe we addressed all the points you raised (and then some ;-) ). Please
check and let us know, thanks.
Sign in to reply to this message.
hazmat
looks good, thanks for the changes. https://codereview.appspot.com/6652055/diff/7001/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6652055/diff/7001/app/views/environment.js#newcode857 app/views/environment.js:857: if (bpair.display_name === ...
13 years, 2 months ago (2012年10月15日 18:19:47 UTC) #7
looks good, thanks for the changes.
https://codereview.appspot.com/6652055/diff/7001/app/views/environment.js
File app/views/environment.js (right):
https://codereview.appspot.com/6652055/diff/7001/app/views/environment.js#new...
app/views/environment.js:857: if (bpair.display_name === undefined) {
aha i was wondering where this was being done.
Sign in to reply to this message.
|
Powered by Google App Engine
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /