diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 37358a2..4a0204b 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -6,6 +6,7 @@ import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi +import androidx.work.Data import androidx.work.WorkInfo import androidx.work.WorkManager import kotlinx.coroutines.CoroutineDispatcher @@ -71,6 +72,119 @@ data class InputFile( val probe: InputProbe? = null, ) +/** + * One update about a running conversion, as WorkManager last reported it. + * + * Only the fields [conversionStateFrom] reads — the same shape, and for the same reason, as + * `JobSnapshot` beside `Reattachment.choose`: the rule stays testable on the JVM because nothing + * in it needs a `WorkInfo`, which a test cannot readily build. + * + * [outputData] stays a `Data` rather than being unpacked into five nullable strings. It is what a + * test already builds with `workDataOf` everywhere in this suite, so unpacking would move the same + * reads without making anything easier to drive. + */ +internal data class ConversionUpdate( + val state: WorkInfo.State, + val progressPercent: Int, + val runAttemptCount: Int, + val outputData: Data, +) + +/** + * What the screen should show, given what WorkManager last said about the job. + * + * ## Why this is a function rather than the body of a `collect` + * + * It was the body of one. `workManager` is built in the constructor from `WorkManager.getInstance`, + * `observe` is private, and nothing could hand either a chosen `WorkInfo` — so every arm below ran + * only when a real worker happened to produce it. A real worker produces a terminal state with + * well-formed output, which meant six of these arms had never been chosen by any test: the progress + * read, both sides of the retry check, a success with no file, a failure with nothing to say, and + * the two that map to a state the user cannot otherwise reach. + * + * That is the argument #141 made for `MediaProbe`'s track walk, against `WorkManager` instead of a + * media fixture, and it takes the same answer: the branch matrix is a pure function, and what is + * left needing the framework — the flow, the null check, the ownership check — is the thin edge. + * + * ## What is deliberately *not* in here + * + * The ownership check stays at the call site. Its comment is explicit that it guards the file + * ownership the `SUCCEEDED` arm takes, not merely the assignment, so moving it inside would change + * what it protects. And this function takes no responsibility for the staged file: it returns the + * state, and the caller reads the file off it. A pure function that deletes files is not a seam. + * + * @param cancelled where a cancellation lands, which differs for a reattached job — see [observe]. + * @param fallbackSpec the current settings, read only when finished work predates the worker + * reporting its own name and MIME type. + */ +@UnstableApi +internal fun conversionStateFrom( + update: ConversionUpdate, + input: InputFile, + cancelled: ConversionState, + fallbackSpec: OutputSpec, +): ConversionState = when (update.state) { + WorkInfo.State.RUNNING -> ConversionState.Converting(input, update.progressPercent) + + // ENQUEUED after a run means a retry is pending. Either the six-hour foreground budget ran out + // mid-job, or the system refused to let the job start again while the app was in the background + // — the second being the likelier of the two, since it needs only a process restart. Nothing + // here can tell them apart, and nothing needs to: the answer is the same. + WorkInfo.State.ENQUEUED -> + if (update.runAttemptCount > 0) { + ConversionState.Waiting(input) + } else { + ConversionState.Converting(input, 0) + } + + WorkInfo.State.SUCCEEDED -> convertedFrom(update.outputData, input, fallbackSpec) + + // A worker that dies before it can report anything leaves no output data at all — a + // foreground-service start refused after a process restart is one way — and an exception's + // message can be an empty string. Both would read as a failure with nothing said, so blank + // falls back like missing does. + WorkInfo.State.FAILED -> ConversionState.Failed( + update.outputData.getString(ConversionWorker.KEY_ERROR) + ?.takeIf { it.isNotBlank() } + ?: ConversionWorker.GENERIC_FAILURE_MESSAGE, + ) + + WorkInfo.State.CANCELLED -> cancelled + WorkInfo.State.BLOCKED -> ConversionState.Converting(input, 0) +} + +/** + * The `SUCCEEDED` arm, which is the only one that reads more than one field. + * + * Split out so [conversionStateFrom] stays a table of one line per state. A success with no output + * path is a failure: the job said it finished and named nothing, and there is no file to offer. + */ +@UnstableApi +private fun convertedFrom(outputData: Data, input: InputFile, fallbackSpec: OutputSpec): ConversionState { + val path = outputData.getString(ConversionWorker.KEY_OUTPUT_PATH) + ?: return ConversionState.Failed(SUCCEEDED_WITHOUT_A_FILE_MESSAGE) + return ConversionState.Converted( + input = input, + staged = File(path), + engineUsed = outputData.getString(ConversionWorker.KEY_ENGINE_USED).orEmpty(), + routeReason = outputData.getString(ConversionWorker.KEY_ROUTE_REASON).orEmpty(), + // Work enqueued before the worker reported this carries nothing, and WorkManager keeps + // finished work for about a week -- so this branch is ordinary for a few days rather than a + // corner. It is the old derivation, kept because it is the same guess the app already made + // and there is genuinely nothing better available for such a job. New work never reaches it. + suggestedName = outputData.getString(ConversionWorker.KEY_SUGGESTED_NAME) + ?.takeIf { it.isNotBlank() } + ?: ConversionWorker.outputNameFor(input.displayName, fallbackSpec), + mimeType = outputData.getString(ConversionWorker.KEY_MIME_TYPE) + ?.takeIf { it.isNotBlank() } + ?: fallbackSpec.mimeType, + ) +} + +/** A job that reported success and named no file. There is nothing to offer the user to save. */ +internal const val SUCCEEDED_WITHOUT_A_FILE_MESSAGE: String = + "Conversion reported success but produced no file." + sealed interface ConversionState { data object Idle : ConversionState data class Ready(val input: InputFile) : ConversionState @@ -442,75 +556,24 @@ class ConversionViewModel @JvmOverloads constructor( // that either. The state and `pendingStaged` are meant to refer to the same file // or to no file, and this is where that stays true. if (!ownership.stillHeldBy(token)) return@collect - _state.value = when (info.state) { - WorkInfo.State.RUNNING -> ConversionState.Converting( - input, - info.progress.getInt(ConversionWorker.KEY_PROGRESS, 0), - ) - - // ENQUEUED after a run means a retry is pending. Either the six-hour - // foreground budget ran out mid-job, or the system refused to let the job - // start again while the app was in the background — the second being the - // likelier of the two, since it needs only a process restart. Nothing here - // can tell them apart, and nothing needs to: the answer is the same. - WorkInfo.State.ENQUEUED -> - if (info.runAttemptCount > 0) { - ConversionState.Waiting(input) - } else { - ConversionState.Converting(input, 0) - } - - WorkInfo.State.SUCCEEDED -> { - val path = info.outputData.getString(ConversionWorker.KEY_OUTPUT_PATH) - if (path == null) { - ConversionState.Failed("Conversion reported success but produced no file.") - } else { - val staged = File(path) - // Take responsibility for the file at the same moment the state - // starts referring to it, so the two cannot disagree. - pendingStaged = staged - ConversionState.Converted( - input = input, - staged = staged, - engineUsed = info.outputData - .getString(ConversionWorker.KEY_ENGINE_USED).orEmpty(), - routeReason = info.outputData - .getString(ConversionWorker.KEY_ROUTE_REASON).orEmpty(), - suggestedName = info.outputData - .getString(ConversionWorker.KEY_SUGGESTED_NAME) - ?.takeIf { it.isNotBlank() } - // Work enqueued before the worker reported this carries - // nothing, and WorkManager keeps finished work for about a - // week -- so this branch is ordinary for a few days rather - // than a corner. It is the old derivation, kept because it is - // the same guess the app already made and there is genuinely - // nothing better available for such a job. New work never - // reaches it. - ?: ConversionWorker.outputNameFor( - input.displayName, - _settings.value.spec, - ), - mimeType = info.outputData - .getString(ConversionWorker.KEY_MIME_TYPE) - ?.takeIf { it.isNotBlank() } - ?: _settings.value.spec.mimeType, - ) - } - } - - // A worker that dies before it can report anything leaves no output data at - // all — a foreground-service start refused after a process restart is one - // way — and an exception's message can be an empty string. Both would read - // as a failure with nothing said, so blank falls back like missing does. - WorkInfo.State.FAILED -> ConversionState.Failed( - info.outputData.getString(ConversionWorker.KEY_ERROR) - ?.takeIf { it.isNotBlank() } - ?: ConversionWorker.GENERIC_FAILURE_MESSAGE, - ) - - WorkInfo.State.CANCELLED -> cancelled - WorkInfo.State.BLOCKED -> ConversionState.Converting(input, 0) - } + val next = conversionStateFrom( + ConversionUpdate( + state = info.state, + progressPercent = info.progress.getInt(ConversionWorker.KEY_PROGRESS, 0), + runAttemptCount = info.runAttemptCount, + outputData = info.outputData, + ), + input = input, + cancelled = cancelled, + fallbackSpec = _settings.value.spec, + ) + // Take responsibility for the file at the same moment the state starts referring + // to it, so the two cannot disagree. Read off the result rather than assigned + // inside the mapping: `Converted` is the only state that carries a staged file, so + // "the state and `pendingStaged` refer to the same file or to no file" is now the + // shape of the code rather than a rule two branches have to keep. + if (next is ConversionState.Converted) pendingStaged = next.staged + _state.value = next } } } diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionStateMappingTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionStateMappingTest.kt new file mode 100644 index 0000000..5f81723 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionStateMappingTest.kt @@ -0,0 +1,238 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.WorkInfo +import androidx.work.workDataOf +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner + +/** + * Every answer [conversionStateFrom] can give, chosen rather than stumbled into. + * + * ## What this revises + * + * The mapping is not cold code and never was: `ConversionViewModel$observe$1$1` reported 28 covered + * lines before this file existed, because every test that drives a real worker runs it. What no + * test did was **choose which arm it took**. A real worker reaches a terminal state with + * well-formed output, so `SUCCEEDED`-with-a-path and `FAILED`-with-a-message were the only arms any + * test had ever produced — the other six ran never. + * + * A `grep` for `WorkInfo.State.` across the JVM suite makes that look untrue: all six constants are + * there. They are in `ReattachmentTest`, driven into **`Reattachment.choose`** — a different + * function that encodes the same enqueued-means-retry rule. So that rule had a test in one of its + * two homes, and the copy the user's screen reads had none. + * + * ## Why the seam, and why these assertions + * + * `WorkManager.getInstance` is called in the ViewModel's constructor and `observe` is private, so + * nothing could hand this a chosen `WorkInfo`. Cutting the `when` out as a pure function over + * [ConversionUpdate] is the answer #141 took for `MediaProbe`, and `JobSnapshot` beside + * `Reattachment.choose` is the same shape again. + * + * The assertions are on the whole state, not on its type. `Converting(input, 40)` and + * `Converting(input, 0)` are both `Converting`, and a mapping that dropped the progress read would + * pass any test that only asked which class came back. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConversionStateMappingTest { + + // --- running ------------------------------------------------------------ + + @Test + fun `a running job reports the progress it published`() { + // The percent is read from `progress`, not from `outputData`, and not from the settings. + // A mapping that returned Converting(input, 0) for every RUNNING would leave the bar + // pinned at zero for the whole conversion. + val state = map(WorkInfo.State.RUNNING, progress = 40) + + assertEquals(ConversionState.Converting(INPUT, 40), state) + } + + @Test + fun `a running job with no published progress reports zero rather than failing`() { + // getInt's default. A worker that has started but not yet called setProgress is ordinary, + // and must not read as an error. + val state = map(WorkInfo.State.RUNNING, progress = null) + + assertEquals(ConversionState.Converting(INPUT, 0), state) + } + + // --- enqueued: the rule that had a test only in its other home ---------- + + @Test + fun `an enqueued job that has already run is waiting to retry`() { + val state = map(WorkInfo.State.ENQUEUED, runAttemptCount = 1) + + assertEquals( + "an ENQUEUED after a run is a pending retry, which the user is told about", + ConversionState.Waiting(INPUT), + state, + ) + } + + @Test + fun `an enqueued job that has never run is simply starting`() { + // The other side, and the reason the test above is not enough on its own: a mapping that + // ignored runAttemptCount and always answered Waiting would pass that one and fail this. + val state = map(WorkInfo.State.ENQUEUED, runAttemptCount = 0) + + assertEquals(ConversionState.Converting(INPUT, 0), state) + } + + // --- succeeded ---------------------------------------------------------- + + @Test + fun `a success that named no file is a failure, not an empty success`() { + // The job said it finished and named nothing. There is no file to offer, so `Converted` + // would put a Save button over a path that does not exist. + val state = map(WorkInfo.State.SUCCEEDED, data = Data.EMPTY) + + assertEquals(ConversionState.Failed(SUCCEEDED_WITHOUT_A_FILE_MESSAGE), state) + } + + @Test + fun `a success carries the worker's own name and type, not the current settings`() { + val state = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to "/cache/conversions/out.mkv", + ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mkv", + ConversionWorker.KEY_MIME_TYPE to "video/x-matroska", + ConversionWorker.KEY_ENGINE_USED to "FFMPEG", + ConversionWorker.KEY_ROUTE_REASON to "container needs FFmpeg", + ), + ) + + val converted = state as ConversionState.Converted + assertEquals("holiday.mkv", converted.suggestedName) + assertEquals("video/x-matroska", converted.mimeType) + assertEquals("FFMPEG", converted.engineUsed) + assertEquals("container needs FFmpeg", converted.routeReason) + } + + @Test + fun `a success from older work falls back to the current settings for name and type`() { + // WorkManager keeps finished work about a week, so a job enqueued before the worker + // reported these is ordinary for a few days rather than a corner case. + val state = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/cache/conversions/out.mp4"), + ) + + val converted = state as ConversionState.Converted + assertEquals(FALLBACK_SPEC.mimeType, converted.mimeType) + assertEquals( + ConversionWorker.outputNameFor(INPUT.displayName, FALLBACK_SPEC), + converted.suggestedName, + ) + } + + @Test + fun `a blank name or type falls back the same way a missing one does`() { + // A blank string is not an answer. Without takeIf, the save dialog opens named "" and + // registered for a MIME type of "", which no provider will accept. + val state = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to "/cache/conversions/out.mp4", + ConversionWorker.KEY_SUGGESTED_NAME to "", + ConversionWorker.KEY_MIME_TYPE to " ", + ), + ) + + val converted = state as ConversionState.Converted + assertEquals(FALLBACK_SPEC.mimeType, converted.mimeType) + assertEquals( + ConversionWorker.outputNameFor(INPUT.displayName, FALLBACK_SPEC), + converted.suggestedName, + ) + } + + // --- failed ------------------------------------------------------------- + + @Test + fun `a failure carries the reason the worker gave`() { + val state = map( + WorkInfo.State.FAILED, + data = workDataOf(ConversionWorker.KEY_ERROR to "Not enough free space to convert."), + ) + + assertEquals(ConversionState.Failed("Not enough free space to convert."), state) + } + + @Test + fun `a failure with nothing said still says something`() { + // A worker killed before it could write output data leaves none at all -- a refused + // foreground start after a process restart is one way. Failed("") would render as a blank + // error card. + val state = map(WorkInfo.State.FAILED, data = Data.EMPTY) + + assertEquals(ConversionState.Failed(ConversionWorker.GENERIC_FAILURE_MESSAGE), state) + } + + @Test + fun `a failure whose message is blank falls back like a missing one`() { + val state = map( + WorkInfo.State.FAILED, + data = workDataOf(ConversionWorker.KEY_ERROR to " "), + ) + + assertEquals(ConversionState.Failed(ConversionWorker.GENERIC_FAILURE_MESSAGE), state) + } + + // --- cancelled and blocked --------------------------------------------- + + @Test + fun `a cancellation lands wherever the caller said it should`() { + // Not a fixed state: a conversion started here goes back to Ready with the picked file, + // while one picked up by reattach goes to Idle, because the URI that job holds belongs to + // a process that no longer exists. `observe`'s KDoc is where that distinction is set. + val toReady = map(WorkInfo.State.CANCELLED, cancelled = ConversionState.Ready(INPUT)) + val toIdle = map(WorkInfo.State.CANCELLED, cancelled = ConversionState.Idle) + + assertEquals(ConversionState.Ready(INPUT), toReady) + assertEquals(ConversionState.Idle, toIdle) + } + + @Test + fun `a blocked job looks like one that is starting`() { + // BLOCKED is a job waiting on a prerequisite. There is nothing useful to say about it that + // differs from "starting", and inventing a state for it would put a word on screen the + // user cannot act on. + val state = map(WorkInfo.State.BLOCKED) + + assertEquals(ConversionState.Converting(INPUT, 0), state) + } + + private fun map( + state: WorkInfo.State, + progress: Int? = null, + runAttemptCount: Int = 0, + data: Data = Data.EMPTY, + cancelled: ConversionState = ConversionState.Ready(INPUT), + ): ConversionState = conversionStateFrom( + ConversionUpdate( + state = state, + // Modelled on the call site, which reads `getInt(KEY_PROGRESS, 0)` -- so "no progress + // published" is the default reaching the mapping, not a null it has to handle. + progressPercent = progress ?: 0, + runAttemptCount = runAttemptCount, + outputData = data, + ), + input = INPUT, + cancelled = cancelled, + fallbackSpec = FALLBACK_SPEC, + ) + + private companion object { + val INPUT = InputFile(Uri.parse("content://test/holiday.mov"), "holiday.mov", 4096L) + val FALLBACK_SPEC = OutputFormat.MP4_H265.spec + } +}