Fix custom key binding ignored, #3692 #3693
Conversation
The commit in the pull request will: - Add swift-algorithms to the list of packages - Change MenuController.updateKeyEquivalentsFrom to ignore key bindings that have been overridden when searching for keys to be used as shortcuts for menu items
|
Please see my comments in #3692, I think that's a better fix. Would you like to update your PR or I push my proposed fix instead? |
|
Caught me on a rare super busy day. Not sure how well the brain is working tonight. I say we toss my commit in the trash and go with your fix. BUT we should be coordinating with @svobs who has just proposed several fixes for key binding issues #3710, #3716 and #3718. See the PRs he posted. So lets close this PR. |
Hi @lhc70000! Let me just preface this by saying I'm a longtime Mac user, longtime developer, but very recent Mac developer. I'd tried several video players over the years and been kind of shocked at how mediocre they all were, and in fact had settled on bare-bones no-UI MPV for a long time before I discovered IINA :) Anyway, I'd like to help keep it going, and it's also a good chance to learn more about MPV integration. So I understand the need to have a separate layer on top of MPV - must have been tons of work - but the hot reload of key bindings and smoother MacOS integration is excellent. Still, I get the sense that there are a small but very influential group of power users which it might be worth catering to in order to ensure IINA's ultimate success. So my presumption is: IINA should behave as much like MPV as possible, unless there's a good reason not to. It seems like it should be able to support Lua scripts and other advanced things but still have a nice user experience for the more casual user. I'm willing to work on a lot of this stuff. So, sorry for being so long-winded - was kinda making the case for my other PRs here as well since I just went and did them without much discussion, so let me know what you think. As for this one - @lhc70000 would it be possible to implement both fixes for this issue? I agree that bumping the range to 60sec would be an improvement. But larger values will still lead to a discrepancy between IINA and MPV, like this: Specifically, the discrepancy arises when there are two lines with the same keysym, and the first is inside the matching range but the second is outside of it. The first one becomes attached to a menu item while the second does not, and at keyDown a menu item is always higher priority than a non menu item. In the PR for #3710 I thought I independently fixed the problem by picking the last binding which matched, instead of the first. But that only fixed it in the case where they both matched. I did some testing and there is a small merge conflict with @low-batt's fix but it should be easy to remedy. |
Yes, of course, we don't want such a discrepancy. It's a bug. I will push a fix later to make sure the behavior is consistent with mpv as well as bumping the range (to make the shortcut displayed in the menu). BTW,
IINA supports mpv lua scripts and mpv config files in Preferences - Advanced. It supports all mpv arguments in the bundled There's a Javascript-based plugin system on the |
|
The fix in this PR is not correct. This PR has been replaced by PR #3728. Closing this one. |
The commit in the pull request will:
Add swift-algorithms to the list of packages
Change MenuController.updateKeyEquivalentsFrom to ignore key bindings
that have been overridden when searching for keys to be used as
shortcuts for menu items
This change has been discussed with the author.
It implements / fixes issue Custom key binding ignored #3692.
Description:
Note the addition of Swift Algorithms.