mp.input: use unique event handlers for input.get requests

[CogentRedTester]

This makes changes to mp.input and console.lua so that every input.get
request uses a unique script-message to handle input events.

Previously, making new input.get requests shortly after the termination
of a previous request made by the same script could cause a race
condition where the input handler was closed but the new request
was still being drawn in the UI. This was caused by the closed event
for the previous request being received only after the new request was
registered, hence closing the event handler for the new request instead.

In addition, this commit makes the behaviour of calling input.get while
another request is active more consistent. When a new request is
received it overwrites the in-progress request, sending a closed
event. However, previously, the closed event could not be sent if both
requests came from the same script, as the new request would have
overwritten the event handler. Now, the closed event is called
regardless of where the new request comes from.

Edit: this PR has largely been superseded by #17256, but since that PR
requires changes to the mp.input interface and this one doesn't, I'll
keep this open until I find out if that PR has interest in being merged.

Examples

mp.add_key_binding('KP1', nil, function()
    input.get({
        prompt = 'prompt1 >',
        edited = print,
        submit = function()
            input.get({
                prompt = 'prompt2 >',
                edited = print,
            })
        end
    })
end)

mp.add_key_binding('KP2', nil, function()
    input.get({
        prompt = 'prompt1 >',
        edited = print,
        submit = function()
            mp.add_timeout(0.1, function()
                input.get({
                    prompt = 'prompt2 >',
                    edited = print,
                })
            end)
        end
    })
end)

If you use the first keybind specified above you can see that after every
keypress the contents of the input are printed to the console.
After pressing enter, the submit callback is run, creating a new prompt.
However, while the new prompt is being drawn on the screen, no callbacks
are being triggered. This is because console.lua not only sent a submit
callback, it also sent a closed callback which will only be processed
by the event loop once the submit callback is done.
Since the submit callback creates a new input.get request, overriding
the event handler with the handler for the new request, close ends up
closing the new request instead. The second keybind above delays the new
input.get request by 0.1 seconds to give the closed callback a chance to run first,
and so should work in the current version of mpv.

The changes in this PR mean that this race condition cannot occur, and so
both keybinds behave the same (other than perhaps a small flash caused by the
timeout).

mp.add_key_binding('KP3', nil, function()
    input.select({
        prompt = 'select1',
        items = {'one', 'two'},
        submit = print,
    })
    input.terminate()
    input.select({
        prompt = 'select2',
        items = {'three', 'four'},
        submit = print,
    })
end)

mp.add_key_binding('KP4', nil, function()
    input.select({
        prompt = 'select1',
        items = {'one', 'two'},
        submit = print,
    })
    input.terminate()
    mp.add_timeout(0.1, function()
        input.select({
            prompt = 'select2',
            items = {'three', 'four'},
            submit = print,
        })
    end)
end)

This race condition can also occur when explicitly closing a request with input.terminate.
In the first of the above keybinds, the first select prompt is overridden by the second,
which is drawn to the screen. However, the input.terminate call triggers a closed
event that removes the second select request's event handler. As such, selecting an
item does not print anything to the screen. The second example again uses a timeout
to fix the behaviour on the current version of mpv.

mp.add_key_binding('KP5', nil, function()
    input.select({
        prompt = 'select1',
        items = {'one', 'two'},
        closed = function() print('select1 closed') end,
        submit = print,
    })
    input.select({
        prompt = 'select2',
        items = {'three', 'four'},
        closed = function() print('select2 closed') end,
        submit = print,
    })
end)

Overriding an existing request without first terminating the input does work currently,
as shown by the above keybind. However, the single event handler means that console.lua
cannot send a closed event to the first select request like it does when a request is overridden
by a request from a different script. The changes in this PR mean that the closed event is sent
for the overridden request regardless of the source.

[CogentRedTester]

I'm not sure if there's any documentation I should be modifying for this as the current docs don't really explain anything regarding the behaviour of multiple input.get requests.

[guidocella]

Is there anything wrong with passing keep_open = true to make nested calls? I thought that was better than allowing it in other ways because it avoids showing the console quickly closing and reopening.

[CogentRedTester]

