-
Notifications
You must be signed in to change notification settings - Fork 3.1k
select.lua: populate the context menu #16816
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
base: master
Are you sure you want to change the base?
Conversation
Download the artifacts for this pull request: Windows |
submenu with the relative items: ``$playlist``, ``$video-tracks``, | ||
``$audio-tracks``, ``$sub-tracks``, ``$secondary-sub-tracks``, ``$chapters``, | ||
``$editions``, ``$audio-devices``. These menus are automatically disabled when | ||
empty. |
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.
Those special things, like tracks are not working, they are not populated.
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.
Are you using mpv-menu-plugin
? I changed @
to $
so they look more like variables though it is not important
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.
Are you using mpv-menu-plugin?
Do I need to install it?
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.
No I was assuming you were using a plugin with a slightly different syntax. I don't know why else they wouldn't work for you as they work just fine with --no-config
for me.
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 don't know either. I only see
[select] Menu entry "50%" error on evaluating: @select.lua:818: attempt to index local 'name' (a number value)
in log.
How can I help debug this?
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.
That was an unrelated error in the window scale conditions. Fixed that. Still no idea about the builtin submenus.
3f1acb1
to
f809c83
Compare
player/lua/select.lua
Outdated
} | ||
end | ||
|
||
mp.add_key_binding(nil, "context-menu", function (info) |
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 this? The context-menu is invoked through select.lua
? All select.lua
has to do is to parse menu from textual definition and set menu-data
. It should be done once and not on every context menu open.
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.
That is what select.lua does?
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.
Why doesn't it set context menu by default? Who is responsible for calling this command? Why it's key binding?
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 you are referring to observing for property changes to update menu-data
, currently it is not done because that seemed overkill for the initial implementation. It just calculates the menu everytime you right click to activate this binding. But it's not like it's slow.
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.
My mpv -v
is disagreeing in doing it on every click, it completely obliterates the logs. Also depending on implementation, you don't have to have binding to open menu, which is open by OS if set correctly.
I see that observing everything that influence menu may be tricky, but updating menu on every open even if nothing changed is also not good.
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 thought this was already too big without having observers in the initial implementation, but sure I can implement it later.
We can have to evaluated on some triggers, one of which can be opening menu. Though, probably would be good to have some caching to avoid spamming if nothing changes.
Well how is it implemented in mpv? [...] And obviously we need a binding at least for the ASS one.
Yes, we need keybinding for opening ASS menu, or any other if needed. But we should separate opening menu and updating menu. Because they are different topic. And while opening can try to update. I don't see it has to be so tightly connected. That it's proxied thorough select.lua
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 don't see how to update menu-data
outside of the keybinding without bringing back #15264
I can reproduce that even with mpv-menu-plugin
, because it observes playlist
.
So it's pick your poison between opening the menu slightly slower and observing 40 properties and updating menu-data
when any changes.
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.
shortcut is taken from input-bindings
property, and this property doesn't support change notification, even if observing property changes to update the menu-data
, it is still necessary to update the shortcut information of menu-data
every time the context menu pops up.
mpv-menu-plugin
's shortcut is taken from input.conf, so it doesn't need to consider this matter.
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 don't see how to update
menu-data
outside of the keybinding without bringing back #15264
You can define menu update event and trigger it whenever needed. It doesn't have to observe each property, only need to update menu when appropriate.
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.
Well it would have to update the playlist menu when changing playlist entry
e501456
to
681a185
Compare
Haven't looked at the code yet, but works fine on Windows too. |
681a185
to
b2b82d9
Compare
The formatting of input.conf commands is totally inconsistent. At least for the commands that will be used in the context menu, make it consistent. The next commit will define the context menu commands, and the keys bound to these commands will be shown in the menu only when they match exactly, so this avoids copy pasting commands with inconsistent formatting just to make them match.
b2b82d9
to
0001780
Compare
I implemented the observers. On the first time you open the context menu it parses Since For #15264 I hacked it as such: if |
476b57f
to
164b26c
Compare
if i == current_edition + 1 then | ||
items[i].state = {"checked"} | ||
end | ||
end |
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.
script-binding select/select-edition
command shows "No available editions." when there's only 1 edition.
In the case of only 1 edition, I think the behavior of context menu and selection menu should be consistent, with both showing or neither showing the edition list.
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 does disable it when there's only 1 edition.
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 does disable it when there's only 1 edition.
that's make sense, I didn't notice it.
but why disable $playlist when there's only 1 playlist entry, it's inconsistent with the behavior of selection menu,
clicking on the currently playing entry can be used to reload the current file, which is not a meaningless action.
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 enabled it for 1 playlist entry.
2b63799
to
616159d
Compare
616159d
to
7d20140
Compare
Add a default context menu definition in etc/menu.conf. It will be used to populate the context menu. The format is meant to be easily readable with just tabs as separators and submenus denoted by indentation. It avoids complicating the builtin input.conf like similar scripts do. By not defining the shortcut keys in the builtin input.conf, menu entries won't be associated to wrong shortcuts when the user's input.conf changes key bindings without redefining the menu. Note that external scripts don't have this issue. I used 3+ spaces as separators at first but then realized that since we have Windows users tabs will look better in editors with proportional fonts like notepad, though it still won't be perfectly aligned depending on the font. But I am still not sure what is better. On a curious note, etc/menu.conf already existed in the past as mplayer used to have a menu. git log -p etc/menu.conf will show commits from 2002 to 2010.
This property contains the builtin etc/menu.conf, and will be used by select.lua. It is undocumented because it is for internal usage.
93edd66
to
2d2844d
Compare
Make select.lua parse menu.conf, fill menu-data and open the context menu. This is done from select.lua to reuse the code to format the data and retrieve key bindings. The first time the context menu is opened, menu-data is filled initially, and the referenced properties are observed so that menu-data is already updated by the next time you open the context menu. So if you never use the context menu there is no extra overhead. While the context menu is scrollable, for the playlist it is better to start from around the current entry rather than from the beginning, and since menu-data has no way to specify where to start, when there are more than 25 items playlist items this adds … entries that open the scrollable console menu when clicked. input-bindings is parsed to show the shortcuts bound to commands. Since it is not observable, it just uses the bindings from the first time the context menu is opened. console and stats key bindings are skipped in case the context menu is opened together with those scripts. Multimedia and numpad keys are not shown to reduce clutter. The old Context Menu section of the docs is merged in the more detailed context_menu.rst to not repeat the same information.
Rebind right click and MENU to show the context menu. Shift+F10 is also bound because it is a standard context menu binding. MENU is not added to restore-old-bindings.conf because it is uncommon.
2d2844d
to
e8dabce
Compare
input.conf: format some commands more consistently
The formatting of input.conf commands is totally inconsistent. At least for the commands that will be used in the context menu, make it consistent. The next commit will define the context menu commands, and the keys bound to these commands will be shown in the menu only when they match exactly, so this avoids copy pasting commands with inconsistent formatting just to make them match.
menu.conf: add this file
Add a default context menu definition in etc/menu.conf. It will be used to populate the context menu.
The format is meant to be easily readable with just tabs as separators and submenus denoted by indentation. It avoids complicating the builtin input.conf like similar scripts do.
By not defining the shortcut keys in the builtin input.conf, menu entries won't be associated to wrong shortcuts when the user's input.conf changes key bindings without redefining the menu. Note that external scripts don't have this issue.
I used 3+ spaces as separators at first but then realized that since we have Windows users tabs will look better in editors with proportional fonts like notepad, though it still won't be perfectly aligned depending on the font. But I am still not sure what is better.
On a curious note, etc/menu.conf already existed in the past as mplayer used to have a menu. git log -p etc/menu.conf will show commits from 2002 to 2010.
command: add the default-menu property
This property contains the builtin etc/menu.conf, and will be used by select.lua. It is undocumented because it is for internal usage.
select.lua: populate the context menu
Make select.lua parse menu.conf, fill menu-data and open the context menu. This is done from select.lua to reuse the code to format the data and retrieve key bindings.
While the context menu is scrollable, for the playlist it is better to start from around the current entry rather than from the beginning, and since menu-data has no way to specify where to start, when there are more than 25 items playlist items this adds … entries that open the scrollable console menu when clicked.
input-bindings is parsed to show the shortcuts bound to commands. console and stats key bindings are skipped in case the context menu is opened together with those scripts. Multimedia and numpad keys are not shown to reduce clutter.
The old Context Menu section of the docs is merged in the more detailed context_menu.rst to not repeat the same information.
input.conf: add context menu bindings
Rebind right click and MENU to show the context menu. Shift+F10 is also bound because it is a standard context menu binding.
MENU is not added to restore-old-bindings.conf because it is uncommon.