Merge remote-tracking branch 'origin/main' into merge-117-tmp

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()

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") }
+                        }
                     }
                 }
             }

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.
  *

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") }
+                        }
                     }
                 }
             }

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()

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"

app/src/main/java/org/libremediaconverter/ui/theme/Theme.kt

@@ -25,8 +25,18 @@ private val LightColorScheme = lightColorScheme(
  * Material 3 theme.
  *
  * Dynamic color (Material You) needs API 31+; minSdk is 33, so it is available
- * unconditionally and no version guard is required. It stays switchable so users can
- * opt back to the brand palette.
+ * unconditionally and no version guard is required.
+ *
+ * [dynamicColor] has no caller. `MainActivity` is the single call site and takes the
+ * default, so the parameter is always `true`, the two dynamic branches always win, and
+ * [DarkColorScheme] and [LightColorScheme] are dead: nothing in the app can opt back to the
+ * brand palette. `ThemeColorSchemeTest` reaches those two branches only by passing
+ * [dynamicColor] explicitly -- a test doing it, not a feature.
+ *
+ * That is known rather than an oversight. #68 holds the choice between adding a switch,
+ * deleting the dead branches together with the template palette, and replacing that palette
+ * first; it is undecided, so nothing here should be read as a promise that any of them
+ * happens.
  */
 @Composable
 fun LibreMediaConverterTheme(