Skip to content

Fix second ASS Subtitle keep its own style, #4433 #5132

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
Merged
merged 7 commits into from
Aug 14, 2024
Merged

Fix second ASS Subtitle keep its own style, #4433 #5132

merged 7 commits into from
Aug 14, 2024

Conversation

low-batt
Copy link
Contributor

@low-batt low-batt commented Aug 12, 2024 •
edited
Loading

This commit will:

  • Add a new IINA setting secondarySubOverrideLevel to Preferences
  • Add a no enum constant to Preferences.SubOverrideLevel
  • Add a preference.sub_override_level.no key to base and English Localizable.strings
  • Change PrefSubViewController.ASSOverrideLevelTransformer and PrefSubViewController.ASSOverrideLevelValueTransformer to support the new no enum constant
  • Remove support from ignoreAssStyles from MPVController.mpvInit
  • Add support for secondarySubOverrideLevel to MPVController.mpvInit
  • Remove the Ignore ASS styles checkbox from the ASS Subtitles section on the Subtitle tab of IINA's settings
  • Remove the text explaining the what the checkbox does
  • Add a no setting to the slider, replacing the checkbox
  • Add a segmented control to the ASS Subtitles section to allow the slider to work for both the primary subtitles and secondary subtitles
  • Add a text field that contains text explaining what the setting selected by the slider does
  • Add a help button to the slider linked to the mpv manual entry for the option

These changes will also fix:
Sub ass style overrides always applied, #4927.

Description:

@low-batt
Fix second ASS Subtitle keep its own style, #4433
152e14f 
This commit will:
- Add a new IINA setting secondarySubOverrideLevel to Preferences
- Add a "no" enum constant to Preferences.SubOverrideLevel
- Add a preference.sub_override_level.no key to base and English
  Localizable.strings
- Change PrefSubViewController.ASSOverrideLevelTransformer and
  PrefSubViewController.ASSOverrideLevelValueTransformer to support the
  new "no" enum constant
- Remove support from ignoreAssStyles from MPVController.mpvInit
- Add support for secondarySubOverrideLevel to MPVController.mpvInit

These are the backend changes needed to support changing the IINA
settings Subtitle tab to include a control for setting the secondary
subtitle override level. The user interface changes still need to be
done.

These changes will also fix:
Sub ass style overrides always applied, #4927.
@low-batt low-batt marked this pull request as draft August 12, 2024 20:27
@low-batt
Copy link
Contributor Author

@uiryuu I did not include a UI change as I was worred you had already started on the UI changes and including it might cause merge conflicts for you. The change is minor. I added 32 to the width of the slider to allow for the addition of the no setting. I increased the number of ticks by one as well as the maximum value. This is the modified file incase it is useful:
PrefSubViewController.xib.txt

PrefSubViewController.xib changes: 
low-batt@gag iina (sub-ass-override-2 *$%>)$ git diff  iina/Base.lproj/PrefSubViewController.xib
diff --git a/iina/Base.lproj/PrefSubViewController.xib b/iina/Base.lproj/PrefSubViewController.xib
index c9699d0b..d4d6730a 100644
--- a/iina/Base.lproj/PrefSubViewController.xib
+++ b/iina/Base.lproj/PrefSubViewController.xib
@@ -960,11 +960,11 @@
                     </textFieldCell>
                 </textField>
                 <slider verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="7UX-IL-hdk">
-                    <rect key="frame" x="202" y="5" width="128" height="20"/>
+                    <rect key="frame" x="202" y="5" width="160" height="20"/>
                     <constraints>
-                        <constraint firstAttribute="width" constant="124" id="EJa-Kl-S4K"/>
+                        <constraint firstAttribute="width" constant="156" id="EJa-Kl-S4K"/>
                     </constraints>
-                    <sliderCell key="cell" controlSize="small" continuous="YES" state="on" alignment="left" maxValue="3" doubleValue="3" tickMarkPosition="below" numberOfTickMarks="4" allowsTickMarkValuesOnly="YES" sliderType="linear" id="63l-gO-fEV"/>
+                    <sliderCell key="cell" controlSize="small" continuous="YES" state="on" alignment="left" maxValue="4" doubleValue="4" tickMarkPosition="below" numberOfTickMarks="5" allowsTickMarkValuesOnly="YES" sliderType="linear" id="63l-gO-fEV"/>
                     <connections>
                         <binding destination="5Up-Ab-aAm" name="value" keyPath="values.subOverrideLevel" id="jz2-FJ-liL">
                             <dictionary key="options">
@@ -973,8 +973,8 @@
                         </binding>
                     </connections>
                 </slider>
-                <textField horizontalHuggingPriority="251" verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="FR3-Ar-5Dw">
-                    <rect key="frame" x="302" y="8" width="28" height="14"/>
+                <textField focusRingType="none" horizontalHuggingPriority="251" verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="FR3-Ar-5Dw">
+                    <rect key="frame" x="366" y="8" width="28" height="14"/>
                     <textFieldCell key="cell" controlSize="small" scrollable="YES" lineBreakMode="clipping" sendsActionOnEndEditing="YES" title="strip" id="3H8-Ei-cTm">
                         <font key="font" metaFont="message" size="11"/>
                         <color key="textColor" name="labelColor" catalog="System" colorSpace="catalog"/>
