From 46ad95350b8c1c8f0187ba0949e27fa0605480da Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 19:06:26 -0500 Subject: [PATCH] Give both screens somewhere for a state to come from `ConverterScreen` and `JoinScreen` each inlined their whole `when (state)` inside the public entry point, and state arrived only as `viewModel.state`. That left four of the twelve state branches across the two screens with no test that could ever reach them: driving a real ViewModel needs a WorkManager and a media probe in the constructor, and even then `Waiting` follows a denied foreground start and `Converted`/`Joined` follow a worker run that has already succeeded. So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`, which take the state, the settings, the validation and an actions holder. The entry points keep the three launchers and `collectAsStateWithLifecycle` and nothing else. The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose parameters because detekt's `LongParameterList` sits at its default threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` -- `AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn a clean detekt run red; data classes are exempt from the rule. Nothing else changed. The body was cut and pasted rather than retyped, so the U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable, and deliberately kept -- survives the move. The diff stops above `FormatPicker` in one file and above `FileRow` in the other, which is why the leaf suites #57-#60 landed pass unedited: every one of them composes a leaf directly and none references either entry point. The two new tests are the bite, one per screen and one per direction of the seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back the name the finished job chose. The state matrix itself is #62 and #63. --- .../convert/ConverterScreen.kt | 117 +++++++++++++++--- .../libremediaconverter/join/JoinScreen.kt | 58 +++++++-- .../convert/ConverterScreenContentTest.kt | 115 +++++++++++++++++ .../join/JoinScreenContentTest.kt | 83 +++++++++++++ 4 files changed, 346 insertions(+), 27 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/ConverterScreenContentTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/join/JoinScreenContentTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index bfdf9c8..09f2312 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -87,6 +87,89 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode ActivityResultContracts.RequestPermission(), ) { viewModel.convert() } + ConverterScreenContent( + state = state, + settings = settings, + validation = validation, + actions = ConverterActions( + onPickInput = { pickInput.launch(arrayOf("*/*")) }, + onPreset = viewModel::setPreset, + onContainer = viewModel::setContainer, + onVideoCodec = viewModel::setVideoCodec, + onAudioCodec = viewModel::setAudioCodec, + onSuggestion = viewModel::applySuggestion, + onQuality = viewModel::setQuality, + onEnginePreference = viewModel::setEnginePreference, + onConvert = { requestNotifications.launch(Manifest.permission.POST_NOTIFICATIONS) }, + onCancel = viewModel::cancel, + onSave = { suggestedName -> chooseDestination.launch(suggestedName) }, + onReset = viewModel::reset, + ), + modifier = modifier, + ) +} + +/** + * Everything [ConverterScreenContent] can ask for, in one value. + * + * A holder rather than twelve parameters because detekt's `LongParameterList` sits at its default + * threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` the way it + * relaxes `LongMethod` and `CyclomaticComplexMethod` -- `AdvancedPicker` already sits exactly on + * that threshold. The rule exempts data classes, so the callbacks travel together. + * + * In production every one of these is a launcher or a `ConversionViewModel` call. Naming them here + * instead of handing the content a ViewModel is the whole point of the seam: a test can render a + * [ConversionState] no ViewModel can be driven into, since `Waiting` needs a denied foreground + * start and `Converted` needs a worker run that has already succeeded. + */ +internal data class ConverterActions( + /** Open the document picker. The `Idle` and `Ready` branches both offer it. */ + val onPickInput: () -> Unit, + val onPreset: (OutputFormat) -> Unit, + val onContainer: (Container) -> Unit, + val onVideoCodec: (VideoCodec) -> Unit, + val onAudioCodec: (AudioCodec) -> Unit, + val onSuggestion: (OutputSpec) -> Unit, + val onQuality: (QualityTier) -> Unit, + val onEnginePreference: (EnginePreference) -> Unit, + /** + * Start the job. It asks for the notification permission first, which is why the screen never + * calls `convert` directly -- the launcher's result callback does, whichever way it went. + */ + val onConvert: () -> Unit, + val onCancel: () -> Unit, + /** + * Open the save dialog for the finished output. + * + * Takes the suggested name rather than reading it back off the state, because the name comes + * from the job -- see `ConversionWorker.KEY_SUGGESTED_NAME` -- and the branch that renders the + * button is the only place that has it. + */ + val onSave: (suggestedName: String) -> Unit, + val onReset: () -> Unit, +) + +/** + * The converter screen, with its state handed in. + * + * Split from [ConverterScreen] so that state has somewhere to come from other than a live + * `ConversionViewModel`. Driving the screen through a real one needs a `WorkManager` and a media + * probe in the constructor, and even then two of the six states are unreachable: `Waiting` follows + * a denied foreground start and `Converted` follows a completed worker. + * + * `internal` rather than private, because `src/test` is a friend of `main` and this is what the + * state tests compose. The leaves below stay exactly where they were -- this function is a move, + * not a redesign, and the tests that already pin those leaves are what says so. + */ +@UnstableApi +@Composable +internal fun ConverterScreenContent( + state: ConversionState, + settings: ConversionSettings, + validation: Validation, + actions: ConverterActions, + modifier: Modifier = Modifier, +) { Column( modifier = modifier .fillMaxSize() @@ -115,7 +198,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode modifier = Modifier.padding(bottom = 16.dp), ) Button( - onClick = { pickInput.launch(arrayOf("*/*")) }, + onClick = actions.onPickInput, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) @@ -132,21 +215,19 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode is ConversionState.Ready -> { FileCard(s.input) - FormatPicker(settings.matchingPreset, viewModel::setPreset) + FormatPicker(settings.matchingPreset, actions.onPreset) AdvancedPicker( spec = settings.spec, validation = validation, - onContainer = viewModel::setContainer, - onVideoCodec = viewModel::setVideoCodec, - onAudioCodec = viewModel::setAudioCodec, - onSuggestion = viewModel::applySuggestion, + onContainer = actions.onContainer, + onVideoCodec = actions.onVideoCodec, + onAudioCodec = actions.onAudioCodec, + onSuggestion = actions.onSuggestion, ) - QualityPicker(settings.quality, viewModel::setQuality) - EnginePicker(settings.enginePreference, viewModel::setEnginePreference) + QualityPicker(settings.quality, actions.onQuality) + EnginePicker(settings.enginePreference, actions.onEnginePreference) Button( - onClick = { - requestNotifications.launch(Manifest.permission.POST_NOTIFICATIONS) - }, + onClick = actions.onConvert, // The Advanced picker lets an impossible combination be selected on // purpose, so this is what stops it from being run. enabled = validation.isValid, @@ -156,7 +237,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode .testTag(TestTags.Converter.CONVERT), ) { Text("Convert") } OutlinedButton( - onClick = { pickInput.launch(arrayOf("*/*")) }, + onClick = actions.onPickInput, modifier = Modifier .fillMaxWidth() .testTag(TestTags.Converter.CHOOSE_DIFFERENT_FILE), @@ -173,7 +254,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode .testTag(TestTags.Converter.PROGRESS), ) OutlinedButton( - onClick = viewModel::cancel, + onClick = actions.onCancel, modifier = Modifier.fillMaxWidth().testTag(TestTags.CANCEL), ) { Text("Cancel") } } @@ -192,7 +273,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode style = MaterialTheme.typography.bodyMedium, ) OutlinedButton( - onClick = viewModel::cancel, + onClick = actions.onCancel, modifier = Modifier.fillMaxWidth().testTag(TestTags.CANCEL), ) { Text("Cancel") } } @@ -211,14 +292,14 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode AssistChip(onClick = {}, label = { Text(s.routeReason) }) } Button( - onClick = { chooseDestination.launch(s.suggestedName) }, + onClick = { actions.onSave(s.suggestedName) }, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) .testTag(TestTags.SAVE_FILE), ) { Text("Save file") } OutlinedButton( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER), ) { Text("Start over") } } @@ -226,7 +307,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode is ConversionState.Saved -> { Text("Saved ${s.displayName}.", style = MaterialTheme.typography.bodyLarge) Button( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) @@ -241,7 +322,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode style = MaterialTheme.typography.bodyMedium, ) Button( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) diff --git a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt index a247d65..765ac1b 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt @@ -53,6 +53,46 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) }, ) { uri -> uri?.let(viewModel::save) } + JoinScreenContent( + state = state, + actions = JoinActions( + onPickInputs = { pickInputs.launch(arrayOf("video/*")) }, + onJoin = viewModel::join, + onCancel = viewModel::cancel, + onSave = { suggestedName -> chooseDestination.launch(suggestedName) }, + onReset = viewModel::reset, + ), + modifier = modifier, + ) +} + +/** + * Everything [JoinScreenContent] can ask for, in one value. + * + * Five callbacks would fit under detekt's `LongParameterList` threshold, unlike the converter's + * twelve. It is a holder anyway, so both screens present the same shape to the state tests and + * neither one has to be reworked the first time a branch grows a button. + */ +internal data class JoinActions( + /** Open the multi-document picker. The `Idle` and `Ready` branches both offer it. */ + val onPickInputs: () -> Unit, + val onJoin: () -> Unit, + val onCancel: () -> Unit, + /** Open the save dialog. Takes the name the job chose -- see `ConcatWorker.KEY_SUGGESTED_NAME`. */ + val onSave: (suggestedName: String) -> Unit, + val onReset: () -> Unit, +) + +/** + * The join screen, with its state handed in. + * + * The same split as [org.libremediaconverter.convert.ConverterScreenContent], for the same reason: + * `JoinState.Waiting` follows a denied foreground start and `JoinState.Joined` follows a completed + * concatenation, so neither is reachable by driving a real `JoinViewModel`. + */ +@UnstableApi +@Composable +internal fun JoinScreenContent(state: JoinState, actions: JoinActions, modifier: Modifier = Modifier) { Column( modifier = modifier .fillMaxSize() @@ -81,7 +121,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod modifier = Modifier.padding(bottom = 16.dp), ) Button( - onClick = { pickInputs.launch(arrayOf("video/*")) }, + onClick = actions.onPickInputs, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) @@ -99,14 +139,14 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod is JoinState.Ready -> { s.inputs.forEach { FileRow(it) } Button( - onClick = viewModel::join, + onClick = actions.onJoin, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) .testTag(TestTags.Join.JOIN), ) { Text("Join ${s.inputs.size} files") } OutlinedButton( - onClick = { pickInputs.launch(arrayOf("video/*")) }, + onClick = actions.onPickInputs, modifier = Modifier .fillMaxWidth() .testTag(TestTags.Join.CHOOSE_DIFFERENT_FILES), @@ -124,7 +164,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod .testTag(TestTags.Join.PROGRESS), ) OutlinedButton( - onClick = viewModel::cancel, + onClick = actions.onCancel, modifier = Modifier.fillMaxWidth().testTag(TestTags.CANCEL), ) { Text("Cancel") } } @@ -138,7 +178,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod style = MaterialTheme.typography.bodyMedium, ) OutlinedButton( - onClick = viewModel::cancel, + onClick = actions.onCancel, modifier = Modifier.fillMaxWidth().testTag(TestTags.CANCEL), ) { Text("Cancel") } } @@ -157,14 +197,14 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod style = MaterialTheme.typography.bodySmall, ) Button( - onClick = { chooseDestination.launch(s.suggestedName) }, + onClick = { actions.onSave(s.suggestedName) }, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) .testTag(TestTags.SAVE_FILE), ) { Text("Save file") } OutlinedButton( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER), ) { Text("Start over") } } @@ -172,7 +212,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod is JoinState.Saved -> { Text("Saved ${s.displayName}.", style = MaterialTheme.typography.bodyLarge) Button( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) @@ -187,7 +227,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod style = MaterialTheme.typography.bodyMedium, ) Button( - onClick = viewModel::reset, + onClick = actions.onReset, modifier = Modifier .fillMaxWidth() .height(PrimaryButtonHeight) diff --git a/app/src/test/java/org/libremediaconverter/convert/ConverterScreenContentTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConverterScreenContentTest.kt new file mode 100644 index 0000000..3f377ba --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConverterScreenContentTest.kt @@ -0,0 +1,115 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.Validation +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner +import java.io.File + +/** + * The seam carries a `ConversionState` in and an action back out. + * + * The defect this bites on is the extraction having quietly stopped being an extraction: a + * `ConverterScreenContent` that ignores the `state` it was handed, or renders the finished job's + * affordances without wiring them to the callbacks the entry point supplies. Neither shows up at + * compile time -- an unread parameter compiles, and a `Button` whose `onClick` does nothing is a + * valid `Button` -- and neither is visible from the leaf tests, which compose `FileCard`, + * `AdvancedPicker` and the pickers directly and never see a state at all. + * + * **Both assertions were unreachable before R38.5**, which is the point of the ticket rather than + * a remark about it. `ConversionState.Converted` is produced only by a `ConversionWorker` run that + * has already succeeded, so no test can drive a real `ConversionViewModel` into it: it would need + * a `WorkManager`, a media probe, a staged output file and a completed job. Handing the state in + * is the only way to ask what the screen does with it. + * + * Deliberately not the state matrix. Which affordances each of the six `ConversionState`s renders + * is R38.6 (#62); this file asserts only that the injection point exists and works in both + * directions, so the two PRs cannot collide over the same cases. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConverterScreenContentTest { + + // Not `createComposeRule()` directly: see [org.libremediaconverter.drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + /** What the screen asked to save, in the order it asked. Empty until Save is tapped. */ + private val savedAs = mutableListOf() + + @Test + fun `a converted job renders the save button`() { + setContent(converted()) + + composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertExists() + } + + /** + * The direction that did not exist before this change. + * + * Asserting the *name* rather than just that something was called: the suggested name comes + * from the job -- `ConversionWorker.KEY_SUGGESTED_NAME` -- and is what the save dialog opens + * with, so a Save button wired to the wrong branch's state would hand over the wrong one and + * a bare "was called" check would stay green. + */ + @Test + fun `tapping save hands back the name the finished job chose`() { + setContent(converted()) + + composeRule.onNodeWithTag(TestTags.SAVE_FILE).performScrollTo().performClick() + + assertEquals(listOf("holiday.mp4"), savedAs) + } + + /** + * `staged` names a file that does not exist, on purpose. + * + * The branch renders `formatBytes(s.staged.length())`, and `length()` answers `0L` for a + * missing path rather than throwing, so the size line reads `0 B` and no temporary folder is + * needed. `routeReason` stays blank, which is what keeps the routing chip out of the tree -- + * that chip is R38.6's case, not this file's. + */ + private fun converted() = ConversionState.Converted( + input = InputFile( + uri = Uri.parse("content://test/holiday.mkv"), + displayName = "holiday.mkv", + sizeBytes = 12_345_678L, + ), + staged = File("no-such-staged-output.mp4"), + suggestedName = "holiday.mp4", + mimeType = "video/mp4", + ) + + private fun setContent(state: ConversionState) { + composeRule.setContent { + ConverterScreenContent( + state = state, + settings = ConversionSettings(), + validation = Validation.Valid, + actions = ConverterActions( + onPickInput = {}, + onPreset = {}, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + onQuality = {}, + onEnginePreference = {}, + onConvert = {}, + onCancel = {}, + onSave = { suggestedName -> savedAs += suggestedName }, + onReset = {}, + ), + ) + } + } +} diff --git a/app/src/test/java/org/libremediaconverter/join/JoinScreenContentTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinScreenContentTest.kt new file mode 100644 index 0000000..478e814 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinScreenContentTest.kt @@ -0,0 +1,83 @@ +package org.libremediaconverter.join + +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner +import java.io.File + +/** + * The join screen's half of the same seam, and the same two directions. + * + * The defect is the one `ConverterScreenContentTest` describes -- a content composable that + * ignores the state handed to it, or renders the finished job's affordances unwired -- and it has + * to be asked separately here because the two screens share no code. `JoinScreen` and + * `ConverterScreen` were extracted in the same commit by the same hand, which is exactly the + * circumstance in which one of them gets the wiring right and the other does not. + * + * `JoinState.Joined` is unreachable through a real `JoinViewModel` for the same reason + * `ConversionState.Converted` is: only a `ConcatWorker` run that has already succeeded produces + * one, carrying the strategy it chose and the name it picked. + * + * `JoinScreenKt` is the honest remaining coverage gap on this repo, and closing it is R38.7 (#63), + * not this file. Which affordances each `JoinState` renders belongs there; this asserts only that + * the injection point exists. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinScreenContentTest { + + // Not `createComposeRule()` directly: see [org.libremediaconverter.drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + /** What the screen asked to save, in the order it asked. Empty until Save is tapped. */ + private val savedAs = mutableListOf() + + @Test + fun `a finished join renders the save button`() { + setContent(joined()) + + composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertExists() + } + + @Test + fun `tapping save hands back the name the finished join chose`() { + setContent(joined()) + + composeRule.onNodeWithTag(TestTags.SAVE_FILE).performScrollTo().performClick() + + assertEquals(listOf("joined.mp4"), savedAs) + } + + /** `staged` names a missing file deliberately -- see the same helper on the converter side. */ + private fun joined() = JoinState.Joined( + staged = File("no-such-staged-output.mp4"), + strategy = ConcatStrategy.STREAM_COPY, + suggestedName = "joined.mp4", + mimeType = "video/mp4", + ) + + private fun setContent(state: JoinState) { + composeRule.setContent { + JoinScreenContent( + state = state, + actions = JoinActions( + onPickInputs = {}, + onJoin = {}, + onCancel = {}, + onSave = { suggestedName -> savedAs += suggestedName }, + onReset = {}, + ), + ) + } + } +}