vo: add win32 context menu support

[tsl0922]

This is a draft implementation of #13677 for win32, from mpv-menu-plugin .

I’m not sure if the code should live here, correct me if I’m wrong.

test.lua

local items = {
    {title="foo", cmd="show-text 'foo'", shortcut="Ctrl+A"},
    {type="separator"},
    {title="hello"},
    {type="separator"},
    {title="dir", type="submenu", submenu={
        {title="foo", cmd="#hello"},
        {title="bar", cmd="show-text 'bar'"},
    }},
    {title="select", type="submenu", submenu={
        {title="item 1", state={"checked"}, cmd="show-text 'item 1'", shortcut="Ctrl+1"},
        {type="separator"},
        {title="item 2", cmd="show-text 'item 2'", shortcut="Ctrl+2"},
        {title="item 3", cmd="show-text 'item 3'"},
        {type="separator"},
        {title="item 4", state={"disabled"}, cmd="show-text 'item 4'"},
        {title="item 5", state={"checked", "disabled"}, cmd="show-text 'item 5'"},
    }},
}

mp.add_key_binding('MBTN_RIGHT', nil, function()
    mp.commandv('context-menu')
end)

-- Important: update menu-data when vo is active
mp.observe_property('current-vo', 'native', function(name, val)
    if val then
        mp.set_property_native("menu-data", items)
    end
end)

TODOs / Questions:

• there maybe a chance that the vo is not created when setting menu-data, we should query menu-data and update menu on vo creation too (added document for it)
• foo\tCtrl+A is a windows syntax, it will show Ctrl+A as shortcut in the right, do you think we should add a new field for shortcut, or just use the \t syntax for all platforms? (added shortcut field)
• do we need multiple menu definitions support? then you can choose to show specific one (or reuse it in other places like main application menu on macOS, @Akemi). (no)

[Akemi]

• foo\tCtrl+A is a windows syntax, it will show Ctrl+A as shortcut in the right, do you think we should add a new field for shortcut, or just use the \t syntax for all platforms?

on the macOS side we already have something 'equivalent'. it's just the normal menu instead of a context menu, though the general structure for a context menu is the same.

on macOS the 'title' and 'shortcut' need to be separated, see https://github.com/mpv-player/mpv/blob/master/osdep/mac/menu_bar.swift#L271.

without looking closely at those changes. i think we had some discussions before. like

• how are shortcuts handled, are they actual shortcuts or is it just 'functionless' information that don't need to represent the actual configured shortcuts
• do we need an api to query shortcuts by function
• which shortcuts take precedence if those shortcuts are actual functional

[tsl0922]

how are shortcuts handled, are they actual shortcuts or is it just 'functionless' information that don't need to represent the actual configured shortcuts

It's functionless in menu, but working in mpv-menu-plugin because it comes from input.conf's key field. It's possible to make it work in win32 menu too (Keyboard Accelerators).

which shortcuts take precedence if those shortcuts are actual functional

Maybe an extra field can be added to menu item definition to determine whether it should override mpv key binding.

[tsl0922]

on macOS the 'title' and 'shortcut' need to be separated, see https://github.com/mpv-player/mpv/blob/master/osdep/mac/menu_bar.swift#L271.

added a shortcut field, please read DOCS/man/input.rst for more detail.

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32

macOS

• mpv-macos-14-arm
• mpv-macos-13-intel
• mpv-macos-12-intel

[Akemi]

• do we need multiple menu definitions support? then you can choose to show specific one (or reuse it in other places like main application menu on macOS, @Akemi).

i am not sure about this. for one the current definition is imo not flexible enough for a main menu, especially on different platforms. like on macOS you have some convention one should adhere to, but on other platforms those are useless or contrary (like a special touch bar menu or the help menu). anyway i thinks it is fine as a context menu.

Maybe an extra field can be added to menu item definition to determine whether it should override mpv key binding.

just as an info, the shortcuts on macOS take precedence over the ones defined in input.conf. though one can remove those shortcuts, or add new ones, with normal system settings.

[ahaoboy]

The display of the context menu depends on the implementation of the UI library, it not only needs to be able to display the menu, but also need to be able to set different styles (dark mode, light mode), fonts, internationalization, as well as the use of plugins to dynamically modify it, perhaps we should define the data format of the context menu for the MPV (similar to the playlist), the specific display should be determined by the UI library

[hooke007]

The display of the context menu depends on the implementation of the UI library, it not only needs to be able to display the menu, but also need to be able to set different styles (dark mode, light mode), fonts, internationalization, as well as the use of plugins to dynamically modify it, perhaps we should define the data format of the context menu for the MPV (similar to the playlist), the specific display should be determined by the UI library

Let's focus on one objective at the time. There is working win32 backend already implemented, it can be improved / changed, but this issue is about the mpv integration, not WinUI.

Think about those things later. Not necessary for this PR.

