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

"mpv Default" keyboard shortcuts display discrepancies in Preferences/Playback #4050

Closed
Kergariou opened this issue Nov 8, 2022 · 14 comments · Fixed by #4069
Closed

"mpv Default" keyboard shortcuts display discrepancies in Preferences/Playback #4050

Kergariou opened this issue Nov 8, 2022 · 14 comments · Fixed by #4069

Comments

@Kergariou
Copy link

System and IINA version:

  • macOS Monterey 12.6.1
  • IINA 1.3.1, 1.3.0

Expected behavior:
The accelerating/decelerating speed shortcuts for "mpv Default" are the same in the "Preferences…" panel and in the menu "Playback".

Actual behavior:
The accelerating/decelerating speed shortcuts for "mpv Default" are "[, ], {, }" in the "Preferences…" panel and "*, $, ¨" in the "Playback" menu.
Furthermore, the shortcuts for "reducing speed 0.9" and "back to 1.0" are not displayed in the "Playback" menu.
IINA 1 3 1 French Playback Menu
IINA 1 3 1 French Preferences Shortcuts

@Kergariou Kergariou changed the title "mpv Default" keyboard shortcuts display errors in Preferences/Playback "mpv Default" keyboard shortcuts display discrepancies in Preferences/Playback Nov 8, 2022
@svobs
Copy link
Contributor

svobs commented Nov 9, 2022

There are two issues here.

First - the difference in wording between the two places.

The names in the menu items are generally IINA's names for things, which are usually clearer (or attempt to be) than the names that mpv has chosen, while the names in the Key Bindings configuration need to be recognizable as mpv commands. It's really two separate systems which happen to overlap in lots of places. Most of the menu items will still be there even if there are no entries in your current Key Bindings configuration. If you think the wording is inaccurate or confusing then maybe it should be changed, but I don't favor IINA adopting mpv's terminology for things in general.

Second -
I was unable to reproduce the key binding inconsistencies. I show this:
Screenshot 2022-11-08 at 22 58 28

It's possible, although unlikely, that the file was changed from outside of IINA. Can you see if you can find this file and post it here?

  1. Right-click on IINA icon in the Dock, and choose Options > Show in Finder
  2. Right-click on the IINA app in the Finder, and choose Show Package Contents. Screenshot 2022-11-08 at 23 10 07
  3. Go to Contents > Resources > Config > input.conf Screenshot 2022-11-08 at 23 10 28
  4. Post that file here.

Also, what language are you using? Perhaps it's a locale-specific bug.

@Kergariou
Copy link
Author

Kergariou commented Nov 9, 2022

Hello,
I am using French language.
Please find attached the content input.conf file.

input.conf.txt

Best regards

@svobs
Copy link
Contributor

svobs commented Nov 11, 2022

@Kergariou thank you! I was able to reproduce the issue by changing my input source to a French AZERTY keyboard.

Screenshot 2022-11-10 at 22 46 57

It looks like letters are ok, but numbers and symbols are off.

This looks like a regression which was introduced in IINA 1.3.0. IINA 1.2.0 has the correct behavior.

Will post back when I have more info.

@Kergariou
Copy link
Author

Congratulations for your work!
Indeed, it was working fine in 1.2.0.
Have a nice day.

@lhc70000
Copy link
Member

This looks weird because I didn't find any change to the key binding handling logic between 1.2.0 and 1.3.0. I will investigate more today. I think it's critical and we should release a v1.3.2 soon to fix this problem.

@lhc70000
Copy link
Member

I built v1.2.0 on macOS 12.6 with Xcode 13. It still contains this bug. Something changed in the macOS SDK?

@lhc70000
Copy link
Member

lhc70000 commented Nov 12, 2022

Hi @Kergariou, I want to confirm that you are using a physical French AZERTY keyboard, right?

Edit:
Were you using the ] shortcut before? (It should be hard to press for an AZERTY keyboard?)

@svobs
Copy link
Contributor

svobs commented Nov 12, 2022

I built v1.2.0 on macOS 12.6 with Xcode 13. It still contains this bug. Something changed in the macOS SDK?

Interesting. I checked the release timeline:

+ 2020-11-12  MacOS 11 Big Sur released
|
+ 2021-01-20        IINA 1.2.0 released
|
+ 2021-10-25 MacOS 12 Monterey released
|
+ 2022-05-30        IINA 1.3.0 released

Must have been a Monterey thing. Which led me to discover this is actually a feature (video: starts at 5:23). How crazy is it that Apple releases critical documentation updates exclusively by video? Great way to ensure good attendance at your developer conferences, I guess ;)

I will draft a PR.

@lhc70000
Copy link
Member

@svobs Nice! Thanks a lot. I'll merge it soon.

However, I'm still thinking about whether IINA’s default key shortcuts are suitable for non-US key layouts. If the user needs to press Shift+RightOpt+- to produce a ] symbol, the Cmd+] key binding will be a disaster.

@svobs
Copy link
Contributor

svobs commented Nov 12, 2022

However, I'm still thinking about whether IINA’s default key shortcuts are suitable for non-US key layouts. If the user needs to press Shift+RightOpt+- to produce a ] symbol, the Cmd+] key binding will be a disaster.

This makes sense, though I'm probably not the best person to answer it.

It looks like the mpv project grappled with the same problem, but ended up just giving up and not trying to support other keyboards.

But at the same time, it seems Apple's solution doesn't always make sense, and would also leave us with the problem of having to figure out what keys Apple remapped them to each time, so that we can be consistent when displaying the Key Bindings UI.

Maybe try to use locale-specific versions of the built-in input conf files? Only the user's currently set locale would be displayed, and it wouldn't be editable by them anyway. Each localization could rely on community input, sort of like how other parts of the UI are localized using Crowdin. Though I don't know how realistic it would be to judge one over another, it's the best option I can think of.

@Kergariou
Copy link
Author

Hi @Kergariou, I want to confirm that you are using a physical French AZERTY keyboard, right?

Edit: Were you using the ] shortcut before? (It should be hard to press for an AZERTY keyboard?)

Yes, I confirm ! I am using a French AZERTY keyboard, and I was using the ']' before.
And as you say, it was very hard… 😁
That's why the new shortcuts $,*,^,¨are more practical/convenient.

@low-batt
Copy link
Contributor

I would expect Apple's Human Interface Guidelines to have an extensive discussion of this, but I only found the following from the keyboard-shortcuts section:

Let the system localize and mirror your keyboard shortcuts as needed. For example, iPadOS automatically localizes a shortcut’s
primary key and modifier keys to support the currently connected keyboard.

A better discussion can be found in the documentation for the method applicationShouldAutomaticallyLocalizeKeyEquivalents:

If you already localize your app’s shortcuts for different languages, or if you allow someone to customize your app’s shortcuts, you can return false to disable the automatic remapping behavior. When you return false, the system doesn’t change your app’s shortcuts. Instead, you are responsible to make any required changes to support localized keyboards.

The "if you allow someone to customize your app’s shortcuts" applies to IINA. Disabling this new AppKit feature is the correct fix.

I had checked AppKit Release Notes for macOS Monterey 12 for this kind of unexpected change. But Apple did not mention this change.

Need to remember to check the What's new in AppKit web site.

@low-batt
Copy link
Contributor

Fix has been merged into develop.

@Kergariou
Copy link
Author

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants