From e5887dfa15108ec533fccae40af987fd555077b5 Mon Sep 17 00:00:00 2001 From: Damontecres Date: Wed, 24 Jun 2026 19:26:05 -0400 Subject: [PATCH] Better UX for separate SDR vs HDR subtitle styles (#1622) ## Description This PR moves the separated HDR subtitle style behind a toggle. This means users must now opt-in to using separated SDR vs HDR subtitle styles. This is a better UX for users who want to customize the style but use the same styles for both. Users who have changed the HDR subtitles previously will have the toggle enabled automatically. If the user hasn't changed HDR style or their SDR & HDR styles are the the same, the toggle will be disabled. Also, fixes a bug where when playback switched between SDR or HDR, the subtitle styles might not be updated. ### Related issues Closes #1603 ### Testing Emulator & unit tests for migration ## Screenshots N/A ## AI or LLM usage None --- .../wholphin/services/AppUpgradeHandler.kt | 9 + .../wholphin/ui/playback/PlaybackPage.kt | 19 +- .../subtitle/SubtitlePreferencesContent.kt | 204 +++++++++--------- .../preferences/subtitle/SubtitleSettings.kt | 49 ++++- .../preferences/subtitle/SubtitleStylePage.kt | 16 +- app/src/main/proto/WholphinDataStore.proto | 1 + app/src/main/res/values/strings.xml | 1 + .../wholphin/test/TestSubtitleHdrMigration.kt | 54 +++++ 8 files changed, 239 insertions(+), 114 deletions(-) create mode 100644 app/src/test/java/com/github/damontecres/wholphin/test/TestSubtitleHdrMigration.kt diff --git a/app/src/main/java/com/github/damontecres/wholphin/services/AppUpgradeHandler.kt b/app/src/main/java/com/github/damontecres/wholphin/services/AppUpgradeHandler.kt index 018b50ba..cc814210 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/services/AppUpgradeHandler.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/services/AppUpgradeHandler.kt @@ -29,6 +29,7 @@ import com.github.damontecres.wholphin.preferences.updateSearchPreferences import com.github.damontecres.wholphin.preferences.updateSubtitlePreferences import com.github.damontecres.wholphin.ui.preferences.PreferencesViewModel import com.github.damontecres.wholphin.ui.preferences.subtitle.SubtitleSettings +import com.github.damontecres.wholphin.ui.preferences.subtitle.shouldEnableSeparateHdrToggle import com.github.damontecres.wholphin.ui.setup.seerr.migrateSeerrUrl import com.github.damontecres.wholphin.util.Version import dagger.hilt.android.qualifiers.ApplicationContext @@ -409,5 +410,13 @@ class AppUpgradeHandler Timber.e(ex, "Error saving migrated tabs") } } + + if (previous.isEqualOrBefore(Version.fromString("1.0.2-8-g0"))) { + appPreferences.updateData { + it.updateSubtitlePreferences { + useSeparateHdr = it.interfacePreferences.shouldEnableSeparateHdrToggle() + } + } + } } } diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt index 6fe79290..e72dcc8a 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/playback/PlaybackPage.kt @@ -4,7 +4,6 @@ import android.view.Gravity import android.view.ViewGroup import android.widget.FrameLayout import androidx.activity.compose.BackHandler -import androidx.annotation.Dimension import androidx.annotation.OptIn import androidx.compose.animation.AnimatedVisibility import androidx.compose.animation.core.LinearEasing @@ -84,8 +83,8 @@ import com.github.damontecres.wholphin.ui.playback.overlay.PlaybackOverlay import com.github.damontecres.wholphin.ui.playback.overlay.SkipIndicator import com.github.damontecres.wholphin.ui.playback.overlay.SkipSegmentButton import com.github.damontecres.wholphin.ui.playback.overlay.rememberSeekBarState +import com.github.damontecres.wholphin.ui.preferences.subtitle.SubtitleSettings.applyTo import com.github.damontecres.wholphin.ui.preferences.subtitle.SubtitleSettings.calculateEdgeSize -import com.github.damontecres.wholphin.ui.preferences.subtitle.SubtitleSettings.toSubtitleStyle import com.github.damontecres.wholphin.ui.seasonEpisode import com.github.damontecres.wholphin.ui.tryRequestFocus import com.github.damontecres.wholphin.util.ExceptionHandler @@ -421,10 +420,13 @@ fun PlaybackPageContent( analyticsState = state.analyticsState, ) + var subtitleStyleAppliedForHdr by remember { mutableStateOf(state.currentMediaInfo.videoStream?.hdr == true) } val subtitleSettings = remember(state.currentMediaInfo) { Timber.v("subtitle choice: ${state.currentMediaInfo.videoStream?.hdr}") - if (state.currentMediaInfo.videoStream?.hdr == true) { + val useSeparate = + preferences.appPreferences.interfacePreferences.subtitlesPreferences.useSeparateHdr + if (useSeparate && state.currentMediaInfo.videoStream?.hdr == true) { preferences.appPreferences.interfacePreferences.hdrSubtitlesPreferences } else { preferences.appPreferences.interfacePreferences.subtitlesPreferences @@ -447,11 +449,7 @@ fun PlaybackPageContent( AndroidView( factory = { context -> SubtitleView(context).apply { - subtitleSettings.let { - setStyle(it.toSubtitleStyle()) - setFixedTextSize(Dimension.SP, it.fontSize.toFloat()) - setBottomPaddingFraction(it.margin.toFloat() / 100f) - } + subtitleSettings.applyTo(this) playerInstance.assHandler?.let { assHandler -> if (prefs.overrides.assPlaybackMode == AssPlaybackMode.ASS_LIBASS) { Timber.v("Adding AssSubtitleView") @@ -470,6 +468,11 @@ fun PlaybackPageContent( } }, update = { subtitleView -> + if (subtitleStyleAppliedForHdr != state.currentMediaInfo.videoStream?.hdr) { + // If the media has switched between SDR & HDR, reapply subtitle settings + Timber.v("Switching SDR/HDR, reapplying subtitle style") + subtitleSettings.applyTo(subtitleView) + } subtitleView.setCues(state.subtitleCues) if (state.subtitleCues.size > cueCount) { // The output creates a painter for each cue, so need to apply the changes when the number of cues increases diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitlePreferencesContent.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitlePreferencesContent.kt index 8abc6dbe..02f70d11 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitlePreferencesContent.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitlePreferencesContent.kt @@ -10,11 +10,14 @@ import androidx.compose.foundation.background import androidx.compose.foundation.interaction.MutableInteractionSource import androidx.compose.foundation.interaction.collectIsFocusedAsState import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.rememberLazyListState +import androidx.compose.foundation.relocation.BringIntoViewRequester +import androidx.compose.foundation.relocation.bringIntoViewRequester import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue @@ -79,112 +82,119 @@ fun SubtitlePreferencesContent( LaunchedEffect(Unit) { focusRequester.tryRequestFocus() } - LazyColumn( - state = state, - horizontalAlignment = Alignment.Start, - verticalArrangement = Arrangement.spacedBy(0.dp), - contentPadding = PaddingValues(16.dp), + Column( modifier = Modifier.background(MaterialTheme.colorScheme.surfaceColorAtElevation(1.dp)), ) { - stickyHeader { - Text( - text = title, - style = MaterialTheme.typography.headlineSmall, - color = MaterialTheme.colorScheme.onSurface, - textAlign = TextAlign.Center, - modifier = - Modifier - .fillMaxWidth() - .padding(vertical = 8.dp), - ) - } - prefList.forEachIndexed { groupIndex, group -> - item { - Text( - text = stringResource(group.title), - style = MaterialTheme.typography.titleMedium, - color = MaterialTheme.colorScheme.onSurface, - textAlign = TextAlign.Start, - modifier = - Modifier - .fillMaxWidth() - .padding(top = 8.dp, bottom = 4.dp), - ) - } - val groupPreferences = - group.preferences + - group.conditionalPreferences - .filter { it.condition.invoke(preferences) } - .map { it.preferences } - .flatten() - groupPreferences.forEachIndexed { prefIndex, pref -> - pref as AppPreference + Text( + text = title, + style = MaterialTheme.typography.headlineSmall, + color = MaterialTheme.colorScheme.onSurface, + textAlign = TextAlign.Center, + modifier = + Modifier + .fillMaxWidth() + .padding(vertical = 8.dp), + ) + LazyColumn( + state = state, + horizontalAlignment = Alignment.Start, + verticalArrangement = Arrangement.spacedBy(0.dp), + contentPadding = PaddingValues(16.dp), + ) { + prefList.forEachIndexed { groupIndex, group -> item { - val interactionSource = remember { MutableInteractionSource() } - val focused = interactionSource.collectIsFocusedAsState().value - LaunchedEffect(focused) { - if (focused) { - focusedIndex = Pair(groupIndex, prefIndex) - onFocus.invoke(groupIndex, prefIndex) + Text( + text = stringResource(group.title), + style = MaterialTheme.typography.titleMedium, + color = MaterialTheme.colorScheme.onSurface, + textAlign = TextAlign.Start, + modifier = + Modifier + .fillMaxWidth() + .padding(top = 8.dp, bottom = 4.dp), + ) + } + val groupPreferences = + group.preferences + + group.conditionalPreferences + .filter { it.condition.invoke(preferences) } + .map { it.preferences } + .flatten() + groupPreferences.forEachIndexed { prefIndex, pref -> + pref as AppPreference + item { + val interactionSource = remember { MutableInteractionSource() } + val bringIntoViewRequester = remember { BringIntoViewRequester() } + val focused = interactionSource.collectIsFocusedAsState().value + LaunchedEffect(focused) { + if (focused) { + focusedIndex = Pair(groupIndex, prefIndex) + onFocus.invoke(groupIndex, prefIndex) + } } - } - when (pref) { - SubtitleSettings.Reset -> { - ClickPreference( - title = stringResource(pref.title), - onClick = { - scope.launch(ExceptionHandler()) { - val newValue = - SubtitlePreferences - .newBuilder() - .apply { resetSubtitles() } - .build() - onPreferenceChange.invoke(newValue) - } - }, - interactionSource = interactionSource, - ) + if (preferences.useSeparateHdr && pref == SubtitleSettings.SeparateHdr) { + LaunchedEffect(Unit) { + bringIntoViewRequester.bringIntoView() + } } - - else -> { - val value = pref.getter.invoke(preferences) - ComposablePreference( - preference = pref, - value = value, - onNavigate = viewModel.navigationManager::navigateTo, - onValueChange = { newValue -> - val validation = pref.validate(newValue) - when (validation) { - is PreferenceValidation.Invalid -> { - // TODO? - Toast - .makeText( - context, - validation.message, - Toast.LENGTH_SHORT, - ).show() + when (pref) { + SubtitleSettings.Reset -> { + ClickPreference( + title = stringResource(pref.title), + onClick = { + scope.launch(ExceptionHandler()) { + val newValue = + SubtitlePreferences + .newBuilder() + .apply { resetSubtitles() } + .build() + onPreferenceChange.invoke(newValue) } + }, + interactionSource = interactionSource, + ) + } - PreferenceValidation.Valid -> { - scope.launch(ExceptionHandler()) { - onPreferenceChange.invoke( - pref.setter( - preferences, - newValue, - ), - ) + else -> { + val value = pref.getter.invoke(preferences) + ComposablePreference( + preference = pref, + value = value, + onNavigate = viewModel.navigationManager::navigateTo, + onValueChange = { newValue -> + val validation = pref.validate(newValue) + when (validation) { + is PreferenceValidation.Invalid -> { + // TODO? + Toast + .makeText( + context, + validation.message, + Toast.LENGTH_SHORT, + ).show() + } + + PreferenceValidation.Valid -> { + scope.launch(ExceptionHandler()) { + onPreferenceChange.invoke( + pref.setter( + preferences, + newValue, + ), + ) + } } } - } - }, - interactionSource = interactionSource, - modifier = - Modifier - .ifElse( - groupIndex == focusedIndex.first && prefIndex == focusedIndex.second, - Modifier.focusRequester(focusRequester), - ), - ) + }, + interactionSource = interactionSource, + modifier = + Modifier + .ifElse( + groupIndex == focusedIndex.first && prefIndex == focusedIndex.second, + Modifier.focusRequester(focusRequester), + ).bringIntoViewRequester(bringIntoViewRequester), + ) + } } } } diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleSettings.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleSettings.kt index b7f3d764..0fa5469f 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleSettings.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleSettings.kt @@ -4,6 +4,7 @@ import android.content.res.Configuration import android.graphics.Typeface import android.os.Build import android.view.Display +import androidx.annotation.Dimension import androidx.annotation.OptIn import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.toArgb @@ -12,6 +13,7 @@ import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp import androidx.media3.common.util.UnstableApi import androidx.media3.ui.CaptionStyleCompat +import androidx.media3.ui.SubtitleView import com.github.damontecres.wholphin.R import com.github.damontecres.wholphin.preferences.AppChoicePreference import com.github.damontecres.wholphin.preferences.AppClickablePreference @@ -20,9 +22,12 @@ import com.github.damontecres.wholphin.preferences.AppSliderPreference import com.github.damontecres.wholphin.preferences.AppSwitchPreference import com.github.damontecres.wholphin.preferences.BackgroundStyle import com.github.damontecres.wholphin.preferences.EdgeStyle +import com.github.damontecres.wholphin.preferences.InterfacePreferences import com.github.damontecres.wholphin.preferences.SubtitlePreferences +import com.github.damontecres.wholphin.preferences.resetSubtitles import com.github.damontecres.wholphin.ui.indexOfFirstOrNull import com.github.damontecres.wholphin.ui.nav.Destination +import com.github.damontecres.wholphin.ui.preferences.ConditionalPreferences import com.github.damontecres.wholphin.ui.preferences.PreferenceGroup import com.github.damontecres.wholphin.util.mpv.MPVLib import com.github.damontecres.wholphin.util.mpv.setPropertyColor @@ -245,6 +250,16 @@ object SubtitleSettings { setter = { prefs, _ -> prefs }, ) + val SeparateHdr = + AppSwitchPreference( + title = R.string.different_for_hdr, + defaultValue = false, + getter = { it.useSeparateHdr }, + setter = { prefs, value -> + prefs.update { useSeparateHdr = value } + }, + ) + val HdrSettings = AppDestinationPreference( title = R.string.hdr_subtitle_style, @@ -299,7 +314,14 @@ object SubtitleSettings { title = R.string.hdr, preferences = listOf( - HdrSettings, + SeparateHdr, + ), + conditionalPreferences = + listOf( + ConditionalPreferences( + condition = { it.useSeparateHdr }, + preferences = listOf(HdrSettings), + ), ), ), ) @@ -333,6 +355,13 @@ object SubtitleSettings { ) } + @OptIn(UnstableApi::class) + fun SubtitlePreferences.applyTo(view: SubtitleView) { + view.setStyle(toSubtitleStyle()) + view.setFixedTextSize(Dimension.SP, fontSize.toFloat()) + view.setBottomPaddingFraction(margin.toFloat() / 100f) + } + fun SubtitlePreferences.calculateEdgeSize(density: Density): Float = with(density) { (edgeThickness / 2f).dp.toPx() } fun SubtitlePreferences.applyToMpv( @@ -400,3 +429,21 @@ object SubtitleSettings { } inline fun SubtitlePreferences.update(block: SubtitlePreferences.Builder.() -> Unit): SubtitlePreferences = toBuilder().apply(block).build() + +fun InterfacePreferences.shouldEnableSeparateHdrToggle(): Boolean { + // If already separate, leave it + if (subtitlesPreferences.useSeparateHdr) return true + val defaultStyle = + SubtitlePreferences + .newBuilder() + .apply { + resetSubtitles() + }.build() + // If SDR and HDR are the same, don't separate + return if (subtitlesPreferences == hdrSubtitlesPreferences) { + false + } else { + // If different, but HDR has not been changed from original settings, don't separate + hdrSubtitlesPreferences != defaultStyle + } +} diff --git a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleStylePage.kt b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleStylePage.kt index 52b3570a..a96ab8fe 100644 --- a/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleStylePage.kt +++ b/app/src/main/java/com/github/damontecres/wholphin/ui/preferences/subtitle/SubtitleStylePage.kt @@ -51,7 +51,7 @@ import timber.log.Timber @Composable fun SubtitleStylePage( initialPreferences: AppPreferences, - hdrSettings: Boolean, + isPageForHdrSettings: Boolean, modifier: Modifier = Modifier, viewModel: PreferencesViewModel = hiltViewModel(), ) { @@ -67,7 +67,7 @@ fun SubtitleStylePage( if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { DisposableEffect(context) { - if (hdrSettings) { + if (isPageForHdrSettings) { Timber.v("Switching color mode to HDR") context.findActivity()?.window?.colorMode = ActivityInfo.COLOR_MODE_HDR } @@ -77,7 +77,7 @@ fun SubtitleStylePage( } } val prefs = - if (hdrSettings) { + if (isPageForHdrSettings) { preferences.interfacePreferences.hdrSubtitlesPreferences } else { preferences.interfacePreferences.subtitlesPreferences @@ -192,8 +192,8 @@ fun SubtitleStylePage( } val display = LocalView.current.display val prefList = - remember(hdrSettings, display) { - if (!hdrSettings && SubtitleSettings.shouldShowHdr(display)) { + remember(isPageForHdrSettings, display) { + if (!isPageForHdrSettings && SubtitleSettings.shouldShowHdr(display)) { // If not on HDR page and display is HDR capable, then show the HDR button SubtitleSettings.preferences + SubtitleSettings.hdrPreferenceGroup } else { @@ -202,13 +202,13 @@ fun SubtitleStylePage( } SubtitlePreferencesContent( title = - if (hdrSettings) { + if (isPageForHdrSettings) { stringResource(R.string.hdr_subtitle_style) } else { stringResource(R.string.subtitle_style) }, preferences = - if (hdrSettings) { + if (isPageForHdrSettings) { preferences.interfacePreferences.hdrSubtitlesPreferences } else { preferences.interfacePreferences.subtitlesPreferences @@ -217,7 +217,7 @@ fun SubtitleStylePage( onPreferenceChange = { newSubtitlePrefs -> viewModel.preferenceDataStore.updateData { it.updateInterfacePreferences { - if (hdrSettings) { + if (isPageForHdrSettings) { hdrSubtitlesPreferences = newSubtitlePrefs } else { subtitlesPreferences = newSubtitlePrefs diff --git a/app/src/main/proto/WholphinDataStore.proto b/app/src/main/proto/WholphinDataStore.proto index 12631e17..d2443c60 100644 --- a/app/src/main/proto/WholphinDataStore.proto +++ b/app/src/main/proto/WholphinDataStore.proto @@ -141,6 +141,7 @@ message SubtitlePreferences{ int32 margin = 11; int32 edge_thickness = 12; int32 image_subtitle_opacity = 13; + bool use_separate_hdr = 14; } message LiveTvPreferences { diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 3dbbe6de..dd999158 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -828,4 +828,5 @@ Root folder Popularity Production status + Use different style for HDR diff --git a/app/src/test/java/com/github/damontecres/wholphin/test/TestSubtitleHdrMigration.kt b/app/src/test/java/com/github/damontecres/wholphin/test/TestSubtitleHdrMigration.kt new file mode 100644 index 00000000..fec67c6b --- /dev/null +++ b/app/src/test/java/com/github/damontecres/wholphin/test/TestSubtitleHdrMigration.kt @@ -0,0 +1,54 @@ +package com.github.damontecres.wholphin.test + +import com.github.damontecres.wholphin.preferences.InterfacePreferences +import com.github.damontecres.wholphin.preferences.SubtitlePreferences +import com.github.damontecres.wholphin.preferences.resetSubtitles +import com.github.damontecres.wholphin.ui.preferences.subtitle.shouldEnableSeparateHdrToggle +import org.junit.Assert +import org.junit.Test + +class TestSubtitleHdrMigration { + private fun subtitles(block: SubtitlePreferences.Builder.() -> Unit): SubtitlePreferences = + SubtitlePreferences + .newBuilder() + .apply { + resetSubtitles() + block.invoke(this) + }.build() + + private fun shouldSeparate( + sdr: SubtitlePreferences, + hdr: SubtitlePreferences, + ): Boolean = + InterfacePreferences + .newBuilder() + .apply { + subtitlesPreferences = sdr + hdrSubtitlesPreferences = hdr + }.build() + .shouldEnableSeparateHdrToggle() + + @Test + fun `Test separate enabled`() { + val sdr = subtitles { fontSize = 60 } + val differentHdr = subtitles { fontSize = 30 } + val shouldSeparate = shouldSeparate(sdr, differentHdr) + Assert.assertTrue(shouldSeparate) + } + + @Test + fun `Test separate disabled`() { + val sdr = subtitles { fontSize = 60 } + val differentHdr = subtitles { fontSize = 60 } + val shouldSeparate = shouldSeparate(sdr, differentHdr) + Assert.assertFalse(shouldSeparate) + } + + @Test + fun `Test HDR unchanged`() { + val sdr = subtitles { fontSize = 60 } + val differentHdr = subtitles { } + val shouldSeparate = shouldSeparate(sdr, differentHdr) + Assert.assertFalse(shouldSeparate) + } +}