From ce4d0ff7d445cf93af7869a73247f01c76e474f4 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 19:32:33 -0500 Subject: [PATCH] Keep the selected tab through the recreation targetSdk 37 guarantees AppRoot held the selected tab in `remember`, which survives recomposition and nothing else. MainActivity declares no configChanges, so every rotation and every resize destroys and recreates the Activity, and the tab went back to Convert each time. The KDoc directly above that line is the argument for why it matters: from targetSdk 37 Android ignores screenOrientation, resizableActivity and the aspect-ratio limits on any display at least 600dp wide, and the Android 16 opt-out is gone, so the app is resized and rotated whether or not it is ready. The shell was written for that case and then lost its own state to it. Both ViewModels are Activity-scoped and come back intact, so a conversion in flight was never at risk -- only the tab, which is what makes this visibly wrong rather than merely stale. rememberSaveable, with a Saver that writes the constant's NAME. Three ways to make an enum saveable and the reasons for this one: - autoSaver already accepts it. An enum is Serializable, so plain `rememberSaveable { mutableStateOf(Destination.CONVERT) }` compiles, works, and passes the restoration test below unchanged. That is a reason to be explicit, not a reason not to be: nothing in the declaration says Destination has to stay Serializable, so the implicit route keeps working right up until someone makes it a value class or a sealed interface -- and then stops, silently, on a path only a rotation reaches. - The ordinal is a position, not an identity. Inserting a tab between Convert and Join would redefine every value already written down. A name only changes when someone renames a constant, which is an edit that shows up in a diff. It also reads as itself in a Bundle dump. - An unknown name restores to null, which rememberSaveable treats as "nothing saved" and falls back to Convert. That is exactly what a downgrade or a renamed constant leaves behind, and Convert is the right answer for it. The test runs on the JVM, which took two changes to reach. AppRoot and Destination are `internal` rather than `private` -- the unit test source set is a friend of main, so this stays invisible outside the module -- and AppRoot takes its `content` as a defaulted parameter instead of calling Content() directly. Nothing in the app passes it. It is there because both screens resolve a ViewModel, which builds a WorkManager and a media probe, and none of that has anything to do with which tab is selected; the test hands in a tagged Box and drives the shell alone. Content() stays private and is still what the app gets. compose-ui-test-junit4 joins the JVM test source set. It was already in the catalog for androidTest, it is inside the prerelease guard via its androidx. group, and its version comes from the BOM, so this adds no new pinning argument. It is there because createComposeRule() runs under Robolectric: a red test in androidTest is one nobody on this host can execute (CLAUDE.md), which is not a loop anyone can work in. ui-test-manifest is NOT repeated on that source set. It supplies the ComponentActivity the rule launches, and the existing debugImplementation entry already puts it in the merged manifest the unit tests build against -- checked by removing the line and watching AppRootRestorationTest stay green, rather than assumed. What the test does and does not prove. StateRestorationTester's emulateSavedInstanceStateRestore() disposes the composition and rebuilds it, so anything held only by `remember` is gone -- that is what makes it bite. It saves into an in-memory map rather than parcelling through a Bundle, so it cannot tell a name from an ordinal from autoSaver. The saved representation is pinned separately by three pure-JVM tests over the Saver itself, which is where the choice above is actually held down. Verified by writing it red first, against the restructured AppRoot with `remember` still in place: all three restoration tests failed at the post-restore assertion with "Expected exactly '1' node but could not find any node that satisfies: (TestTag = 'content:JOIN')", while every assertion before the restore -- including the bar's own selected state -- passed. Six new tests, 186 green in all. Not addressed here, and deliberately: MainActivity still declares no configChanges, and should not. Handling the configuration change is not the same as keeping one enum, and Compose's saved-state machinery is the mechanism the platform intends for it. Co-Authored-By: Claude Opus 5 (1M context) --- app/build.gradle.kts | 16 +++ .../org/libremediaconverter/MainActivity.kt | 59 ++++++++- .../AppRootRestorationTest.kt | 113 ++++++++++++++++++ .../DestinationSaverTest.kt | 45 +++++++ 4 files changed, 227 insertions(+), 6 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 5db1707..66bedbe 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -290,6 +290,22 @@ dependencies { // drive a ViewModel through a real WorkManager to SUCCEEDED, which is where the cleanup // handle is set -- the wiring the leak actually lived in. testImplementation(libs.androidx.work.testing) + // Compose's own test rules, on the JVM source set as well as androidTest. Already in the + // catalog, already inside the prerelease guard via its androidx. group, and versioned by + // the BOM, so this adds no new pinning argument. + // + // Here rather than only in androidTest because ui-test-junit4 runs under Robolectric: + // createComposeRule() drives a real composition on the JVM. The defect it was added for + // -- the selected tab not surviving recreation -- is caught by StateRestorationTester, + // and putting that test where the instrumented suite lives would mean nobody on this + // host could ever watch it go red. + // + // ui-test-manifest is deliberately NOT repeated here. It supplies the ComponentActivity + // the rule launches, and the debugImplementation entry below already puts it in the + // merged manifest the unit tests build against -- checked by removing it and watching + // the tests stay green. + testImplementation(platform(libs.compose.bom)) + testImplementation(libs.compose.ui.test.junit4) androidTestImplementation(platform(libs.compose.bom)) androidTestImplementation(libs.androidx.junit) diff --git a/app/src/main/java/org/libremediaconverter/MainActivity.kt b/app/src/main/java/org/libremediaconverter/MainActivity.kt index 6fd219f..b03f7c1 100644 --- a/app/src/main/java/org/libremediaconverter/MainActivity.kt +++ b/app/src/main/java/org/libremediaconverter/MainActivity.kt @@ -20,7 +20,8 @@ import androidx.compose.material3.windowsizeclass.calculateWindowSizeClass import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember +import androidx.compose.runtime.saveable.Saver +import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.media3.common.util.UnstableApi @@ -28,11 +29,41 @@ import org.libremediaconverter.convert.ConverterScreen import org.libremediaconverter.join.JoinScreen import org.libremediaconverter.ui.theme.LibreMediaConverterTheme -private enum class Destination(val label: String) { +/** + * The tabs of the adaptive shell. + * + * `internal` rather than `private` so the unit tests can name a tab. The JVM test source + * set is a friend of `main`, so this stays invisible to anything outside the module. + */ +internal enum class Destination(val label: String) { CONVERT("Convert"), JOIN("Join"), } +/** + * Saves a [Destination] as its constant name. + * + * A saver is needed at all because `rememberSaveable`'s default only accepts what a + * `Bundle` can hold. An enum does qualify -- it is `Serializable`, so `autoSaver` would take + * it without complaint -- and that is the reason to be explicit rather than the reason not + * to be: nothing in the declaration says this type has to stay `Serializable`, so the + * implicit route would keep working until someone made it a value class or a sealed + * interface, and then quietly stop. + * + * The name and not the ordinal. An ordinal is a position, so inserting a tab between the + * existing two would silently redefine every value already written down; the name only + * changes when someone renames a constant, which is a visible edit. It also reads as itself + * in a `Bundle` dump. + * + * An unknown name restores to null, which `rememberSaveable` treats as "nothing saved" and + * falls back to the default tab. That is the state a downgrade or a renamed constant + * produces, and landing on Convert is the right answer for it. + */ +internal val DestinationSaver: Saver = Saver( + save = { it.name }, + restore = { name -> Destination.entries.firstOrNull { it.name == name } }, +) + @UnstableApi class MainActivity : ComponentActivity() { @@ -57,12 +88,28 @@ class MainActivity : ComponentActivity() { * `resizableActivity` and aspect-ratio limits on any display at least 600dp wide, and * the Android 16 opt-out no longer applies. The app will be resized and rotated * whether or not it is ready, so it has to lay out properly at every width. + * + * [content] is a parameter with a default rather than a direct call to [Content] so a test + * can drive the shell -- which tab is selected, and whether that survives recreation -- + * without standing up either screen. Both screens resolve a ViewModel, which builds a + * WorkManager and a media probe, none of which the tab selection depends on. The app + * itself never passes it. */ @UnstableApi @OptIn(ExperimentalMaterial3Api::class) @Composable -private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { - var destination by remember { mutableStateOf(Destination.CONVERT) } +internal fun AppRoot( + widthSizeClass: WindowWidthSizeClass, + content: @Composable (Destination, Modifier) -> Unit = { destination, modifier -> + Content(destination, modifier) + }, +) { + // rememberSaveable, NOT remember. MainActivity declares no configChanges, so every + // rotation and every resize recreates it -- exactly the case the KDoc above says the + // shell exists for -- and remember does not survive that. + var destination by rememberSaveable(stateSaver = DestinationSaver) { + mutableStateOf(Destination.CONVERT) + } val useRail = widthSizeClass != WindowWidthSizeClass.Compact if (useRail) { @@ -78,7 +125,7 @@ private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { } } Scaffold(modifier = Modifier.fillMaxSize()) { padding -> - Content(destination, Modifier.padding(padding)) + content(destination, Modifier.padding(padding)) } } } else { @@ -97,7 +144,7 @@ private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { } }, ) { padding -> - Content(destination, Modifier.padding(padding)) + content(destination, Modifier.padding(padding)) } } } diff --git a/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt new file mode 100644 index 0000000..b0e1c78 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt @@ -0,0 +1,113 @@ +package org.libremediaconverter + +import androidx.compose.foundation.layout.Box +import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.assertIsSelected +import androidx.compose.ui.test.junit4.StateRestorationTester +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The selected tab has to survive activity recreation, not just recomposition. + * + * `remember` covers recomposition only, and `MainActivity` declares no `configChanges`, so + * every rotation and every resize destroys and recreates the Activity. That is the exact + * case [AppRoot]'s own KDoc says the shell exists for: from targetSdk 37 the app is resized + * and rotated whether or not it is ready. + * + * [StateRestorationTester] is the tool for it -- `emulateSavedInstanceStateRestore()` + * disposes the composition and rebuilds it, so anything held only by `remember` is gone and + * only saved state comes back. It is Compose's own stand-in for the recreation rather than + * the real thing: it saves into an in-memory map instead of parcelling through a `Bundle`, + * so it proves `rememberSaveable` is being used -- not that a particular saved + * representation survives a `Bundle` round trip. A JVM round-trip test on the + * saver covers the representation. + * + * Robolectric rather than the instrumented suite, deliberately. The instrumented tests + * cannot run on the development host at all (see CLAUDE.md), and a red test nobody can + * execute is not a loop anyone can work in. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AppRootRestorationTest { + + @get:Rule + val composeRule = createComposeRule() + + private val restoration = StateRestorationTester(composeRule) + + /** + * The stub screen is matched by a test tag rather than by text: the label on the bar + * ("Join") and the enum constant ("JOIN") differ only in case, and a matcher that could + * pick up either is not an assertion. + */ + private fun tagFor(destination: Destination) = "content:${destination.name}" + + private fun assertShowing(destination: Destination) { + composeRule.onNodeWithTag(tagFor(destination)).assertExists() + composeRule.onNodeWithText(destination.label).assertIsSelected() + } + + private fun setShell(width: () -> WindowWidthSizeClass) { + restoration.setContent { + AppRoot(width()) { destination, modifier -> + Box(modifier.testTag(tagFor(destination))) + } + } + } + + @Test + fun `the selected tab survives recreation on a phone`() { + setShell { WindowWidthSizeClass.Compact } + assertShowing(Destination.CONVERT) + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } + + @Test + fun `the selected tab survives recreation on the rail layout`() { + setShell { WindowWidthSizeClass.Expanded } + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } + + /** + * The real rotation: the width class changes across the recreation, so the shell comes + * back as a rail where it went out as a bottom bar. The tab still has to be the one the + * user chose. + */ + @Test + fun `the selected tab survives a rotation that also changes the width class`() { + var width by mutableStateOf(WindowWidthSizeClass.Compact) + setShell { width } + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + width = WindowWidthSizeClass.Expanded + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } +} diff --git a/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt b/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt new file mode 100644 index 0000000..3f13363 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt @@ -0,0 +1,45 @@ +package org.libremediaconverter + +import androidx.compose.runtime.saveable.SaverScope +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * What [AppRootRestorationTest] cannot see. + * + * `StateRestorationTester` saves into an in-memory map, so it proves the shell uses + * `rememberSaveable` and stops there -- it would be just as green if the saved value were an + * ordinal, or if the enum were left to `autoSaver`. The saved *representation* is a separate + * decision with separate consequences, and this is where it is pinned. + */ +class DestinationSaverTest { + + /** `canBeSaved` is the host registry's question; a String always can. */ + private val scope = SaverScope { true } + + private fun save(destination: Destination): Any? = with(DestinationSaver) { scope.save(destination) } + + @Test + fun `a destination is saved as its constant name, not its position`() { + // JOIN is ordinal 1. If this ever reads `1`, inserting a tab above it silently + // redefines every value already saved. + assertEquals("CONVERT", save(Destination.CONVERT)) + assertEquals("JOIN", save(Destination.JOIN)) + } + + @Test + fun `every destination survives the round trip`() { + Destination.entries.forEach { destination -> + assertEquals(destination, DestinationSaver.restore(save(destination) as String)) + } + } + + @Test + fun `a name no longer in the enum restores to nothing`() { + // A downgrade, or a renamed constant, leaves a name that no longer resolves. + // Returning null is what makes rememberSaveable fall back to the default tab + // instead of throwing on the way back from a rotation. + assertNull(DestinationSaver.restore("SETTINGS")) + } +}