Add subtitle offset possibility#5597
Conversation
|
I feel I need to add a 'help' box. Choosing + or - always confuses me. Some localization needs to be improved as well. I hope to make a new commit by Saturday. |
Done. |
|
Creating a help dialog box was a mistake: it was impossible to operate the buttons while the help text was visible. A help text is a much more logical approach. It also means less code, and less processing. Also, I realized that " s" would not always be correct to indicate seconds. In Cyrillic languages, that would be " с", for example. Therefore, I went a bit further with the localization. I uploaded a new APK as "release" in my forked repository. |
|
A friend told me that Claude AI is also capable of looking at an enitre repository, so I asked it to. It suggested a couple of things, which came to be these commits:
Some of these suggestions look logical even for me (extracting the @nielsvanvelzen, I do understand that all this AI interference is far from the ideal approach. So there surely might be a whole lot wrong with it. But there's a chance this actually leads somewhere, and I didn't want to run the risk of missing this feature. And I'm willing to make changes, obviously. So thanks in advance for your input! (I released my latest APK in my forked repository - for some reason the newest is below, not at the top of the list.) |
d097a9d to
4651671
Compare
|
I just discovered LLM/AI guidelines... In these AI times, I acknowledge the need for such a policy, and I understand the strictness that was chosen. I went through the commits, to check whether they don't touch anything that's not related to the intended added feature. I don't think they do. I did discover I had pushed a commit I hadn't intended to push, as the changes of that commit don't work. I removed it. (I also rebased on the updated master branch, while I was at it.) But I see I clearly violated rule 3 of the instructions. My apologies. I'm not capable of explaining many of the changes I suggest, and the bodies of my latest commits are indeed written by Claude. My expectations of this PR being merged, therefore dropped significantly. However, I want to stress that I had been missing this feature for a long time, and have used it a lot since I compiled the debug APK. So I hope that someone who does have enough background sees this PR and is able to submit it properly. I will probably keep using my own APK until then... |
If the synchronization of subtitles is off, there's now no way to make them appear earlier or later. This change introduces such a possibility. I didn't write the code of this commit, the credit goes to @pulpul-s. Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
I struggled deciding whether I needed to push the `+` or the `-` buttons, so I added a help dialog. I also noticed the number strings weren't subject to localization. For some reason, I had to increase the horizontal padding in the buttons, otherwise the "p" of "Help" would be pushed to the next line. Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
Creating a help dialog box was a mistake: it was impossible to operate the buttons while the help text was visible. A help text is a much more logical approach. It also means less code, and less processing. Also, I realized that " s" would not always be correct to indicate seconds. In Cyrillic languages, that would be " с", for example. Therefore, I went a bit further with the localization. Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
…Factory The factory previously constructed Cea608Decoder and Cea708Decoder by hand, passing internal constructor parameters (MIN_DATA_CHANNEL_TIMEOUT_MS, initializationData) that are part of the @UnstableApi surface. If Media3 ever changes those constructors — which it is explicitly allowed to do without notice — the build breaks silently. Those formats are already handled by SubtitleDecoderFactory.DEFAULT, so we can simply delegate to it. The manual branches in supportsFormat() are removed for the same reason: DEFAULT already returns true for CEA types, making them redundant. The net effect is that CEA-608/708 decoders are created exactly as before (through DEFAULT), while all other parser-backed formats continue to be handled by the SubtitleParser.Factory path. (This was all Claude's idea.) Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com> (cherry picked from commit 08bc321)
1. View elements ---------------- The dialog view was built entirely in Kotlin code, mixing layout decisions with business logic, hardcoding pixel math via density multiplications, and making the layout impossible to preview in Android Studio's layout editor. This commit introduces `res/layout/dialog_subtitle_offset.xml` and rewrites SubtitleOffsetPopup.kt to inflate it with ViewBinding. Business logic (key repeat, offset apply/reset, show/dismiss) is unchanged. Key improvements: - Dimensions live in dp in the XML; no more `(18 * density).toInt()` arithmetic. - The dialog background, text colors, and button styles are expressed as resource references, making them theme-aware. - The layout is previewable and easier to review in isolation. - SubtitleOffsetPopup.kt is significantly shorter and easier to follow. Note: also add new dimension to res/values/dimens.xml. 2. Offset formatter ------------------- Two issues addressed: 1. formatSubtitleOffset() was duplicated between SubtitleOffsetPopup.kt (legacy overlay) and VideoPlayerControls.kt (Compose player). The two implementations were nearly identical but operated on different types (Long microseconds vs kotlin.time.Duration). Extracted to a new SubtitleOffsetFormat.kt in the action package. 2. The last line of the private formatOffset() function used spaces instead of tabs, inconsistent with the rest of the file. This is fixed by removing the function entirely (replaced by the shared utility). (This was all Claude's idea.) Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com> (cherry picked from commit e3458b4)
…itting shared variable
The refactor that introduced SubtitleTimingOffsetRenderersFactory collapsed the
AssSubtitleParserFactory and DefaultSubtitleParserFactory into a single
`legacySubtitleParserFactory` variable typed as SubtitleParser.Factory, then
immediately cast it back to AssSubtitleParserFactory in the libass branch to
pass it to withAssMkvSupport().
The cast is safe today because the two `if (enableLibass)` branches are in sync,
but nothing enforces this — a future refactor could silently break it.
Fix: give each branch its own correctly-typed local variable so no cast is needed.
The small duplication of the apply{} block is the right trade-off: the type is
simply correct from construction, and the structural coupling disappears entirely.
The same pattern is fixed in both ExoPlayerBackend.kt and VideoManager.java.
(This was all Claude's idea.)
Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
(cherry picked from commit 1bc0aa0)
…gle source of truth VideoPlayerAdapter.hasTimingAdjustableSubtitle() duplicated the list of supported MIME types that is already declared in SubtitleTimingOffsetFormats.kt::isSubtitleTimingOffsetSupported(). If a new format were added to the engine layer, the UI gate would silently stay out of date and the "Subtitle offset" menu item would never appear for users of the new format. Fix: introduce a companion Kotlin helper VideoManagerHelper.getSubtitleMimeType() that VideoPlayerAdapter can call, and refactor hasTimingAdjustableSubtitle() to use SubtitleTimingOffsetFormats.isSubtitleTimingOffsetSupported() for the MIME-type check. The delivery-method guard (ENCODE/DROP) is correct and is left in place. Note: isSubtitleTimingOffsetSupported() lives in the :playback:media3:exoplayer module which is already a dependency of :app, so no new inter-module dependency is introduced. Note: this also requires a small change to SubtitleTimingOffsetFormats.kt to expose a string-based overload that VideoPlayerAdapter (Java) can call without constructing a full Format object. (This was all Claude's idea.) Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com> (cherry picked from commit 976c5a1)
Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
4651671 to
70ac9cc
Compare
Changes
This pull request enables offsetting subtitles, which is a big need in my opinion. It's also a feature request with more than 150 upvotes (https://features.jellyfin.org/posts/3036/subtile-offset-on-android-tv), and it's (part of) an open issue (#2479).
Currently the changed code has a "Subtitle offset" option in the subtitle menu if the selected subtitle supports offsetting (for example
srt). In that menu, there are buttons for+0.1s,-0.1s,+0.5s,-0.5sandreset. Theupanddownbuttons on the remote add an offsett of respectively+0.1sand-0.1s, or larger offset if they are kept pressed.The code isn't mine, but written by @pulpul-s. He (or she) deserves all the credit. But (s)he doesn't have enough time available to open up the PR. However, I think the feature is important enough to be added, so with @pulpul-s's permission, I'm opening this PR.
Code assistance
I asked GitHub's Copilot to take a look at the commit; if I'm not mistaken, my "conversation" with it can be read here: https://github.com/copilot/share/82231314-0904-80e5-9803-ec0120da2929. I also asked Claude to take a look: https://claude.ai/share/e068e7aa-def8-44d8-a726-a89708c4a4e2
One thing seems like it might be of value (in my low-value estimation):
Both agents saw some possible pitfall with
assSubtitleParserFactory. Copilot wanted a change it inExoPlayerBackend.kt, namely:Claude wanted to change something to
VideoManager.java:Either:
or:
Does anyone have any suggestions/feelings about this?
Of course, there might be other things that need improvement. Happy with any feedback!
Issues
#2479 (partly)