[kasper93]

there maybe a chance that the vo is not created when setting menu-data, we should query menu-data and update menu on vo creation too

Shouldn't menu-data be cached in common code and send to vo on changes? Probably should just get menu-data property on vo init if it is already set in mdata.

[tsl0922]

there maybe a chance that the vo is not created when setting menu-data, we should query menu-data and update menu on vo creation too

Shouldn't menu-data be cached in common code and send to vo on changes?

It's already cached and watched (the code only work on none null vo), but a script may set menu-data before vo creation, it's the purpose of using mp.register_idle in the test script (to ensure it is run after vo creation on mpv start).

It seems there's no good way to read a property in vo, this is the reason why it is not done in the PR.

As I see currently it relies on specific VO to keep the menu state, but what if someone changes VO in runtime? I think this needs improvements.

I’m not sure if the code should live here (said in the PR body), do you have any suggestions?

It should work on vos that using w32_common, other vos like sdl will need new patches. I don't think it is an issue, because the code in menu.c can be reused in vos.

[kasper93]

-- there maybe a chance the vo is not created at this time
mp.register_idle(function()
    mp.set_property_native("menu-data", items)
end)

You can probably observe current-vo to know when vo is really there.

I’m not sure if the code should live here (said in the PR body), do you have any suggestions?

Not sure I understand what exactly you refer to. But if you say about mdata, it is ok, we need to have to somewhere and command context is good as any.

What I really mean is that on vo create or init, somewhere it should pull mdata state and mp_win32_update_menu on it's own using already remembered menu state. It shouldn't be on lua script to track if VO valid menu status.

[tsl0922]

What I really mean is that on vo create or init, somewhere it should pull mdata state and mp_win32_update_menu on it's own using already remembered menu state. It shouldn't be on lua script to track if VO valid menu status.

This is what I mean too. If we do it in vo, the mp.register_idle trick will become unnecessary in the test scipt.

It seems there's no good way to read a property in vo, this is the reason why it is not done in the PR.
Is it OK to call mp_property_do with vo->global->client_api->mpctx in w32_common.c (or mp_win32_menu_init)?

[kasper93]

It seems there's no good way to read a property in vo, this is the reason why it is not done in the PR. Is it OK to call mp_property_do with vo->global->client_api->mpctx in w32_common.c (or mp_win32_menu_init)?

Indeed, properties are handled fully in command.c and in case of "set" updates appropriate state directly. Unlike options they are not cached. But I think it is not terrible to call mp_property_do to get it.

In most cases there are more direct way to get data, but here we store menu state as mpv_node. Alternative would be to have implementation agnostic menu representation structure in C instead of keeping it in mpv_node format. But this is just converting things around without much gain.

[tsl0922]

Is it OK to call mp_property_do with vo->global->client_api->mpctx in w32_common.c (or mp_win32_menu_init)?

But I think it is not terrible to call mp_property_do to get it.

Well, sorry I found it's not possible to get struct MPContext via vo->global->client_api->mpctx (required to call mp_property_do). Do you have any suggestions, or may I add struct MPContext to struct mpv_global?

[na-na-hi]

What I really mean is that on vo create or init, somewhere it should pull mdata state and mp_win32_update_menu on it's own using already remembered menu state. It shouldn't be on lua script to track if VO valid menu status.

This can already be done with video-reconfig event. I think we shouldn't make a potential architectural change when something can be done by the script.

[kasper93]

This can already be done with video-reconfig event. I think we shouldn't make a potential architectural change when something can be done by the script.

The problem is that it requires the script to be aware of implementation details and when the menu data has to be restored.

[na-na-hi]

The problem is that it requires the script to be aware of implementation details and when the menu data has to be restored.

Just document that the scripts need to restore the value after a VO reconfig. They already need to do the same for similar stuffs like ao-volume when switching AOs..

[kasper93]

Just document that the scripts need to restore the value after a VO reconfig. They already need to do the same for similar stuffs like ao-volume when switching AOs..

Ok.

[kasper93]

Generally looks good. Clean enough to not be a burden. Although I have concerns about extensibility of this the property approach, because it works if we think from script point of view. But what if in future we decide to include menu definition in input.conf? It would probably be bad idea for input.c to create property. So likely would need another internal representation.

But anyway, it is small enough that we can completely refactor it later if needed and script facing property will not change.

[tsl0922]

But what if in future we decide to include menu definition in input.conf negatively? It would probably be bad idea for input.c to create property. So likely would need another internal representation.

The dyn_menu.lua in mpv-menu-plugin is what you want (need to add support for this PR), although implementing it in mpv core may be better, but it's tricky to implement support for dynamic lists in C.

[na-na-hi]

Just some documentation language style changes.

[tsl0922]

ping @kasper93, is there anything else that blocking this PR to be merged? please let me know.

[kasper93]

We can go with it, thanks.