low-batt@gag iina (sub-ass-override-2 *$%>)$

One of the contributing factors to IINA's confusion over the sub-ass-override option has to do with the yes setting:

yes: Apply all the --sub-ass-* style override options. Changing the default for any of these options can lead to incorrect subtitle rendering (default).

Trouble is IINA does not provide controls for any of the --sub-ass-* options. They are:

We probably should add support for some of these so that the yes setting makes sense. Let me know if you want me to add backend code for any of these. We can postpone this to Beta 2.

This was linked to issues Aug 12, 2024
Second ASS Subtitle keep its own style. #4433
Closed
Sub ass style overrides always applied #4927
Closed
@low-batt low-batt mentioned this pull request Aug 12, 2024
Closed
2 tasks
@uiryuu uiryuu marked this pull request as ready for review August 13, 2024 04:13
@uiryuu
Copy link
Member

uiryuu commented Aug 13, 2024

I've finished setting up the UI. Feel free to give suggestions

We probably should add support for some of these so that the yes setting makes sense. Let me know if you want me to add backend code for any of these. We can postpone this to Beta 2.

Let's make them in beta 2, at least not in this PR.

@uiryuu uiryuu force-pushed the sub-ass-override-2 branch from fabe773 to 267a891 Compare August 13, 2024 04:16
low-batt
low-batt commented Aug 13, 2024
View reviewed changes
Copy link
Contributor Author

@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.

GitHub won't let me request changes because I'm one of the authors. See my comments on the descriptions and changes needed.

I did a bunch of testing and the IINA code looks good, but I think mpv is broken in that force and strip are not applying sub-ass* as they should per the doc. I think we ignore than and proceed. I will report it to mpv when I get a chance.

iina/Base.lproj/Localizable.strings Outdated
@@ -167,6 +167,12 @@
"preference.sub_override_level.strip" = "strip";
"preference.sub_override_level.yes" = "yes";

"preference.sub_override_level.descriptive_text.no" = "No override: render subtitles as specified by the subtitle scripts.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The No override prefix is inconsistent with the other descriptions. For this one I would use the exact description from the mpv manual:
Render subtitles as specified by the subtitle scripts, without overrides.

iina/Base.lproj/Localizable.strings Outdated
@@ -167,6 +167,12 @@
"preference.sub_override_level.strip" = "strip";
"preference.sub_override_level.yes" = "yes";

"preference.sub_override_level.descriptive_text.no" = "No override: render subtitles as specified by the subtitle scripts.";
"preference.sub_override_level.descriptive_text.force" = "Only allow override using the --sub-ass-* mpv options.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not what force does. This sounds more like what yes does. Maybe something like:
Apply all subtitle styles.

iina/Base.lproj/Localizable.strings Outdated
@@ -167,6 +167,12 @@
"preference.sub_override_level.strip" = "strip";
"preference.sub_override_level.yes" = "yes";

"preference.sub_override_level.descriptive_text.no" = "No override: render subtitles as specified by the subtitle scripts.";
"preference.sub_override_level.descriptive_text.force" = "Only allow override using the --sub-ass-* mpv options.";
"preference.sub_override_level.descriptive_text.scale" = "Allow adjusting scale of ASS subtitles and override using the --sub-ass-* mpv options.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is desirable to keep IINA high level and avoid explicitly mentioning mpv options if we can. This description should build on the description for yes. See my comment below on yes. Maybe something like:
Apply the scale setting in addition to the style settings in this section.

Unfortunately since we only support setting the scale on the quick setting panel this might be a little confusing to users. We may want to add scale support in settings to make this easier for a user to grasp and allow users to be able to change the scale in a way that persists.

iina/Base.lproj/Localizable.strings Outdated
"preference.sub_override_level.descriptive_text.no" = "No override: render subtitles as specified by the subtitle scripts.";
"preference.sub_override_level.descriptive_text.force" = "Only allow override using the --sub-ass-* mpv options.";
"preference.sub_override_level.descriptive_text.scale" = "Allow adjusting scale of ASS subtitles and override using the --sub-ass-* mpv options.";
"preference.sub_override_level.descriptive_text.strip" = "Allow adjusting scale and style of ASS subtitles.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not what strip does. For strip I would use the mpv manual text:
Radically strip all ASS tags and styles from the subtitle.

@low-batt
Copy link
Contributor Author

Changes look good to me.

@low-batt low-batt merged commit 034e25b into develop Aug 14, 2024
1 check passed
@uiryuu uiryuu deleted the sub-ass-override-2 branch August 17, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sub ass style overrides always applied Second ASS Subtitle keep its own style.
2 participants
@low-batt @uiryuu