Fixed leak in menu.js file. Fixes #12765

[daytonjallen]

Fixes #12765

[ficristo]

I know it is a bit confusing, but please, revert the changes to src/config.json.

[daytonjallen]

I don't remember touching that file but I've reverted the changes. Sorry about that. Now the test is failing, looking into that now.

[daytonjallen]

Checking the CI logs, I think the reason for the build failure isn't anything on my end. Right?

[marcelgerber]

Yes, the build failure was on our side. It passed now 👍

cc @zaggino for review

[zaggino]

@daytonallen the code is great, but is missing a test and that should be done in this case as test for this is easy to write, please add it somewhere here: https://github.com/adobe/brackets/blob/master/test/spec/Menu-test.js#L660-L737

You should write it in a way that it fails when you comment out the removeMenuItemEventListeners(commandObj); call you added and passes when you uncomment it.

You'll need to compare contents of ._eventHandlers[eventName].length before and after calling removeMenuItem in the test.

[daytonjallen]

How do I run the tests locally to check if it's done correctly? I tried the debug's menu run tests option but it doesn't seem to be detecting a new test case. @zaggino

[zaggino]

@daytonallen did you setup your installed Brackets with your cloned Brackets folder like in here https://github.com/adobe/brackets/wiki/How-to-Hack-on-Brackets ? See the setup_for_hacking thingy.

[zaggino]

If you did, please push the testcase here, maybe something else is wrong with it, you can still correct it with additional commits.

[zaggino]

@daytonallen you need to look for it here, it runs for me:

[daytonjallen]

Turns out I was testing under the wrong grouping.

[zaggino]

you shouldn't add event manually with .on, you should just do expect for events that are added by addMenuItem

[zaggino]

@daytonallen
You can remove stuff copied from other tests, it should be somthing simple like

menu.addMenuItem(commandId);
expect(command._eventHandlers....);
menu.removeMenuItem(commandId);
expect(command._eventHandlers....);

everything else doesn't need to be there

[zaggino]

looking at the code now, _enabledChanged and other methods live on MenuItem not on command ... are you sure that you're not calling .off("enabledStateChange", undefined) ???

[zaggino]

Thanks @daytonallen merged to master! 👍

[daytonjallen]

My pleasure. Thanks for all the guidance also.