Cannot remove menu dividers on Mac

[lkcampbell]

OS Version: Mac OSX Mavericks

Brackets Version: Sprint 35

Background: The purpose of this issue is to provide a simulation of what occurs when I have attempted (unsuccessfully) to remove a menu divider from a Mac menu. Brackets menu divider ids are created by a counter to keep them unique. I chose a couple of high numbered ids (1000 and 1001) to decrease the chance of overwriting existing divider ids that may already be added to Brackets.

Scenario 1:
Open Brackets source code, then open Dev Tools to Console and paste the following code snippet into the Console:

brackets.app.addMenuItem("view-menu", "---", "brackets-menuDivider-1000", "", "", undefined, undefined, function (err) { console.log(err); });

Look at the view menu. There should be a divider added at the bottom.

Now paste the following code snippet in the Console:

brackets.app.removeMenuItem("view-menu-brackets-menuDivider-1000", function (err) { console.log(err); });

The menu divider has not been removed. It is still at the bottom of the view menu.

The number "3" is printed in the console log. brackets.app.removeMenuItem() has thrown an error.

Scenario 2:
Open Brackets source code, then open Dev Tools to Console and paste the following code snippet into the Console:

brackets.app.addMenuItem("view-menu", "---", "brackets-menuDivider-1001", "", "", undefined, undefined, function (err) { console.log(err); });

Look at the view menu. There should be a second divider added at the bottom.

Now paste the following code snippet in the Console:

brackets.app.removeMenuItem("brackets-menuDivider-1001", function (err) { console.log(err); });

The menu divider has not been removed. There are now two dividers at the bottom of the view menu.

The number "3" is printed in the console log. brackets.app.removeMenuItem() has thrown another error.

If I remember correctly, Scenario 2 works for Windows. I switched over to a Mac recently and can't currently test Windows, so any fix should be double checked for Windows as well.

Neither scenario works for Mac, so I need to know why there is a difference and what ID string I need to provide to successfully remove a menu divider in Mac.

[lkcampbell]

@redmunds, this is the final issue that needs to be addressed before issue #5078 can be addressed.

I went ahead and assigned it to you per our previous discussions. Please let me know if you have any questions or see any obvious errors in my code, and I will provide additional help as you need it. Thanks for looking at this.

[redmunds]

After looking at the native code, I don't think the removeMenuItem() API call will work. Internally, there is not a single map of all MenuItem ids -- there is a map for each menu. So, I think we'll need something more like:

brackets.app.removeMenuItem("view-menu", "brackets-menuDivider-1000", function (err) { console.log(err); });

Notice a new menu id parameter similar to the addMenuItem() call.

The Native Menu unit tests all pass, so we should take a closer look at what the tests are doing.

I'm not clear on what all of the goals are, so I created a test extension: https://gist.github.com/redmunds/5c4f54ebd0a0a097ca86 . This extension uses the Menu API (not calling brackets.app directly).

What I'm seeing:

• Menu.removeMenuItem() does not seem to work on either Mac or Windows
• Menu.removeMenu() seems to work on both Mac or Windows
• Menu.removeMenu() followed by a Reload does not show dividers on either Mac or Windows

Is the goal to fix just the dividers? All of the above? Something else?

[lkcampbell]

@redmunds, for issue #5078, the goal is to be able to remove all menus just before Reload Without User Extensions. This gives a clean slate to then load all of the core menus. It effectively filters out any User Extension menu changes.

The more specific goal would be to fix brackets.app.removeMenuItem() and make it easier to understand how to use the method, on all platforms (thinking of Linux in the future as well), because I have been struggling with it for quite a while and still can't get it to work properly. Through that understanding, removeMenuDivider() should be easy to fix because it uses removeMenuItem(), and removeMenuItem() uses brackets.app.removeMenuItem().

Here's some more history now that you have investigated the initial problem. Understand that some of this history I am about to give you is speculation since I don't know exactly how the app shell methods work.

