Skip to content

fix: group subtitles by language - #3127

Open
refirestream wants to merge 1 commit into
recloudstream:masterfrom
Sarlay:fix/subtitle-language-grouping
Open

fix: group subtitles by language#3127
refirestream wants to merge 1 commit into
recloudstream:masterfrom
Sarlay:fix/subtitle-language-grouping

Conversation

@refirestream

Copy link
Copy Markdown
  • Group subtitle tracks by language name derived from IETF tag instead of raw originalName (e.g. "fr" and "French [SUB]" now land in the same "French" group)
  • Fall back to "Unknown" group when language cannot be resolved
  • Label tracks within a group by originalName instead of nameSuffix
  • Filter subtitles by IETF tag match instead of name substring, matching primary subtag so regional variants (pt-br) match (pt)
  • Keep subtitles with unresolvable language instead of dropping them (goes into the Unknown group)

- Group subtitle tracks by language name derived from IETF tag
  instead of raw originalName (e.g. "fr" and "French [SUB]" now
  land in the same "French" group)
- Fall back to "Unknown" group when language cannot be resolved
- Label tracks within a group by originalName instead of nameSuffix
- Filter subtitles by IETF tag match instead of name substring,
  matching primary subtag so regional variants (pt-br) match (pt)
- Keep subtitles with unresolvable language instead of dropping them
- Only exempt DOWNLOADED_FILE from filtering so embedded tracks
  are also filtered across player reloads
- Use getApiProviderLangSettings + fromCodeToLangTagIETF for
  proper tag normalization in the filter list
@refirestream

Copy link
Copy Markdown
Author

Doing this pull request, I discovered that there is a setting to filter out "useless" languages in :
Settings > Player > Subtitles > "Filter by preferred media language".
I believe that should be enabled by default. What do you think ?

People that have not selected the correct languages during setup might be annoyed though. If we enable the "Filter by preferred media language" setting now, will it change the value for existing users ?

@refirestream

Copy link
Copy Markdown
Author

I tested the app manually and its working well

@fire-light42 fire-light42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good idea and minimal code changes, but needs some small changes.


val unknownGroupName = ctx.getString(R.string.subtitles_group_unknown)
fun groupName(sub: SubtitleData): String {
return fromTagToLanguageName(sub.getIETF_tag())?.takeIf { it.isNotBlank() }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would prefer if you save the results to a list instead of calling this function since:

  1. Calling the function again creates an assumption that the function will always return the same value, which can lead to bugs when refactoring in the future.

  2. I dislike calling potentially expensive functions unnecessarily.

.getOrNull(subtitleGroupIndex - 1)?.value?.map { subtitle ->
val nameSuffix = subtitle.nameSuffix.html()
nameSuffix.ifBlank {
val label = subtitle.originalName.html()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This completely breaks the subtitle suffix system and makes it very confusing to precisely select between different subtitles of the same language. Is there any reason for this change?

@fire-light42

Copy link
Copy Markdown
Collaborator

Doing this pull request, I discovered that there is a setting to filter out "useless" languages in :
Settings > Player > Subtitles > "Filter by preferred media language".
I believe that should be enabled by default. What do you think ?

Changing default behavior needs a lot of motivation, since users have come to expect that this is how CloudStream works. I think it would be too confusing. I would rather recommend a more subtle and visible solution, for example always placing preferred languages at the top of subtitle languages. However, this is best done as a separate pull request.

People that have not selected the correct languages during setup might be annoyed though. If we enable the "Filter by preferred media language" setting now, will it change the value for existing users ?

It would change the value for all users who have not previously changed the value, unless it is explicitly migrated using code.

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