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

defaults.lua: fix canceled key bindings handling #14311

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

na-na-hi
Copy link
Contributor

@na-na-hi na-na-hi commented Jun 6, 2024

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.

In certain situations (including but not limited to begin window dragging),
it is desired to cancel the current command completely. However, commands
which have on_updown flag set require the command to be invoked in this
situation. There is currently no way to know if the command is triggered
normally or triggered because it is dropped.

This adds a canceled state to mp_cmd which indicates this.
This allows libmpv clients to know whether the key binding is canceled
and thus should normally be ignored.
Copy link

github-actions bot commented Jun 6, 2024

Download the artifacts for this pull request:

Windows
macOS

@avih
Copy link
Member

avih commented Jun 6, 2024

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
Copy link
Contributor Author

na-na-hi commented Jun 6, 2024

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.

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.
@avih avih merged commit 31ae111 into mpv-player:master Jun 6, 2024
19 checks passed
@avih
Copy link
Member

avih commented Jun 6, 2024

Thanks.

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.

3 participants