Added RemoveMenuItem #1193

tpryan · 2012-06-29T01:35:01Z

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

peterflynn · 2012-06-29T16:20:57Z

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

peterflynn · 2012-06-29T16:27:03Z

src/command/Menus.js

-    // function removeMenuItem(id) {
-    //    NOT IMPLEMENTED
-    // }
+    function _removeMenuItem(id) {


Does this need to be broken out as a separate helper function? If it's only ever called from Menu.removeMenuItem(), could you just inline this code in there?

tpryan · 2012-06-29T18:00:23Z

@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?

peterflynn · 2012-07-02T00:03:41Z

@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 :-)

peterflynn · 2012-07-02T00:04:57Z

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

tpryan · 2012-07-02T01:00:25Z

@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.

peterflynn · 2012-07-09T17:02:13Z

@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?

peterflynn · 2012-07-09T17:04:27Z

src/command/Menus.js

+     *
+     * @param {!string} id - the unique identifier for the menu.
+     *
+     * Note, keyBindings are not effected at all by removing a menu item. 


Also, two little nits here: "effected" -> "affected"; and there's an extra space after "a" on the first line.

tpryan · 2012-07-11T00:31:11Z

@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?

peterflynn · 2012-07-12T18:52:03Z

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

tpryan · 2012-07-13T22:05:06Z

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.

peterflynn · 2012-07-17T08:22:45Z

src/command/Menus.js

+            }
+        } else {
+            commandID = this.id + "-" + command.getID();
+        }


There are a couple problems with dropping this code in here:

The var commandID is not actually a command ID (the Command.id field) -- it's an internal MenuItem id (the MenuItem.id field). Should be renamed.

There's some unneeded code here:

the DIVIDER case is unneeded (it won't work anyway since divider IDs are all uniquely generated)

there's no need to go through menuItemMap since this.id + "-" + command is the same thing as menuItemMap[this.id + "-" + command].id.

This is somewhat in conflict with the code in Menu._getRelativeMenuItem(), which maps a command ID to a MenuItem in a much more roundabout way. Could you create a new utility function (e.g. "Menu._menuItemForCommandID()") that is called by both these methods?

Renamed

Removed

But I'm not getting a menu item in this function, I'm getting as you point out the menuItem.id. Am I missing what you want me to do?

… contains it.

…uld work.

… function

…tem function.

peterflynn · 2012-10-03T13:56:24Z

@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.

peterflynn · 2012-10-03T14:00:11Z

src/command/Menus.js

+                throw new Error("removeMenuItem(): command not found: " + command);
+            }
+
+            menuItemID = menuItemMap[this.id + "-" + command].id;


No need to go through menuItemMap here -- you're just mapping the menu item ID back to itself.

Also, both this and lin e385 below can use the new Menu._getMenuItemId() API to simplify this code:
menuItemID = this._getMenuItemId(command) here, and
menuItemID = this._getMenuItemId(command.getID() below

peterflynn · 2012-10-03T14:06:21Z

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

peterflynn · 2012-11-05T06:21:21Z

@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.

tpryan · 2012-11-05T18:31:22Z

@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.

peterflynn · 2012-11-07T01:37:34Z

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

Terrence Ryan added 2 commits June 28, 2012 21:23

Checking in the addition of RemoveMenuItem. Also made file pass jslint.

5048d29

Merge remote-tracking branch 'upstream/master' into removeMenuItem

5299d3f

peterflynn reviewed Jun 29, 2012
View reviewed changes

ghost assigned peterflynn Jun 29, 2012

Made changes to code in line with @peterflyn feedback here: #1193

074cd9c

peterflynn reviewed Jul 9, 2012
View reviewed changes

ghost assigned tvoliter Jul 9, 2012

Fixing comment grammar mistake.

935a9b7

Terrence Ryan added 2 commits July 13, 2012 16:39

Merge remote-tracking branch 'upstream/master' into removeMenuItem

9c5bc28

Added ability to accept commands as well as id's.

8d9ab5a

peterflynn reviewed Jul 17, 2012
View reviewed changes

ghost assigned peterflynn Jul 17, 2012

Terrence Ryan added 8 commits August 2, 2012 14:31

Merge remote-tracking branch 'upstream/master' into removeMenuItem

25f0acb

Fixed remove menu item to remove the parent <li> of the <a> link that…

cfce664

… contains it.

Cleaning out some whitespace from my addition to the test.

e22cf20

Removing Extraneous DIVIDER code

93a5dfc

Adding test to verify that removing by command or commandID string wo…

e4cdd4f

…uld work.

Added exception handling for cases where someone throws junk into the…

fb5f19c

… function

Testing for proper handling of junk being thrown into the removeMenuI…

5b01676

…tem function.

Renaming variable to make it, you know, refer to what it actually is.

c13722f

peterflynn reviewed Oct 3, 2012
View reviewed changes

peterflynn mentioned this pull request Nov 7, 2012

removeMenuItem() API #2072

Merged

peterflynn closed this Nov 7, 2012

This was referenced Aug 29, 2021

[CLOSED] Added RemoveMenuItem brackets-archive/bracketsIssues#1178

Open

[CLOSED] removeMenuItem() API brackets-archive/bracketsIssues#2003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added RemoveMenuItem #1193

Added RemoveMenuItem #1193

tpryan commented Jun 29, 2012

peterflynn commented Jun 29, 2012

peterflynn Jun 29, 2012

tpryan commented Jun 29, 2012

peterflynn commented Jul 2, 2012

peterflynn commented Jul 2, 2012

tpryan commented Jul 2, 2012

peterflynn commented Jul 9, 2012

peterflynn Jul 9, 2012

tpryan commented Jul 11, 2012

peterflynn commented Jul 12, 2012

tpryan commented Jul 13, 2012

peterflynn Jul 17, 2012

tpryan Aug 2, 2012

peterflynn commented Oct 3, 2012

peterflynn Oct 3, 2012

peterflynn commented Oct 3, 2012

peterflynn commented Nov 5, 2012

tpryan commented Nov 5, 2012

peterflynn commented Nov 7, 2012

Added RemoveMenuItem #1193

Added RemoveMenuItem #1193

Conversation

tpryan commented Jun 29, 2012

peterflynn commented Jun 29, 2012

peterflynn Jun 29, 2012

Choose a reason for hiding this comment

tpryan commented Jun 29, 2012

peterflynn commented Jul 2, 2012

peterflynn commented Jul 2, 2012

tpryan commented Jul 2, 2012

peterflynn commented Jul 9, 2012

peterflynn Jul 9, 2012

Choose a reason for hiding this comment

tpryan commented Jul 11, 2012

peterflynn commented Jul 12, 2012

tpryan commented Jul 13, 2012

peterflynn Jul 17, 2012

Choose a reason for hiding this comment

tpryan Aug 2, 2012

Choose a reason for hiding this comment

peterflynn commented Oct 3, 2012

peterflynn Oct 3, 2012

Choose a reason for hiding this comment

peterflynn commented Oct 3, 2012

peterflynn commented Nov 5, 2012

tpryan commented Nov 5, 2012

peterflynn commented Nov 7, 2012