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

Added controls to QuickSettingsPanel to control second subtitles position and delay #5020

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

uiryuu
Copy link
Member

@uiryuu uiryuu commented Jun 24, 2024


Description:
mpv recently added secondary-sub-pos and secondary-sub-delay options, which enables user to control position and delay separately for primary subtitles and secondary subtitles. This PR implement these two new options by added a SegmentedControl in the QuickSettingsPanel to let user choose from primary or secondary subtitles when adjusting subtitle position and delay.

Note that the other options (e.g. sub-scale) apply to both primary subtitles and secondary subtitles, so they are outside of the NSBox.

Screen.Recording.2024-06-24.at.12.36.33.mov

The layout of the subtitle tab of the QuickSettingPanel also got changed to better illustrate the new feature (before -> after):

image image

@uiryuu uiryuu requested a review from low-batt June 24, 2024 03:47
Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the segmented controls. They look great and should be easy for users to understand.

I pushed a commit to add the segmented control label names to QuickSettingViewController.strings. With that I think this can be merged as is. The next step is to add the corresponding OSD messages for secondary sub position and delay changing like we have for the primary sub.

Need to merge the simple PR #5019 that adds a couple of missing secondary options to watch-later-options.

"Secondary" is the proper term. We should consider changing the menu items that use the term "Second".

And of course subtitle preferences needs some enhancement in this area.

To test this I used the YouTube video 10 Minute Timer (count-up stopwatch) with these two subtitle files:
sub-1.srt.txt
sub-2.srt.txt

I checked RTL:
delay

The messed up label at the end of the delay slider is issue #4895 / PR #4896. That PR is in draft mode because it needs a UI expert to take over from me and solve the last little issue shown in the screenshots in this comment.

@low-batt
Copy link
Contributor

One more related topic. The secondary-sub-pos, like sub-pos has a range of 0% to 150%. IINA tries to limit it to 100%. I believe this is because unlike mpv IINA ties the window size to the video size. I know removing that restriction has been reported in multiple issues, for example #4712. @svobs has looked into this. Anyway, I said "tries" because if you keep pressing the T key you can go past 100%. Given how IINA does not provide a way to have a black border you could move subtitles into I think it makes sense to restrict the sliders to 100%. I only mention this as a reminder of another subtitle feature that could be improved.

@uiryuu
Copy link
Member Author

uiryuu commented Jun 25, 2024

I pushed a commit to add the segmented control label names to QuickSettingViewController.strings.

Sorry I forgot to do that, and thanks!

And of course subtitle preferences needs some enhancement in this area.

I was unsure of how to design the UI to add secondary subtitles in, so I didn't include that in this PR.

"Secondary" is the proper term. We should consider changing the menu items that use the term "Second".

Besides this, I remember I've heard that the word "subtitle" should always be in plural form, is that correct? Should we enforce that (in the future)?

@low-batt
Copy link
Contributor

It usually is "subtitles" but not always "subtitles".

It is a "subtitle file" as can be seen in the mpv manual entry for the sub-file option:

--sub-files=<file-list>, --sub-file=<filename>
Add a subtitle file to the list of external subtitles.

It is a "subtitle stream" as can be seen in the mpv manual entry for the sid option:

--sid=<ID|auto|no>
Display the subtitle stream specified by <ID>. auto selects the default, no disables subtitles.

The IINA menu items are about selecting a "subtitle stream".

One label that should be changed is the Subtitle tab name in IINA's settings. that should be Subtitles, just like the Subtitles tab on the quick settings panel and the Subtitles menu.

@lhc70000
Copy link
Member

Update the styling and also enabled the macOS 13+ NSColorWell.

@lhc70000 lhc70000 merged commit f731bd2 into develop Jun 26, 2024
1 check passed
@lhc70000 lhc70000 deleted the secsub-settings branch June 26, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate control for secondary subtitle delay Make the second subtitle position adjustable
3 participants