New more modern PopupMenu style

[jules]

Just a heads-up that I’ve added some features to PopupMenu that everyone should probably start using!

We already had a method PopupMenu::addItem (PopupMenu::Item), but it was a bit of a faff to create an Item object, set all its flags, and then pass it into the addItem method, so I’ve added a bunch of methods to the Item class to allow chained function calls to set one up:

WeAreROLI/JUCE/blob/fd4125d964dc42936d3fb8b3249d1e882f4ad38f/modules/juce_gui_basics/menus/juce_PopupMenu.h#L184

So the idea is you can now write things like this (excerpt from the Projucer):

PopupMenu colourSchemeMenu;

colourSchemeMenu.addItem (PopupMenu::Item ("Dark")
                            .setTicked (selectedColourSchemeIndex == 0)
                            .setAction ([this] { setColourScheme (0, true); updateEditorColourSchemeIfNeeded(); }));

colourSchemeMenu.addItem (PopupMenu::Item ("Grey")
                            .setTicked (selectedColourSchemeIndex == 1)
                            .setAction ([this] { setColourScheme (1, true); updateEditorColourSchemeIfNeeded(); }));

colourSchemeMenu.addItem (PopupMenu::Item ("Light")
                            .setTicked (selectedColourSchemeIndex == 2)
                            .setAction ([this] { setColourScheme (2, true); updateEditorColourSchemeIfNeeded(); }));

This makes it much more readable than the old addItem style with some true/false parameters to set the active/ticked state where it was never obvious which one was which, and rather than needing to attach a single async callback for the whole menu and then have item IDs for each item, you just provide a lambda for each one when you add it, and then kick off the menu asynchronously to do its stuff.

There are a few more examples in the codebase if you search for “.setAction”, and we’ll gradually transition to this style for most menus. No plans to deprecate the old methods as they’re so widely used, and whether or not you want to refactor your own code is up to you, but I’d strongly recommend everyone to use this style for any new menu code they write!

[RolandMR]

Very nice. Doing async PopupMenus has been a pain for so long.

How do you recommend making sure the source component is still valid by the time the action function is called? Get a WeakReference to the source component in every lambda?

[jules]

Yeah, you can capture a WeakReference but in practice this is such a common use-case that I was thinking of adding something specifically to handle it, e.g. maybe a function like Item::setComponentToWatch or something. Thinking of the right name is tricky though… “setComponentToCheckForDeletionBeforeInvokingAction” would be a bit long!

[RolandMR]

Putting it in the item would be a lot of repetition for each menu item. Would be nice if it was in the PopupMenu::Options and then if it was null no callbacks would fire. Both places would be fine, but I can’t think of an use cases where you’d depend on a different component for each menu item.

[jules]

yeah, that’s probably a good idea. And if you really do need a per-item check, it’s still easy enough to capture WeakReferences for them.

[basteln]

Thanks Looks nice, I like the “builder” approach from PopupMenu::Options.

