NSMenuDelegate menuNeedsUpdate


#1

Hello, I am working on a complex popup system which is working perfectly fine on Windows, but I am having some issues getting it to work on OSX. For the system to be able to work I need to be able to update the NSMenu before it is shown to the user. In the JUCE code the JuceMenuCallbackClass adopting the NSMenuDelegate protocol does receive a menuNeedsUpdate but only the first time it is called. When I remove the call to menuBarItemsChanged I do get a second callback when a menu is requested. I tried setting the addDelegate bool in the createMenu method to try to listen to all the menus in order to get a second callback, but even then menuNeedsUpdate is never called again.

My question boils down to this:
How can I get a callback to reconstruct my menu whenever a user clicks on the menubar (before the menu is shown)?


#2

I think I managed to find a fix:

void updateTopLevelMenu (NSMenuItem* parentItem, const PopupMenu& menuToCopy, const String& name, int menuId, int topLevelIndex)
    {
        // Note: This method used to update the contents of the existing menu in-place, but that caused
        // weird side-effects which messed-up keyboard focus when switching between windows. By creating
        // a new menu and replacing the old one with it, that problem seems to be avoided..
		
		//ORIGINAL JUCE CODE: NSMenu* menu = [[NSMenu alloc] initWithTitle: juceStringToNS (name)];
		NSMenu* menu = createMenu (menuToCopy, name, menuId, topLevelIndex, true);
		
        //for (PopupMenu::MenuItemIterator iter (menuToCopy); iter.next();)
        //    addMenuItem (iter, menu, menuId, topLevelIndex);

        [menu setAutoenablesItems: false];
        [menu update];
        [parentItem setSubmenu: menu];
        [menu release];
    }

Whenever the updateTopLevelMenu function was called a new NSMenu was created to replace the top level menu but the createMenu function was not called in which the NSMenuDelegate was attached to the NSMenu. By using createMenu here we reattach the delegate and we get our second menuNeedsUpdate callback etc.

Is this intended?


#3

Hmm… Might be a good change. I vaguely remember there being subtle problems but looking back through the logs I can’t see anything that indicates this would cause problems.

So just this would work in your case?

void updateTopLevelMenu (NSMenuItem* parentItem, const PopupMenu& menuToCopy, const String& name, int menuId, int topLevelIndex)
{
    NSMenu* menu = createMenu (menuToCopy, name, menuId, topLevelIndex, true);
    [parentItem setSubmenu: menu];
    [menu release];
}

#5

I still have a problem:

In the NSMenu documentation it says the following:

menuNeedsUpdate:
Invoked when a menu is about to be displayed at the start of a tracking session.

If i call menuUpdate or menuBarItemsChanged directly from this callback the isOpen bool is already at true which defers the update till after I close the menu. However I need it to happen before the menu is opened.

Discussion
Using this method, the delegate can change the menu by adding, removing, or modifying menu items. If populating the menu will take a long time, implement numberOfItemsInMenu: and menu:updateItem:atIndex:shouldCancel: instead.
Menu item validation occurs after this method is called. If the menu is updated because the user pressed a command key, only the menu item with the matching command key is validated; if the menu is updated because the user opened it, then every menu item is validated.

From what I read here it should be possible to update the menu before showing it right? But when I force the update I get a bad a bad memory access from the NSApp run.

How come the menu is already open before menuNeedsUpdate is called?


#6

Oh, I have no idea how to help with weird NSMenu edge-cases… The existing code has worked fine for everything I’ve done with it - if you can suggest a sensible change then I’ll consider it, but am also afraid to touch the way it currently works because the whole NSMenu system does seem extremely fragile.


#7

I came up with the following now, which will make it behave similar to how you would build a menu bar on windows.

static void menuNeedsUpdate (id self, SEL, NSMenu* menu)
{
	JuceMainMenuHandler* const owner = getIvar<JuceMainMenuHandler*> (self, "owner");
	owner->updateTopLevelMenu(menu);
}

void updateTopLevelMenu (NSMenu* menu)
{
	NSMenu* superMenu = [menu supermenu];
	const StringArray menuNames = currentModel->getMenuBarNames();
	const int indexOfMenu = [superMenu indexOfItemWithSubmenu: menu];
	[menu removeAllItems];
	PopupMenu updatedPopup = currentModel->getMenuForIndex(1, menuNames[indexOfMenu - 1]);
	for (PopupMenu::MenuItemIterator iter (updatedPopup); iter.next();)
           addMenuItem (iter, menu, 1, indexOfMenu);
	[menu update];
}

This one works because it only updates one menu, and doesn’t require any checking whether the menu is already open etc, since we are just touching the menu that apple gives us. This seems to be the way that menuNeedsUpdate(NSMenu*) is supposed to be used. Updating all the menus in the menuNeedsUpdate callback caused problems.


#8

OK… but it looks like this discards a lot of code that deals with edge-cases. The updateMenus() method does a bunch of stuff to avoid problems when PopupMenus are active as non-menubar menus, and it triggers its update asynchronously. Your change seems to make sense but just feels like we’d get a trickle of subtly broken edge-case bugs if I went for it. (And it’s very difficult with this kind of thing to really know what to test, as there are so many possible scenarios)


#9

The code that was supposed to deal with that which was called from menuNeedsUpdate was never even used, because the NSMenuDelegates were never reattached so menuNeedsUpdate only got called once when the menu was opened for the first time. I’m not saying you should use any of this, just be aware that the current juce implementation probably doesn’t work as intended, since the menu delegates are not listening.


#10

Why the hard-coded 1?


#11

Yeah good question, that is something I did not change. There was a variable for it before but it never changed.


#12

I already pushed some changes for this - you might want to try it out and let me know if you still have any problems.


#13

Great I will take a look soon :slight_smile: