clap icon indicating copy to clipboard operation
clap copied to clipboard

Investigate plugin context menu

Open abique opened this issue 2 years ago • 67 comments

abique avatar Dec 23 '21 11:12 abique

I might be better to implement this 'the other way round': the host provides a list of menu item descriptions and a click handler function. The plugin can then add these items to its menus.

This approach avoids the hassle of having to deal with the lifetime of the menu.

But this is rightfully labeled 'later'. First things first :-)

Bremmers avatar Jan 19 '22 15:01 Bremmers

yeah vst3 let you do that but it was painful (remember the host menu with a ref count, grab the actions, rebuild, release on trigger, etc...)

baconpaul avatar Jan 19 '22 18:01 baconpaul

The simplest form would be something like this:

typedef struct clap_host_contextmenu { uint32_t (*count)(const clap_host_t *host, clap_id param_id);

bool (*gettitle)(const clap_host_t *host, clap_id param_id, uint32_t index, char *display, uint32_t size);

void (*clicked)(const clap_host_t *host, clap_id param_id, uint32_t index); }

The plugin asks for the number of menu items for the param, gets the titles and adds them to its menu. It calls clicked if an item is clicked.

But it probably should support sub-menus too.

Bremmers avatar Jun 22 '22 15:06 Bremmers

sub menus menu titles ~~accelerator keys~~ checkmarks active/inactive state

all of those things at least

baconpaul avatar Jun 22 '22 15:06 baconpaul

maybe not accelerator keys

baconpaul avatar Jun 22 '22 15:06 baconpaul

Yeah, accelerator keys can conflict with the plugin itself.

Bremmers avatar Jun 22 '22 15:06 Bremmers

checkmarks and active/inactive state could be a field with flags

Bremmers avatar Jun 22 '22 15:06 Bremmers

This can do it I think. count() could optionally be removed (you can call getitem() until it returns false instead)

// the plugin calls count(.., .., CLAP_INVALID_ID) to get the toplevel menu item count // if count > 0 it fetches the items using getitem(.., .., CLAP_INVALID_ID, ..) // items with CLAP_MENU_FLAG_HASSUBMENU in flags have a submenu. The plugin calls count(.., .., menu_id) etc. // submenu items can have a submenu again etc.

// clicked() is called on clicking a menu item // clicked() isn't called for menu items that have a submenu

enum { CLAP_MENU_FLAG_HASSUBMENU = 1 << 0, CLAP_MENU_FLAG_DISABLED = 1 << 1, CLAP_MENU_FLAG_CHECKED = 1 << 2, }

typedef struct clap_menuitem { char name[CLAP_NAME_SIZE]; uint32_t flags;
clap_id menu_id; }

typedef struct clap_host_contextmenu { uint32_t (*count)(const clap_host_t *host, clap_id param_id, clap_id menu_id);

bool (*getitem)(const clap_host_t *host, clap_id param_id, clap_id menu_id, uint32_t index, clap_menuitem *menuitem);

void (*clicked)(const clap_host_t *host, clap_id param_id, clap_id menu_id); }

Bremmers avatar Jun 22 '22 19:06 Bremmers

Clicked would also need to have an uint32_t index, and I'd define a separate static const CLAP_CONSTEXPR clap_id CLAP_EXT_CONTEXT_MENU_ROOT = 0; for the root menu. Having an invalid menu be the top level menu is a bit counter intuitive.

robbert-vdh avatar Jun 22 '22 20:06 robbert-vdh

Clicked would also need to have an uint32_t index, Each menu item has it's own menu_id. 'index' can't be used because menus can be nested (a submenu in a submenu). Typically, a host that uses a simple one-dimensional menu will make menu_id equal to index for all items.

I'd define a separate static const CLAP_CONSTEXPR clap_id CLAP_EXT_CONTEXT_MENU_ROOT = 0; for the root menu. Having an invalid menu be the top level menu is a bit counter intuitive.

Agreed. 0 is probably not a good idea as the host would probably want to use that (see above)

Bremmers avatar Jun 22 '22 20:06 Bremmers

Can we use the word "deactivated" instead of "disabled"?

but yes a little struct you populate and a root id are exactly the thing i was thinking.

baconpaul avatar Jun 22 '22 21:06 baconpaul

// the plugin calls count(.., .., CLAP_EXT_CONTEXT_MENU_ROOT) to get the toplevel menu item count
// if count > 0 it fetches the items using getitem(.., .., CLAP_EXT_CONTEXT_MENU_ROOT, ..)

// items with CLAP_MENU_FLAG_HASSUBMENU in flags have a submenu. The plugin calls count(.., .., menu_id) etc.
// submenu items can have a submenu again etc.
// a plugin that can't display submenus shouldn't display the parent menu item (with CLAP_MENU_FLAG_HASSUBMENU in flags) either

static CLAP_CONSTEXPR const char CLAP_EXT_CONTEXT_MENU[] = "clap.context-menu.draft/0";

