free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.76k stars 97 forks source link

Context menu #221

Closed abique closed 1 year ago

nakst commented 1 year ago

I propose that the label parameter on the add_entry and add_checkbox functions should have a special behaviour when it contains the character &.

If the GUI library the menu is to be displayed in has some sort of access key functionality (see https://learn.microsoft.com/en-us/windows/win32/uxguide/inter-keyboard#menu-access-keys), then the letter (only uppercase and lowercase Latin A-Z are allowed) after the & should be the access key, and & should be hidden.

If the GUI library does not have access key functionality, the & should be ignored and hidden.

mkruselj commented 1 year ago

What if the menu entry actually wants to show the & character? On Windows, you'd have to write && for that, but I don't think it's a good idea to enforce this sort of characeter escaping to non-Windows-first developers. So maybe a better idea is to provide a second argument that would be unsigned int, which would point to which character in the menu label to underline.

Trinitou commented 1 year ago

As this is similar to #222 as @robbert-vdh mentioned (thank you!) -> Is this context menu extension about context menus for inidividual parameters or also for the whole plugin instance? Maybe it might be helpful to clarify that.

robbert-vdh commented 1 year ago

Yes, it's for parameters. You can think of this extension as a streamlined version of VST3's context menu interface.

Trinitou commented 1 year ago

Yes, it's for parameters. You can think of this extension as a streamlined version of VST3's context menu interface.

Maybe then it should be named accordingly. Something like "param-context-menu" ?

abique commented 1 year ago

Yes, it's for parameters. You can think of this extension as a streamlined version of VST3's context menu interface.

Maybe then it should be named accordingly. Something like "param-context-menu" ?

You can have entries unrelated to parameter if you pass CLAP_INVALID_ID.

robbert-vdh commented 1 year ago

I propose that the label parameter on the add_entry and add_checkbox functions should have a special behaviour when it contains the character &.

If the GUI library the menu is to be displayed in has some sort of access key functionality (see https://learn.microsoft.com/en-us/windows/win32/uxguide/inter-keyboard#menu-access-keys), then the letter (only uppercase and lowercase Latin A-Z are allowed) after the & should be the access key, and & should be hidden.

If the GUI library does not have access key functionality, the & should be ignored and hidden.

I had already proposed this elsewhere, but my preference would be to move one letter accelerators to a second argument that would take a single character case insensitive string. If the host doesn't support accelerators, then it wouldn't need to do anything except for maybe escaping the string to match their GUI toolkit's context menu label format (which they'd need to do anyways). If the host does support accelerators, it would use that letter as an accelerator (and perhaps underline the first occurrence of that letter if that's how they indicate accelerators). That way the label string doesn't contain magic symbols, doesn't need to be parsed for those magic symbols (to either remove or replace them), and isn't tied to the format expected by a single GUI toolkit (Win32 controls in the case of &).

abique commented 1 year ago

Hi, I've improved this extension to have a concept of target, so for now it is either global or param, but we could think of having different targets in the future. I've also added a accelerator description, which allows for key combo too (up to four).

abique commented 1 year ago

The description will be improve but first let's see if the design is clear and if we like it.

robbert-vdh commented 1 year ago

The current version supports both modifier keys and also entire key chords for context menu accelerators. Do we want this? Is there any mainstream GUI toolkit that supports this? Or is this functionally intended as part of extending context menus into a full generic action API? The way it's currently set up makes the extension quite a bit more complex than the initial draft was.

abique commented 1 year ago

I think vscode has it so I though key combo was a common thing now. If one doesn't have accelerator, it is possible to pass NULL.

Yes it is a bit more extended than the original design, but tomorrow we may have triggers or other objects for which we could spawn a popup menu, so having the target makes the menu much more future proof.

nakst commented 1 year ago

As I said in my previous comment, I think there should be a way to specify the access key for a menu item. As robbert-vdh has argued, this is probably best done by adding an argument char access_key to add_entry and add_checkbox.

Also, to address robbert-vdh's concerns about inclusion of keyboard accelerators, I think the CLAP documentation should make it clear that the accelerator parameter in this API does not create any form of binding, but only exists as a decoration for the menu item.

robbert-vdh commented 1 year ago

I think vscode has it so I though key combo was a common thing now. If one doesn't have accelerator, it is possible to pass NULL.

Just opened VsCode, the keybindings it shows there are global keybindings that can always be used, even when the context menu is not open. So it's just a reminder of how to access that action with a keyboard shortcut. In fact, VsCode doesn't seem to use single letter accelerator keys in their context menus at all.

You also can't use those shortcuts while the context menu is open. They really are just reminders of the global key bindings. So my question still is, are these supposed to be global shortcuts that are always active even when no context menu is visible? If so, how do you handle clashes with the DAWs own shortcuts?

abique commented 1 year ago

I think vscode has it so I though key combo was a common thing now. If one doesn't have accelerator, it is possible to pass NULL.

Just opened VsCode, the keybindings it shows there are global keybindings that can always be used, even when the context menu is not open. So it's just a reminder of how to access that action with a keyboard shortcut. In fact, VsCode doesn't seem to use single letter accelerator keys in their context menus at all.

You also can't use those shortcuts while the context menu is open. They really are just reminders of the global key bindings. So my question still is, are these supposed to be global shortcuts that are always active even when no context menu is visible? If so, how do you handle clashes with the DAWs own shortcuts?

Usually they are just reminders, so you can learn the shortcuts while reading the menu.

abique commented 1 year ago

I think the CLAP documentation should make it clear that the accelerator parameter in this API does not create any form of binding, but only exists as a decoration for the menu item.

Yes

robbert-vdh commented 1 year ago

Usually they are just reminders, so you can learn the shortcuts while reading the menu.

Exactly. They're super useful for that, but because they're reminders of global hotkeys there's also no reason for them to be in this extension. Unless this extension is also supposed to register global hotkeys on the host.

abique commented 1 year ago

Usually they are just reminders, so you can learn the shortcuts while reading the menu.

Exactly. They're super useful for that, but because they're reminders of global hotkeys there's also no reason for them to be in this extension. Unless this extension is also supposed to register global hotkeys on the host.

I agree, they are not essential.

baconpaul commented 1 year ago

Sorry to have missed most of this conversation

The build construct is super clever. I really like it. Easy to implement also.

The condition in which the host calls the plugin with a builder (as opposed to vice versa) is so the plugin can add items to the hosts plugin menu? That's awesome!! I will definitely use that.

the accelerator stuff is tricky I agree but looks like a good implementation here. I could definitely implement the surge features we have if this api was in place.

nakst commented 1 year ago

Sorry if this has been addressed, but how do you specify menu access keys with this proposed API? That is, single key keyboard shortcuts for each menu item scoped only while the menu is open, and typically shown by underlining the first corresponding letter of the item label.

robbert-vdh commented 1 year ago

Yeah I strongly feel like the complex keyboard shortcuts in the current version of this PR should be replaced by a simple single character accelerator string. Complex keyboard shortcuts like this cannot be used when the context menu is open (so they'd be dead on arrival), and in other applications that do list keyboard shortcuts in context menus they're reminders of global shortcuts, which aren't a thing in this case.

Bremmers commented 1 year ago

Well, having global shortcuts is not entirely impossible. Parameter menus could have an 'Set all params to default' item for example. And there can be global menus that aren't tied to a param, plugins could use these for anything.

I agree the Windows '&' accelerators should be supported too.

Bremmers commented 1 year ago

On second thought I think we don't need to do anything for the Windows '&' accelerators: the side that shows the menu can just assign a suitable character itself. This avoids convoluting the API, and there can't be conflicts between the host and plugin menu item hotkeys. And it's easy to do. Delphi menus, for example, can even assign the hotkeys automatically.

abique commented 1 year ago

Thank you everyone for your review, I'll merge this one in next and we can continue discussing it on the next to main review. I'll improve the documentation soon.