Skip to content

[WIP] Subtitles Toggles #3053

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

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

WowbaggersLiquidLunch
Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch commented Jul 29, 2020


This is a draft pull request, wherein not all features are fully implemented yet. In addition to taking advantage of the CI, it acts as a reference point for progress-tracking, and allows everyone an early glimpse of the code itself. The planned features are as follows:

  • Add a subtitles state toggle that keeps subtitles visible but switches subtitles between user-selected tracks and the None tracks.
    • Add the toggling logic. (The groundwork is done by @unique-username313)
    • Bind its shortcut to the S key.
    • Provide the user with multiple ways/routes of toggling the subtitles state.
  • Add subtitles visibility toggle that keeps the subtitles' track selection but hides/unhides the subtitles' rendering.
    • Add the toggling logic.
    • Bind its shortcut to the V key.
    • Provide the user with multiple ways/routes of toggling the subtitles visibility.
  • Add preferences options for enabling/disabling either toggle features.
  • Prevent OSD messages from hiding each other.

slaps roof of pull request this bad boy can fit so much feature enhancement

WowbaggersLiquidLunch and others added 9 commits July 28, 2020 14:04
This is a very early version of the toggle. It sets subs to None when enabled, restores the previously selected tracks when disabled. Currently mapped as a default IINA command to the "Ctrl+v" shortcut and to the menu item "Disable Subtitles" under the Subtitles menu. It will update it's status automatically to On if the user manually sets both sub tracks to None. In that case, when the toggle is activated, it will set --sid=1 and --secondary-sid=0 by default. It will also automatically resets to Off whenever the user manually activates a subtitle track.
At the same time rename it from `disableSubtitles` to `toggleSubtitlesState`.

This commit also includes some non-functional changes automatically generated by Interface Builder.
Since `V` is mpv's subtitle visibility toggle, it's probably better to keep the key bindings as separate as possible. The plan is to have both visibility and state toggles in iina, and have them be user-configurable in preferences. The user can select which subtitle toggle features are enabled: neither, one, or both. The default setting is having the state toggle enabled and visibility toggle disabled.
Rename `subDisabled` to `subtitlesAreDisabled`, then change it to a computed property that equals to the negation of a new stored property `subtitlesAreEnabled`. The renaming brings it in line with Swift API guidelines. Favouring the positive version avoid double-negatives and make reasoning easier.

Consolidate sparsely located subtitles state-toggling functions into property observers for the subtitles state variable `subtitlesAreEnabled` and subtitles' track indices. This approach lowers coupling among different components.

This commit could've been separated into smaller commits, but Xcode has its limit in picking code snippet for staging.
@WowbaggersLiquidLunch WowbaggersLiquidLunch marked this pull request as draft July 29, 2020 05:25
@WowbaggersLiquidLunch
Copy link
Contributor Author

@unique-username313 Can I either merge or cherry-pick relevant commits from your subtitle-visibility branch, as the basis for the subtitles visibility toggle?

@unique-username313
Copy link
Contributor

@WowbaggersLiquidLunch Of course you can, but keep in mind that I haven't rebased that branch onto develop for some time now so you might have to deal with some conflicts.

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.

2 participants