Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Context Menu API #13677

Closed
tsl0922 opened this issue Mar 10, 2024 · 14 comments
Closed

RFC: Context Menu API #13677

tsl0922 opened this issue Mar 10, 2024 · 14 comments

Comments

@tsl0922
Copy link
Contributor

tsl0922 commented Mar 10, 2024

Expected behavior of the wanted feature

I'd like to upstream the C plugin part of mpv-menu-plugin to mpv.

See also: #5500 (comment)

Things need to be do in mpv:

  • Add a new property or command to update the menu
  • Add a new command to show the context menu

The reference implementation for win32 can be found in menu.c.

Benefits

  • Less core menu logic in C, support for macOS/Linux can be added later
  • menu conf syntax can be implemented with scripts

dyn_menu.lua is a lua script with support for reading menu from input.conf, and support dynamic menu items like tracks/chapters/...

Additional information

menu data property structure used by mpv-menu-plugin:

MPV_FORMAT_NODE_ARRAY
  MPV_FORMAT_NODE_MAP (menu item)
     "type"           MPV_FORMAT_STRING     (supported type: separator, submenu)
     "title"          MPV_FORMAT_STRING     (required if type is not separator)
     "cmd"            MPV_FORMAT_STRING     (optional)
     "state"          MPV_FORMAT_NODE_ARRAY[MPV_FORMAT_STRING] (supported state: checked, disabled, hidden)
     "submenu"        MPV_FORMAT_NODE_ARRAY[menu item]         (required if type is submenu)

If this RFC is not accepted, I would request #13163 to be resolved (to make mpv-menu-plugin possible on macOS).

@sfan5
Copy link
Member

sfan5 commented Mar 10, 2024

support for macOS/Linux can be added later

Which GUI toolkit do you want mpv to import to implement the context menu?
The question is pointless however because mpv has always avoided GUI toolkits (see e.g. Wayland CSD) and I don't foresee that changing.

Effectively such a feature would need a libass-based fallback anyway. That would also fix the concern of adding a feature (with high visiblity) that only works one on or two platforms.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Mar 10, 2024

Which GUI toolkit do you want mpv to import to implement the context menu?

I prefer the native API provided by each platform (for native UI experience), for example win32 Menus is used in mpv-menu-plugin.

Effectively such a feature would need a libass-based fallback anyway

It's possible to add a uosc like implementation with lua script.

@kasper93
Copy link
Contributor

Effectively such a feature would need a libass-based fallback anyway. That would also fix the concern of adding a feature (with high visiblity) that only works one on or two platforms.

For two biggest platforms (macOS and Windows) it can be done using native UI elements. For other platforms like linux, libass fallback can be used or none at all.

@Andarwinux
Copy link
Contributor

win32 menus fonts rendering very badly on 2K/4K monitors, whereas libass has no such problems. WinUI should be a better solution on Windows.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Mar 10, 2024

win32 menus fonts rendering very badly on 2K/4K monitors, whereas libass has no such problems. WinUI should be a better solution on Windows.

Maybe. Win32 menu does scale on 2k/4k, but the default font size and item padding looks bad when the player is in full screen mode, and you can't change it unless creating owner-draw menu, this will introduce complexity.

@na-na-hi
Copy link
Contributor

Some of my thoughts on this:

  • There needs to be another property for querying whether native context menu is supported. The script must query this value, and draw the menu on the script side if native menu isn't supported. This also means that the new properties and commands are only useful for native menus.
  • Making this feature high visibility necessarily requires changing the default effect of MBTN_RIGHT. Lots of users might dislike this, considering that the existing behavior has been here for a long time and play/pause is a much more frequently used feature. Also MBTN_LEFT can't be used until Moving window and cycle pause on MBTN_LEFT #7563 is resolved.
  • As with the current licensing status quo, any code you want to upstream needs to be relicensed to LGPL.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Mar 10, 2024

As with the current licensing status quo, any code you want to upstream needs to be relicensed to LGPL.

I'm OK with it, or we may write new code to implement it with WinUI on Windows (if desired).

@kasper93
Copy link
Contributor

kasper93 commented Mar 10, 2024

There needs to be another property for querying whether native context menu is supported.

Ok, we need to detect whether native menu is working or should we fallback to libass one. In other words detect which menu backend to use.

The script must query this value, and draw the menu on the script side if native menu isn't supported. This also means that the new properties and commands are only useful for native menus.

I don't understand this part? We can use exactly the same interface for libass fallback menu. Backend of handling menu shouldn't change the way it works.

I'm OK with it, or we may write new code to implement it with WinUI on Windows (if desired).

I don't think using WinUI is feasible. Also I don't think there will be much gain, but in the same time feel free to prove me wrong.

@na-na-hi
Copy link
Contributor

na-na-hi commented Mar 10, 2024 via email

@Andarwinux
Copy link
Contributor

I don't think using WinUI is feasible. Also I don't think there will be much gain, but in the same time feel free to prove me wrong.

win32 programs using WinUI3 sounds difficult, but not impossible. MinGW GCC/Clang toolchain already has the most basic WinRT support.
microsoft/microsoft-ui-xaml#8321
mstorsjo/llvm-mingw#307
https://github.com/driver1998/WinUI-MinGW

@kasper93
Copy link
Contributor

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.

@Dudemanguy
Copy link
Member

This plugin uses the window-id property right? How important is that exactly because that will never work on wayland.

@tsl0922
Copy link
Contributor Author

tsl0922 commented Mar 13, 2024

This plugin uses the window-id property right? How important is that exactly because that will never work on wayland.

The window handle is used to check whether the mouse is clicked on window, it's not required to context menu.

In fact, a C plugin always run in it's own thread, this make it complicated to implement GUI code, because GUI code typically need to run in the GUI thread (may or may not required, ex: main thread is required on macOS). This is a reason why I want to merge the code to mpv core, it's easier because we can write code in the GUI thread directly.

@kasper93
Copy link
Contributor

It has been merged as #13700

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants