diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index c2542b2..6519fad 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -101,7 +101,42 @@ sealed interface ConversionState { val mimeType: String = "", ) : ConversionState data class Saved(val displayName: String) : ConversionState - data class Failed(val message: String) : ConversionState + + /** + * The job, or the save that followed it, could not be finished. + * + * [retry] is non-null for exactly one cause: a [ConversionViewModel.save] whose copy to the + * user's destination threw. That save deliberately keeps the staged file -- it can be the only + * copy of an hour of transcoding -- and this is what lets the screen offer it again. Every + * other failure leaves it null, because there is nothing staged to offer: a transcode that + * died produced no output, and a save that found the file gone has nothing left to save. + * + * Nullable rather than a `SaveFailed` state of its own. What the screen does with the message + * is identical either way, so a second variant would make every exhaustive `when` grow an arm + * that duplicates this one. + * + * A view of the file, not a second owner of it -- see [PendingSave]. + */ + data class Failed(val message: String, val retry: PendingSave? = null) : ConversionState +} + +/** + * The staged output a save would target from this state, or null when there is nothing to save. + * + * One function for two callers that have to agree. [ConversionViewModel.save] picks the file to + * copy with it, and `ConverterScreen` registers its `CreateDocument` contract with the MIME type + * it returns; when those two read the state separately, a retry offered after a failed save opened + * the dialog with the *picker's* current type instead of the finished job's -- wrong for any job + * whose spec has been edited since, and for every reattached job, whose spec was never in these + * settings at all. + * + * Top-level and `internal` rather than a member of the ViewModel, so the screen can call it + * without one -- which is also what makes the derivation testable on the JVM. + */ +internal fun ConversionState.pendingSave(): PendingSave? = when (this) { + is ConversionState.Converted -> PendingSave(staged, suggestedName, mimeType) + is ConversionState.Failed -> retry + else -> null } @UnstableApi @@ -161,10 +196,12 @@ class ConversionViewModel @JvmOverloads constructor( /** * The staged output this ViewModel is responsible for deleting. * - * A field rather than something read back out of [_state], because the state machine - * cannot answer the question on the path that needs it most: a failed [save] lands on - * [ConversionState.Failed], which carries a message and no file at all. By then the - * only remaining reference would have been lost. + * A field rather than something read back out of [_state], and still one now that + * [ConversionState.Failed] carries a [PendingSave] after a failed [save]. That handle is a + * view for the screen to offer a retry through; this one is the single reference [reset] + * deletes through, and keeping the two apart is what stops a second owner appearing. Reading + * the file back out of the state machine instead would mean trusting every state that has no + * file -- `Idle`, `Saved`, a transcode failure -- to say so. */ private var pendingStaged: File? = null @@ -426,35 +463,50 @@ class ConversionViewModel @JvmOverloads constructor( /** * Copies the staged result out to the destination the user picked. * + * Reached from [ConversionState.Converted] and again from a [ConversionState.Failed] that an + * earlier save left carrying its file. [pendingSave] is what makes those one call rather than + * two, so a retry cannot drift from the first attempt in what it copies or what it calls it. + * * The existence check is not redundant with the one reattachment already made. That one ran * inside a tag query which, for a result offered on launch, can be hours older than the tap — * and `cacheDir` is exactly the directory the OS empties when it wants space, which is also * what the sweep does to anything a day old. Without it the file's absence arrived as - * `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. + * `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. A retry + * meets that same check a second time, which is the point of reusing it here. */ fun save(destination: Uri) { - val converted = _state.value as? ConversionState.Converted ?: return - if (!converted.staged.isFile) { + val pending = _state.value.pendingSave() ?: return + if (!pending.staged.isFile) { + // No retry handle: the file such a state would offer again is exactly the one that + // has gone, so carrying it would put a button on screen that cannot do anything. _state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE) return } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { - publisher.publish(converted.staged, destination) - converted.staged.delete() + publisher.publish(pending.staged, destination) + pending.staged.delete() } }.onSuccess { // publish() already deleted it; nothing left to clean up. pendingStaged = null - _state.value = ConversionState.Saved(converted.suggestedName) + _state.value = ConversionState.Saved(pending.suggestedName) }.onFailure { e -> // Deliberately NOT cleared. A failed save may mean the staged file is the // only copy of an hour of transcoding, and the user's destination did not // receive it -- deleting here would destroy the work to tidy up a cache // directory. It stays collectable: by a later reset(), or by the sweep once // it is old enough to be certain nobody is coming back for it. - _state.value = ConversionState.Failed(e.message ?: "Could not save the file.") + // + // `pending` rides on the state so the screen can offer that file again. It used + // to live only in `pendingStaged`, where nothing on screen could reach it -- so + // the single button this branch rendered was "Start over", which deletes the very + // file the paragraph above goes out of its way to keep. It is `pending` rather + // 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) } } } @@ -468,6 +520,12 @@ class ConversionViewModel @JvmOverloads constructor( * cancelled with [viewModelScope] if the Activity finishes first, so it is a best * effort rather than a guarantee. `OutputPublisher.sweepStaging` is the backstop for * the times it does not run. + * + * **It still deletes from a [ConversionState.Failed] carrying a [PendingSave], and that is a + * decision rather than something inherited.** Deletion is acceptable there only because the + * alternative was offered first: the screen puts "Try saving again" directly above this + * button, so reaching it is the user saying the work is not worth keeping. Until that button + * existed, this delete was the only thing a failed save could lead to — which was the defect. */ fun reset() { observer?.cancel() diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index d9c2478..250a31d 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -73,8 +73,11 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode // stands: some providers rewrite a document's extension to match it, so an MP3 offered as // video/webm can arrive with the wrong one. Read straight off the collected state, so this // recomposes because it depends on that rather than because an unrelated line happens to. + // Through pendingSave() rather than a cast to Converted, so a retry offered after a failed + // save opens the dialog with the type its first attempt used -- the cast answered null for a + // Failed, and the fallback below is the current picker, which a reattached job never set. // Remembered against the type so the launcher re-registers only when it actually changes. - val destinationMime = (state as? ConversionState.Converted)?.mimeType ?: settings.spec.mimeType + val destinationMime = state.pendingSave()?.mimeType ?: settings.spec.mimeType val chooseDestination = rememberLauncherForActivityResult( remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) }, ) { uri -> uri?.let(viewModel::save) } @@ -325,13 +328,36 @@ internal fun ConverterScreenContent( color = MaterialTheme.colorScheme.error, style = MaterialTheme.typography.bodyMedium, ) - Button( - onClick = actions.onReset, - modifier = Modifier - .fillMaxWidth() - .height(PrimaryButtonHeight) - .testTag(TestTags.START_OVER), - ) { Text("Start over") } + val retry = s.retry + if (retry == null) { + // Nothing was staged, so "Start over" is the whole of what is on + // offer and stays the primary button. + Button( + onClick = actions.onReset, + modifier = Modifier + .fillMaxWidth() + .height(PrimaryButtonHeight) + .testTag(TestTags.START_OVER), + ) { Text("Start over") } + } else { + Button( + onClick = { actions.onSave(retry.suggestedName) }, + modifier = Modifier + .fillMaxWidth() + .height(PrimaryButtonHeight) + .testTag(TestTags.RETRY_SAVE), + ) { Text("Try saving again") } + // Start over still deletes the file this state is carrying, and that + // is deliberate: `reset()` is what stops a full-size output sitting in + // cache until the sweep. What makes the delete acceptable is the + // button above it. Deletion is the user's choice only once the + // alternative has been offered -- and until that button existed, this + // one was the only thing a failed save could lead to. + OutlinedButton( + onClick = actions.onReset, + modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER), + ) { Text("Start over") } + } } } } diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 430b135..4c8aceb 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -23,6 +23,24 @@ 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." +/** + * A staged file that is still there to be saved, and everything the save dialog needs to offer it. + * + * The three travel together because a save cannot be repeated without all of them: the file to + * copy, the name to suggest, and the MIME type `CreateDocument` has to be registered with. None of + * them can be rederived from the pickers once the job is over -- they come from the job's own + * output `Data`, and a reattached job's spec was never in the current settings at all. + * + * Kept next to [STAGED_FILE_GONE_MESSAGE] for the same reason it is: both ViewModels need it and + * staging is what it is about. + * + * **A view of the staged file, never an owner of it.** The delete still runs through each + * ViewModel's own `pendingStaged` field, so a state carrying one of these can be dropped without + * losing the only reference -- which is what keeps "a `Failed` that carries a file" from being a + * new way to leak one. + */ +data class PendingSave(val staged: File, val suggestedName: String, val mimeType: String) + /** * Staging and publication of conversion output. * diff --git a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt index 765ac1b..a30b9e9 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt @@ -46,9 +46,10 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod // The contract's MIME type comes from the finished job rather than from a literal: some // providers rewrite a document's extension to match it, so naming MP4 for a join that is not - // one can hand the user a file the extension lies about. Remembered against that type so the - // launcher re-registers only when it actually changes. - val destinationMime = (state as? JoinState.Joined)?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType + // one can hand the user a file the extension lies about. Through pendingSave() rather than a + // cast to Joined, so a retry after a failed save opens with the type its first attempt used. + // Remembered against that type so the launcher re-registers only when it actually changes. + val destinationMime = state.pendingSave()?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType val chooseDestination = rememberLauncherForActivityResult( remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) }, ) { uri -> uri?.let(viewModel::save) } @@ -226,13 +227,32 @@ internal fun JoinScreenContent(state: JoinState, actions: JoinActions, modifier: color = MaterialTheme.colorScheme.error, style = MaterialTheme.typography.bodyMedium, ) - Button( - onClick = actions.onReset, - modifier = Modifier - .fillMaxWidth() - .height(PrimaryButtonHeight) - .testTag(TestTags.START_OVER), - ) { Text("Start over") } + val retry = s.retry + if (retry == null) { + // Nothing staged, so "Start over" is all there is and stays primary. + Button( + onClick = actions.onReset, + modifier = Modifier + .fillMaxWidth() + .height(PrimaryButtonHeight) + .testTag(TestTags.START_OVER), + ) { Text("Start over") } + } else { + Button( + onClick = { actions.onSave(retry.suggestedName) }, + modifier = Modifier + .fillMaxWidth() + .height(PrimaryButtonHeight) + .testTag(TestTags.RETRY_SAVE), + ) { Text("Try saving again") } + // Start over still deletes the carried file, for the reason the + // converter screen writes out next to the same pair of buttons: + // the delete is a choice only once the alternative is on screen. + OutlinedButton( + onClick = actions.onReset, + modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER), + ) { Text("Start over") } + } } } } diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 849adeb..7e2d8e6 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.withContext import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.PendingSave import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker @@ -44,7 +45,30 @@ sealed interface JoinState { val mimeType: String, ) : JoinState data class Saved(val displayName: String) : JoinState - data class Failed(val message: String) : JoinState + + /** + * The join, or the save that followed it, could not be finished. + * + * [retry] is non-null for exactly one cause, and for the same reason as on + * `ConversionState.Failed`: a [JoinViewModel.save] whose copy to the user's destination threw + * keeps the staged file, and this is what lets the screen offer it again. Every other failure + * leaves it null — a join that died produced no output, and a save that found the file gone + * has nothing left to save. + */ + data class Failed(val message: String, val retry: PendingSave? = null) : JoinState +} + +/** + * The staged output a save would target from this state, or null when there is nothing to save. + * + * The join tab's half of `ConversionState.pendingSave`, and it exists for the same reason: `save` + * and `JoinScreen`'s `CreateDocument` registration both have to answer this question, and answering + * it twice is how a retry ends up opening the dialog with a type the finished job never chose. + */ +internal fun JoinState.pendingSave(): PendingSave? = when (this) { + is JoinState.Joined -> PendingSave(staged, suggestedName, mimeType) + is JoinState.Failed -> retry + else -> null } @UnstableApi @@ -70,9 +94,10 @@ class JoinViewModel @JvmOverloads constructor( * The staged output this ViewModel is responsible for deleting. * * Held here rather than read back out of [_state] for the same reason as in - * `ConversionViewModel`: a failed [save] lands on [JoinState.Failed], which carries a - * message and no file, so the state machine cannot answer this on the one path that - * most needs it. + * `ConversionViewModel`, and still held here now that [JoinState.Failed] carries a + * [PendingSave] after a failed [save]: that handle is a view for the screen to offer a retry + * through, this one is the single reference [reset] deletes through, and keeping the two + * apart is what stops a second owner of the file appearing. */ private var pendingStaged: File? = null @@ -229,29 +254,38 @@ class JoinViewModel @JvmOverloads constructor( * join offered by reattachment was last seen during a tag query that may be hours old, and * `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence * reached the screen as a raw ENOENT path. + * + * Reached from [JoinState.Joined] and again from a [JoinState.Failed] an earlier save left + * carrying its file; [pendingSave] is what makes those the same call. */ fun save(destination: Uri) { - val joined = _state.value as? JoinState.Joined ?: return - if (!joined.staged.isFile) { + val pending = _state.value.pendingSave() ?: return + if (!pending.staged.isFile) { + // No retry handle -- the file it would offer again is the one that has gone. _state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE) return } viewModelScope.launch { runCatching { withContext(Dispatchers.IO) { - publisher.publish(joined.staged, destination) - joined.staged.delete() + publisher.publish(pending.staged, destination) + pending.staged.delete() } }.onSuccess { // publish() already deleted it; nothing left to clean up. pendingStaged = null - _state.value = JoinState.Saved(joined.suggestedName) + _state.value = JoinState.Saved(pending.suggestedName) }.onFailure { e -> // Deliberately NOT cleared -- see the same branch in ConversionViewModel. // A failed save can leave the staged file as the only copy of the work, so // it is left for a later reset() or for the sweep to collect once its age // makes it certain nobody is coming back for it. - _state.value = JoinState.Failed(e.message ?: "Could not save the file.") + // + // `pending` travels on the state so the screen can offer the file again rather + // 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) } } } @@ -261,6 +295,10 @@ class JoinViewModel @JvmOverloads constructor( * * Best effort, not a guarantee: the delete is cancelled with [viewModelScope] if the * Activity finishes first. `OutputPublisher.sweepStaging` is the backstop. + * + * It deletes from a [JoinState.Failed] carrying a [PendingSave] too, deliberately and for the + * reason `ConversionViewModel.reset` writes out: the screen offers "Try saving again" above + * this button, so deletion is what the user chose rather than all this state could do. */ fun reset() { observer?.cancel() diff --git a/app/src/main/java/org/libremediaconverter/ui/TestTags.kt b/app/src/main/java/org/libremediaconverter/ui/TestTags.kt index 899fcfd..f6fe883 100644 --- a/app/src/main/java/org/libremediaconverter/ui/TestTags.kt +++ b/app/src/main/java/org/libremediaconverter/ui/TestTags.kt @@ -45,6 +45,17 @@ object TestTags { const val SAVE_FILE: String = "action.saveFile" + /** + * The retry a `Failed` offers after a save that threw, on both screens. + * + * Its own tag rather than [SAVE_FILE], because the two are different claims about the screen. + * [SAVE_FILE] is the first attempt from a finished job; this one may appear only where a staged + * file survived a failed save. Sharing a tag would collapse "a transcode failure offers nothing + * to save" and "a failed save offers the file again" into one query, and that first assertion + * is the one stopping a Save button from appearing where there is nothing to save. + */ + const val RETRY_SAVE: String = "action.retrySave" + /** `ConverterScreen`. */ object Converter { const val CHOOSE_FILE: String = "converter.chooseFile" diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt index 8b5a8a7..71a0ee3 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt @@ -93,8 +93,12 @@ class ConversionViewModelCleanupTest { assertTrue("a failed save must not destroy the only copy", staged.exists()) assertEquals(emptyList(), publisher.discarded) - // Failed carries no file reference at all, so this only works because the handle is - // a ViewModel field rather than something read back out of the state machine. + // The handle is a ViewModel field rather than something read back out of the state + // machine, and stays one now that a save-failed `Failed` also carries a `PendingSave`: + // that is a view for the screen to offer a retry through, never a second owner of the + // file. This delete goes through the field, which is what keeps a state that is dropped + // rather than read from taking the only reference with it. What the state carries, and + // what the screen then does with it, are `FailedSaveRetryTest`'s. viewModel.reset() assertEquals(listOf(staged), publisher.discarded) diff --git a/app/src/test/java/org/libremediaconverter/convert/ConverterStateAffordancesTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConverterStateAffordancesTest.kt index 5d459f8..0180dc8 100644 --- a/app/src/test/java/org/libremediaconverter/convert/ConverterStateAffordancesTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/ConverterStateAffordancesTest.kt @@ -53,10 +53,11 @@ import java.io.File * it -- so it is unobservable from a JVM test, the same limit `FileCardTest` records for * `HorizontalDivider`. The message text itself is asserted; the colour would need a screenshot. * - **The three `assertDoesNotExist` checks on [TestTags.Converter.FILE_CARD] are compile-guarded, - * not guarded by this file.** `Idle` is a `data object`, and `Saved` and `Failed` carry only a - * `displayName` and a `message`; none of the three has an `input`, so `FileCard(s.input)` does not - * compile in those arms. The lines stay because they state the intent cheaply, but they are not - * what stops a `FileCard` appearing there and this file does not claim they are. + * not guarded by this file.** `Idle` is a `data object`, `Saved` carries a `displayName`, and + * `Failed` carries a message and -- after a failed save only -- the staged file it left behind; + * none of the three has an `input`, so `FileCard(s.input)` does not compile in those arms. The + * lines stay because they state the intent cheaply, but they are not what stops a `FileCard` + * appearing there and this file does not claim they are. * - **Which constant each chip hands back** belongs to `ConverterPickerSelectionTest`, and **what * the file card says about an unknown size** to `FileCardTest`. This file asserts that `Ready` * puts those leaves on screen at all, not what they then do. @@ -326,6 +327,22 @@ class ConverterStateAffordancesTest { composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD).assertDoesNotExist() } + /** + * **The assertion that bounds #30's whole change**, and the one worth breaking things to keep. + * + * A transcode that died staged nothing, so its `Failed` carries no [PendingSave] and there is + * nothing for a save dialog to be handed. Making the retry unconditional -- or making the + * `WorkInfo.State.FAILED` arm of `ConversionViewModel.observe` carry a handle it has no file + * for -- puts a button on screen that can only fail, and this is what notices. + */ + @Test + fun `a transcode failure offers no way to save`() { + setContent(ConversionState.Failed(message = "Ran out of space while writing the output.")) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist() + composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist() + } + @Test fun `tapping start over after a failure resets and does nothing else`() { setContent(ConversionState.Failed(message = "Ran out of space while writing the output.")) @@ -335,6 +352,50 @@ class ConverterStateAffordancesTest { assertEquals(listOf("reset"), fired) } + // ----------------------------------------------------- Failed, carrying a file + + /** + * The defect in #30, stated as what the branch must render. + * + * `save()` keeps the staged file on a failure deliberately -- it can be the only copy of an + * hour of transcoding -- and before this the only control here was "Start over", wired to + * `reset()`, which deletes exactly that file. Both buttons, not one: the restart has to stay + * reachable, because leaving a full-size file in cache is the outcome it exists to avoid. + */ + @Test + fun `a failed save offers the file again as well as a restart`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists() + composeRule.onNodeWithTag(TestTags.START_OVER).assertExists() + } + + /** + * The name, not just that something fired: it comes from the finished job, and a retry wired to + * a literal or to the picker's current guess would hand the dialog a name the job never chose. + */ + @Test + fun `tapping try saving again hands back the name the job chose`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick() + + assertEquals(listOf("save:holiday.mp4"), fired) + } + + /** + * Start over from here still resets, and resetting still deletes -- see `reset()`'s KDoc for + * why that is acceptable now and was not before. What it must not do is save on the way past. + */ + @Test + fun `tapping start over after a failed save resets and does not save`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick() + + assertEquals(listOf("reset"), fired) + } + // ------------------------------------------------------------------ Harness private fun input() = InputFile( @@ -348,6 +409,21 @@ class ConverterStateAffordancesTest { * missing file rather than throwing, so the size line reads `0 B` and no temporary folder is * needed to render the arm. */ + /** + * A `Failed` an earlier save left carrying its file, which is the only way [retry] is non-null. + * + * The same missing `staged` path as [converted], and for the same reason: this arm renders no + * size line at all, so nothing here ever touches the filesystem. + */ + private fun failedSave() = ConversionState.Failed( + message = "There was not enough room on the destination.", + retry = PendingSave( + staged = File("no-such-staged-output.mp4"), + suggestedName = "holiday.mp4", + mimeType = "video/mp4", + ), + ) + private fun converted(routeReason: String = "") = ConversionState.Converted( input = input(), staged = File("no-such-staged-output.mp4"), diff --git a/app/src/test/java/org/libremediaconverter/convert/FailedSaveRetryTest.kt b/app/src/test/java/org/libremediaconverter/convert/FailedSaveRetryTest.kt new file mode 100644 index 0000000..e63cb7a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/FailedSaveRetryTest.kt @@ -0,0 +1,370 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +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.pendingSave +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * A failed save has to leave the file *offerable*, not merely undeleted. + * + * The defect is #30, and both halves of it were already written down in `main`. `save()`'s + * `onFailure` kept the staged file on purpose -- "deleting here would destroy the work to tidy up + * a cache directory" -- and then handed the screen a `Failed` carrying a message and nothing else, + * so the single control that branch rendered was "Start over", wired to `reset()`, which deletes + * exactly that file. The intent and the affordance disagreed, and the affordance won. + * + * `ConversionViewModelCleanupTest` already pins the *keeping*: after a failed save the file is + * still on disk and nothing has been discarded. It stays green with the state carrying nothing, + * because it reads the filesystem rather than the state. This file asserts the other half -- that + * the handle reaches the state a screen can read -- and the negative that bounds it: a failure + * with nothing staged behind it must not sprout a save button. + * + * Both ViewModels in one class, following `MissingStagedFileTest`. They are separate state + * machines that can each hold a staged file at once, but this defect and its fix are the same + * shape in both, and splitting them would put the two halves of one invariant in two files. + * + * ### Not asserted here, so each is a decision rather than an omission + * + * - **That the destination received the bytes.** [RecordingPublisher.publish] is a stub, which is + * the only way to make a save fail deterministically -- and making it fail is what every case + * here needs. `OutputPublisherPublishTest` owns what a real publish writes. + * - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest` + * own what each state renders; this file owns what each state carries. + * - **`ConverterScreen`'s `destinationMime` line itself.** It lives in the entry point, above the + * `ScreenContent` seam, and reaching it needs a real ViewModel inside a composition. What it + * reads -- `pendingSave()?.mimeType` -- is asserted directly instead, which is why that + * derivation was moved out of the entry point in the first place. + * - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state + * without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither + * branch renders a picker. It is a pre-existing path this change neither opens nor widens: the + * carried handle is a view of `pendingStaged`, never a second owner of the file. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class FailedSaveRetryTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + // MediaProbe spawns FFprobe, whose loader throws with no native library present. + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + // ------------------------------------------------------------------ Convert + + /** + * The bite named in #30's fix. Emitting a plain `Failed` from `save()`'s `onFailure` -- which + * is what `main` did -- reddens this case on the null handle, and nothing else in the suite. + */ + @Test + fun `a failed save leaves the staged file offerable, not merely undeleted`() { + val viewModel = failedSaveViewModel() + + val failed = viewModel.state.value as ConversionState.Failed + val retry = failed.retry + assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry) + assertEquals("the retry must name the file the conversion actually produced", staged, retry?.staged) + // Both from the job's own output Data rather than from the pickers, so a retry opens the + // same dialog the first attempt did. + assertEquals(SUGGESTED_NAME, retry?.suggestedName) + assertEquals(JOB_MIME_TYPE, retry?.mimeType) + assertTrue("a failed save must not destroy the only copy", staged.exists()) + } + + /** + * The whole point of carrying the handle: the second attempt is a real save, not a new job. + * + * `publishFailure` is cleared between the two calls, so one `RecordingPublisher` plays both a + * full destination and an empty one -- which is exactly the user's situation. + */ + @Test + fun `retrying a failed save publishes the file and leaves nothing staged`() { + val viewModel = failedSaveViewModel() + + publisher.publishFailure = null + viewModel.save(DESTINATION) + + val saved = awaitState(viewModel.state, "Saved") { it is ConversionState.Saved } + assertEquals(SUGGESTED_NAME, (saved as ConversionState.Saved).displayName) + assertFalse("a successful retry should have removed the staged file", staged.exists()) + // Nothing to collect afterwards: the retry published it, so reset() has no work left. + viewModel.reset() + assertEquals(emptyList(), publisher.discarded) + } + + /** + * The second failure must not eat the file the first one kept. + * + * A `Failed` built fresh from `e.message` alone would drop the handle here while every other + * assertion in this file stayed green -- the file is still on disk, and the first failure + * already proved the state can carry it. + */ + @Test + fun `a retry that fails again still carries the file rather than dropping it`() { + val viewModel = failedSaveViewModel() + + publisher.publishFailure = IllegalStateException("destination volume still full") + viewModel.save(DESTINATION) + + // Waited for by the *second* message rather than by `is Failed`: the state was already + // Failed when the retry started, so the type alone would be satisfied before it ran. + val failed = awaitState(viewModel.state, "the second failure") { + it is ConversionState.Failed && it.message == "destination volume still full" + } as ConversionState.Failed + assertEquals("the second failure must offer the same file the first one did", staged, failed.retry?.staged) + assertTrue(staged.exists()) + assertEquals(emptyList(), publisher.discarded) + } + + /** + * "Start over" still deletes, and that is the decision `reset()`'s KDoc records: acceptable + * only because "Try saving again" is on screen beside it. Exactly once, through the publisher. + */ + @Test + fun `start over from a failed save discards the carried file exactly once`() { + val viewModel = failedSaveViewModel() + + viewModel.reset() + + assertEquals(ConversionState.Idle, viewModel.state.value) + assertEquals(listOf(staged), publisher.discarded) + assertFalse(staged.exists()) + } + + /** + * A retry meets the same existence check the first attempt did, so a file collected by the + * sweep or by the OS in between is reported as a sentence rather than as a raw ENOENT path. + * And the state that reports it carries nothing: there is no file left to offer. + */ + @Test + fun `a retry whose staged file has gone says so and offers nothing further`() { + val viewModel = failedSaveViewModel() + assertTrue("the fixture must start with a real staged file", staged.delete()) + + publisher.publishFailure = null + viewModel.save(DESTINATION) + + val failed = viewModel.state.value as ConversionState.Failed + assertEquals(STAGED_FILE_GONE_MESSAGE, failed.message) + assertNull("a file that has gone cannot be offered again", failed.retry) + } + + /** + * The negative that bounds the whole change, and the reason `retry` is nullable. + * + * A transcode that died staged nothing, so there is no file to hand back -- and a `Failed` + * that carried one anyway would put a save button on a screen with nothing to save. Driven + * through a worker that really fails rather than by constructing the state, because the line + * under test is the `WorkInfo.State.FAILED` arm of `observe`. + */ + @Test + fun `a transcode failure carries nothing to save`() { + installFailingTestWorkManager(app, workDataOf(ConversionWorker.KEY_ERROR to "The encoder gave up.")) + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + + viewModel.convert() + + val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed + assertEquals("The encoder gave up.", failed.message) + assertNull("a transcode failure has nothing staged, so it must offer no save", failed.retry) + assertNull("and nothing for the save dialog to open with either", failed.pendingSave()) + } + + /** + * What the save dialog reopens with, which is the entry point's only reader of this state. + * + * The pickers are moved *after* the job finishes, which is what makes this bite: a retry that + * asked the current settings would offer `audio/mpeg` for a file the job wrote as MP4. The + * same gap is permanent for a reattached job, whose spec was never in these settings at all. + */ + @Test + fun `a retry offers the type the job chose, not the one the pickers now show`() { + val viewModel = failedSaveViewModel() + + viewModel.setPreset(OutputFormat.MP3) + + assertEquals( + "the fixture needs the pickers to disagree with the job", + "audio/mpeg", + viewModel.settings.value.spec.mimeType, + ) + assertEquals(JOB_MIME_TYPE, viewModel.state.value.pendingSave()?.mimeType) + } + + // --------------------------------------------------------------------- Join + + @Test + fun `a failed join save leaves the staged file offerable, not merely undeleted`() { + val viewModel = failedJoinSaveViewModel() + + val failed = viewModel.state.value as JoinState.Failed + val retry = failed.retry + assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry) + assertEquals(staged, retry?.staged) + assertEquals(SUGGESTED_NAME, retry?.suggestedName) + assertEquals(JOB_MIME_TYPE, retry?.mimeType) + assertTrue(staged.exists()) + } + + @Test + fun `retrying a failed join save publishes the file and leaves nothing staged`() { + val viewModel = failedJoinSaveViewModel() + + publisher.publishFailure = null + viewModel.save(DESTINATION) + + val saved = awaitState(viewModel.state, "Saved") { it is JoinState.Saved } + assertEquals(SUGGESTED_NAME, (saved as JoinState.Saved).displayName) + assertFalse(staged.exists()) + viewModel.reset() + assertEquals(emptyList(), publisher.discarded) + } + + @Test + fun `a join retry that fails again still carries the file rather than dropping it`() { + val viewModel = failedJoinSaveViewModel() + + publisher.publishFailure = IllegalStateException("destination volume still full") + viewModel.save(DESTINATION) + + // By the second message, not by `is Failed` -- see the converter case above. + val failed = awaitState(viewModel.state, "the second failure") { + it is JoinState.Failed && it.message == "destination volume still full" + } as JoinState.Failed + assertEquals(staged, failed.retry?.staged) + assertTrue(staged.exists()) + assertEquals(emptyList(), publisher.discarded) + } + + @Test + fun `start over from a failed join save discards the carried file exactly once`() { + val viewModel = failedJoinSaveViewModel() + + viewModel.reset() + + assertEquals(JoinState.Idle, viewModel.state.value) + assertEquals(listOf(staged), publisher.discarded) + assertFalse(staged.exists()) + } + + @Test + fun `a join failure carries nothing to save`() { + installFailingTestWorkManager(app, workDataOf(ConcatWorker.KEY_ERROR to "The files could not be joined.")) + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + + viewModel.join() + + val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed + assertEquals("The files could not be joined.", failed.message) + assertNull("a join failure has nothing staged, so it must offer no save", failed.retry) + assertNull(failed.pendingSave()) + } + + // ------------------------------------------------------------------ Harness + + /** + * A ViewModel driven to `Converted` and then through a save that threw. + * + * The WorkManager is installed here rather than in `@Before`, because two cases in this class + * need one whose workers fail instead. + */ + private fun failedSaveViewModel(): ConversionViewModel { + installTestWorkManager(app, conversionOutput()) + // Unconfined so reset()'s delete runs inline instead of on a real IO thread. + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + publisher.publishFailure = IllegalStateException("destination volume full") + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } + return viewModel + } + + /** The join tab's equivalent, driven to `Joined` and then through a save that threw. */ + private fun failedJoinSaveViewModel(): JoinViewModel { + installTestWorkManager(app, joinOutput()) + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + + publisher.publishFailure = IllegalStateException("destination volume full") + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Failed") { it is JoinState.Failed } + return viewModel + } + + /** + * The output `Data` a finished conversion reports. + * + * The name and type are set rather than left out, so the assertions above are about what the + * *job* chose. Both ViewModels fall back to a derivation when they are missing, and a fixture + * that omitted them would be asserting the fallback while looking like it asserted the job. + * + * Spelled out per worker rather than shared with [joinOutput], even though the two constants + * hold the same strings today. A test that leaned on that would be asserting a coincidence. + */ + private fun conversionOutput() = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + /** The output `Data` a finished join reports. See [conversionOutput]. */ + private fun joinOutput() = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME, + ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE, + ) + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + + const val SUGGESTED_NAME = "holiday.mp4" + + /** What the job wrote. [OutputFormat.MP3]'s `audio/mpeg` is what the pickers move to. */ + const val JOB_MIME_TYPE = "video/mp4" + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt index 3ea2462..74ca827 100644 --- a/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt +++ b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt @@ -82,6 +82,28 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() { } } +/** + * Stands in for a worker that died, reporting [outputData] on the way out. + * + * The counterpart to [SucceedingWorkerFactory], and needed for the same reason: the real workers + * cannot run on the JVM, so the only way to ask what a ViewModel does with a `FAILED` `WorkInfo` is + * to produce one. A `Result.failure` carrying `KEY_ERROR` is exactly what both real workers report + * when their engine gives up, and it is the one path where nothing has ever been staged. + * + * `runAttemptCount` is irrelevant here: `Result.failure` is terminal, so WorkManager does not retry + * it and the state goes straight to `Failed` rather than through `Waiting`. + */ +class FailingWorkerFactory(private val outputData: Data) : WorkerFactory() { + + override fun createWorker( + appContext: Context, + workerClassName: String, + workerParameters: WorkerParameters, + ): ListenableWorker = object : Worker(appContext, workerParameters) { + override fun doWork(): Result = Result.failure(outputData) + } +} + /** * Installs a synchronous test WorkManager whose workers succeed with [outputData]. * @@ -89,6 +111,16 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() { */ fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorkerFactory { val factory = SucceedingWorkerFactory(outputData) + installWorkManager(context, factory) + return factory +} + +/** Installs a synchronous test WorkManager whose workers fail, reporting [outputData]. */ +fun installFailingTestWorkManager(context: Context, outputData: Data) { + installWorkManager(context, FailingWorkerFactory(outputData)) +} + +private fun installWorkManager(context: Context, factory: WorkerFactory) { WorkManagerTestInitHelper.initializeTestWorkManager( context, Configuration.Builder() @@ -98,7 +130,6 @@ fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorker .setWorkerFactory(factory) .build(), ) - return factory } /** diff --git a/app/src/test/java/org/libremediaconverter/join/JoinStateAffordancesTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinStateAffordancesTest.kt index f446d2d..c95d4da 100644 --- a/app/src/test/java/org/libremediaconverter/join/JoinStateAffordancesTest.kt +++ b/app/src/test/java/org/libremediaconverter/join/JoinStateAffordancesTest.kt @@ -18,6 +18,7 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremediaconverter.convert.InputFile +import org.libremediaconverter.convert.PendingSave import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.ui.TestTags import org.robolectric.RobolectricTestRunner @@ -219,6 +220,51 @@ class JoinStateAffordancesTest { assertEquals(listOf("reset"), events) } + /** + * The negative that bounds #30 on this screen. A join that died staged nothing, so its `Failed` + * carries no [PendingSave] and there is nothing a save dialog could be handed. A retry button + * rendered unconditionally here could only fail, and this is what notices. + */ + @Test + fun `a failed join offers no way to save`() { + setContent(JoinState.Failed(message = "The second file has no audio track, so joining stopped.")) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist() + composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist() + } + + /** + * #30 on this screen: `save()` keeps the staged file when the copy out throws, and until this + * branch grew a second button the only control it rendered was "Start over" -- `reset()`, which + * deletes exactly that file. Both, not one: the restart still has to be reachable. + */ + @Test + fun `a failed join save offers the file again as well as a restart`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists() + composeRule.onNodeWithTag(TestTags.START_OVER).assertExists() + } + + /** The name comes from the job, so a retry wired to a literal would hand back the wrong one. */ + @Test + fun `tapping try saving again hands back the name the join chose`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick() + + assertEquals(listOf("save:joined.mp4"), events) + } + + @Test + fun `tapping start over after a failed join save resets and does not save`() { + setContent(failedSave()) + + composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick() + + assertEquals(listOf("reset"), events) + } + /** Anything `FileRow` tagged, whichever file it is showing. The prefix comes from the table. */ private val isFileRow = SemanticsMatcher("is a join file row") { node -> node.config.getOrNull(SemanticsProperties.TestTag)?.startsWith(TestTags.Join.fileRow("")) == true @@ -230,6 +276,19 @@ class JoinStateAffordancesTest { sizeBytes = 4_000_000L, ) + /** + * A `Failed` an earlier save left carrying its file, which is the only way `retry` is non-null. + * `staged` names a missing file for the same reason [joined] does -- this arm reads no length. + */ + private fun failedSave() = JoinState.Failed( + message = "There was not enough room on the destination.", + retry = PendingSave( + staged = File("no-such-staged-output.mp4"), + suggestedName = "joined.mp4", + mimeType = "video/mp4", + ), + ) + /** `staged` names a missing file deliberately -- see the same helper in `JoinScreenContentTest`. */ private fun joined(strategy: ConcatStrategy) = JoinState.Joined( staged = File("no-such-staged-output.mp4"),