From 6f3966cc69350e8ee8de35868e7f3e101a4b1fda Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 29 Aug 2026 10:56:44 -0500 Subject: [PATCH] W3 (#156): the screen wiring, and a narrower hazard than the ticket claimed The stateful outer composables hand `ConverterScreenContent` and `JoinScreenContent` a list of `viewModel::` references. No test in the suite had ever seen that list: the content tests build their own `ConverterActions`, so they drive the stateless inner and never touch the wiring. THE TICKET'S PREMISE WAS HALF WRONG, AND CHECKING BEAT ASSUMING. #156 was filed claiming a transposition of any two of seventeen bindings would survive the suite. Measured instead of trusted: onVideoCodec <-> onAudioCodec -> REJECTED: "Inapplicable candidate(s): fun setAudioCodec(codec: AudioCodec)" onCancel <-> onReset -> COMPILES Every typed binding -- container, both codecs, preset, suggestion, quality, engine preference -- takes a distinct parameter type, so the compiler is already the test. Writing assertions against those transpositions would have been theatre, and this file says so rather than quietly including them. WHAT IS ACTUALLY AT RISK is the `() -> Unit` bindings, which are interchangeable to the compiler: two on the converter screen (onCancel, onReset) and *three* on the join screen (onJoin, onCancel, onReset). A Cancel button that discards the finished file, a Start-over that leaves it on screen, or a Join button that cancels -- each is one wrong word and each ships. I got that wrong in the first check too: an early run reported the onCancel/ onReset swap as rejected, from a grep-and-exit-code test that misread a stale build. Re-running it properly printed BUILD SUCCESSFUL with the swap in place. THE SEAM. `converterActions(viewModel, onPickInput, onConvert, onSave)` and `joinActions(viewModel, onPickInputs, onSave)`. The launcher-backed actions stay parameters -- they need an ActivityResultLauncher, which is the part that genuinely needs a composition, and keeping them out means the rest needs none. Told apart by effect rather than by a recording double: `reset()` sets the state to Idle, `cancel()` with no active job leaves it alone (`activeWorkId?.let`, which SettingsEditsTest pins). Mutations -- every transposition caught, each by two tests: converter onCancel <-> onReset | 2 tests join onJoin <-> onCancel | 2 tests join onReset <-> onCancel | 2 tests a typed binding dropped to {} | 1 test a launcher action rerouted | 1 test The two-test symmetry is deliberate: one direction alone passes against a wiring with BOTH actions bound to the same method, which is what a copy-pasted line produces. Three guard assertions earned their place during writing -- the picks land through an injected dispatcher, and without `ParkedPickDispatcher.runAll()` all three state-based tests sat on Idle and would have asserted nothing. They failed loudly instead of passing quietly. `@UnstableApi` on both builders, per CLAUDE.md; lint caught their absence, as it did in W1. 525 -> 537 tests, 88.0% -> 88.9% line, 70.5% -> 75.4% branch. Gate green. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConverterScreen.kt | 57 ++++- .../libremediaconverter/join/JoinScreen.kt | 29 ++- .../convert/ScreenWiringTest.kt | 241 ++++++++++++++++++ 3 files changed, 313 insertions(+), 14 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/ScreenWiringTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index 250a31d..155c8de 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -94,24 +94,61 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode state = state, settings = settings, validation = validation, - actions = ConverterActions( + actions = converterActions( + viewModel = viewModel, 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, ) } +/** + * Which of the ViewModel's methods each affordance on the screen calls. + * + * ## Why this is a function rather than an argument list + * + * It was an argument list, inside [ConverterScreen], which no test reached: `ConverterScreenContent` + * builds its own [ConverterActions], so every test in the suite drove the stateless inner and none + * of them ever saw the wiring. + * + * Most of the list is safe without a test, and saying so is more useful than pretending otherwise: + * `onContainer`, `onVideoCodec`, `onAudioCodec`, `onPreset`, `onSuggestion`, `onQuality` and + * `onEnginePreference` each take a distinct type, so binding one to another's setter does not + * compile. Verified rather than assumed — swapping `onVideoCodec` and `onAudioCodec` fails with + * *"Inapplicable candidate(s): fun setAudioCodec(codec: AudioCodec)"*. + * + * **[ConverterActions.onCancel] and [ConverterActions.onReset] are the exception.** Both are + * `() -> Unit`, so swapping them compiles silently — also verified — and ships a Cancel button that + * throws the conversion away and a Start-over button that leaves it on screen. That pair is what + * `ConverterWiringTest` exists for. + * + * The three launcher-backed actions stay parameters: they need an `ActivityResultLauncher`, which + * is the part that genuinely needs the composition, and keeping them out means the rest can be + * checked without one. + */ +@UnstableApi +internal fun converterActions( + viewModel: ConversionViewModel, + onPickInput: () -> Unit, + onConvert: () -> Unit, + onSave: (suggestedName: String) -> Unit, +): ConverterActions = ConverterActions( + onPickInput = onPickInput, + onPreset = viewModel::setPreset, + onContainer = viewModel::setContainer, + onVideoCodec = viewModel::setVideoCodec, + onAudioCodec = viewModel::setAudioCodec, + onSuggestion = viewModel::applySuggestion, + onQuality = viewModel::setQuality, + onEnginePreference = viewModel::setEnginePreference, + onConvert = onConvert, + onCancel = viewModel::cancel, + onSave = onSave, + onReset = viewModel::reset, +) + /** * Everything [ConverterScreenContent] can ask for, in one value. * diff --git a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt index a30b9e9..ce65b58 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt @@ -56,17 +56,38 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod JoinScreenContent( state = state, - actions = JoinActions( + actions = joinActions( + viewModel = viewModel, onPickInputs = { pickInputs.launch(arrayOf("video/*")) }, - onJoin = viewModel::join, - onCancel = viewModel::cancel, onSave = { suggestedName -> chooseDestination.launch(suggestedName) }, - onReset = viewModel::reset, ), modifier = modifier, ) } +/** + * Which of the ViewModel's methods each affordance on the join screen calls. + * + * The join-side twin of `converterActions`, and the transposition risk here is worse: **three** + * `() -> Unit` bindings rather than two. `onJoin`, `onCancel` and `onReset` are mutually + * interchangeable as far as the compiler is concerned, so a Join button that cancels, or a Cancel + * button that starts the job, is a swap nothing but a test would catch. + * + * See `converterActions` for why the launcher-backed actions stay parameters. + */ +@UnstableApi +internal fun joinActions( + viewModel: JoinViewModel, + onPickInputs: () -> Unit, + onSave: (suggestedName: String) -> Unit, +): JoinActions = JoinActions( + onPickInputs = onPickInputs, + onJoin = viewModel::join, + onCancel = viewModel::cancel, + onSave = onSave, + onReset = viewModel::reset, +) + /** * Everything [JoinScreenContent] can ask for, in one value. * diff --git a/app/src/test/java/org/libremediaconverter/convert/ScreenWiringTest.kt b/app/src/test/java/org/libremediaconverter/convert/ScreenWiringTest.kt new file mode 100644 index 0000000..3767ffe --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ScreenWiringTest.kt @@ -0,0 +1,241 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.workDataOf +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.join.joinActions +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * That each affordance is wired to the ViewModel method it is named after. + * + * ## What this covers that no other test can + * + * `ConverterScreenContentTest`, `ConverterStateAffordancesTest` and `JoinScreenContentTest` all + * drive the **stateless** content composables, which build their own `ConverterActions`. So the + * wiring — the list of `viewModel::` references the stateful outer hands down — was seen by nothing + * in the suite. + * + * ## The hazard is narrower than "seventeen bindings", and this says so + * + * #156 was filed claiming a transposition of any two bindings would survive the suite. That is not + * true, and it was worth checking rather than testing on the assumption: + * + * | swap | result | + * |---|---| + * | `onVideoCodec` ↔ `onAudioCodec` | **rejected by the compiler** | + * | `onCancel` ↔ `onReset` | **compiles** | + * + * Every typed binding — container, both codecs, preset, suggestion, quality, engine preference — + * takes a distinct parameter type, so the compiler is already the test. Writing assertions for + * those would be theatre. + * + * **The `() -> Unit` bindings are the real gap**, because they are interchangeable to the compiler: + * two on the converter screen (`onCancel`, `onReset`) and three on the join screen (`onJoin`, + * `onCancel`, `onReset`). A Cancel that discards the finished file, or a Join that cancels, is a + * one-character mistake that ships. + * + * ## How they are told apart + * + * By effect, not by a recording double. `reset()` sets the state to `Idle`; `cancel()` with no + * active job leaves it alone (`ConversionViewModel.cancel` is `activeWorkId?.let(...)`, and + * `SettingsEditsTest` pins that). Driving each from a non-`Idle` state is therefore enough to say + * which one ran. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ScreenWiringTest { + + private lateinit var app: Application + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + ConversionDependencies.publisher = { RecordingPublisher(app) } + ConversionDependencies.probe = { _, _ -> org.libremediaconverter.model.InputProbe() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + // --- the converter screen ---------------------------------------------- + + @Test + fun `Start over resets, and Cancel does not`() { + // The transposition that compiles. If onReset were bound to cancel, this stays on Ready. + installTestWorkManager(app, Data.EMPTY) + val pick = ParkedPickDispatcher() + val viewModel = ConversionViewModel(app, pickDispatcher = pick) + val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {}) + viewModel.onInputPicked(INPUT_URI) + pick.runAll() + assertNotEquals( + "the fixture needs a non-Idle state or neither action is observable", + ConversionState.Idle, + viewModel.state.value, + ) + + actions.onReset() + + assertEquals(ConversionState.Idle, viewModel.state.value) + } + + @Test + fun `Cancel leaves the picked file on screen`() { + // The other half. Without it, a wiring with BOTH actions bound to reset passes the test + // above -- and that is exactly what a copy-paste of the wrong line produces. + installTestWorkManager(app, Data.EMPTY) + val pick = ParkedPickDispatcher() + val viewModel = ConversionViewModel(app, pickDispatcher = pick) + val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {}) + viewModel.onInputPicked(INPUT_URI) + pick.runAll() + val before = viewModel.state.value + + actions.onCancel() + + assertEquals( + "Cancel must not throw away the pick the way Start over does", + before, + viewModel.state.value, + ) + } + + @Test + fun `each settings affordance reaches the setting it is named after`() { + // The typed bindings. The compiler already rejects a transposition among these, so this is + // not that assertion -- it is the cheaper one that each is bound to *something*, and that a + // binding dropped to `{}` during an edit would be caught. + installTestWorkManager(app, Data.EMPTY) + val pick = ParkedPickDispatcher() + val viewModel = ConversionViewModel(app, pickDispatcher = pick) + val actions = converterActions(viewModel, onPickInput = {}, onConvert = {}, onSave = {}) + + actions.onPreset(OutputFormat.WEBM_VP9) + assertEquals(OutputFormat.WEBM_VP9.spec, viewModel.settings.value.spec) + + actions.onContainer(Container.MKV) + assertEquals(Container.MKV, viewModel.settings.value.spec.container) + + actions.onVideoCodec(VideoCodec.H264) + assertEquals(VideoCodec.H264, viewModel.settings.value.spec.videoCodec) + + actions.onAudioCodec(AudioCodec.FLAC) + assertEquals(AudioCodec.FLAC, viewModel.settings.value.spec.audioCodec) + + actions.onQuality(QualityTier.BEST) + assertEquals(QualityTier.BEST, viewModel.settings.value.quality) + + actions.onEnginePreference(EnginePreference.FORCE_SOFTWARE) + assertEquals(EnginePreference.FORCE_SOFTWARE, viewModel.settings.value.enginePreference) + + actions.onSuggestion(OutputFormat.MP4_H264.spec) + assertEquals(OutputFormat.MP4_H264.spec, viewModel.settings.value.spec) + } + + @Test + fun `the launcher-backed actions are the ones the screen supplies`() { + // Not wired to the ViewModel at all, deliberately -- they need an ActivityResultLauncher. + // Asserted so that a later edit routing one of them at the ViewModel is noticed. + installTestWorkManager(app, Data.EMPTY) + val pick = ParkedPickDispatcher() + val viewModel = ConversionViewModel(app, pickDispatcher = pick) + val called = mutableListOf() + val actions = converterActions( + viewModel, + onPickInput = { called += "pick" }, + onConvert = { called += "convert" }, + onSave = { called += "save:$it" }, + ) + + actions.onPickInput() + actions.onConvert() + actions.onSave("holiday.mp4") + + assertEquals(listOf("pick", "convert", "save:holiday.mp4"), called) + } + + // --- the join screen, where three are interchangeable ------------------- + + @Test + fun `Start over resets the join, and Cancel does not`() { + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null")) + val pick = ParkedPickDispatcher() + val viewModel = JoinViewModel(app, pickDispatcher = pick) + val actions = joinActions(viewModel, onPickInputs = {}, onSave = {}) + viewModel.onInputsPicked(TWO_INPUTS) + pick.runAll() + assertTrue( + "the fixture needs a non-Idle state: ${viewModel.state.value}", + viewModel.state.value !is JoinState.Idle, + ) + + actions.onReset() + + assertEquals(JoinState.Idle, viewModel.state.value) + } + + @Test + fun `Cancel leaves the picked files on screen`() { + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null")) + val pick = ParkedPickDispatcher() + val viewModel = JoinViewModel(app, pickDispatcher = pick) + val actions = joinActions(viewModel, onPickInputs = {}, onSave = {}) + viewModel.onInputsPicked(TWO_INPUTS) + pick.runAll() + val before = viewModel.state.value + + actions.onCancel() + + assertEquals(before, viewModel.state.value) + } + + @Test + fun `Join starts the job rather than cancelling or resetting it`() { + // The third of the join screen's interchangeable trio, and the one whose transposition is + // worst: a Join button bound to cancel does nothing at all, which reads as a dead button. + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null")) + val pick = ParkedPickDispatcher() + val viewModel = JoinViewModel(app, pickDispatcher = pick) + val actions = joinActions(viewModel, onPickInputs = {}, onSave = {}) + viewModel.onInputsPicked(TWO_INPUTS) + pick.runAll() + + actions.onJoin() + + assertTrue( + "Join must leave Ready for a running state, not sit still and not go Idle: " + + "${viewModel.state.value}", + viewModel.state.value is JoinState.Joining || viewModel.state.value is JoinState.Joined, + ) + } + + private companion object { + val INPUT_URI: Uri = Uri.parse("content://test/holiday.mov") + val TWO_INPUTS = listOf( + Uri.parse("content://test/a.mp4"), + Uri.parse("content://test/b.mp4"), + ) + } +}