The scenario that lead to this PR was actually caused by me wanting to trigger input.select from input.get, which you can see in this youtube-search script that I modified earlier today: CogentRedTester/mpv-scripts#40

The fact that an input UI can be visible, but just silently fail to do anything can be very confounding. It took me quite a while to figure out what was going on and necessitated looking through the input.lua and console.lua source code. I don't see any advantage to leaving things as they are, whereas this PR would help prevent developers from encountering this unexpected behaviour in the future.

[guidocella]

You don't need timeouts to call input.select from input.get. You just pass keep_open = true. I guess that needs to be documented.

I think the advantage of leaving it like this is that it encourages to use it in the intended way with keep_open which avoids the flash, and it doesn't make it more complicated to replicate input.lua from C plugins and libmpv.

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-aarch64-pc-windows-msvc
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-aarch64-pc-windows-msvc-pdb
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-i686-w64-mingw32
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-x86_64-pc-windows-msvc
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-x86_64-pc-windows-msvc-pdb
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-x86_64-w64-mingw32

macOS

• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-macos-14-arm
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-macos-15-arm
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-macos-15-intel
• mpv-v0.41.0-dev-gfe72c1c14-pr17251-20886886809-macos-26-arm

[CogentRedTester]

Now that you've pointed it out, I do realise how keep_open works, and it's definitely not at all clear from the documentation (that you're expected to use it with new requests, or that it also works between get/select), so that should be fixed for sure. However, I still think this is a worthwhile change. It adds minimal extra complexity to the code while avoiding a whole class of footguns that developers might accidentally stumble onto. I understand wanting developers to use the library in a specific way, but I also think that a library like this should be robust enough to not break if used differently (and the way I've used it is hardly bizarre, it was what came intuitively to me, so I assume it will for others as well). I think the ideal solution is to clearly document the intended usage (with examples), while also merging this PR to avoid the input handler being closed while the UI is active.

Also, this PR allows the closed event to work consistently no matter the scenario (which I think is valuable considering the documentation says it will be sent, and there's currently an undocumented scenario when it isn't).

[guidocella]

Dunno, this already doesn't update defaults.js, and any extra complexity will have to implemented in every mp.input client, like a future python backend and individual developers replicating input.lua in C. And apparently people already find it difficult to replicate input.lua (#16801).

Not sending closed when replacing the handler is intended since console is not actually closed. I don't think it conflicts with how closed is documented: "A callback invoked when the console is hidden, either because input.terminate() was invoked from the other callbacks, or because the user closed it with a key binding."