I originally tried to remove menus without removing the individual menu items and it failed. Here's the pull request:

#5217

All of the menus would disappear but the menu items would not come back when I tried to reload them. I concluded that the application shell was hanging onto the menu items even though I removed the menus, and that some sort of duplication error was causing the menu items to fail to reappear. I then submitted this pull request to make sure I removed all menu items (including dividers) before removing the menu:

#5384

I put in unit tests but I didn't think to test the scenario where you remove a divider, then add a divider with the same id back in. That's why this problem isn't being caught by the Native Menu unit tests.

I don't know why regular removeMenuItem() is not working for you in either platform. I definitely know that it was working in Windows for me, but I also recall that the specific commandId that the app shell used was difficult to figure out. I will take a look at your test extension today and see if I can figure out why it is failing.

Also, I agree with your assessment that the brackets.app.removeMenuItem() method needs an API similar to brackets.app.addMenuItem(). It needs to specify a menu name (i.e. parentId) and a menu item id. My biggest problem has been trying to guess what the magical value of commandId is for removeMenuItem(). It is particularly difficult when referring to dividers, which are not commands, and do not have a default commandId. I should just be able to give a menu id and a menu item id, and be done with it.

[lkcampbell]

@redmunds, there is an error in the test extension. It's not even reaching removeMenuItem().

Change line 37 from:

var test_menu_item = Menus.getMenuItem(TEST_REMOVE_ITEM4);

To:

var test_menu_item = Menus.getMenuItem("test.remove.menu-" + TEST_REMOVE_ITEM4);

And you will see that removeMenuItem() is working correctly for regular menu items (at least, it is on my Mac).

[redmunds]

Good catch. There's a second call to getMenuItem() that also needed to be updated. I updated test extension gist.

So removeMenuItem() does work on both Mac and Windows. I am still seeing the problem with dividers on both Mac and Windows, which is good news for me since I am more familiar with the VS debugger.

[lkcampbell]

@redmunds , you might want to take a look at the following commit from the old PR I did for "Reload Without User Extensions" which addresses the removeMenuDivider() problem I had in Windows:

https://github.com/lkcampbell/brackets/commit/7f17970c194ef2beba87bec3f165b33b5806f6eb

I had to take a substring of the menuItemID in order to properly match the commandId of the divider I wanted to remove. This solution fixed removeMenuDivider() in Windows but failed to work in Mac, and that's where the PR progress stopped.

Just keep in mind that the solution you discover in Windows, which will probably be the same solution I found above, may not work in Mac, and you will probably still need to figure out what the magical commandId for the Mac menu divider is.

[redmunds]

I think I have it fixed (but haven't done much testing). You need both of these:
brackets: #6333
brackets-shell: adobe/brackets-shell#405

[lkcampbell]

@redmunds, I can test this fix out and rewrite the new PR for "Reload Without User Extensions".

Beyond that, how would you like to proceed on this issue? Did you want me to code review these changes or should we get someone more experienced with Mac app shell to review it? I know C-like languages pretty well but I am unfamiliar with Mac programming.

[redmunds]

@lkcampbell I think these changes are simple enough that you can review and merge these PRs.

#6333 is the same fix as you tried before, except that the divider id is now saved so it can be re-used (as opposed of parsing it out of the menu item id). This fixes problem on Windows.

adobe/brackets-shell#405 is a simple logic error fix to Mac native code. NativeMenuModel.setOsItem() was only being called for non-divider menu items, so this caused RemoveMenuItem() to return before calling NativeMenuModel.removeMenuItem() because NativeMenuModel.getOsItem() returned null. After reload, in the call to AddMenuItem() there was already an entry for each divider (NativeMenuModel.getTag()), so NO_ERROR was returned and they were not re-added to menu on Mac. I moved the call so it's also called for divider menu items.

Let me know if you're not comfortable merging either of those and I'll find someone else to merge them.

[lkcampbell]

Looks like it is fixed with the latest build, thanks.

[redmunds]

Set Milestone to Sprint 36.