input: new option: --no-input-builtin-bindings #9284
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
avih
force-pushed
the
input-builtin-bindings
branch
from
cabe750
to
b3171f1
Compare
|
As requested by @jeeb : reword the new option docs, add a note to interface-changes, move the input-default-bindings docs update to a new commit. (code and commit message unmodified) |
This is similar to [no-]input-default-bindings, but affects only builtin bindings (while input-default-bindings affects anything which config files can override, like scripting mp.add_key_binding). Arguably, this is what input-default-binding should have always done, however, it does not. The reason we add a new option rather than repurpose/modify the existing option is that it behaves differently enough to raise concerns that it will break some use cases for existing users: - The new option is only applied once on startup, while input-default-bindings can be modified effectively at runtime. - They affects different sets of bindings, and it's possible that the set of input-default-bindings is useful enough to keep. Implementation-wise, both options are trivial, so keeping one or the other or both doesn't affect code complexity. It could be argued that it would be useful to make the new option also effective for runtime changes, however, this opens a can of worms of how the bindings are stored beyond the initial setup. TL;DR: it's impossible to differentiate correctly at runtime between builtin bindings, and those added with mp.add_key_bindings. The gist is that technically mpv needs/uses two binding "classes": - weak/builtin bindings - lower priority than config files. - "user" bindings - config files and "forced" runtime bindings. input-default-bindings affects the first class trivially, but input-builtin-bindings would not be able split this class further at runtime without meaningful changes to a lot of delicate code. So a new option it is. It should be useful to some libmpv clients (players) which want to disable mpv's builtin bindings without breaking mp.add_key_bindings for scripts. Fixes mpv-player#8809 (again. the previous fix 8edfe70 only improved the docs, while now we're actually making the requested behavior possible)
avih
force-pushed
the
input-builtin-bindings
branch
from
b3171f1
to
73a6c91
Compare
|
Force-pushed docs capitalization typo at the first commit ("and If used" -> "and if used") and rebased. |
jeeb
approved these changes
Oct 11, 2021
There was a problem hiding this comment.
A very slight note regarding interface-changes.rst, otherwise LGTM since I can see this option being utilized in f.ex. mpvnet-player/mpv.net#271 .
| @@ -43,6 +43,7 @@ Interface changes | |||
| - add a `--watch-later-options` option to allow configuring which | |||
| options quit-watch-later saves | |||
| - make `current-window-scale` writeable and use it in the default input.conf | |||
| - new startup-only option --no-input-builtin-bindings (useful with libmpv). | |||
There was a problem hiding this comment.
Just looking at the nearby lines:
- the wording
add xyzis generally utilized here - the flags/options are surrounded by backticks
- additionally, since this lists the interface changes it might make sense to strip/mark the
no-prefix, as it is just part of the auto-generation of things for cli options - which stopped working through libmpv since circa 0.26?
(mpv_set_option_string(mpv_ctx, "input-builtin-bindings", "no"); would be how an API user would utilize this methinks (before init and after creation of context))
- add `--(no-)input-builtin-bindings` flag to control loading of built-in key bindings during start-up.
The previous few flags have all been off by default so we have not had to think about how to best describe enabled by default flags in here, so I'm open to how exactly it's done. But this is what came out of my mind right now.
|
Thanks, pushed to master as a3ef4c6 interfaces-changes text changed from: to as agreed on IRC. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is similar to [no-]input-default-bindings, but affects only
builtin bindings (while input-default-bindings affects anything which
config files can override, like scripting mp.add_key_binding).
Arguably, this is what input-default-binding should have always done,
however, it does not.
The reason we add a new option rather than repurpose/modify the
existing option is that it behaves differently enough to raise
concerns that it will break some use cases for existing users:
Implementation-wise, both options are trivial, so keeping one or the
other or both doesn't affect code complexity.
More info at the commit message.
Fixes #8809