I know this is probably for more complicated use cases, but personally I found the old addItem(int itemResultID, ... API pretty good. In the example code above, most of the setAction part is now duplicated, only the number passed to setColourScheme changes.
With the old API, you could just pass the menu’s result to setColourScheme, except that 0 is reserved for when the user dismissed the menu. So you would have to subtract -1 etc. But how about allowing the API to change the reserved number from 0 to something else like INT_MAX? This would make a lot of cases very simple, and also make it possible to use the result to index into an array, for example.

Just if someone’s interested, for weak capturing, here’s a very simple helper I use:

template<typename T>
auto weak(T* t)
{
    return juce::WeakReference<T>(t);
}

This way the lambda capture becomes shorter: [weakThis = weak(this)] instead of weakThis = WeakReference<MyClass>(this).

[jules]

Well, I’ve updated maybe a dozen bits of old code to use the new style, including ones where it used the item ID to index into some kind of list, and they’ve all been much cleaner with the new style.

Even if you have a big list of numbered items, I find it much neater to capture each index (or target object) in its lambda, which hides all the crap involved with having a intermediate integer variable which has to be handled correctly by some separate piece of code in another function.

[reFX]

@jules

Even if you have a big list of numbered items, I find it much neater to capture each index (or target object) in its lambda

Then your definition of “big” seems to be different than ours

We have a list of literally over 500+ items, lots of them just repeats with a different offset (for layers 1-16, oscillators 1-64 etc.), so having a simple index that then maps to a string we can store in the preset, is the only sensible way to go.

[jules]

The only difference it’d make in terms of size is that it’d use a few extra bytes per item to store the lambda object, but even for hundreds of items that’s only going to be a few extra unnoticed KB of memory.

If you think the code would be any bigger then you’re probably misunderstanding how you’d write something like this?

[reFX]

I’m not talking about the memory used, but the effort of writing 400 lambda functions instead of just using the index as is. We must misunderstand each other here. You can’t seriously mean for us to write 500+ lambdas by hand just to set a string or int to a certain value. That’s what indices and tables are for.

[jules]

for (int i = 0; i < 1000; ++i)
    m.addItem (PopupMenu::Item ("item " + String (i)).setAction ([=] { userClickedItemNumber (i); });

[reFX]

Yeah, I think we will stick with the old style for stuff like this. This really feels like using the new style because “we can” / “it’s shiny and new”, not because it’s in any way, shape or form better.

We use sub-menus, which can also have sub-menus etc. Now it gets more awkward.

[jules]

The point is that this lets the type system hide that nasty intermediate integer ID rather passing it around and getting things wrong when you handle it. All the bits of code that need to know about a particular item get written in one place rather than being scattered across different functions and interleaved with handlers for all the other items.

I can’t honestly think of a menu-related situation where this style wouldn’t be at least as good as the old system, but you should do whatever floats your boat.

[jules]

Just a follow-up note re: this new menu stuff - I’ve added a PopupMenu::Options::withDeletionCheck() method now, which you can give a Component, and if that component is deleted while the menu is open no action will be performed.

[audiothing]

Thanks for the new methods! One small request, if possibile, instead of using the MenuItemIterator, it would be cool to have a method to retrieve a PopupMenu::Item passing an itemResultID.

[jules]

TBH that would have been a good FR before we started heading down this route - I’d rather encourage people to use lambdas, and the flip side of that is that IDs stop being used.

Can’t really think of a use-case anyway… What info is there in the item that you might need?

[audiothing]

Some crackers love to disable menu items (open registration windows, about etc…). Checking against that in a buried delayed check can give some advantage. We have built a nice selection of delayed checks/time bombs that work really well. So yeah, not something really important, but would be nice to have.

[reFX]

@jules

I’d rather encourage people to use lambdas, and the flip side of that is that IDs stop being used.

Our use case is that we have our own “binding” system to bind arbitrary floats, ints, string etc. to a component (or even multiple components) and then, whenever that component gets changed, the variable in memory gets updated automatically. When we load a preset, we can simply call “updateComponents” and it loops over all bound components and read their respective variable and updates the component accordingly.

So we definitely need indices or assignable IDs.

[rebbur]

Nice addition Jules.
We went a similar route and created our own version of this.
It get’s even more interesting to allow using ValueTree properties in the Menu that are automatically set when item is clicked.
Below you can see a snippet of how it looks like for us. We went quite far with this but indeed makes building PopupMenu’s much easier and fun.

The PopupMenuFactory eventually produces a PopupMenu.

		PopupMenuFactory::addItems(layer,
				popup::Separator(),
				popup::Button(layer->ignoreColumnTrigger),
				popup::Button(layer->triggerFirstClipOnCompositionLoad),
				popup::Button(layer->faderStart),
				popup::Separator(),
				popup::ChoicesSubmenu(layer->maskMode),
				popup::Separator(),
				popup::Button("Lock Content", [] { /*..some code..*/ ).ticked(true),
				popup::Separator(),
				popup::ColorPalette(layer->colorId, color_coding::compositionPalette)
			);

[matkatmusic]

Is it possible to use this in conjunction with the ComboBox class?

ComboBox has a single lambda callback when you select something. but I didn’t see anything in the docs that would let us assign a lambda directly to a menu item…