diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index b8344fe..37358a2 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -505,7 +505,7 @@ class ConversionViewModel @JvmOverloads constructor( WorkInfo.State.FAILED -> ConversionState.Failed( info.outputData.getString(ConversionWorker.KEY_ERROR) ?.takeIf { it.isNotBlank() } - ?: "Conversion failed.", + ?: ConversionWorker.GENERIC_FAILURE_MESSAGE, ) WorkInfo.State.CANCELLED -> cancelled @@ -565,7 +565,7 @@ class ConversionViewModel @JvmOverloads constructor( // than a fresh handle for the second failure's sake: a retry that fails again // lands back here still carrying the file, not on a bare Failed that would take // the offer away. - _state.value = ConversionState.Failed(e.message ?: "Could not save the file.", pending) + _state.value = ConversionState.Failed(e.message ?: SAVE_FAILED_MESSAGE, pending) } } } diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index d532697..7a1d2c1 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -24,6 +24,20 @@ const val STAGED_FILE_GONE_MESSAGE: String = "The finished file is no longer in the cache, so there is nothing left to save. " + "Start over to make it again." +/** + * What to tell the user when the copy into their chosen destination did not finish. + * + * A fallback, not the usual message: `publish` throws with a real reason most of the time — the + * volume filled, the provider revoked the grant — and that reason is better than this. This is for + * the exception that arrives with nothing to say, which would otherwise reach the screen as an + * empty failure. + * + * Kept next to [STAGED_FILE_GONE_MESSAGE] for exactly the reason that one names: **both ViewModels + * need it**, and saving is what it is about. It was written out twice before — `ConversionViewModel` + * and `JoinViewModel` each carried their own copy of the literal, agreeing by coincidence. + */ +const val SAVE_FAILED_MESSAGE: String = "Could not save the file." + /** * A staged file that is still there to be saved, and everything the save dialog needs to offer it. * diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 3d0cd2a..665d7d7 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -19,6 +19,7 @@ import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.convert.PendingSave +import org.libremediaconverter.convert.SAVE_FAILED_MESSAGE import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE import org.libremediaconverter.convert.ScreenOwnership import org.libremediaconverter.model.ConcatStrategy @@ -194,7 +195,7 @@ class JoinViewModel @JvmOverloads constructor( fun onInputsPicked(uris: List) { val token = ownership.claim() if (uris.size < 2) { - _state.value = JoinState.Failed("Pick at least two files to join.") + _state.value = JoinState.Failed(ConcatWorker.TOO_FEW_INPUTS_MESSAGE) return } viewModelScope.launch { @@ -295,7 +296,7 @@ class JoinViewModel @JvmOverloads constructor( WorkInfo.State.FAILED -> JoinState.Failed( info.outputData.getString(ConcatWorker.KEY_ERROR) ?.takeIf { it.isNotBlank() } - ?: "Joining failed.", + ?: ConcatWorker.GENERIC_FAILURE_MESSAGE, ) WorkInfo.State.CANCELLED -> cancelled @@ -346,7 +347,7 @@ class JoinViewModel @JvmOverloads constructor( // than leaving "Start over" -- which deletes it -- as the only thing on offer. // Passing `pending` rather than rebuilding it is what keeps a retry that fails // again on a carrying Failed instead of a bare one. - _state.value = JoinState.Failed(e.message ?: "Could not save the file.", pending) + _state.value = JoinState.Failed(e.message ?: SAVE_FAILED_MESSAGE, pending) } } } diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 23d95d2..bd839c7 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -39,7 +39,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker val uris = inputData.getStringArray(KEY_INPUT_URIS)?.map(Uri::parse) ?: return Result.failure(workDataOf(KEY_ERROR to "No input files.")) if (uris.size < 2) { - return Result.failure(workDataOf(KEY_ERROR to "Pick at least two files to join.")) + return Result.failure(workDataOf(KEY_ERROR to TOO_FEW_INPUTS_MESSAGE)) } // Absent, not zero, when the picker could not size every input -- see the same read in // ConversionWorker and InputQuery for why the two are no longer one number. @@ -107,7 +107,7 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker } FailureOutcome.FAIL -> { Log.e(TAG, "Joining failed.", e) - Result.failure(workDataOf(KEY_ERROR to (e.message ?: "Joining failed."))) + Result.failure(workDataOf(KEY_ERROR to (e.message ?: GENERIC_FAILURE_MESSAGE))) } } } @@ -137,6 +137,34 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker ) companion object { + /** + * What the user is told when a join arrives with fewer than two inputs. + * + * Shared with `JoinViewModel`, which refuses the same condition one layer up so the picker + * can answer without enqueueing anything. Two copies of this sentence existed before, and + * only the one here was pinned by a test (#139) — so the wording could drift on the screen + * without a single test noticing, for one message the user sees from one condition. + * + * Here rather than in the ViewModel because the rule is the worker's: `request(...)` takes + * a `List` and checks nothing about its length, so this is the guard that always runs. + */ + const val TOO_FEW_INPUTS_MESSAGE: String = "Pick at least two files to join." + + /** + * The last resort when a join fails and the exception says nothing. + * + * Shared with `JoinViewModel`, whose `FAILED` arm falls back to the same sentence when the + * output `Data` carries no error at all — a worker killed before it could write one. The two + * are a chain rather than a coincidence: this is what the worker puts *in* `KEY_ERROR`, and + * that is what the ViewModel says when `KEY_ERROR` never arrived. The user cannot tell the + * two apart and should not have to, so they are one sentence. + * + * The `Log.e` above deliberately keeps its own literal. A log line has a different audience + * and carries the exception with it; coupling it to the user-facing wording would mean + * rewording the screen to change a log. + */ + const val GENERIC_FAILURE_MESSAGE: String = "Joining failed." + const val KEY_INPUT_URIS = "input_uris" const val KEY_TOTAL_BYTES = "total_bytes" const val KEY_FORMAT = "format" diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 40e37cc..8e13e01 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -313,7 +313,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo } FailureOutcome.FAIL -> { Log.e(TAG, "Conversion failed.", cause) - Result.failure(workDataOf(KEY_ERROR to (cause.message ?: "Conversion failed."))) + Result.failure(workDataOf(KEY_ERROR to (cause.message ?: GENERIC_FAILURE_MESSAGE))) } } @@ -352,6 +352,20 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo ) companion object { + /** + * The last resort when a conversion fails and the exception says nothing. + * + * Shared with `ConversionViewModel`, whose `FAILED` arm falls back to the same sentence when + * the output `Data` carries no error — a worker killed before it could write one, which a + * refused foreground start after a process restart produces. The two are a chain rather than + * a coincidence: this is what goes *into* `KEY_ERROR`, and that is what is said when + * `KEY_ERROR` never arrived. The user cannot tell those apart and should not have to. + * + * See [ConcatWorker.GENERIC_FAILURE_MESSAGE] for the join-side twin, and the note there + * about why the neighbouring `Log.e` keeps its own literal. + */ + const val GENERIC_FAILURE_MESSAGE: String = "Conversion failed." + const val KEY_INPUT_URI = "input_uri" const val KEY_DISPLAY_NAME = "display_name" const val KEY_SIZE_BYTES = "size_bytes" diff --git a/app/src/test/java/org/libremediaconverter/join/SharedFailureMessagesTest.kt b/app/src/test/java/org/libremediaconverter/join/SharedFailureMessagesTest.kt new file mode 100644 index 0000000..d8c4198 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/SharedFailureMessagesTest.kt @@ -0,0 +1,102 @@ +package org.libremediaconverter.join + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.RecordingPublisher +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * The two layers that refuse a short join, refusing it with one sentence. + * + * ## Why this is not "assert a constant equals itself" + * + * `ConcatWorker` and `JoinViewModel` both reject a join of fewer than two files, and before #158 + * each carried **its own copy of the literal**. Only the worker's was pinned — by `RefusedJobTest`, + * added in #139 — so the wording on the screen could drift away from the wording in the job with no + * test saying anything, for one message the user sees from one condition. + * + * Sharing a constant makes them agree by construction. What it does *not* do is prove that both + * layers still reach it: a refactor that stops `JoinViewModel` refusing at all, or that gives it a + * different message, passes any test that only reads `TOO_FEW_INPUTS_MESSAGE`. So each layer is + * driven for real here — the ViewModel through `onInputsPicked`, the worker through `doWork` — and + * the assertion is that the two answers are **the same string**, taken from two running layers + * rather than from one declaration. + * + * That is the shape `CLAUDE.md` asks for: revert the sharing and this goes red, because the two + * sites drift the moment they are allowed to. + * + * ## Scope + * + * The arity guard's own behaviour on the ViewModel side — that it refuses one file, that it accepts + * two, that it claims ownership first — is #155's, and this deliberately does not duplicate it. + * This file is about the *agreement between layers*, which is what #158 changed. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class SharedFailureMessagesTest { + + private lateinit var app: Application + private lateinit var viewModel: JoinViewModel + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + ConversionDependencies.publisher = { RecordingPublisher(app) } + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null")) + viewModel = JoinViewModel(app) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `both layers refuse a one-file join with the same sentence`() { + // The ViewModel, refusing before anything is enqueued. + viewModel.onInputsPicked(listOf(ONE_FILE)) + val fromScreen = (viewModel.state.value as JoinState.Failed).message + + // The worker, refusing a job that reached the queue anyway -- which it can, because + // ConcatWorker.request(...) takes a List and checks nothing about its length. + val result = runBlocking { worker(ONE_FILE).doWork() } + val fromJob = (result as ListenableWorker.Result.Failure) + .outputData.getString(ConcatWorker.KEY_ERROR) + + assertEquals( + "the screen and the job must say the same thing about the same refusal", + fromScreen, + fromJob, + ) + // And that the shared sentence is the one either layer would have written on its own, + // rather than both having drifted together to something else. + assertEquals(ConcatWorker.TOO_FEW_INPUTS_MESSAGE, fromScreen) + } + + private fun worker(vararg inputs: Uri): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_TOTAL_BYTES to 1024L, + ), + runAttemptCount = 0, + ).build() + + private companion object { + val ONE_FILE: Uri = Uri.parse("content://test/holiday.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt index f988c06..2bbb1f7 100644 --- a/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/RefusedJobTest.kt @@ -146,7 +146,7 @@ class RefusedJobTest { assertEquals( ListenableWorker.Result.failure( - workDataOf(ConcatWorker.KEY_ERROR to "Pick at least two files to join."), + workDataOf(ConcatWorker.KEY_ERROR to ConcatWorker.TOO_FEW_INPUTS_MESSAGE), ), result, )