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