Also a simpler way to make nested calls work without keep_open would be to just not call mp.unregister_script_message("input-event") on closed. That is only there to allow garbage collection of any variables in the scope calling mp.input which is not important. I actually removed that temporarily and brought it back again because I thought it is better to call keep_open = true to avoid showing the UI flashing to the user anyway (I'll be sure to document keep_open).

[CogentRedTester]

Dunno, this already doesn't update defaults.js

Woops, I forgot about JavaScript, I've added it now.

and any extra complexity will have to implemented in every mp.input client, like a future python backend and individual developers replicating input.lua in C. And apparently people already find it difficult to replicate input.lua (#16801).

This really is very minor extra complexity; it's literally just adding an incrementing counter and building a string with it. I don't think it's a valid reason to avoid this change.

Not sending closed when replacing the handler is intended since console is not actually closed. I don't think it conflicts with how closed is documented: "A callback invoked when the console is hidden, either because input.terminate() was invoked from the other callbacks, or because the user closed it with a key binding."

But I don't think that is how the input requests are really presented, the documentation implies (to me) that each input.get request is distinct; there aren't single submit, edited, etc. callbacks that apply to the entire console, there are separate callbacks for every input.get call. Multiple scripts also cannot listen to the console at the same time, they must make separate requests. So I would argue that the implication is really that the callbacks run based on the status of the specific input.get request (which is mostly what happens), and the fact that there are edge cases where that is not the case is very unintuitive. And anyway, the current code seems to agree with this since it sends a closed event to overridden requests when they're not from the same script. I don't think it makes sense for this behaviour to be different when the source is the same; after all, one script can do multiple things and provide multiple inputs triggered by different keybinds.

Also a simpler way to make nested calls work without keep_open would be to just not call mp.unregister_script_message("input-event") on closed. That is only there to allow garbage collection of any variables in the scope calling mp.input which is not important. I actually removed that temporarily and brought it back again because I thought it is better to call keep_open = true to avoid showing the UI flashing to the user anyway (I'll be sure to document keep_open).

I would argue that this is might be worse as then the closed callback would be run on the new request. If a script is relying on this to be called after the submit/edited/etc callbacks are run then this could cause a whole new class of bugs.

[guidocella]

one script can do multiple things and provide multiple inputs triggered by different keybinds.

the closed callback would be run on the new request.

Fair, these are a good points which convinced me.

It is fortunate that I did not document the JSON API yet or you could not change it.

On the other hand it is unfortunate that nesting calls without keep_open doesn't avoid the flicker. That was the main reason for implementing it. If you apply this to go back to how it was before,

diff --git a/player/lua/input.lua b/player/lua/input.lua
index 3dded33dd7..31daf87c1c 100644
--- a/player/lua/input.lua
+++ b/player/lua/input.lua
@@ -42,10 +42,6 @@ local function register_event_handler(t)
                             completion_append or "")
             end
         end
-
-        if type == "closed" then
-            mp.unregister_script_message("input-event")
-        end
     end)
 end
 
diff --git a/player/lua/select.lua b/player/lua/select.lua
index 0ecbff4ea5..9b50ce1610 100644
--- a/player/lua/select.lua
+++ b/player/lua/select.lua
@@ -742,13 +742,8 @@ mp.add_key_binding(nil, "menu", function ()
     input.select({
         prompt = "",
         items = labels,
-        keep_open = true,
         submit = function (i)
             mp.command(commands[i])
-
-            if not commands[i]:find("^script%-binding select/select") then
-                input.terminate()
-            end
         end,
     })
 end)

press Ctrl+p and open submenus, the flicker is quite noticeable. So the manual should still recommend using it for nesting. I will update the other PR accordingly.

[CogentRedTester]

So the manual should still recommend using it for nesting. I will update the other PR accordingly.

No argument there, I have a couple of suggested tweaks to the documentation myself, so maybe hold off on merging that PR until I get a chance to look at it tomorrow?

[guidocella]

Yeah that depends on this PR anyway and it's not like I have merge rights. I am also planning to deprecate opened actually, since it is useless now.

[CogentRedTester]

I am also planning to deprecate opened actually, since it is useless now.

I disagree actually, I think that opened is maybe more useful after this PR given the improved reliability of closed being run. opened and closed act kind of like constructors/destructors for the input request, capable of toggling settings or managing resources. This PR guarantees that closed will be run for all input requests (barring a bug or crash in console.lua), which makes opened now the ideal place to initialise things. E.g.:

    local file
    input.get({
        prompt = 'Append text to file:\n> ',
        opened = function()
            file = io.open('/path/to/file', 'w+')
        end,
        submit = function(line)
            file:write(line)
        end,
        closed =  function()
            file:close()
        end,
    })

Technically, the current code essentially guarantees that any input request will almost immediately be opened, at least briefly. However, the reality is that mp.input is an asynchronous API, and I don't think we should be encouraging people to treat the console like it has been opened the moment they call input.get because it just hasn't. Especially considering that there are scenarios when the console will genuinely not open without any feedback, such as when a user has set --load-console=no.

Currently, the documentation makes no explicit guarantees that a request will actually be made to the user when calling input.get. I think that the documentation should be updated to explicitly state that the console should not be treated as opened until the opened callback has been received and that we cannot completely guarantee that opened will be called in all circumstances.

[guidocella]

opened making a difference only with --load-console=no or crashes is exactly what makes it useless. At that point the user is actively breaking things. It was meant for when console would do nothing if already used by a different script. Now it's just a slower alternative to running code after input.get/select, which I already saw done in the wild in https://framagit.org/Midgard/mpv-subber/-/blob/master/subber.lua.

[CogentRedTester]

Superseded by #17256.