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

console.lua: allow subsequent mp.input calls #15796

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guidocella
Copy link
Contributor

Currently using mp.input while the console is open doesn't do anything. We can allow using mp.input multiple times in a row by keeping the console open, and sending a closed event only when the previous calling script is different from the current one. If it is the same, sending a closed event would unregister the new input-event listener, so it is just never sent in this case as there is no sane way to do it.

Fixes #15795.

Copy link

github-actions bot commented Feb 3, 2025

Download the artifacts for this pull request:

Windows
macOS

Comment on lines +1840 to +1842
if repl_active and input_caller and script_name ~= input_caller then
mp.commandv('script-message-to', input_caller, 'input-event',
'closed', utils.format_json({line, cursor}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks hacky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better suggestions welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a stack with the state, and restore whatever was/is open after select ended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejected in #10282 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the other way, to immediate go to user requested input, but go back to previous state, if requested (closed without select). i.e. allow nested menus in menu type situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it strange to go back to the previous state when it was invoked by a completely different script doing something completely different? I think it should still send closed to different scripts like in this PR. Restoring the previous state from the same script is a separate functionality that can be added later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think close without select or indeed another dedicated function could achieve that.

As for the rest, I don't think other scripts should be able to interrupt the selection that is being made, with implicit way. If it is invoked as "nested" select we can support that to provide "menu" like interface, but I don't see others nuking already open select. That's why I feel that it is hacky, because we allow it to open and send some implicit closed events, but only if script is different, it's weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair though I think one reasonable use case for it is to try out script-binding select/... bindings in the console, this was actually requested in #14087 (comment) (and I misunderstood it as referring to the input queue at that time too).

@guidocella
Copy link
Contributor Author

guidocella commented Feb 3, 2025

This doesn't actually fix calling input.select again within submit because console already closes automatically on select so closed is received after submit and it unregisters the new event listener instead of the old one. A fix would be just not to call mp.unregister_script_message("input-event") whose only purpose was to allow garbage collection of variables defined in the scope calling mp.input. This PR does fix the get -> get and get -> select cases as with get console stays open until it is asked to close, and also select -> select without submitting.

Currently using mp.input while the console is open doesn't do anything.
We can allow using mp.input multiple times in a row by keeping the
console open, and sending a closed event only when the previous calling
script is different from the current one. If it is the same, sending a
closed event would unregister the new input-event listener, so it is
just never sent in this case as there is no sane way to do it. This is
useful e.g. to try out select script bindings in the console, or to
switch to a different mp.input script binding with a key binding without
having to close the current one.

Calling input.select() again within input.select()'s submit handler
needs to be fixed separately because with select the console is
immediately closed on submit (367a6b5), so input.lua receives submit,
requests a new selection, and receives closed wich unregisters the
handler of the new selection. Just not calling
mp.unregister_script_message("input-event") fixes it; its only purpose
was to allow garbage collection of variables defined in the scope using
mp.input.

Fixes mpv-player#15795.
@guidocella
Copy link
Contributor Author

I just removed the unregister_script_message call.

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

Successfully merging this pull request may close these issues.

opening selector multiple times will cause the selection to fail, is it a input.select API bug?
2 participants