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) <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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<Destination, String> = 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))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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"))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user