[CLOSED] Added RemoveMenuItem

[core-ai-bot]

Issue by tpryan
Friday Jun 29, 2012 at 01:35 GMT
Originally opened as adobe/brackets#1193

---

Added the ability to remove Menu Items. This should be useful for extension writers. It works for both the vanilla menus and context menus.

---

tpryan included the following code: https://github.com/adobe/brackets/pull/1193/commits

[core-ai-bot]

Comment by peterflynn
Friday Jun 29, 2012 at 16:20 GMT

---

@tpryan, could you remove the whitespace diffs from your patch?

[core-ai-bot]

Comment by tpryan
Friday Jun 29, 2012 at 18:00 GMT

---

@peterflynn I'm happy to remove the whitespace changes. But they were preventing jslint from reporting no errors. Can we ignore that prescription before doing a pull request?

[core-ai-bot]

Comment by peterflynn
Monday Jul 02, 2012 at 00:03 GMT

---

@tpryan: Ah yes, you hit a confusing quirk of Brackets's coding standards. Unlike vanilla JSLint, we're ok with whitespace on a line as long as the line is completely blank otherwise. Since JSLint has no option for turning off that error, the Brackets feature that runs JSLint automatically filters out those errors. But of course if you run JSLint outside Brackets, it will still complain. We should probably document that better :-)

[core-ai-bot]

Comment by peterflynn
Monday Jul 02, 2012 at 00:04 GMT

---

Adobe is on break this coming week, but I should be able to review the update within a day or two.

[core-ai-bot]

Comment by tpryan
Monday Jul 02, 2012 at 01:00 GMT

---

@peterflynn that's a nice way of saying you caught me using another editor. Point taken.

I work for Adobe too. We shouldn't be having this conversation.

[core-ai-bot]

Comment by peterflynn
Monday Jul 09, 2012 at 17:02 GMT

---

@tpryan: heh, no worries about the other editor. We are strict about JSLint though, and using Brackets definitely makes that part easier :-)

So I caved to the vacation pressure and didn't re-review until now. Thanks for all the updates! I had two high-level thoughts on this (which I wish I'd thought of a week ago):

First -- can you add a unit test or two to Menu-test.js for the new API? (It already has tests for add: append, insert, etc.)

Second -- I think it may be a mistake to cast the API in terms of the internal MenuItem objects' ids instead of commands (or command ids). If you own the code that added the menu item in the first place, you can just hang onto the MenuItem and use its .id field... but if not, there's really no clean way to get the right id -- and ideally, we'd support that use case too. I wonder if we could hoist out the middle of _getRelativeMenuItem() (finding MenuItem based on command id) so that it could be shared by removeMenuItem() too.@tvoliter, any thoughts on that?

[core-ai-bot]

Comment by tpryan
Wednesday Jul 11, 2012 at 00:31 GMT

---

@peterflynn I'm happy to add tests. I'll get to work on them.

As for the API. I'd prefer to pass either a command or commandID like AddMenuItem does. Is that what you're going for?

[core-ai-bot]

Comment by peterflynn
Thursday Jul 12, 2012 at 18:52 GMT

---

Nice meeting you in person yesterday! For anyone else reading: yes, changing it to accept either command or commandID (like addMenuItem()), seems ideal.

[core-ai-bot]

Comment by tpryan
Friday Jul 13, 2012 at 22:05 GMT

---

Added the ability to use commands or id's. I have tested, it works, but I can't get the test runner working. Something's borked with my setup.

[core-ai-bot]

Comment by peterflynn
Wednesday Oct 03, 2012 at 13:56 GMT

---

@tpryan: now that we're both no longer frantically busy... :-) I have a few bits of smaller feedback and then I think this is ready to merge.

[core-ai-bot]

Comment by peterflynn
Wednesday Oct 03, 2012 at 14:06 GMT

---

Lastly: needs a merge with master. The merge looked pretty clean when I tested it locally, so hopefully that's fairly easy.

[core-ai-bot]

Comment by peterflynn
Monday Nov 05, 2012 at 06:21 GMT

---

@tpryan: let me know if you're still interested in working on this. Otherwise I'll plan on making the last few changes myself tomorrow and then merging.

[core-ai-bot]

Comment by tpryan
Monday Nov 05, 2012 at 18:31 GMT

---

@peterflynn I'm still interested, but if you're close to being able to push it out, I don't want to hold you back. I'll be traveling, so won't be able to do the work for a few days.

[core-ai-bot]

Comment by peterflynn
Wednesday Nov 07, 2012 at 01:37 GMT

---

Ok, cool. I've opened #2072 with the updated code.