Hide service menu on service double click.

Hide service menu on service double click. Hide the service menu when the service is double clicked. The browser sends a click event first (which opens the service menu), then the double click, so we need to manually hide the menu before opening the service details. With test. Also, rename the toggleControlPanel service method to toggleServiceMenu (because that's what we call it everywhere else), and split its implementation into two methods: showServiceMenu and hideServiceMenu. Use the latter in place of the toggle one in the places (most of them) where it is the intended meaning. https://code.launchpad.net/~teknico/juju-gui/1099926-no-svc-menu-on-dblclk/+merge/143851 (do not edit description out of merge proposal)

[teknico, 2013-01-18 10:50:58]

Please take a look.

[bac, 2013-01-18 12:24:43]

Thanks for the fix/work-around to very annoying browser behavior. There are two style changes I suggest and the addition of some function documentation. After those changes it is good to land. https://codereview.appspot.com/7131060/diff/1/app/views/topology/relation.js File app/views/topology/relation.js (right): https://codereview.appspot.com/7131060/diff/1/app/views/topology/relation.js#... app/views/topology/relation.js:278: Much clearer. https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js File app/views/topology/service.js (right): https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:762: one 'var' per variable https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:777: one 'var' per variable https://codereview.appspot.com/7131060/diff/1/undocumented File undocumented (right): https://codereview.appspot.com/7131060/diff/1/undocumented#newcode192 undocumented:192: app/views/topology/service.js:128 "initializer" I think it would've been nice to actually document these functions rather than add them here. This file should be decreasing in size.

[teknico, 2013-01-18 12:31:29]

Thanks, Brad. I will add the "var" keywords. I would also like to document the methods, but I'll some help with that. https://codereview.appspot.com/7131060/diff/1/undocumented File undocumented (right): https://codereview.appspot.com/7131060/diff/1/undocumented#newcode192 undocumented:192: app/views/topology/service.js:128 "initializer" bac wrote: > I think it would've been nice to actually document these functions rather than > add them here. This file should be decreasing in size. You are entirely right. I tried to do so but could not, because I am not sure I understand what the calling parameters mean. I looked at the invocations and at other similar methods in the same context, and there is too much variation in names and invocation pattern to be sure of the intended meaning.

[teknico, 2013-01-18 12:32:27]

teknico wrote: > ...I'll some help with that. ...I'll *need* some help... :-/

[gary.poster, 2013-01-18 13:23:05]

Land with changes. Thank you, Nicola. This is a nice branch, with a number of improvements in clarity and a nice test. Gary https://codereview.appspot.com/7131060/diff/1/app/views/topology/relation.js File app/views/topology/relation.js (right): https://codereview.appspot.com/7131060/diff/1/app/views/topology/relation.js#... app/views/topology/relation.js:278: On 2013/01/18 12:24:43, bac wrote: > Much clearer. Agreed. I'm hoping it is in fact "hide" rather than "toggle" and will try to keep an eye out in your branch for that confirmation. https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js File app/views/topology/service.js (right): https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:746: toggleServiceMenu: function(m, view, context) { Yes, I see what we have now. Nicer name here, and nicer clarity below when we actually want to explicitly hide or show within the code base. Thank you. https://codereview.appspot.com/7131060/diff/1/app/views/topology/service.js#n... app/views/topology/service.js:762: On 2013/01/18 12:24:43, bac wrote: > one 'var' per variable We never made this a requirement, for a reason I don't recall, but I agree that it is a good way forward generally. It doesn't buy us much in this particular case, but I don't mind it. https://codereview.appspot.com/7131060/diff/1/test/test_environment_view.js File test/test_environment_view.js (right): https://codereview.appspot.com/7131060/diff/1/test/test_environment_view.js#n... test/test_environment_view.js:503: // Tests for the service menu. Given that we now have explicit methods for showing and hiding, testing those wouldn't be amiss, I think. In both cases, a full set of tests would be to show that it acts when it needs to and is a no-op otherwise. https://codereview.appspot.com/7131060/diff/1/test/test_service_module.js File test/test_service_module.js (right): https://codereview.appspot.com/7131060/diff/1/test/test_service_module.js#new... test/test_service_module.js:131: assert.isFalse(menu.hasClass('active')); Cool, thank you. https://codereview.appspot.com/7131060/diff/1/undocumented File undocumented (right): https://codereview.appspot.com/7131060/diff/1/undocumented#newcode192 undocumented:192: app/views/topology/service.js:128 "initializer" On 2013/01/18 12:31:29, teknico wrote: > bac wrote: > > I think it would've been nice to actually document these functions rather than > > add them here. This file should be decreasing in size. > > You are entirely right. I tried to do so but could not, because I am not sure I > understand what the calling parameters mean. I looked at the invocations and at > other similar methods in the same context, and there is too much variation in > names and invocation pattern to be sure of the intended meaning. While I agree with you that this is not clear, that means it would be a fantastic thing to document, right? :-) bcsaller will be able to help you here, I am sure. Please do document these (and toggleServiceMenu while you are at it) as part of this branch.