Files
LibreMediaConverter/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt
T
JMR-devandClaude Opus 5 104d02de03 W5 (#158): one sentence per user-facing condition, not two
Four messages were written out in two places each, in a codebase that already
had the convention for this and states it in `OutputPublisher.kt`:

    Kept next to [STAGED_FILE_GONE_MESSAGE] for the same reason it is: both
    ViewModels need it and staging is what it is about.

The ticket named three. A wider scan -- `"[A-Z][^"]{8,90}[.!]"` rather than the
{15,70} that produced the original list -- found a fourth, `"Joining failed."`,
which is the exact join-side twin of `"Conversion failed."` and had been missed
because it is fifteen characters long.

  "Pick at least two files to join."  ->  ConcatWorker.TOO_FEW_INPUTS_MESSAGE
  "Joining failed."                   ->  ConcatWorker.GENERIC_FAILURE_MESSAGE
  "Conversion failed."                ->  ConversionWorker.GENERIC_FAILURE_MESSAGE
  "Could not save the file."          ->  SAVE_FAILED_MESSAGE, beside
                                          STAGED_FILE_GONE_MESSAGE

Each constant sits with the layer that owns the condition, which is what the two
existing constants do. The arity rule is the worker's -- `request(...)` takes a
`List<Uri>` and checks nothing about its length -- so `TOO_FEW_INPUTS_MESSAGE`
lives there and the ViewModel reads it, not the other way round.

WHY THE TWO `Log.e` LITERALS STAY. `"Conversion failed."` and `"Joining failed."`
each also appear in a log line beside the failure they describe. Those keep their
own copies: a log has a different audience and carries the exception with it, and
coupling it to the user-facing wording would mean rewording the screen to change
a log. Stated in the KDoc so the next scan does not read them as a miss.

THE TEST IS A CROSS-LAYER ONE, DELIBERATELY. #158's done-when is explicit that "a
test asserting the constant equals its own value is worth nothing". Sharing a
constant makes the two sites agree by construction; what it cannot show is that
both layers still *reach* it. So `SharedFailureMessagesTest` drives each for real
-- the ViewModel through `onInputsPicked`, the worker through `doWork` -- and
asserts the two answers are the same string, taken from two running layers rather
than from one declaration.

That the sharing was worth doing at all is visible in what was pinned before:
`RefusedJobTest` (#139) pinned the worker's copy of the arity message and nothing
pinned the ViewModel's, so the screen's wording could drift with no test saying
anything.

Mutations:

| mutation | result |
|---|---|
| ViewModel keeps its own drifted literal | red |
| ViewModel's arity guard removed entirely | red |

Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin,
ktlintCheck, detekt, lintDebug.

Not done here: `"Saved ${s.displayName}."` appears in both screens. It is left
alone, and the reason is a real distinction rather than an oversight -- the four
above are cases where one layer's message is another layer's *fallback*, so drift
means the user sees different words for one condition. Two screens each wording
their own success text is ordinary UI, and drift there is cosmetic.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-29 10:05:27 -05:00

324 lines
18 KiB
Kotlin

package org.libremediaconverter.convert
import android.content.Context
import android.net.Uri
import android.provider.DocumentsContract
import android.provider.OpenableColumns
import java.io.File
import java.io.OutputStream
/**
* What a save has to say when the staged file is not there any more.
*
* Reachable without anything going wrong: staging lives in `cacheDir`, which the OS reclaims
* whenever it wants the space, and [sweepStaging] collects anything a day old. A result offered by
* reattachment is the likeliest to meet it — the check that decided the file existed ran during a
* tag query that can be hours old by the time the Save button is tapped.
*
* A written sentence rather than the exception's message, which is what used to reach the screen:
* `/data/user/0/org.libremediaconverter/cache/conversions/4b4882….mp4: open failed: ENOENT (No such
* file or directory)` is a true statement about a path the user has never seen and cannot act on.
* Kept next to [OutputPublisher] because both ViewModels need it and staging is what it is about.
*/
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.
*
* 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.
*
* Conversions never write directly to the destination the user picked. FFmpeg and the
* MP4 muxer both need to seek backwards to finalise a file — faststart rewrites the
* moov atom at the end — and a SAF file descriptor is not reliably seekable. Writing
* through one produces a truncated or unplayable file.
*
* So every job writes to app-private cache, which is a real POSIX path with no
* permissions and no scoped-storage rules, and the finished file is copied out to the
* user's chosen destination afterwards.
*
* The cost is one extra copy and transient double disk usage, which is why
* [hasSpaceFor] exists.
*/
open class OutputPublisher(private val context: Context) {
private val stagingDir: File
get() = File(context.cacheDir, "conversions").apply { mkdirs() }
open fun createStagingFile(name: String): File = File(stagingDir, name)
/**
* True if staging can take a further [bytes], with [SPACE_HEADROOM_BYTES] left over.
*
* **The doc this replaces claimed peak usage was "roughly input + output at once" while the
* arithmetic reserved `input + 128 MB`.** The arithmetic is what stays, and this says why
* rather than the two continuing to disagree.
*
* [bytes] is the *input's* size standing in for the output's, because before an engine has
* run there is no other number. It is generous for the ordinary conversion, which is asked
* for precisely because it shrinks its input, and short for the ones that do not — a re-encode
* to a bulkier codec, or a stream copy into a container with more overhead.
*
* The 128 MB absorbs that error, and one more besides: [publish] copies the staged file to
* the user's destination, so while that runs the bytes exist twice on any destination sharing
* this volume. Reserving `input + output` outright would have refused jobs that fit, on a
* device where the destination is usually removable or remote.
*
* So this is a pre-flight check that stops a job which obviously cannot fit from spending
* minutes discovering it — not a guarantee. A conversion that runs out of space anyway fails
* through its engine, with a message of its own.
*
* Open so a test can force a full disk; see `FakeFailures` in the instrumented source set.
*
* Written as `free - headroom > required` rather than the equivalent-looking
* `free > required + headroom`. The second overflows: a request within 128 MiB of
* [Long.MAX_VALUE] wraps the sum negative, every free-space measurement beats a negative
* number, and the check answers "plenty of room" to the largest request it can be given. That
* is reachable rather than theoretical — [InputQuery.total] sums a join's inputs, so the number
* arriving here is not bounded by any single file. Both operands are clamped at zero first, so
* the subtraction cannot underflow and a nonsense negative size decides exactly as zero does
* instead of buying slack.
*/
open fun hasSpaceFor(bytes: Long): Boolean =
stagingDir.usableSpace.coerceAtLeast(0L) - SPACE_HEADROOM_BYTES > bytes.coerceAtLeast(0L)
/**
* The same check for a job whose input size nobody could determine — see [InputQuery].
*
* **This deliberately produces the same number the defect produced by accident**, which is
* worth stating plainly: with no size to reserve for, all that is left to check is the
* headroom. What has changed is that it is now the answer to a question that was asked. The
* old code could not tell an unmeasurable file from an empty one, so it silently made this
* the answer for *both*; now [hasSpaceFor] means "there is room for this many bytes" and
* nothing else claims it.
*
* Refusing instead was considered and rejected. It would turn "no provider answered the
* `SIZE` column" into "this file cannot be converted" — a worse defect than the one being
* fixed, and one the user could do nothing about.
*
* The default answers *through* [hasSpaceFor], which is what keeps a publisher that refuses
* on space — `FakeFailures.FullDisk`, which overrides `hasSpaceFor` and nothing else —
* refusing this too. `SpaceCheckTest` pins that delegation, because an override here that
* stopped delegating would quietly stop honouring a full disk.
*/
open fun hasSpaceForUnknownSize(): Boolean = hasSpaceFor(0L)
/**
* Copies a finished staging file into a user-chosen SAF destination.
*
* A copy that fails partway -- the destination volume filling up is the obvious one, a
* provider giving out mid-write the other -- used to leave the bytes it had managed at
* the name the user picked, while the UI said "Could not save the file". The user was
* then holding a truncated file they had been told was never written, and nothing in the
* app would ever tidy it up: staging cleanup only reaches [stagingDir], never the
* destination.
*
* So a failed copy deletes the document. Three things bound that, because deleting a
* file the user already had would be a far worse defect than the one being fixed:
*
* - **Only a document URI.** `DocumentsContract.deleteDocument` is the only delete this
* has any right to attempt, and it is defined on document URIs. Anything else -- a
* `file://` path, a MediaStore item, a content URI from a provider that is not a
* documents provider -- is left exactly as it is.
* - **Only a destination that was empty when we started.** The size is read before the
* stream is opened, and the delete only runs if the answer was positively zero. Every
* destination reaching here comes from the SAF `CreateDocument` contract, so in
* practice it is a document this app just created; but `publish` cannot verify that
* from a `Uri`, and a provider that hands back an existing document for a name the
* user re-picked would otherwise have its file deleted rather than merely truncated.
* A provider that reports no size at all falls into the same "not known to be empty"
* bucket, so the fix is conservative rather than universal: it will not clean up
* behind such a provider, and it will not delete anything of theirs either.
* - **The original failure is what the caller sees.** Cleanup runs inside its own
* `runCatching`; if it throws, that goes on the original exception as a suppressed
* one. `save()` reports `e.message`, and "could not delete the half-written file" is
* not the thing to tell someone whose disk just filled up.
*
* The whole `use` is guarded, not just the copy: a `close()` that throws while flushing
* IS the disk-full case, and it arrives after `copyTo` has returned. The cost is that a
* file whose every byte reached the provider before a failing flush is deleted too --
* which is the right way round, since a flush that failed means the bytes are not
* durably there to begin with.
*
* `openOutputStream` is inside the guard as well, and the reasoning that used to keep it
* out -- "nothing has been written at that point, so there is nothing of ours to remove" --
* was wrong about what exists. SAF's `CreateDocument` contract creates the document *before*
* this is called, which is why every fixture in `OutputPublisherPublishTest` starts as an
* existing empty file. So a provider that hands out no stream at all -- gone between the
* picker and the write, or simply returning null -- left a zero-byte file at the name the
* user chose while the UI said "Could not save the file". The two bounds above are what make
* removing it safe, and they apply to this case exactly as they do to a failed copy. A
* provider that will not open its own empty document may well refuse to delete it too, which
* is already [deletePartialOutput]'s documented no-op path.
*/
open fun publish(staged: File, destination: Uri) {
val destinationWasEmpty = destinationIsKnownEmpty(destination)
try {
val out = openDestination(destination)
?: error("Could not open destination for writing: $destination")
out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } }
} catch (failure: Throwable) {
if (destinationWasEmpty) deletePartialOutput(destination, failure)
throw failure
}
}
/**
* Opens [destination] for writing, or null when the provider will not.
*
* A seam, and a narrow one: it exists because `openOutputStream` has **two** ways of refusing
* and only one of them is reachable from a test otherwise. A provider that has gone away throws
* `FileNotFoundException` from inside the call; a provider that is present and declines returns
* null. The two are not interchangeable here — the `?: error(...)` above is the only thing that
* turns the second into a failure rather than an NPE further down — and no fake provider can be
* asked to produce a null return on demand.
*
* `protected open` rather than injected, matching `hasSpaceFor` and `createStagingFile`:
* `WorkerStubs.kt`'s publishers already override one method to force one condition.
*/
protected open fun openDestination(destination: Uri): OutputStream? =
context.contentResolver.openOutputStream(destination)
/**
* True only when the destination is *positively known* to hold no bytes yet.
*
* Every other answer -- a provider that does not report `_size`, a query that returns no
* row, a resolver call that throws -- is false, because this decides whether a delete is
* allowed and "I could not tell" must never authorise one.
*
* The column is looked up by name rather than taken as index 0: a projection is a
* request, not a guarantee, and a provider is free to return its own column set.
*/
private fun destinationIsKnownEmpty(destination: Uri): Boolean = runCatching {
context.contentResolver
.query(destination, arrayOf(OpenableColumns.SIZE), null, null, null)
?.use { row ->
val size = row.getColumnIndex(OpenableColumns.SIZE)
size >= 0 && row.moveToFirst() && !row.isNull(size) && row.getLong(size) == 0L
}
}.getOrNull() ?: false
/**
* Removes the half-written document, never at the expense of [cause].
*
* `deleteDocument` reports its own failure two different ways -- `false`, or a thrown
* `FileNotFoundException` -- and neither is worth failing the save over, because the
* save has already failed. Whatever it does, [cause] is what propagates; a thrown
* cleanup failure is attached to it so it is not simply lost.
*/
private fun deletePartialOutput(destination: Uri, cause: Throwable) {
runCatching {
if (DocumentsContract.isDocumentUri(context, destination)) {
DocumentsContract.deleteDocument(context.contentResolver, destination)
}
}.onFailure(cause::addSuppressed)
}
/**
* Deletes one staged file, if it really is one of ours.
*
* This is what a ViewModel's `reset()` calls when the user taps "Start over" on a
* finished-but-unsaved conversion, which is otherwise a full-size copy left in cache
* for the OS to reclaim whenever it feels like it.
*
* The guard is not decoration. The handle reaches the ViewModel as a path string in
* `WorkInfo.outputData` and is turned straight into a `File`, so this is the one place
* that checks where it points before deleting. Comparing the *canonical* parent rather
* than the path as written is what makes `conversions/../something` fail: the naive
* string comparison accepts it.
*
* @return true if a file was deleted. False covers both "not in staging" and "already
* gone", which the caller has no reason to tell apart — a `reset()` after a
* successful save is an ordinary second call.
*/
open fun discardStaged(staged: File): Boolean {
val parent = staged.parentFile?.canonicalOrAbsolute() ?: return false
if (parent != stagingDir.canonicalOrAbsolute()) return false
return staged.delete()
}
/**
* Deletes staged files old enough to have been abandoned.
*
* The backstop for everything `discardStaged` cannot reach: a process killed between
* finishing a conversion and saving it, a worker that failed before its output ever
* became a `Converted` state, or a `reset()` whose delete was cancelled with the
* Activity. [StagingSweep] owns the rule and its reasoning.
*
* Deliberately not the `clearStaging()` this replaces. That deleted the directory's
* whole contents, and the convert tab, the join tab and `ConcatEngine`'s list file all
* share this directory — so a blanket delete could destroy a live job's file. Per-job
* staging names ([StagingNames]) stop two jobs from *sharing* a file; they say nothing
* about whether a file's job is still running, which is the question here.
*
* [nowMs] is a parameter so the clock is the caller's, not a hidden global.
*/
open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) {
val dir = stagingDir
val listing = dir.listFiles() ?: return
val entries = snapshot(listing)
StagingSweep.collectable(entries, nowMs).forEach { name ->
val file = File(dir, name)
// Re-read the timestamp rather than trusting the snapshot above. Between the
// listing and here, a worker resumed by WorkManager -- which runs in this same
// process -- could have started writing this very file, and unlinking an inode a
// running job still holds open would end with the job reporting success for a
// path that no longer exists.
if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete()
}
}
/**
* The name and age of everything [sweepStaging] found, read once.
*
* A seam for the *race*, not for the clock — [sweepStaging] already takes `nowMs`, so the clock
* is the caller's. What has no seam otherwise is the window between this snapshot and the
* per-file re-read below it, and that window is the entire reason the re-read exists.
*
* **It has to be here and not around `listFiles()`.** A test that changes a file before the
* listing, or during it, changes what `StagingSweep.collectable` is given — so the file is
* never proposed for deletion and the re-read is never reached. The race being modelled is a
* file that *was* collectable when the snapshot was taken and is not by the time the delete
* comes round, which is exactly one worker resuming in this same process.
*/
protected open fun snapshot(listing: Array<File>): List<StagingSweep.Entry> =
listing.map { StagingSweep.Entry(it.name, it.lastModified()) }
private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile)
private companion object {
const val SPACE_HEADROOM_BYTES = 128L * 1024 * 1024
}
}