Fix: conflicting key bindings in default conf files (issue #4786)

[svobs]

• I have read CONTRIBUTING.md
• This implements/fixes issue Conflicting key bindings in default configs #4786.

---

Description:
Attempts to fix conflicts reported in the issue above. These are inherently subjective changes, so I took my best shot. I did look at VLC and Movist to observe their bindings, but there were no major insights there. For each conflict I generally tried to decide which command was more commonly used, and give that the easier key.

Notes / justifications for my changes:

• movist-default has both bigger-window and multiply window-scale 1.1 (also the comparable small versions) which are somewhat similar, but not completely. The bigger-window menu item increases window width by 10 pixels at a time. No idea which would be more commonly used in this case, but I gave the multiply [...] commands the favored keys because they more closely resemble Movist commands.
• Also in movist-default: I noticed that those bigger / smaller commands were not mapped to complementary keys. for example: Meta+- → smaller-window, Meta++ → bigger-window. But while + looks appropriate for "bigger" when read from text, it requires holding down Shift to type it. So from a typing perspective, the complement of Meta+- should be Meta+=. Likewise, the complement of Meta++ should be Meta+_. At least on a US keyboard.
  -#@iina Meta++ bigger-window
  +#@iina Meta+= bigger-window
• iina-default and vlc-default had swapped keys for ab-loop and cycle-values loop "inf" "no", respectively. This seemed wrong to me, so I changed both in vlc-default, using Meta+l (lowercase L) for ab-loop instead of uppercase, which matches iina-default.
• I decided to leave Meta+Shift+L for Window > Log Viewer, and instead changed the mapping for cycle-values loop "inf" "no" to Alt+Meta+l. Maybe I should have done it the other way around, since Log Viewer will only be used by developers?

[low-batt]

I'm thinking this is the wrong fix. IINA should match the VLC key bindings. When we choose to extend the VLC key bindings to provide access to IINA features like the log viewer, that key binding must avoid conflicting with the bindings defined by VLC. It is more important to match VLC than use the same key binding for IINA features in the different defaults.

[low-batt]

As the log viewer is the new feature I would think it would be better to pick a key binding that doesn't conflict with an existing binding.

Why does the log view binding not appear in the key bindings file like we do for the keys that bring up other panels?

[low-batt]

Let's see what the other reviewers think about my concerns.

[svobs]

IINA should match the VLC key bindings. When we choose to extend the VLC key bindings to provide access to IINA features like the log viewer, that key binding must avoid conflicting with the bindings defined by VLC.

I agree with this, but I don't know what went into the decisions for some of these. I did some further research and found Meta+Shift+L for ab-loop. But it looks like its Normal/Loop/Repeat command is bound to Shift+L, not Meta+Shift+L. So I don't know why that was chosen.

Why does the log view binding not appear in the key bindings file like we do for the keys that bring up other panels?

The Log Viewer is hard-coded to that key - it's a menu item key equivalent. There are 5 sources of key bindings that I've seen, but only one (input config file) is shown in the Key Bindings table.

Let me try this again...

[svobs]

See my latest comment in the issue. Here's a copy:

• (4) above is clearly a bug. Second item should be Shift+z.
• Did an audit of the entire Movist set against the Movist player. Found that Movist offers "Version 1" and "Version 2" bindings sets. IINA's Movist configuration is a hodgepodge of both, and also contains redundant mappings not found in either. I went through and used only the Version 2 mappings. This takes care of (6) and (7).
• (2) and (5) can be resolved if the IINA panel bindings in VLC and Movist configs are changed from Alt+Meta+* to Shift+Meta+*. It seems like this might be a good solution, because they would then match the bindings in IINA's config.
• This only leaves (1) and (3). They can be resolved by changing the Log Viewer key equivalent. Using Ctrl+Meta+l (that's a lowercase L) will work. [EDIT: Alt+Meta+l will also work, and might be easier to type / more Mac-like]

It was a pain to go through and update Movist, but I stand by it. It doesn't really make sense to combine some of its Version 1 bindings with its modern ones, because there are conflicts between them.
Will wait to see what @uiryuu would like to do for the Log Viewer key equivalent. This fix isn't complete until that is addressed.

[low-batt]

This has turned out to be a much bigger problem than I expected. Glad you are taking a close look at it.

I was thinking this really needed to be addressed in 1.3.5 because the log viewer is new, so the conflicts introduced by the new key binding is a recent regression, which is what 1.3.5 is supposed to be addressing. However the changes to the Movist and VLC bindings are a big change that seems like something that should go into 1.4.0b1.

On the Movist change, would it make sense to leave the current bindings and create a new Movist V2 Default set of bindings?

I'm not clear on what criteria IINA uses for deciding to hardcode a key binding vs. adding a #@iina command and adding a changable key binding. The key binding to a command has the advantage of being visible when you are looking at key bindings in the settings panel and allowing the user to change the binding. This means you are less likely to create a new key binding that conflicts. Adding a hardcoded binding has the advantage of it being immediately available to users who are using their own custom key bindings file.

Need to hear from @uiryuu on this.

[svobs]

On the Movist change, would it make sense to leave the current bindings and create a new Movist V2 Default set of bindings?

You could certainly label the version that I submitted as "Movist V2". But what's in develop right now couldn't be called "Movist V1" because it contains some bindings for V2 and a bunch of bindings that aren't present in either. Possibly Movist has changed their default bindings incrementally and it's not as simple the V1 and V2 that they now show. I don't know. But adding a proper "Movist V1" would be some additional work and would look a lot different than the present file.

I'm not clear on what criteria IINA uses for deciding to hardcode a key binding vs. adding a #@iina command and adding a changable key binding.

I don't know if it that was actually an intentional decision. Hard-coding is the easiest solution because it's just entering a key binding into a text field in the XIB. It presently takes a lot more effort to make menu items configurable because each will need a text-based identifier and some extra code to match each menu item to a user key binding. Some apps (e.g. Movist) let the user configure every single menu item, including standardized commands like Window > Minimize, Edit > Undo, Edit > Redo, etc. Personally I think that would be the ideal. The potential downside would be that it makes user support a tiny bit harder because one would not be able to rely on a key binding, but that's rather minor IMHO. Although I might leave File > Quit hard-coded due to its importance as the guaranteed escape hatch. Even that is a little bit of hair-splitting.

Why not just make Log Viewer configurable then? Because at present, there's no support for built-in defaults when you do that. Each Configuration would need to have a line containing the Log Viewer key binding, or else it won't be present. Users who are upgrading or importing their mpv conf files would need to add a lot of #@iina lines to each. A better approach might be to create a mechanism for setting defaults. But that would require some design and code changes which are outside the scope of this PR.

[low-batt]

On Movist V2 Default ,I brought that up over worries that users who are used to certain key bindings will be unhappy with them changing. I'm not suggesting changing the current Movist bindings to match up with V1. Really need guidence from @uiryuu.

[lhc70000]

Let's move this to 1.4.0 beta 1, because it will be a breaking change.

I'm not clear on what criteria IINA uses for deciding to hardcode a key binding vs. adding a #@iina command and adding a changable key binding.

We currently don't have iina command for all external windows (other examples include the inspector). I think these key bindings should be configurable, so we need to update the Swift code before merging this. We can even set a default key binding for a command when it's not occupied and not specified by the conf. I will implement this soon if there's no other concerns.

For Movist keybinding, maybe we can have two separate configurations? Just keep the current one and add a movist-v2--default-input.conf.

[svobs]

For Movist keybinding, maybe we can have two separate configurations? Just keep the current one and add a movist-v2--default-input.conf.

Sounds good.

Let's move this to 1.4.0 beta 1, because it will be a breaking change.

👌 - I will change status to draft for the time being. Will update for the above change when I can, and won't worry about the Log Viewer binding conflict as that can be addressed in a separate PR. Sounds like there is still some uncertainty about what will be done with #@iina commands but addressing that should be trivial.

We currently don't have iina command for all external windows (other examples include the inspector). I think these key bindings should be configurable, so we need to update the Swift code before merging this. We can even set a default key binding for a command when it's not occupied and not specified by the conf. I will implement this soon if there's no other concerns.

Mostly agree, but want to clarify some details about how default bindings would work. For system bindings (File, Edit stuff) it makes sense to have implicit defaults, but for IINA stuff like Inspector, Log Viewer, etc, it would be nice if the user could leave them unbound without too much difficulty...

I'll just put this idea out there. Maybe there could be a separate table added to Settings... > Key Bindings for the IINA/system commands, with a fixed number of rows, one for each command. Only the Key column would be editable, and if left blank row's action would not have a key binding. It would have to be a separate table because it would not be part of the conf file and would not change when the user selects a different configuration. But the bindings in it would have lower precedence than those in the current conf file, so each could be easily overridden if the conf file has a binding for the same key.