-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
More mappings for click and scroll actions via preferences #5348
base: develop
Are you sure you want to change the base?
Conversation
This looks like a nice enhancement. Currently IINA is on the verge of releasing the first beta of the next release, so only some fixes for regressions are currently being merged until we get the beta out. As it turns out some of the changes planned for the second beta have to do with input controls, this kind of enhancement. So this submission is perfectly timed for B2. Today is busy for me so I only had a chance to take a very quick look. I will take a closer look when I have a chance. One change that is needed is due to localization. IINA's integration with Crowdin requires that whenever new translations are added to PrefControlViewController.strings.zip Check and make sure you agree and if so, add a commit with that file to the PR. |
I had a chance to try out the changes. I'm really liking these enhancements. On this:
I agree, we don't need settings to change these limits, but we should consider using different values for the limits. The first consideration are the limits imposed by mpv on the value of the mpv speed option which is what IINA is adjusting. From the mpv manual:
Whatever we decide, IINA must stay within these limits. IINA already has hardcoded constants related to speed that can be found here: Lines 23 to 28 in d8ce998
I'm thinking we probably should match up with the limits used for the playback speed slider in the video side panel. I'm a junior IINA developer. Let's see if the senior developers agree. We probably won't hear from them until the first beta is out, which hopefully will be any day now. A side note. I found it surprising that IINA has a |
On this:
I like the ability to configure the sensitivity. I definitely think we should keep that. |
Personally, I'd love for the minimum to be lower than 0.25x. It's not uncommon for me to actually use a playback speed in the 0.05-0.1x range, with a very short loop, as I am closely analyzing very fast motions. |
I would be fine with that. Lets see what the senior developers have to say. I defer to them on UI changes. |
I added the localization updates as you created them, thank you for that guidance. I have not worked with localization in Xcode before, it seems like you may have used a tool that automatically created the entries in that If you're able to point me towards the tool/resource you used to do this, it would be much appreciated - but no need to take time to write out instructions just to show me. |
I'm always happy to explain things. Here is more than you probably want to know… I am not the expert on IINA localization, @uiryuu is. In fact I'm a little worried as to whether my knowledge is out of date. Brain not working well tonight. I remember we were trying to move away from the current way we do localization to using to the new string catalogs feature. I vaguely remember that effort had to be postponed because we found Crowdin did not support string catalogs? @uiryuu may correct me on this. So what IINA is doing is the old way of doing localization. Of course XIBs are also the old way of doing things. I know there has been some investigation into slowly moving to Swift UI. But there are more pressing matters that need to be addressed first. To produce that file I used two tools. I first used ibtool to generate a new strings file from the XIB. I directed the output to a temporary file like so:
I have found that this produces extraneous differences with the existing source due to declarations not being in the same order, frequently due to developers not using For merging I use Meld. Unfortunately the Meld project does not directly support macOS. I installed it using brew. However I did not have it installed on my second development machine and just this week went to go install it and found the homebrew formula is disabled. The problem is that the 3rd party macOS meld builds come from yousseb who hasn't generated a new release for a while. I had to grab it from my other machine and then struggle to get past the new Sequoia protections against apps that are not officially signed. Although I recommend and use Meld it is a bummer that it can't be installed right now using brew. I also frequently use Meld to eliminate the many extraneous changes added by Xcode when I make changes to XIBs. Once a PR has been merged with changes to Base and en translations then some sort of script is run to push the changes to IINA's instance of Crowdin. Periodically another script is run to pull translations from Crowdin and merge them into the strings files for other languages. @uiryuu knows the details of this. |
Description:
This adds more actions that click and scroll actions can be set to in preferences. The new click actions are the setting/clearing of A-B loop points (like the "A-B loop" action that keybinds can be mapped to), and resetting the playback speed to 1x. The new scroll action is to adjust the playback speed, with configurable sensitivity.
I added this feature because I am frequently using my macbook to learn a physical skill or musical routine by watching tutorial videos, and I am often looping sections or slowing them down. I wanted to be able to access these controls via the trackpad, since it is closer to me, which matters when I don't have my hands on the keyboard, but am reaching towards the macbook from a distance away, or when I am using a magic trackpad and the keyboard is too far away to reach without moving. The fast and easy fine-grained control over playback speed afforded by scroll control is also very nice. I've been using this feature for several months, and use it very frequently.
Possible changes:
I'm not sure if it's really necessary to have configurable sensitivity for the playback speed adjustment, It may be fine to have it fixed. I've found that multiplying the scroll delta by
0.005
is what works best for me, this is the 2nd-highest value in the sensitivity slider (with the mappings that I picked). If we don't want this configurable sensitivity,45e8a15c
can just be dropped.The playback speed limits that scrolling can reach are hardcoded to a minimum of 0.05x and a maximum of 4x. I don't think it's necessary to add a preference that controls these limits, but we may want to adjust them.