-
Notifications
You must be signed in to change notification settings - Fork 3.2k
context_menu.lua: add this script #16726
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
Conversation
|
Could you separate implementation of context menu to separate PR from the definition of default menu. Also there are unrelated changes sprinkled in input.conf for example, which should also be put in another PR or at least another commit. To make it easier to review and discuss. |
|
Are you sure? I did not separate them because it makes it much easier to test the context menu with real-world menu entries, else you have to write a script to populate it yourself, handling all possible cases like disabled items, checkboxes, hidden items, nested submenus, disabled submenus, which is what I had to do before updating select.lua. input.conf changes are just cosmetic and don't make sense on their own. As written in the commit message they are meant to have the exact same commands in menu.conf which shows the key in the menu. But I guess I can add another commit. |
Yes. Unless you want this never to be merged, bundling controversial changes, like default menu definition and right click changes will bikesheed this PR to death. We already have working context menu on windows and various scripts to populate it. Implementing context menu in ass is orthogonal from any default menu definition or input changes.
Bundling unrelated changes only makes PR harder to review and approve. I don't want to police git usage, but if you drop |
50a078d to
db4aa80
Compare
|
Download the artifacts for this pull request: Windows |
| ``--load-context-menu=<yes|no>`` | ||
| Enable the builtin script that implements a context menu. Defaults to | ||
| ``yes`` on platforms where integration with a native context menu is not | ||
| implemented, and to ``no`` on platform where it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be auto instead? I am not sure if some would prefer the OSD menu than GUI menu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It already behaves like auto by changing default value based on the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the explicit value auto. Is there any reason to make the option special?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the default value can be determined at compile time unlike every other auto option, removing the need to implement a context-menu-loaded property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it wouldn't be conflict with GUI menu. I think I will always set it to yes to use two menus together in different cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what you mean by GUI menu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean Windows users cannot use the ASS menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--load-context-menu defaults to no on Windows so that select.lua will prefer opening the native context menu, but nothing stops you from ignoring this option and opening either menu in a third party script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the only advantage of the native context menu I see is that it can be drawn outside of mpv's window. Whereas the advantage of the ASS menu is that you can customize its appearance.
|
| .lua_load_select = true, | ||
| .lua_load_positioning = true, | ||
| .lua_load_commands = true, | ||
| #ifndef _WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks iffy. What about macOS, or other possible implementations of context menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is iffy? When the macOS context menu is implemented this needs to be updated to #if !defined(_WIN32) && !defined(HAVE_COCOA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the macOS context menu is implemented
macOS has its own context menu implemented already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementing the macOS context menu is literally listed as a goal in #13608. Only the top bar menu is implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the goal of disabling context menu script anyway? It's not bound to any key, so there is no conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just reduces the number of threads running.
I thought the native Windows context menu is not scrollable (I asked whether it is on IRC 4 months ago but no one replied) hence why https://github.com/tsl0922/mpv-menu-plugin implements
28 items fit for me with window scaling ¯\(ツ)/¯. I just picked default sizes that look good to me, I'm sure everyone will have different preferences. I will update them to whatever you prefer.
I made 5 pixels overlap but it looks cluttered to me. GTK/Qt/tk menus don't do this.
They are more distant in real-world menus because the shortcuts are right aligned and you will have longer texts than "Play". The GTK menu has similar distance from the longest entry. Still I increased the minimum spaces from 2 to 4.
All native context menus activate items on right click?
Made it start at
I assume you are referring to open submenus. They stay open until
It is DPI scaled with |
Agree. The behavior of most applications is as follows: |
Context menu is scrollable. Especially useful when navigating with mouse. However that size of system context menu is generally not so big, so the capacity before you need to scroll is way bigger. To reproduce the size of mpv's menu I needed to set 350% scaling.
I would match system menu sizes, which is also why I suggested to DPI scale it. Unlike OSC, context menu feels like something separate.
Something to discuss. We could not do that, I think at least on Windows there is small drop shadow that makes it look stacked one on top of another.
Should this be 1 or 2 tabs, instead of spaces?
They don't on Windows. I think it's not a big deal, just something that don't have much use probably.
I would put it just ad top left (0, 0) to maximize available space
No, I meant that moving mouse over items first highlight new item and after delay unhighlight previous item. I can't repro it now,
See my size response. |
Why in hell does mpv-menu-plugin cut playlist items then?
Do you want do remove window scaling then? It was disabled in console because scaling both with the window and DPI made text too big.
This has a shadow too if you set
Seems to be the oddball. Anway I don't know about pressing down right click but activating when releasing right click is quite useful to activate the first item in 1 click, e.g. to go back in browser.
I think it looks better with more centering but done.
I've never seen that other than with the open submenus I just modified. |
It's probably still nicer to limit the size, instead of filling whole screen with huge list.
Not sure what's best, if we want to mimic system context menu like or more something inside of mpv. I would disable window scaling and enable dpi scaling instead and see where it gets us. |
|
The problem is that we can't emulate the system menu that well because it can't render outside of the window. So the context menu in a small window will barely fit any items without scaling with the window, whereas the system one can just render outside of mpv's window at full size. |
As discussed on IRC, it can be scaled with window as is. |
|
Implemented scrolling only of 1 page at a time as you suggested. |
6eafd98 to
e64cdbc
Compare
This implements a ASS context menu to be used on platforms other than Windows. The select script message will allow selecting an item with a single click when releasing a mouse button, like in native context menus. This is mainly useful to cycle pause with one click.
Just wanted to point it out that menu entries do activate with right-mouse click on my current Windows10 box as it does on my windows 8.1 one, and yes I'm talking native Windows context-menus, like right-clicking the taskbar and right-clicking any option. Pretty sure that is the case for the longest time. |
It does not it current mpv context menu. |
Ah ok, thought you're talking about Windows menus in general. |
kasper93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine, we can iterate on this, thanks.

This implements a ASS context menu to be used on platforms other than Windows.
The select script message will allow selecting an item with a single click when releasing a mouse button, like in native context menus. This is mainly useful to cycle pause with one click.