mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.69k stars 2.83k forks source link

defaults.lua: fix canceled key bindings handling #14311

Closed na-na-hi closed 3 weeks ago

na-na-hi commented 3 weeks ago

There is a subtle behavior difference for built-in/input.conf key bindings and key bindings registered by scripts: when a key binding is canceled (e.g. a mouse button is bound to a command, is pressed down, and then another key is pressed which is bound to another command), the command is not invoked if the binding is built-in/input.conf, but is invoked if it's registered by scripts, because it's handled with a different mechanism, which gives no way for scripts to detect this.

Fix this by using the newly available canceled flag to detect this. If a key binding is canceled, the callback is now not invoked unless the key binding is registered with the complex option. In this situation, the callback is invoked with the canceled state available so that scripts can detect and handle this situation.

github-actions[bot] commented 3 weeks ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1575492469.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1575504317.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1575524384.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1575491492.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1575496485.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1575489385.zip)
avih commented 3 weeks ago

As far as I understand, events which the client/script now receives with a "canceled" flag were also received in the past - before this PR, just without the canceled flag, so the client couldn't distinguish canceled from normal keyup, right?

I presume the value of event is still up also when it's canceled? I'm pondering whether this should be maybe something else, but not sure what would be better/worse for scripts which are unaware of the canceled flag (like all current user scripts). Thoughts? (this is probably orthogonal to this PR, but maybe easier with this PR because default.script can now identify this state).

It would probably help if the docs gave examples of how events get canceled, maybe one KB example and one mouse example (because they trigger on different down/up phase). Otherwise, IMO it's not necessarily obvious how to understand/interpret/use "the key is logically released but not physically released" explanation to "canceled".

Currently is_mouse is used for a boolean value in the complex state. Maybe use is_canceled for this new flag (not 100% sure about that, but I tend towards it because naming consistency)?

This should also apply to js.

na-na-hi commented 3 weeks ago

Added the js change, and clarified the cancellation a bit with examples in the documentation.

As far as I understand, events which the client/script now receives with a "canceled" flag were also received in the past - before this PR, just without the canceled flag, so the client couldn't distinguish canceled from normal keyup, right?

Correct.

I presume the value of event is still up also when it's canceled? I'm pondering whether this should be maybe something else, but not sure what would be better/worse for scripts which are unaware of the canceled flag (like all current user scripts).

The scripts which don't use the complex flag shouldn't be affected: it just does the same as built-in/input.conf key bindings. However, scripts which use the flag may use it to track key up/down state. That's why the value of event is still up when the key is canceled to keep compatibility with these scripts.

Maybe use is_canceled for this new flag (not 100% sure about that, but I tend towards it because naming consistency)?

I think canceled is clear enough to indicate that it's a boolean state.

avih commented 3 weeks ago

Thanks.