static const CLAP_CONSTEXPR clap_id CLAP_EXT_CONTEXT_MENU_ROOT = CLAP_INVALID_ID - 1;

enum {
  CLAP_MENU_FLAG_HASSUBMENU = 1 << 0,
  CLAP_MENU_FLAG_DEACTIVATED = 1 << 1,
  CLAP_MENU_FLAG_CHECKED = 1 << 2,
  CLAP_MENU_FLAG_ISSEPARATOR = 1 << 3,
}

typedef struct clap_menuitem {
  char name[CLAP_NAME_SIZE];
  uint32_t flags;
  clap_id menu_id;
}

typedef struct clap_host_context_menu {
  //get number of menu items
  uint32_t (*count)(const clap_host_t *host, clap_id param_id, clap_id menu_id);

  // get menu item description.
  // Each item has a unique menu_id which the plugin can pass to clicked()
  bool (*getitem)(const clap_host_t *host, clap_id param_id, clap_id menu_id, uint32_t index, clap_menuitem *menuitem);

  // called on clicking a menu item
  // clicked() isn't called for menu items that have CLAP_MENU_FLAG_HASSUBMENU, CLAP_MENU_FLAG_DEACTIVATED or CLAP_MENU_FLAG_ISSEPARATOR in flags
  void (*clicked)(const clap_host_t *host, clap_id param_id, clap_id menu_id);
}

Bremmers avatar Jun 23 '22 09:06 Bremmers

@Bremmers what about implementing this extension on a branch and making a pull request? ;-)

abique avatar Jun 23 '22 09:06 abique

I'm still hoping someone who knows how to do that turns this into proper-looking C and makes a PR :-)

Bremmers avatar Jun 23 '22 11:06 Bremmers

@Bremmers top GitHub tip. If you include code in triple back quotes followed by cpp you get formatted code

int main() { 
   return std::max(2,3); 
}
Screen Shot 2022-06-23 at 7 18 26 AM

baconpaul avatar Jun 23 '22 11:06 baconpaul

Anyway I really want this feature also. Happy to type up or review, but may not get to it today.

baconpaul avatar Jun 23 '22 11:06 baconpaul

I think the ability to pop up a host context menu for a parameter would also still be very useful. But my proposal there would be to limit that option to just popping up a context menu for a parameter, without the ability for a plugin to add its own context menu items. That would require complex callbacks and lifetime management, and it's what makes the VST3 implementation complicated.

So if a plugin wants to have both host context menu items as well as its own, it should just implement its own context menu and integrate the host's items there (so you still wouldn't need any lifetime management). But for plugins that just want to allow context menu integration for parameters with common options like resetting the parameter, creating an automation lane, doing DAW MIDI learn, linking parameters etc. then this would be really nice to have. The plugin would only need to make a single clap_host_context_menu::popup(clap_host, param_id) call to do that (that may or may not block until the menu returns, not sure what the best approach is there). Many VST3 plugins already use the context menus this way, and as a developer integrating this would be trivial since you don't need to keep track of any state.

robbert-vdh avatar Jun 23 '22 11:06 robbert-vdh

So as long as there is a data-only access which doesn't require doing the nonsense of getting a menu and stripping it apart to add it to your menu, I"m happy.

But that popup also needs a screen location right? Or should it always use current mouse location?

baconpaul avatar Jun 23 '22 11:06 baconpaul

Wouldn't that cause all kinds of UI framework problems, especially on Linux? Z order and what not.

Bremmers avatar Jun 23 '22 11:06 Bremmers

So as long as there is a data-only access which doesn't require doing the nonsense of getting a menu and stripping it apart to add it to your menu, I"m happy.

Yes, that's exactly what I'm suggesting. Just a simple "pop up a host context menu for a parameter". The same thing you'd get when right clicking on the parameter in the generic UI. If you have any FabFilter plugins installed, try right clicking on any of the parameters in the VST3 version and you'll see what I mean.

Regarding screen positions, I think just having the menu always pop up at the mouse location is the best option here. If you pass coordinates as an argument then those coordinates need to be translated from local window coordinates to screen coordinates which is something the host can do wrong. And HiDPI can also be handled incorrectly. In other situations context menus also always pop up at the mouse cursor, so just doing that seems like a good choice to me.

Wouldn't that cause all kinds of UI framework problems, especially on Linux? Z order and what not.

Why would it? The context menu would just be displayed on top of the plugin window/any other window. Quite a few VST3 plugins also use these context menus in the same way, and I have not yet seen any host/plugin compatibility issues here.

robbert-vdh avatar Jun 23 '22 11:06 robbert-vdh

Ah, that works with fabfilter indeed. Didn't know that.

So do you think we should just add a popup function to clap_host_context_menu? Or perhaps there should be two host interfaces, one for plugins that have their own menus, and one for plugins that don't.

Could there be a situation where the plugin UI has become invisible but the menu is still visible? The host would have to hide the menu in the gui.h 'hide' functions perhaps.

Bremmers avatar Jun 23 '22 11:06 Bremmers

