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.

Sign up for GitHub

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

Jump to bottom

Conflicting key bindings in default configs #4786

Open
svobs opened this issue Jan 22, 2024 · 7 comments · May be fixed by #4807
Open

Conflicting key bindings in default configs #4786

svobs opened this issue Jan 22, 2024 · 7 comments · May be fixed by #4807
Assignees
@svobs
Labels
bug feature: ui/ux progress: pr ready

Comments

@svobs
Copy link
Contributor

svobs commented Jan 22, 2024

System and IINA version:

  • macOS 14.2.1
  • IINA develop, dc81e9e

Expected behavior:
Each configuration in Settings > Key Bindings should have at most one key binding for a unique key, and should not use a key which is already used elsewhere (e.g. built-in menu item).

Actual behavior:
The following conflicting pairs of bindings are found. Only the second binding in each pair will be enabled.

IINA Default

#1
Meta+L  cycle-values loop "inf" "no"
Meta+L  Window > Log Viewer

VLC Default

#2
Alt+Meta+s  #@iina sub-panel
Alt+Meta+s  screenshot

#3
Meta+L  ab-loop
Meta+L  Window > Log Viewer

#4
Meta+z  playlist-shuffle
Meta+z  cycle-values window-scale 2 1 0.5 0.25

Movist Default

#5
Alt+Meta+s  #@iina sub-panel
Alt+Meta+s  screenshot

#6
Meta+-  #@iina smaller-window
Meta+-  multiply window-scale 0.9

#7
Meta++  #@iina bigger-window
Meta++  multiply window-scale 1.1
@uiryuu
Copy link
Member

uiryuu commented Jan 22, 2024

For the log viewer, we can change it into mata+shift+L

@low-batt
Copy link
Contributor

low-batt commented Feb 5, 2024

@svobs Would be good to fix this one soon. We are getting very close to cutting the regression bug fix release.

@svobs svobs linked a pull request Feb 7, 2024 that will close this issue
Draft
2 tasks
@svobs
Copy link
Contributor Author

svobs commented Feb 7, 2024

@low-batt I just filed #4807 with some best-guess changes to fix the conflicts. I'm guessing that the original developers will want to sign off on them.

@uiryuu it's not obvious from the way the mappings are written above, but Window > Log Viewer is already using Meta+Shift+L. (In mpv's key format, Meta+Shift+L == Meta+Shift+l == Meta+L). In the PR I changed the other bindings instead, but as noted in the PR I'm unsure about that choice.

@svobs
Copy link
Contributor Author

svobs commented Feb 7, 2024

Bit of a side note for my second bullet point in the PR. On the subject of + vs -, and how it seems incongruent that Shift needs to be held down to get +, but - does not... I was looking at Google Chrome's View menu, where it has Zoom In and Zoom Out items. They are clearly labelled ⌘ + and ⌘ -. But it looks like they fudged things a bit to show that. They mapped both the shift-key versions and non-shift versions. So you can also type ⌘ = and ⌘ _ to get the same result. Never noticed that, but kinda cute. Not sure that would be the most appropriate for IINA though.

@low-batt low-batt linked a pull request Feb 7, 2024 that will close this issue
Fix: conflicting key bindings in default conf files (issue #4786) #4807
Draft
2 tasks
@svobs
Copy link
Contributor Author

svobs commented Feb 7, 2024

On closer examination...

  • (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.

@uiryuu
Copy link
Member

uiryuu commented May 22, 2024

Sorry for being sooooo late for this. Changing the shortcut for the Log Viewer to Ctrl+Meta+l looks good to me. And also do we also want the users to be able to change the keybinding for Log Viewer as well? I.e. make it a IINA command and put it into the conf file.

@uiryuu
Copy link
Member

uiryuu commented May 28, 2024

The shortcut for log viewer has been changed to ctrl-meta-l on develop (efa7709)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@svobs svobs

Labels
bug feature: ui/ux progress: pr ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: conflicting key bindings in default conf files (issue #4786) svobs/iina-advance
3 participants
@svobs @uiryuu @low-batt