fix: deduplicate secondary subtitle lines - #173
Open
kuator wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses duplicated text when collecting/combining secondary subtitles (e.g., when one subtitle expands and repeats the previous line), reducing the need for manual cleanup during export.
Changes:
- Add an optional
deduplicate_linesmode tosubtitles.sub_listto remove duplicated secondary lines while concatenating. - Enable line deduplication for secondary subtitle buffers in
subtitles/observer.lua. - Add basic regression tests for subtitle list concatenation behavior in
tests/run.lua.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mpvacious/subtitles/sub_list.lua |
Adds optional line-deduplication behavior when concatenating subtitle text. |
mpvacious/subtitles/observer.lua |
Enables the new deduplication mode for secondary subtitle collections used during export. |
tests/run.lua |
Adds subtitle list tests covering duplicated-line behavior for secondary subtitles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+14
to
+28
| local append_text = function(speech, seen, text) | ||
| if not deduplicate_lines then | ||
| table.insert(speech, text) | ||
| return | ||
| end | ||
| local normalized_text = text:gsub('\r\n', '\n'):gsub('\r', '\n') | ||
| for line in (normalized_text .. '\n'):gmatch('(.-)\n') do | ||
| if line == '' or not seen[line] then | ||
| table.insert(speech, line) | ||
| if line ~= '' then | ||
| seen[line] = true | ||
| end | ||
| end | ||
| end | ||
| end |
Comment on lines
+48
to
+52
| local secondary_subs = sub_list.new(true) | ||
| secondary_subs.insert(first) | ||
| secondary_subs.insert(expanded) | ||
| assert(secondary_subs.get_text() == "First line\nSecond line") | ||
| assert(secondary_subs.get_n_text(first, 2) == "First line\nSecond line") |
kuator
force-pushed
the
fix/deduplicate-secondary-lines
branch
2 times, most recently
from
August 6, 2026 19:17
ae4c9a8 to
bc6748f
Compare
| } | ||
| h.assert_equals(pub.join_fields(new_note, old_note).SentKanji, old_note.SentKanji) | ||
|
|
||
| -- Equivalent subtitle punctuation must not duplicate a Yomitan sentence. |
Member
There was a problem hiding this comment.
Please don't mention fake clones of Rikaitan anywhere on the internet.
Read this: https://ajatt.top/blog/avoid-fake-clones-of-rikaitan.html
Contributor
Author
There was a problem hiding this comment.
Okay, I did check with rikaitan, it has the same issue
| local all_dialogs = sub_list.new() | ||
| local all_secondary_dialogs = sub_list.new() | ||
| local all_secondary_dialogs = sub_list.new(true) | ||
| local user_timings = timings.new() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| ------------------------------------------------------------ | ||
|
|
||
| print("Running subtitle list tests...") |
Member
There was a problem hiding this comment.
Subtitle list tests violate the module test convention. This code dumps assertions directly into the test runner. Each testable module should expose run_tests() and tests/run.lua should call module run_tests() only. Move these assertions into mpvacious/subtitles/sub_list.lua as sub_list.run_tests(), then call sub_list.run_tests() from tests/run.lua and main.lua.
Comment on lines
+59
to
+132
| print("Running subtitle list tests...") | ||
| local sub_list = require('subtitles.sub_list') | ||
| local first = Subtitle:new { text = "First line", start = 0, ['end'] = 2 } | ||
| local expanded = Subtitle:new { text = "First line\nSecond line", start = 1, ['end'] = 3 } | ||
| local secondary_subs = sub_list.new(true) | ||
| secondary_subs.insert(first) | ||
| secondary_subs.insert(expanded) | ||
| assert(secondary_subs.get_text() == "First line\nSecond line") | ||
| assert(secondary_subs.get_n_text(first, 2) == "First line\nSecond line") | ||
| local primary_subs = sub_list.new() | ||
| primary_subs.insert(first) | ||
| primary_subs.insert(expanded) | ||
| assert(primary_subs.get_text() == "First line\nFirst line\nSecond line") | ||
| assert(primary_subs.get_n_text(first, 2) == "First line\nFirst line\nSecond line") | ||
| local formatted_subs = sub_list.new(true) | ||
| formatted_subs.insert(Subtitle:new { text = "First line\n\nSecond line", start = 0, ['end'] = 2 }) | ||
| assert(formatted_subs.get_text() == "First line\n\nSecond line") | ||
| local repeated_subs = sub_list.new(true) | ||
| repeated_subs.insert(Subtitle:new { text = "Yes", start = 0, ['end'] = 1 }) | ||
| repeated_subs.insert(Subtitle:new { text = "No", start = 1, ['end'] = 2 }) | ||
| repeated_subs.insert(Subtitle:new { text = "Yes\nAgain", start = 2, ['end'] = 3 }) | ||
| assert(repeated_subs.get_text() == "Yes\nNo\nYes\nAgain") | ||
| local aligned_subs = sub_list.new(true) | ||
| aligned_subs.insert(Subtitle:new { text = "First line", start = 0, ['end'] = 1.5 }) | ||
| aligned_subs.insert(Subtitle:new { text = "First line\nSecond line", start = 1.5, ['end'] = 3 }) | ||
| aligned_subs.insert(Subtitle:new { text = "Outside", start = 4, ['end'] = 5 }) | ||
| assert(aligned_subs.get_overlapping_text(1, 3) == "First line Second line") | ||
| assert(aligned_subs.get_overlapping_text(3, 4) == "") | ||
| assert(aligned_subs.insert(Subtitle:new { text = "First line", start = 10, ['end'] = 11 })) | ||
| assert(not aligned_subs.insert(Subtitle:new { text = "First line", start = 10.1, ['end'] = 11.1 })) | ||
| print("subtitle list tests passed.") | ||
|
|
||
| ------------------------------------------------------------ | ||
|
|
||
| print("Running subtitle observer tests...") | ||
| local subs_observer = require('subtitles.observer') | ||
| subs_observer.import_subs { | ||
| Subtitle:new { text = "Japanese", start = 1, ['end'] = 3 }, | ||
| Subtitle:new { text = "First line", start = 0, ['end'] = 1.5, is_secondary = true }, | ||
| Subtitle:new { text = "First line\nSecond line", start = 1.5, ['end'] = 3, is_secondary = true }, | ||
| Subtitle:new { text = "Outside", start = 4, ['end'] = 5, is_secondary = true }, | ||
| } | ||
| local mined_sub = subs_observer.collect_from_current() | ||
| assert(mined_sub.text == "Japanese") | ||
| assert(mined_sub.secondary == "First line Second line") | ||
| print("subtitle observer tests passed.") | ||
|
|
||
| ------------------------------------------------------------ | ||
|
|
||
| print("Running quick subtitle observer tests...") | ||
| local mp = require('mp') | ||
| local properties = { | ||
| ['sub-text'] = "Japanese", | ||
| ['sub-start'] = 1, | ||
| ['sub-end'] = 3, | ||
| ['secondary-sub-text'] = "First line", | ||
| ['secondary-sub-start'] = 0, | ||
| ['secondary-sub-end'] = 1.5, | ||
| ['sub-delay'] = 0, | ||
| ['audio-delay'] = 0, | ||
| } | ||
| mp.get_property = function(name) return properties[name] end | ||
| mp.get_property_number = function(name) return properties[name] end | ||
| mp.get_property_native = function(name) return properties[name] end | ||
| subs_observer.clear_all_dialogs() | ||
| subs_observer.all_subs_until_now() | ||
| properties['secondary-sub-text'] = "First line\nSecond line" | ||
| properties['secondary-sub-start'] = 1.5 | ||
| properties['secondary-sub-end'] = 3 | ||
| subs_observer.all_subs_until_now() | ||
| local quick_sub = subs_observer.collect_from_all_dialogues(1) | ||
| assert(quick_sub.text == "Japanese") | ||
| assert(quick_sub.secondary == "First line Second line") | ||
| print("quick subtitle observer tests passed.") |
Member
There was a problem hiding this comment.
I've added the sub_list.run_tests() function on the master branch. Now you can add your tests there. Don't forget to squash your work then rebase it.
| end | ||
| return text:gsub('%s+', ' '):match('^%s*(.-)%s*$') | ||
| end | ||
| return normalize_plaintext(new_text), normalize_plaintext(old_text) |
Member
There was a problem hiding this comment.
This can be a separate PR. Easier to merge.
kuator
force-pushed
the
fix/deduplicate-secondary-lines
branch
from
August 7, 2026 17:59
bc6748f to
664c573
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There's a problem when text get duplicated between two subtitles. Then I have to remove the duplicated text manually which is a pain. The other thing that annoys me is if I have Japanese cue with, let's say, 3 corresponding english cues, I have to manually select these overlapping english cues. This mr simply grabs all the corresponding english cues that fall into the range of the japanese cue.