Not quite sure. Maybe splitting it up is indeed a better idea, since a host may be able to provide some context menu items but it doesn't have its own way of popping up a context menu (and having some of these functions always return false would also be strange behavior).

Could there be a situation where the plugin UI has become invisible but the menu is still visible?

The host would be the one hiding the plugin's UI (and if the window was closed through the window manager, it would already handle the event for that). So I don't think that would cause any issues, the host would just need to close the context menu in those places. And if they don't, then it would probably only be a cosmetic problem with the host that doesn't affect any functionality.

robbert-vdh avatar Jun 23 '22 12:06 robbert-vdh

The thing I like about not splitting them is: it will stop daw developers from 'just' doing popup (or 'just' doing data).

To be a menu provider you need both.

I would be OK with popup returning a bool though in the event it couldn't popup obviously.

baconpaul avatar Jun 23 '22 12:06 baconpaul

but also: is popup sync or async? I presume async.

baconpaul avatar Jun 23 '22 12:06 baconpaul

To be a menu provider you need both.

But then how would you prevent the host from simply only implementing one of the two ways to use the interface and just returning false/0 for the other one? I could see that happening.

but also: is popup sync or async? I presume async.

I'm not sure yet. I'd also presume async would be fine, so I'm trying to think of situations where that could be problematic.

robbert-vdh avatar Jun 23 '22 12:06 robbert-vdh

I'm wondering if the 'mouse pointer position' is always available. It could be a longtap on a touchscreen, or a pen. In theory these devices wouldn't have to affect the mouse pointer position using the Windows API, in reality they probably will.

So perhaps the plugin should set the mouse pointer position if it uses other input devices.

Bremmers avatar Jun 23 '22 12:06 Bremmers

If the location is really not available (but for those long tap like things it should be), then the host can also just open the context menu in the center of the screen. Or they probably have some way to get the position from the touch API themselves. The potential problem with passing coordinates here is that you need to take HiDPI into account and you also need to translate window coordinates to global coordinates. So if you don't need to do these things, there are two fewer ways for hosts and plugins to make mistakes here

robbert-vdh avatar Jun 23 '22 12:06 robbert-vdh

static CLAP_CONSTEXPR const char CLAP_EXT_CONTEXT_MENU[] = "clap.context-menu.draft/0";


// clap_host_plugin_context_menu lets a plugin integrate host menu items in its own menus

// the plugin calls count(.., .., CLAP_EXT_CONTEXT_MENU_ROOT) to get the toplevel menu item count
// if count > 0 it fetches the items using getitem(.., .., CLAP_EXT_CONTEXT_MENU_ROOT, ..)

// items with CLAP_MENU_FLAG_HASSUBMENU in flags have a submenu. The plugin calls count(.., .., menu_id) etc.
// submenu items can have a submenu again etc.
// a plugin that can't display submenus shouldn't display the parent menu item (with CLAP_MENU_FLAG_HASSUBMENU in flags) either

static const CLAP_CONSTEXPR clap_id CLAP_EXT_CONTEXT_MENU_ROOT = CLAP_INVALID_ID - 1;

enum {
  CLAP_MENU_FLAG_HASSUBMENU = 1 << 0,
  CLAP_MENU_FLAG_DEACTIVATED = 1 << 1,
  CLAP_MENU_FLAG_CHECKED = 1 << 2,
  CLAP_MENU_FLAG_ISSEPARATOR = 1 << 3,
}

typedef struct clap_menuitem {
  char name[CLAP_NAME_SIZE];
  uint32_t flags;
  clap_id menu_id;
}

typedef struct clap_host_plugin_context_menu {
  //get number of menu items
  uint32_t (*count)(const clap_host_t *host, clap_id param_id, clap_id menu_id);

  // get menu item description.
  // Each item has a unique menu_id which the plugin can pass to clicked()
  bool (*getitem)(const clap_host_t *host, clap_id param_id, clap_id menu_id, uint32_t index, clap_menuitem *menuitem);

  // called on clicking a menu item
  // clicked() isn't called for menu items that have CLAP_MENU_FLAG_HASSUBMENU, CLAP_MENU_FLAG_DEACTIVATED or CLAP_MENU_FLAG_ISSEPARATOR in flags
  void (*clicked)(const clap_host_t *host, clap_id param_id, clap_id menu_id);
}

// clap_host_host_context_menu.popup can be called by plugins that don't have their own menus
// the menu should appear at the current mouse pointer position
typedef struct clap_host_host_context_menu {
  void (*popup)(const clap_host_t *host, clap_id param_id);
}

Bremmers avatar Jun 23 '22 12:06 Bremmers

Yes, something like that. With the clap_host_plugin_context_menu and clap_host_host_context_menu extensions having their own extension IDs, of course.

robbert-vdh avatar Jun 23 '22 12:06 robbert-vdh

Hmm, this would be the first extension with two IDs I think.... perhaps it should even be two extensions?

Perhaps it should be a single interface after all.

Bremmers avatar Jun 23 '22 12:06 Bremmers