Add feature-gated dual subtitle MVP with merged subtitle track#2549
Add feature-gated dual subtitle MVP with merged subtitle track#2549beng1z wants to merge 5 commits intorecloudstream:masterfrom
Conversation
- isDualSubtitleTrackSelectionEnabled: use intermediate variable to fix Elvis operator precedence (?: binds lower than &&), avoiding incorrect evaluation when the preference key is unset. - isDualSubtitleCombinationSupported in CS3IPlayer: accept nullable params and add null guards to match GeneratorPlayer's version, eliminating behavioral inconsistency between the two implementations.
- Faz 6: Add contentDescription to Primary/Secondary subtitle tab buttons in both landscape and portrait layouts - Faz 3: Add cleanDualSubtitleCache() to delete old dual_sub_*.vtt files on episode change and before new merge - Faz 5 Part 1: Add 6 new tests for DualSubtitleComposer (WebVTT output validity, primary/secondary-only, negative duration)
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
First round of comments, for the easiest to spot issues.
I will give you more once I can properly test it as the app reliably freezes when testing it.
We will not merge anything half-finished, as to keep CloudStream maintainable. We will therefore not merge a MVP. This feature will need to be polished before getting accepted.
|
|
||
| \n%s</string> | ||
| <string name="delete_message_series_section" formatted="true">رح كمان تمحي ل الأبد كل الحلقات اللي ب هيدا المسلسل؟ | ||
| \n |
There was a problem hiding this comment.
Why all of these removals?
There was a problem hiding this comment.
Most of that diff was unintended newline and whitespace churn from touching the locale file. In the follow-up I trimmed the locale changes back and removed the AI-added dual subtitle strings from translated files; for now the new keys are only kept in the base strings file.
| return Pair(listOf(merged.first), listOf(merged.second)) | ||
| } | ||
|
|
||
| event(ErrorEvent(ErrorLoadingException(context.getString(R.string.subtitles_secondary_load_failed)))) |
There was a problem hiding this comment.
These events cause an error loop wherein the player goes to the next source and the subtitles are loaded again. This freezes the app.
There was a problem hiding this comment.
I removed the ErrorEvent path here and changed it to clear the secondary selection and fall through to the normal single subtitle load path instead. That was meant to avoid the reload loop you pointed out.
|
|
||
| event(ErrorEvent(ErrorLoadingException(context.getString(R.string.subtitles_secondary_load_failed)))) | ||
| dualMergedTrackId = null | ||
| return Pair(emptyList(), emptyList()) |
There was a problem hiding this comment.
You should fall back to single subtitles if an error occurs.
There was a problem hiding this comment.
Changed this in the follow-up. Unsupported or failed dual subtitle cases now drop back to the existing single subtitle path instead of returning empty sources or surfacing a dual subtitle load error from here.
| private fun cleanDualSubtitleCache(context: Context, exclude: File? = null) { | ||
| try { | ||
| context.cacheDir.listFiles { file -> | ||
| file.name.startsWith("dual_sub_") && file.name.endsWith(".vtt") && file != exclude |
There was a problem hiding this comment.
The name should be in a constant, as to prevent issues when someone in the future renames the subtitle file. The subtitle files should preferably also be in its own folder to ease future debugging.
There was a problem hiding this comment.
Adjusted this. The generated files now live under a dedicated dual_subtitles cache subdirectory, and the dir, prefix, and extension are pulled into constants instead of being inlined.
| return try { | ||
| when (subtitle.origin) { | ||
| SubtitleOrigin.URL -> { | ||
| val mergedHeaders = mutableMapOf<String, String>().apply { |
There was a problem hiding this comment.
Why merged headers? Other subtitles do not have merged headers.
There was a problem hiding this comment.
Removed that header merge in the follow-up and switched this back to subtitle.headers only. I left the broader header and referer handling out of this PR.
| } | ||
|
|
||
| private fun isDualSubtitleTrackSelectionEnabled(): Boolean { | ||
| val enabled = getKey<Boolean>(SUBTITLE_DUAL_ENABLED_KEY) ?: false |
There was a problem hiding this comment.
WET code, use the function you defined
There was a problem hiding this comment.
Changed this to use player.isDualSubtitleCombinationSupported(...) and moved the shared check onto IPlayer and CS3IPlayer, so GeneratorPlayer no longer duplicates the logic.
|
|
||
| <string name="subtitles_dual_enabled">ଡୁଆଲ୍ ସବ୍ଟାଇଟ୍ (ପରୀକ୍ଷାମୂଳକ)</string> | ||
| <string name="subtitles_edit_primary">ପ୍ରାଥମିକ</string> | ||
| <string name="subtitles_edit_secondary">ଦ୍ Secondary ିତୀୟ</string> |
There was a problem hiding this comment.
Do not translate strings using AI, let real people do the translations.
There was a problem hiding this comment.
Removed the AI-added dual subtitle strings from the translated locale files in the follow-up. For now the new subtitle keys are only kept in the base strings file.
2 participants
Summary
offby default).What changed
IPlayerAPI expanded with secondary subtitle methods.CS3IPlayerimplements secondary state, dual merge pipeline, and merged track selection.GeneratorPlayerupdated for Primary/Secondary selection UX and Apply-based commit behavior.Dual subtitles (experimental)switch.DualSubtitleComposer+ unit tests.AI usage disclosure
Testing
./gradlew testStableDebugUnitTest assembleStableDebug --no-daemonpasses.Notes