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

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()
+                    }
+                }
+            }
         }
     }

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 {
+                        subtitleSettings.applyTo(this)
-                            setStyle(it.toSubtitleStyle())
-                            setFixedTextSize(Dimension.SP, it.fontSize.toFloat())
-                            setBottomPaddingFraction(it.margin.toFloat() / 100f)
-                        }
                         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

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(
+        Column(
-            state = state,
-            horizontalAlignment = Alignment.Start,
-            verticalArrangement = Arrangement.spacedBy(0.dp),
-            contentPadding = PaddingValues(16.dp),
             modifier = Modifier.background(MaterialTheme.colorScheme.surfaceColorAtElevation(1.dp)),
         ) {
-            stickyHeader {
+            Text(
-                Text(
+                text = title,
-                    text = title,
+                style = MaterialTheme.typography.headlineSmall,
-                    style = MaterialTheme.typography.headlineSmall,
+                color = MaterialTheme.colorScheme.onSurface,
-                    color = MaterialTheme.colorScheme.onSurface,
+                textAlign = TextAlign.Center,
-                    textAlign = TextAlign.Center,
+                modifier =
-                    modifier =
+                    Modifier
-                        Modifier
+                        .fillMaxWidth()
-                            .fillMaxWidth()
+                        .padding(vertical = 8.dp),
-                            .padding(vertical = 8.dp),
+            )
-                )
+            LazyColumn(
-            }
+                state = state,
-            prefList.forEachIndexed { groupIndex, group ->
+                horizontalAlignment = Alignment.Start,
-                item {
+                verticalArrangement = Arrangement.spacedBy(0.dp),
-                    Text(
+                contentPadding = PaddingValues(16.dp),
-                        text = stringResource(group.title),
+            ) {
-                        style = MaterialTheme.typography.titleMedium,
+                prefList.forEachIndexed { groupIndex, group ->
-                        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<SubtitlePreferences, Any>
                     item {
-                        val interactionSource = remember { MutableInteractionSource() }
+                        Text(
-                        val focused = interactionSource.collectIsFocusedAsState().value
+                            text = stringResource(group.title),
-                        LaunchedEffect(focused) {
+                            style = MaterialTheme.typography.titleMedium,
-                            if (focused) {
+                            color = MaterialTheme.colorScheme.onSurface,
-                                focusedIndex = Pair(groupIndex, prefIndex)
+                            textAlign = TextAlign.Start,
-                                onFocus.invoke(groupIndex, prefIndex)
+                            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<SubtitlePreferences, Any>
+                        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)
+                                }
                             }
-                        }
+                            if (preferences.useSeparateHdr && pref == SubtitleSettings.SeparateHdr) {
-                        when (pref) {
+                                LaunchedEffect(Unit) {
-                            SubtitleSettings.Reset -> {
+                                    bringIntoViewRequester.bringIntoView()
-                                ClickPreference(
+                                }
-                                    title = stringResource(pref.title),
-                                    onClick = {
-                                        scope.launch(ExceptionHandler()) {
-                                            val newValue =
-                                                SubtitlePreferences
-                                                    .newBuilder()
-                                                    .apply { resetSubtitles() }
-                                                    .build()
-                                            onPreferenceChange.invoke(newValue)
-                                        }
-                                    },
-                                    interactionSource = interactionSource,
-                                )
                             }
-
+                            when (pref) {
-                            else -> {
+                                SubtitleSettings.Reset -> {
-                                val value = pref.getter.invoke(preferences)
+                                    ClickPreference(
-                                ComposablePreference(
+                                        title = stringResource(pref.title),
-                                    preference = pref,
+                                        onClick = {
-                                    value = value,
+                                            scope.launch(ExceptionHandler()) {
-                                    onNavigate = viewModel.navigationManager::navigateTo,
+                                                val newValue =
-                                    onValueChange = { newValue ->
+                                                    SubtitlePreferences
-                                        val validation = pref.validate(newValue)
+                                                        .newBuilder()
-                                        when (validation) {
+                                                        .apply { resetSubtitles() }
-                                            is PreferenceValidation.Invalid -> {
+                                                        .build()
-                                                // TODO?
+                                                onPreferenceChange.invoke(newValue)
-                                                Toast
-                                                    .makeText(
-                                                        context,
-                                                        validation.message,
-                                                        Toast.LENGTH_SHORT,
-                                                    ).show()
                                             }
+                                        },
+                                        interactionSource = interactionSource,
+                                    )
+                                }
 
-                                            PreferenceValidation.Valid -> {
+                                else -> {
-                                                scope.launch(ExceptionHandler()) {
+                                    val value = pref.getter.invoke(preferences)
-                                                    onPreferenceChange.invoke(
+                                    ComposablePreference(
-                                                        pref.setter(
+                                        preference = pref,
-                                                            preferences,
+                                        value = value,
-                                                            newValue,
+                                        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,
-                                    interactionSource = interactionSource,
+                                        modifier =
-                                    modifier =
+                                            Modifier
-                                        Modifier
+                                                .ifElse(
-                                            .ifElse(
+                                                    groupIndex == focusedIndex.first && prefIndex == focusedIndex.second,
-                                                groupIndex == focusedIndex.first && prefIndex == focusedIndex.second,
+                                                    Modifier.focusRequester(focusRequester),
-                                                Modifier.focusRequester(focusRequester),
+                                                ).bringIntoViewRequester(bringIntoViewRequester),
-                                            ),
+                                    )
-                                )
+                                }
                             }
                         }
                     }

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<SubtitlePreferences>(
+            title = R.string.different_for_hdr,
+            defaultValue = false,
+            getter = { it.useSeparateHdr },
+            setter = { prefs, value ->
+                prefs.update { useSeparateHdr = value }
+            },
+        )
+
     val HdrSettings =
         AppDestinationPreference<SubtitlePreferences>(
             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
+    }
+}

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) {
+            remember(isPageForHdrSettings, display) {
-                if (!hdrSettings && SubtitleSettings.shouldShowHdr(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

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 {

app/src/main/res/values/strings.xml

@@ -828,4 +828,5 @@
     <string name="root_folder">Root folder</string>
     <string name="popularity">Popularity</string>
     <string name="production_status">Production status</string>
+    <string name="different_for_hdr">Use different style for HDR</string>
 </resources>

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)
+    }
+}