Support for service positions from server

Support for service positions from server

Provides support for loading initial and updated position 
from annotations stored on the server. Updates occur
via a shared codepath with the drag event handler
to ensure proper updates are applied over time.

There is still an issue with deltas interrupting the drag.

https://code.launchpad.net/~bcsaller/juju-gui/persistent-layout/+merge/143980

(do not edit description out of merge proposal)

[bcsaller, 2013-01-19 07:10:26]

Please take a look.

[bcsaller, 2013-01-19 15:02:17]

Please take a look.

[bcsaller, 2013-01-19 15:02:45]

Thanks for taking a peek, I resolved the merge and proposed again.

[bcsaller, 2013-01-19 15:15:40]

Please take a look.

[gary.poster, 2013-01-20 15:58:45]

Hi Ben.  Thank you for this branch.  Land with changes (specifically reinstating
some of Nicola's changes that you lost, as mentioned below).

Tests pass for me.

The functionality works well, except for variants of the pre-existing bug
1099921: moving services while the delta stream is being processed leads to
unexpected and undesired behavior.  Either the drag is stopped, or the drag
completes but the processed location then moves the service to where it was
before.  Hopefully we can get that addressed soon.

If it helps move things along in terms of Monday/Tuesday delivery, I would be OK
with you landing this and then responding to the second/subsequent reviews in a
separate branch.  OTOH, if it doesn't help you, don't do it. :-)

Thanks

Gary

https://codereview.appspot.com/7132061/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/app.js#newcode351
app/app.js:351: active.update();
cool

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:28: mouseout: 'serviceStatusMouseOut'
Moving the functions out of the event registration does make it easier to read. 
However, it makes the diff much harder to read.  For instance, AFAICT, there are
no changes to these functions, but I'm coming to that conclusion by eyeball
rather than by mechanical diff.  This would be the kind of change I'd like to
see in a separate branch to keep the branch size down, to reduce the development
time, and to ease reviews of the pertinent code changes to actually implement
this feature.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:33: * relation.
We now have this comment both in the event registration and in the function
definition.  I'm not sure that inline event handler functions don't have their
place, to be honest.  If we do separate them, I think we can only expect to
maintain comments in one place.  I believe that the place should be the
function, so I would recommend deleting this copy of the comment.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:65: toggleControlPanel: {callback: function() {
I prefer the change that Nicola made, that you have reverted.  In that change,
he renamed "ControlPanel" to "ServiceMenu," which seemed like a naming
improvement to Nicola and to his two reviewers.  He also was able to be explicit
that we only needed to "hide" here, which would be nice if we can manage it.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:178:
self.service_click_actions.toggleControlPanel(null, self);
You've reverted Nicola's changes.  I think they were improvements, but even if
you disagree, socially a discussion would be better.  Please don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:189: .toggleControlPanel(box, context);
You've reverted Nicola's changes.  Please don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:201: .toggleControlPanel(box, context);
You've reverted Nicola's changes.  Please don't do this.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:285:
self.service_click_actions.toggleControlPanel(null, self);
If you use Nicola's changes, this will need to change as well.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:316: drag: function(d, self, pos,
includeTransition) {
Cool.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:869: * @param {object} module The service module..
Thank you for correcting this.

https://codereview.appspot.com/7132061/diff/6001/undocumented
File undocumented (left):

https://codereview.appspot.com/7132061/diff/6001/undocumented#oldcode1
undocumented:1: app/app.js:599 "callback"
You increased the undocumented count from 241 to 246.  It would be nice if we at
least kept the number stable across these refactorings.  Given my desire to be
able to declare this complete by Monday, or Tuesday at the latest, I won't stop
you from landing for this, but maybe you could come back to write up five
methods later.

[bcsaller, 2013-01-20 17:10:46]

Thanks for the review, I'll attempt to address the issues today.

[frankban, 2013-01-21 11:19:45]

Hi Ben, thanks for this branch.
I have not so much to add to Gary's review. Please see just the one comment
below.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:514: topo.fire('serviceMoved', {service: d});
Isn't serviceMoved already fired by self.drag?

[bcsaller, 2013-01-21 15:40:31]

https://codereview.appspot.com/7132061/diff/6001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/app.js#newcode351
app/app.js:351: active.update();
On 2013/01/20 15:58:45, gary.poster wrote:
> cool

Thanks, cutting to this should actually be a big improvement. I was worried
there would still be more work left before this would work properly but it seems
fine like this. This cuts the number of render passes substantially and reduces
the amount of work we do quite a lot.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:33: * relation.
On 2013/01/20 15:58:45, gary.poster wrote:
> We now have this comment both in the event registration and in the function
> definition.  I'm not sure that inline event handler functions don't have their
> place, to be honest.  If we do separate them, I think we can only expect to
> maintain comments in one place.  I believe that the place should be the
> function, so I would recommend deleting this copy of the comment.

Agreed with comment placement, this was the result of trying to quickly cut a
smaller branch, bad porting.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:65: toggleControlPanel: {callback: function() {
On 2013/01/20 15:58:45, gary.poster wrote:
> I prefer the change that Nicola made, that you have reverted.  In that change,
> he renamed "ControlPanel" to "ServiceMenu," which seemed like a naming
> improvement to Nicola and to his two reviewers.  He also was able to be
explicit
> that we only needed to "hide" here, which would be nice if we can manage it.

Not intentional, this is how the trunk merge fell out. I'll restore his version
(which I had a hand in as well and agreed with).

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:178:
self.service_click_actions.toggleControlPanel(null, self);
On 2013/01/20 15:58:45, gary.poster wrote:
> You've reverted Nicola's changes.  I think they were improvements, but even if
> you disagree, socially a discussion would be better.  Please don't do this.

Bad Merge, fixed.

https://codereview.appspot.com/7132061/diff/6001/app/views/topology/service.j...
app/views/topology/service.js:201: .toggleControlPanel(box, context);
On 2013/01/20 15:58:45, gary.poster wrote:
> You've reverted Nicola's changes.  Please don't do this.

Same

https://codereview.appspot.com/7132061/diff/6001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7132061/diff/6001/test/test_environment_view.j...
test/test_environment_view.js:112: db.on_delta({data:
Y.clone(environment_delta)});
This change may be worth of review comment. This is needed because we delete
annotations once their x/y has been read and applied. This creates an issue in
testing because its a reference all the way back to the original test data stub.

https://codereview.appspot.com/7132061/diff/6001/undocumented
File undocumented (left):

https://codereview.appspot.com/7132061/diff/6001/undocumented#oldcode1
undocumented:1: app/app.js:599 "callback"
On 2013/01/20 15:58:45, gary.poster wrote:
> You increased the undocumented count from 241 to 246.  It would be nice if we
at
> least kept the number stable across these refactorings.  Given my desire to be
> able to declare this complete by Monday, or Tuesday at the latest, I won't
stop
> you from landing for this, but maybe you could come back to write up five
> methods later.

Added some doc strings, I think documenting callback handlers with a full doc
strings when they have uniform arguments and appeal to a documented usage
pattern is a bit of an anti-pattern (not that this is the only type of doc I
haven't written ;) . Happy to talk about this in a Friday call if its an issue.

[bcsaller, 2013-01-21 17:08:58]

*** Submitted:

Support for service positions from server

Provides support for loading initial and updated position 
from annotations stored on the server. Updates occur
via a shared codepath with the drag event handler
to ensure proper updates are applied over time.

There is still an issue with deltas interrupting the drag.

R=gary.poster, frankban
CC=
https://codereview.appspot.com/7132061