b41341a1cb865c1daad6c988ae346a6fc14ffd9a
20
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
ddfb1dd78e |
W1 (#154): cut the conversion state mapping into a seam, and choose all six arms
`ConversionViewModel.observe` maps a `WorkInfo` onto a `ConversionState`. That is
the app's main UI state machine, and no test had ever chosen which arm it took.
NOT COLD CODE, WHICH IS THE POINT. `ConversionViewModel$observe$1$1` already
reported 28 covered lines and 24 covered branches: every test that drives a real
worker runs this. But a real worker only ever reaches a terminal state with
well-formed output, so `SUCCEEDED`-with-a-path and `FAILED`-with-a-message were
the only arms any test had produced. The other six ran never -- the progress
read, both sides of the retry check, a success naming no file, a failure with
nothing to say, `CANCELLED`, and `BLOCKED`.
A grep makes that look untrue: all six `WorkInfo.State` constants appear in the
JVM suite. They are in `ReattachmentTest`, driven into `Reattachment.choose` --
a *different* function encoding the same enqueued-means-retry rule. So the rule
had a test in one of its two homes, and the copy the user's screen reads had
none.
THE SEAM. `workManager` comes from `WorkManager.getInstance` in the constructor
and `observe` is private, so nothing could hand this a chosen `WorkInfo`. The
`when` is now `conversionStateFrom`, a pure function over a `ConversionUpdate`
carrying only the fields it reads -- the same shape as `JobSnapshot` beside
`Reattachment.choose`, and its KDoc gives the same reason. `outputData` stays a
`Data`, which this suite already builds with `workDataOf` everywhere; unpacking
it into five nullable strings would move the same reads without helping.
TWO THINGS DELIBERATELY LEFT OUTSIDE IT:
- The ownership check stays at the call site. Its comment says it guards the
file ownership the SUCCEEDED arm takes, not merely the assignment, so moving
it inside would change what it protects.
- The mapping takes no responsibility for the staged file. It returns the
state; the caller reads the file off the result. That is strictly better
than the original, where `pendingStaged = staged` happened inside one arm:
"the state and `pendingStaged` refer to the same file or to no file" is now
the shape of the code rather than a rule two branches have to keep.
Mutations, each killing exactly the test it should:
| mutation | red test |
|---------------------------------------------|-----------------------------|
| progress read ignored | reports the progress |
| runAttemptCount ignored -> always Waiting | never run is simply starting|
| runAttemptCount ignored -> never Waiting | already run is waiting |
| success with no path -> empty Converted | named no file is a failure |
| blank name/type no longer falls back | blank falls back like missing|
| blank error no longer falls back | blank message falls back |
| cancellation ignores the caller's state | lands where caller said |
| BLOCKED remapped | blocked looks like starting |
`ENQUEUED` needs both mutations and both tests: either one alone passes against a
mapping that ignores `runAttemptCount` entirely.
The extracted functions carry `@UnstableApi` rather than swallowing the marker
with `@OptIn`, per CLAUDE.md -- lint's UnsafeOptInUsageError caught their absence.
An early `@Suppress("ReturnCount")` turned out to be unnecessary and was removed
rather than left: detekt is clean without it, and the file now carries none.
502 -> 516 tests, 87.1% -> 87.7% line, 69.1% -> 70.4% branch. Gate green:
assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck,
detekt, lintDebug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
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>
|
||
|
JMR-devandClaude Opus 5
|
c4bb7d4d2d |
Quote the rate the ticket settled on, and point the save gap at its ticket
Two accuracy fixes to notes the earlier commits left behind. The test KDocs carried "roughly 1-in-130" and a 400-leg-attempt denominator. Both come from earlier comments on #49 that its own census later replaced -- that ticket has three recorded corrections to its rate claims, and a superseded figure in a permanent comment is the exact thing its author kept having to fix. What survives the corrections is the count and the spread: four occurrences, API 33, 35 and 36, every one on attempt 1 and green on re-run. The save exemption described a real defect with nowhere to look it up. It is #123 now, so the KDoc names a number instead of trailing off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3599307040 |
Say what the save exemption does not cover, rather than implying it is total
The note claimed `save` is left unguarded because nothing can overwrite what it writes. That half is true -- the only observation that could belongs to a job already in a terminal state. The other half was missing: a save whose copy is still in flight when the user taps Start over lands `Saved` on a screen they have just cleared. Guarding it would drop that write instead, which reports nothing for a file that may genuinely have reached the destination. That is a question about what the screen should offer during a save, and answering it in a race fix would be deciding it by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
cc424dd08f |
Let the user's pick keep the screen a reattachment was about to take
`reattach()` read `_state.value`, found it `Idle`, and then handed the job to `observe()` -- which launches a *separate* coroutine that cannot write until its `collect` has resumed with a `WorkInfo`. So the check happened at one moment and the write landed at another, with a whole pick able to fit in between: the user tapped, their metadata query suspended, the guard saw an empty screen, and the finished job from an earlier session wrote over `Ready(picked)` a moment later. The comment above that guard said "no suspension point between this check and the assignment below, so nothing can interleave". There is no assignment below, and the two lines are in different coroutines. That sentence is why this sat as flaky CI for two days rather than being read as the product race it is. `ScreenOwnership` makes the answer the test already encodes -- the user's pick wins -- true rather than probable. A claim is taken synchronously when the user acts; every write that lands after a suspension point checks the claim it was made under and drops itself if that claim has been superseded. Dropped, not reordered: a write that is dropped cannot come back later. Cancelling the superseded observer was never enough on its own. `Job.cancel` is honoured at the next suspension point, and a collector that has already resumed and is on its way to `_state.value = ...` has none left; the write lands anyway. It also cannot help at all in the case reported, where nothing supersedes the observation until after it has been launched. `JoinViewModel` had the identical shape and nothing watching it, so it gets the same fix and the counterpart test that was missing. Its pick dispatcher becomes injectable for the same reason `ConversionViewModel`'s already was: without that seam there is no way to ask what happens while a pick is still in flight. Closes #49 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c887af0d83 |
Offer the file again after a failed save, rather than only offering to delete it
save()'s onFailure keeps the staged file on purpose -- it can be the only copy of an hour of transcoding, and the destination did not receive it -- and then handed the screen a Failed carrying a message and nothing else. That branch rendered exactly one control: "Start over", wired to reset(), which discards precisely the file the comment above it goes out of its way to keep. The intent was already written down in main; the UI did not honour it, and the only rescue was process death followed by reattach -- unadvertised, and bounded by a sweep that collects anything a day old. Failed now carries a PendingSave, and only where the failure came from save(). A transcode that died staged nothing and must not sprout a save button, so the handle is nullable and the observe() arm leaves it null; so does a save that found the file already gone. The branch renders "Try saving again" above "Start over", opening the same CreateDocument flow with the same name and type the first attempt used. A retry that fails again lands back on a carrying Failed rather than a bare one, so the second failure cannot eat what the first kept. Start over still deletes from there, and that is a decision rather than an inheritance: deletion is the user's choice only once the alternative has been offered. pendingStaged remains the single owner of the delete, so the carried handle is a view of it rather than a second owner and no path out of the state can drop a file the old shape could not. pendingSave() exists so save() and each screen's CreateDocument registration answer "what would a save target" once instead of twice -- the entry points cast to Converted/Joined, which answered null for a Failed and fell back to the current pickers, wrong for any spec edited since the job ran and for every reattached job. Both tabs, since JoinViewModel and JoinScreen have the same shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dbba213c51 |
Give the pick a dispatcher, so an escaped error fails the test that caused it
`onInputPicked` hops to a hard-coded `Dispatchers.IO` inside a `launch` with no exception handler -- deliberate, because a real OutOfMemoryError should reach the thread's default handler and take the process down. On the JVM there is no such handler: kotlinx-coroutines-test installs a process-wide collector, once per classloader and never removed, which keeps the error and rethrows it at whichever `runTest` starts next. Every Compose rule is a `runTest`, so the OOM raised by `ConversionViewModelProbeFailureTest` failed some *other* Compose class, and which one moved between runs of identical, green code. Naming the dispatcher gives the throw somewhere to land. With the pick inline inside a `runTest`, the collector's callback belongs to the test that caused the error, so it is handed over and consumed rather than stored for a stranger. Both hops of a pick rather than only the probe, which is where this differs from the seam issue #66 sketched: leaving the metadata query on a real IO thread makes the coroutine resume on a main looper Robolectric leaves paused, and that bounce is exactly the asynchrony that made delivery unpredictable. That buys the assertion the test could not make before -- the real OutOfMemoryError instance, not an inference from a card that never filled in, which is also what a probe returning null looks like. Reverting the hop to `Dispatchers.IO` turns it red: "expected java.lang.OutOfMemoryError to be thrown, but nothing was thrown". Refs #66 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
c5c4c5323b |
Test the edge that feeds reattachment, and stop it reporting ENOENT
Reattachment.choose has twenty tests and every mutation aimed at it bites. Everything that computes its inputs had none, and five mutations there passed the whole 257-test suite. Four are closed here, each verified by applying the mutation and watching the new test go red. jobSnapshots() is the half that has to touch WorkManager and the filesystem, so it is where the untested values live. JobSnapshotsTest drives it against a real WorkManager and a real cacheDir: - A zero-byte staged file is not an output. Relaxing the filter to `exists()` -- which is what a job killed before its engine wrote anything leaves behind -- made the snapshot claim a result, and the user would meet a Save button for a zero-byte "conversion". The same case pins that the path is still reported and that the mtime stays 0 for a file that is not a result. - Each result carries its own file's mtime. Hardcoding it to zero starves the newest-file tie-break of the only data it has, which is precisely the failure the tie-break exists to prevent: the query has no ORDER BY, so an arbitrary winner keeps winning every launch. Timestamps are set with setLastModified and compared against what the filesystem stored, because mtime granularity is not this test's claim to make. ReattachGuardsTest covers the two decisions the ViewModel makes that the pure rule cannot: - A file picked while the query was still in flight is not reattached over. Deleting the guard turns the user's pick into yesterday's job -- with the Save button pointing at a file the card does not name. Made deterministic by holding WorkManager's task executor rather than by racing two IO hops: the query cannot finish until the pick has landed. The test also asserts the brake really gripped, so a reattachment that never arrived cannot pass for one that was refused. - An Ambiguous result is offered without being attributed. Two finished jobs naming one staged file is what the device produced before staging was keyed on the job id; taking the first job's tags labels the file with the other conversion's name, which is the confident lie the KDoc rejects. The neutral label and the absent size are both pinned. ReattachmentTest's FAILED exclusion was only ever tested with pathless FAILED jobs, so a narrow regression ranking a FAILED job that carries a file like a result passed all 257 tests. The live shape is the 2 MB orphan the device pass found: a job killed mid-write leaves a partial, and under that regression the user is offered a truncated file with a Save button. One fixture with outputPath and outputExists set closes it. save() re-checks the staged file, in both ViewModels. The check reattachment made ran inside a tag query that can be hours older than the tap, and cacheDir is what the OS empties when it wants space and what the sweep collects after a day. The file's absence used to arrive as staged.inputStream() throwing, and e.message put "/data/user/0/.../4b4882....mp4: open failed: ENOENT" on screen -- a true statement about a path the user has never seen and cannot act on. It now reads as a sentence with an action in it. The message is one constant next to OutputPublisher because both ViewModels need it and staging is what it is about. Reattachment's KDoc claimed a defect that was fixed in the commit before it -- that "Start over" keeps its staged file -- which would send a maintainer to re-fix D2. Rewritten to say what is actually true: the delete happens, and the gap it leaves is the reset() whose delete is cancelled with the Activity, which is the sweep's job and is named in the sweep's own KDoc. R1 / #10, R2 / #11, R24 / #33, R25 / #34 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b86df47c43 |
Tell an unknown input size apart from an empty file
`queryFile` started at `var size = 0L` and only moved off it when a provider answered the
`OpenableColumns.SIZE` column, which the platform documents providers *may* omit. So "this file
is empty" and "nobody would tell me how big it is" reached `OutputPublisher.hasSpaceFor` as the
same number, and `hasSpaceFor(0)` is not a space check -- it is "is there 128 MB free", which any
phone with a working camera passes.
The reachable value is not an inference. On a Pixel 10 Pro XL, `contentResolver.query` on a
`file://` URI returns null outright, so the cursor block never runs at all and the default
survives untouched: `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that
exactly -- null query, and a descriptor that reports 4321 bytes for the same file -- which is why
every test here is a JVM test rather than a device one.
`InputQuery` replaces the two copies of `queryFile`, which were byte for byte identical in
`ConversionViewModel` and `JoinViewModel`, so a fix to either would have been a fix to half the
app. It asks for the size twice: what the provider says, and then what the file itself says
through `openFileDescriptor(uri, "r").statSize`. The second needs no cooperation beyond the input
being openable, which a conversion is about to require anyway, and it is what answers the device
case above. Only when both decline is the answer null, and `InputFile.sizeBytes` is `Long?` so
that null cannot be spelled the same way as zero again.
**What an unknown size does was the decision, and it is deliberately not a refusal.**
`OutputPublisher.hasSpaceForUnknownSize()` produces the same number the defect produced by
accident -- with no size to reserve for, the headroom is all there is left to check -- and that is
worth saying plainly rather than dressing up. What changed is that it is now the answer to a
question that was asked. `hasSpaceFor` means "there is room for this many bytes" and nothing else
claims it.
Refusing was the obvious alternative and would have been worse than the bug: it turns "no
provider answered the SIZE column" into "this file cannot be converted", for a user who can do
nothing about either. A fixed floor was the other, and there is no honest number for it -- a
1 GB floor refuses a 10 MB conversion on a device with 500 MB free, which is the same failure in
a costume. `SpaceCheckTest` pins the choice from both sides: an unmeasurable input must not end
the job, and a full disk must still refuse it.
That second half is why the default answers *through* `hasSpaceFor`. `FakeFailures.FullDisk` in
the instrumented suite overrides `hasSpaceFor` and nothing else, so the delegation is the only
reason it still refuses an unknown-size job. Replacing the delegation with a bare `true` leaves
the full-disk test red with `expected:<Failure {error : Not enough free space to convert.}> but
was:<Failure {error : FFmpegKit failed to start on brand: robolectric...}>` -- the job sailed past
the guard and died at the engine instead.
Both workers get the same shape. The size arrives as input `Data`, which has no null, so the
absence of the key *is* the unknown -- `getLong(key, 0L)` was the other half of the conflation.
When it is absent the worker measures the input itself, which it can do because it holds the URI:
that covers a `request(...)` built by hand and work enqueued before the size became optional, and
it costs an ordinary job nothing because it runs only on the fallback. `ConversionWorker.request`
writes neither the `Data` entry nor the `JobTags.sizeBytes` tag for a size nobody knows, since a
tag reading `size-bytes:0` would come back through `Reattachment` as a confident claim that the
user's file is empty -- and `reattach()`'s `?: 0L` is gone for the same reason.
A join's total is `InputQuery.total`, which is null the moment a *single* input cannot be sized.
Summing the ones that answered was the competing reading and is rejected: a lower bound is
indistinguishable from a real total once it reaches the space check, so the guard would reserve
for half the job and pass. Reverting it to `sumOf { it ?: 0L }` records `[1111]` for a two-file
join whose second input nothing can measure.
Work already in the queue keeps the old conflation, and there is no fixing it. The previous
`request()` always wrote `putLong(KEY_SIZE_BYTES, sizeBytes)`, so a job enqueued before this
commit for a file nothing could size carries the key *present* and set to zero -- which reads
back as a declared size of zero and is trusted, exactly as before. Its `lmc.size-bytes:0` tag
reads back the same way, so `reattach()` shows such a card "0 B" rather than "Size unknown".
Nothing can separate that from a genuinely empty file after the fact, and a rule that treated a
declared zero as suspect would only rebuild the conflation facing the other way. WorkManager
keeps finished work for about a week, so this is a bounded window that clears itself; new work
never enters it.
`hasSpaceFor`'s KDoc claimed peak usage was "roughly input + output at once" while the arithmetic
reserved `input + 128 MB`. The arithmetic is what stays and the doc now says why: `bytes` is the
input's size standing in for the output's, generous for the ordinary conversion (which is asked
for precisely because it shrinks its input) and short for a re-encode to a bulkier codec; the
128 MB absorbs that error and the transient double copy while `publish` runs. Reserving
`input + output` outright would refuse jobs that fit. This is a pre-flight check that stops an
obviously impossible job from spending minutes finding out, not a guarantee -- a conversion that
runs out of space anyway still fails through its engine.
The measurement side of that line is untouched on purpose. `StorageManager.getAllocatableBytes`
is a separate entry with its own device evidence and its own `informational += "UsableSpace"` in
the lint block; this commit is about the number going *in*. `hasSpaceFor(bytes: Long)` keeps its
signature and stays open, so nothing overriding it had to change.
The file card says "Size unknown" rather than `0 B`. Handled at the call site rather than inside
`formatBytes`, because a formatter that invented a number would be the defect on screen; the card
already degrades in words for a file nothing could read.
Five of the seven new tests were red before a line of production code moved, with the numbers
they were about: `expected:<4321> but was:<0>` for a picked file, `expected null, but was:<0>` for
one nothing can measure, `expected:<[1111, 2222]> but was:<[0, 0]>` for the join picker, and
`expected:<[4321]> but was:<[0]>` and `expected:<[3333]> but was:<[0]>` for what the two workers
asked the space check. The tests assert on the *question* rather than the verdict, which matters:
one that only checked whether the job ran would have passed against the defect, since the defect
is that the guard is vacuous rather than that it refuses.
The remaining two needed the new call to exist first, so each was proved by mutation instead.
Answering the unknown with `hasSpaceFor(0L)` inline leaves `expected:<[]> but was:<[0]>` in both
workers; refusing it instead leaves `an unknown size must not end the job; got Failure {error :
Not enough free space to convert.}`. Making the worker always measure rather than trust a declared
size leaves `expected:<[9999]> but was:<[4321]>`.
`join()`'s own use of `InputQuery.total` is tested separately from the function, because
`StagingCleanupSupport` already records what that distinction costs: a tool can be provably right
while nothing calls it. `SucceedingWorkerFactory` now keeps the input `Data` of every request that
reaches a worker -- the only place it is legible, since `WorkInfo` hands back a job's tags and its
output and never the `Data` it was built with -- and the test reads the enqueued total off it.
Restoring `inputs.sumOf { it.sizeBytes ?: 0L }` leaves every other test in the change green and
this one red with `a total that could not be worked out must not be enqueued as a number`.
`ConversionViewModelProbeFailureTest` expected `InputFile(INPUT, "input", 0L)` for an authority no
provider serves. It expects `sizeBytes = null` now, which is the behaviour change stated where a
reader will meet it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
49535998b5 | Merge branch 'fix/worker-durability-and-naming' into scratch/integrate-d2-d3 | ||
|
JMR-devandClaude Opus 5
|
159320dfa0 |
Name a finished file after the job that made it
Six places decided what a converted or joined file should be called, and not one of them asked
the job. `JoinViewModel` reported `JoinState.Saved("joined.mp4")` whatever the format;
`JoinScreen` opened `CreateDocument("video/mp4")` and launched it with `"joined.mp4"`. On the
convert side `save()` and `suggestedOutputName()` built the name from `_settings.value.spec`, and
the screen took the MIME type from the same place -- the picker as it stands *now*, which is not
the spec the job ran with.
All six are right today, and all six are right by accident. The join screen has no format picker,
so the three MP4 literals agree with `ConcatWorker.request`'s default. The conversion pickers are
drawn only in the `Ready` state, so the settings cannot move between enqueue and save. Neither
accident is load-bearing anywhere it is written down.
One of them has already stopped holding, quietly. A job picked up by `reattach()` ran with a spec
that was never in this ViewModel's settings, because those settings belong to a process that no
longer exists -- so a reattached MP3 conversion is offered `.mp4` and `video/mp4` today. The
previous commit made that path more reachable rather than less: `Reattachment` exists precisely
to find work this ViewModel did not start.
The fix is to ask the only thing that knows. The spec travels to the worker as input `Data` and
`WorkInfo` hands input `Data` back to nobody, so the worker is the single point at which the
input's name and the spec that ran are both in scope. Both workers now report the two derived
strings -- the name to suggest and the type to open the dialog with -- in their output `Data`,
and they ride on `ConversionState.Converted` and `JoinState.Joined` from there. `save()` reports
what it saved rather than recomputing it, and both screens read the name and the MIME type off
the state they already collect, remembering their `CreateDocument` contract against that type
instead of a literal. The MIME type is not cosmetic: some providers rewrite a document's
extension to match it, so an MP3 offered as `video/webm` can arrive with the wrong one.
`suggestedOutputName()` is deleted rather than repaired. With the answer on the state there is no
caller left for it, and an accessor recomputing the same string would only be a second place for
it to be wrong -- which is what it was.
`ConcatWorker` gains `DEFAULT_FORMAT` and `outputNameFor(format)`. Three copies of "MP4" is the
shape this entry is about, and the input-Data default, `request`'s parameter default and the
ViewModel's fallback for a job that predates this change are exactly three copies.
Work already in the queue carries neither string, and WorkManager keeps finished work for about a
week, so that is the ordinary case for a few days rather than a corner. Those fall back to the
old derivation, which is a guess -- but it is the same guess the app was already making, it is
confined to jobs enqueued before this commit, and for such a job there is genuinely nothing
better to hand. New work never reaches it. The join's fallback is not even a guess: the format
`ConcatWorker.request` has always defaulted to is the format such a job really used.
`reattach()`'s KDoc carried this as a known wart it was deliberately leaving alone. That
paragraph is now a description of the fix rather than of a defect.
Tested at both ends, since either alone would pass while the other was wrong.
`WorkerOutputNamingTest` runs the real worker on an MP3 job -- nothing like the default preset, so
a name built from the picker is visibly wrong rather than accidentally right -- and compares the
whole success `Result`, which is what makes a missing key fail rather than go unnoticed. The two
ViewModel tests drive a finished job and then move the picker, which is what a reattached job
amounts to from the ViewModel's point of view.
Restoring just the two name sources -- `save()`'s `outputNameFor(displayName, _settings.value.spec)`
and `JoinState.Saved("joined.mp4")` -- turns them red with
`expected:<holiday_converted.mp3> but was:<input_converted.webm>` and
`expected:<joined.mkv> but was:<joined.mp4>`. The convert side loses the extension *and* the name:
`_settings.value` had been moved to WebM, and the display name a reattached job cannot supply had
already fallen back to the placeholder.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
2a68f03134 |
Give every job a staging path of its own
`<cacheDir>/conversions/` is shared by the convert tab, the join tab and `ConcatEngine`, and
until now none of the three named a file that belonged to one job. A conversion derived its name
from the input's display name, so two `holiday.mp4` from different folders wrote the same file.
A join used the constant `joined.<ext>`, so any two joins of one format did. The list file was
the constant `concat_list.txt`, so any two joins at all did, and one of them would read the
other's input list.
The naming half is not a hypothesis. Two independent conversions on a Pixel each computed
`cache/conversions/input_converted.mp4`, the second silently overwrote the first, and a tag query
in a fresh process then returned **two SUCCEEDED `WorkInfo`s naming that one file** with one file
on disk. That is the collision reaching the point where it makes a *fix* ambiguous rather than
just a file: `Reattachment` can offer the bytes, because they are the user's either way, but it
cannot say which job produced them.
`StagingNames` keys the name on the WorkManager request id. That id is what stays still across a
retry -- `WorkerWrapper` builds `WorkerParameters` from the `WorkSpec` id and only increments
`runAttemptCount` -- which matters more here than uniqueness does, and matters more since the
previous commit made retries routine. A failed attempt deletes its staged file on the way out,
and that only collects the partial the *previous* attempt left when the name has not moved.
Opaque rather than sanitised, deliberately. The staged name is never shown to anyone: `save()`
recomputes a suggested name and the user picks the real one in the SAF dialog. So there was
nothing to lose by dropping the display name, and something to gain -- a provider-supplied
display name can contain a separator, be empty, or be four kilobytes long, and `File(stagingDir,
"../escape_converted.mp4")` resolves to a path outside staging. That was reachable before this
commit and is now unreachable by construction rather than by a sanitiser that has to be right
about every case. There is a test for exactly that name.
The extension stays, and is not decoration: `FFmpegConcatCommand` names no output muxer, so
FFmpeg infers it from the output path. A fully opaque name would quietly produce the wrong
container.
`ConcatEngine`'s list file is derived from the output it belongs to rather than taking another
parameter, so the two cannot drift apart, a directory listing shows which list belongs to which
join, and the sweep ages them together.
Three neighbouring comments claimed things that are no longer true, and are corrected rather than
left to mislead the next reader:
- `Reattachment.Ambiguous` said it "resolves on its own once each job stages under a name of
its own". It now does -- for work enqueued from here on. The case is **kept**, because the
queue outlives the change: WorkManager holds finished work for about a week, and the jobs
likeliest to be sitting in it when this code first runs are the ones named the old way.
Behaviour is unchanged and `ReattachmentTest` is untouched.
- `OutputPublisher.sweepStaging` justified its age rule partly on there being "no per-job
namespacing". There is now, and the rule still stands on its own: per-job names stop two jobs
from sharing a file, and say nothing about whether a file's job is still running, which is the
question a sweep actually asks. Same for `StagingSweep` and the note in
`LibreMediaConverterApp`.
- Both ViewModels' `reattach()` explained aliasing as something nothing prevented. Narrowed to
what is still true of work already in the queue.
`ConcatEngineTest` asks `StagingNames` for the list file's name instead of spelling out
`concat_list.txt`. That is the difference between a test and a tautology: a literal there would
have gone on passing after the rename while asserting that a file nothing creates does not exist.
The same trap was live in the two worker tests from the previous commits, whose staged-file
assertions computed a path of their own -- they now assert on the staging directory being empty,
which cannot go vacuous when a name moves.
`PerJobStagingTest` drives the real worker, because the naming function was never the part that
was wrong: what was wrong was which name the worker asked for. Two jobs converting one file must
leave two files; a second attempt at one job must not leave a second; and a display name that
climbs out of staging must not. The first and third fail before the change with "each job must
have staged its own file, found [input_converted.mp4] expected:<2> but was:<1>" and "the output
belongs in staging expected:<1> but was:<0>" -- the latter because the file had landed in
`cacheDir` instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
5c27801c77 |
Retry work the system refused to start, instead of failing it terminally
`ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated
reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute
transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with
no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the
system refused it:
WM-ForceStopRunnable: Found unfinished work, scheduling it.
ActivityManager: Background started FGS: Disallowed [callingPackage:
org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED]
WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException
WM-WorkerWrapper: Worker result FAILURE
WM-Processor: Processor 3d1c9862 executed; reschedule = false
`Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was
ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have
carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the
runtime guarantee once a service is running, not permission to start one.
The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its
throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the
`workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the
device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data`
of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed."
with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging.
`ConcatWorker` had the same shape.
So `setForeground` moves inside the `try` in both workers, and the staging handle is named just
above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it,
so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the
`try` for the same reason and had the same problem; it is now inside too. On a retry the staged
name is unchanged, so the delete on the way out collects the partial the killed attempt left
behind rather than orphaning it.
What to return was the real decision. `Result.retry()`, because the denial is about *when* the
job ran and not about the job: the file is fine, the settings are fine, and the one thing that
grants an app permission to start a foreground service is being in the foreground, which the
user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded
retry means a job nobody comes back for waking the device forever while the screen says "paused"
and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against
the default backoff rather than as a round number: exponential from
`WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS`
(5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can
act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones --
WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget
will fail on its first denial rather than getting ten of its own. That is accepted rather than
overlooked, and the timeout branch ignores the count entirely so the budget's own retries are
untouched.
The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate
a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason
*and* the cause, with precedence stated rather than left to branch order -- once something has
stopped the worker, the exception it was holding describes that stop and not a reason of its own.
The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its
`IllegalStateException` supertype: a muxer that was never started throws one of those too, and
matching the supertype would retry every ordinary failure for eight hours.
Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler
expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the
exemption it buys is for starting, and the quota it costs would be paid by every job.
Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for
the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must
not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout
ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect
actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground`
throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it
propagates the exception to the caller and `ListenableFuture.await()` unwraps the
`ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of
those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same
line the device logged.
Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting`
said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause,
where a denied restart is now the likelier of the two; and `Reattachment.choose` justified
excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is
the sentence this commit falsifies. The exclusion stands on its own and says so now.
The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through
a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh
attempt meets an empty screen rather than the message. What the bound reliably buys is the end of
the retrying.
Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind
it, which cannot distinguish the two causes that now reach it, and the old copy named only the
six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along"
covers both, and for a denied start it is not filler but the actual remedy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
97fdc49f01 |
Guard the native boundary against what it actually throws
D14: picking a file died instead of reporting an unreadable one when FFmpegKit's native library could not load. `probeWithFFprobe` guarded its call with `catch (e: Exception)`, and `ConversionViewModel.onInputPicked` guarded nothing, so the failure escaped a `viewModelScope.launch` -- which has no handler, and on a device ends the process. All three of the obvious narrow guards catch nothing, which is why this needed reading the AAR rather than guessing. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` raises and rethrows a *bare* `java.lang.Error` wrapping it, so `UnsatisfiedLinkError` never escapes and the escaping type carries no information at all. Every touch of the class after the first is a different type again -- `NoClassDefFoundError` -- so a guard written for the first shape lets the second pick onwards crash, which is the harder half to notice. Both are in the test output verbatim. `catch (Throwable)` was the wrong answer for the reason the audit gave: it would swallow a genuine `OutOfMemoryError` in a method that spawns a native process, turning "this device is out of memory" into "this file looks unreadable" and letting the app act on it. So the line is drawn by a named predicate, `isNativeLoadFailure`, rather than by the catch clause -- every class-loading shape is a `LinkageError`, and nothing that means the JVM is failing is one. That disjointness is what makes the guard narrow. This is consistent with the position `config/detekt/detekt.yml` already takes for `TooGenericExceptionCaught`: the boundary's failure types are undocumented, so guessing crashes the app on a file it could have reported. One level up the opposite mistake is available too, and the predicate is what lets both be avoided at once. `TooGenericExceptionThrown` is relaxed for the test source sets only. A test that reproduces a failed native load has to throw what the library throws, and a tidier subclass would leave it passing against a defect it no longer reproduces. Main source is untouched by that and throws nothing generic. `ConversionDependencies.probe`'s KDoc is rewritten rather than left. It said this hazard was "deliberately not fixed here ... its own commit, with its own test", which this is -- leaving it would have replaced one true comment with a false one, which is the same defect class as the D11 work. Both halves are covered independently: reverting the `MediaProbe` catch reds only the two `MediaProbeNativeLoadTest` cases, reverting the ViewModel guard reds only the two injected-seam cases, and widening the ViewModel guard to `Throwable` reds the OutOfMemoryError case -- so the narrowness is pinned, not just the catch. Audited the sibling boundaries named in the audit and left all three alone: `FFmpegEngine` and `ConcatEngine` both construct and run under `catch (e: Throwable)` in their workers, and `Media3Engine` has no native loader of this kind and already routes failures through `runCatching`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-dev
|
eb37f9ebea |
Merge branch 'fix/reattach-unfinished-work' into scratch/integrate-d2-d3
# Conflicts: # app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt |
||
|
JMR-devandClaude Opus 5
|
ec969c41dc |
Reattach to conversions and joins the ViewModel did not start
The queue surviving process death is the stated reason this app uses WorkManager, and
the ViewModel was where that protection stopped. `activeWorkId` and `observer` are
plain fields, so a process reclaimed after a conversion finished came back to Idle
while the output sat in `cacheDir` with nothing in the UI able to reach it. The
realistic window is not a crash mid-transcode -- it is the job finishing, the user not
saving yet, and the process being reclaimed hours later as an ordinary background one.
Both ViewModels now query their own worker's class name on init and pick up what they
find. Nothing is persisted for it, and nothing needed to be: `WorkRequest.Builder`
seeds every request's tag set with `workerClass.name` (`tags = mutableSetOf(
workerClass.name)`, work-runtime 2.11.2), and R8 keeps those names through
work-runtime's own consumer rule, `-keepnames class * extends
androidx.work.ListenableWorker`. A UUID in a `SavedStateHandle` would have been both
more machinery and less: it cannot find work enqueued by a previous install.
What the query cannot return is the job's input. `WorkInfo` hands back id, state, tags,
progress, output and run-attempt count -- never the `Data` a request was enqueued with
-- so a ViewModel could see that a conversion existed and where its output went, but
not which file it was converting. The display name and size therefore ride on tags too,
which is the whole of the production change to the request builders. The picked `Uri`
deliberately does not: nothing in a reattached state reads it, and a `content://` grant
taken by a picker in a process that no longer exists is not something to hand back as
though it still worked.
The decision is a pure function on the JVM test stack, following `FailureOutcome`:
`Reattachment.choose` takes what WorkManager reported and answers which job, if any.
Cancelled work is excluded -- the user already said no. Failed work is excluded, which
matters more than it looks now that a device has shown an interrupted worker coming
back FAILED rather than retried, its restart's `setForeground` refused as a background
foreground-service start: nothing marks a failure as seen, so it would otherwise
reappear on every launch. A success whose staged file is gone is excluded, because a
Save button that fails on tap is worse than no button. Live work outranks a finished
result, since a running job holds a foreground notification and someone opening the app
while that notification is in the shade is looking for that conversion.
Where several jobs qualify, the newest staged file wins, and that is not a detail.
Losing a tie is not the same as waiting for the next launch: the tag query has no
`ORDER BY`, so its order is unspecified but stable, and an arbitrary winner would keep
winning every launch while the other result stayed unreachable for as long as its file
existed. It is reachable today -- dismiss one result with "Start over", which leaves its
file behind, then convert something else and do not save it. `WorkInfo` carries no
timestamp of any kind, but the edge is already stat'ing the file, so the file's own
mtime is the ordering. A clock that moves backwards makes it a heuristic; an order that
is unspecified and repeats itself is worse.
Aliases are the tie that does not resolve, and that case is not hypothetical. A tag
query on a device returned two SUCCEEDED jobs whose output paths were both
`.../conversions/input_converted.mp4`, with one file on disk -- the staging name is
derived from the input's display name, so a later job overwrites an earlier one's output
and both go on reporting it. Checking the file does not separate them, and neither does
its mtime, since they share it. So the two halves are separated instead. The file is
offered, because it is the user's file either way and losing it is the defect being
fixed; the label is not, because saying which job produced it would be a guess.
`Reattachment.Ambiguous` says so and the card falls back to a neutral name rather than
borrowing the other job's. Aliases whose tags are identical -- the ordinary case, the
same file converted twice -- stay attributed, since nothing turns on which wrote it.
Age is not filtered on, and cannot be: WorkManager keeps finished work about a week and
prunes on its own schedule, so a query in a fresh process routinely returns jobs from
earlier sessions. Whether the staged file is still there is the only signal separating a
result still worth offering from one already dealt with, which is why that check carries
the weight.
Two smaller behaviours fall out of reattaching rather than starting:
- A reattached job that is cancelled lands on Idle rather than Ready. Ready would put
a Convert button over an input URI that belongs to a dead process. `observe()` takes
the cancelled destination as a defaulted parameter, so a job started here is
unchanged.
- A FAILED job's message falls back when blank, not only when absent. A worker killed
before it can report leaves no output data at all, and an exception's message can be
the empty string; both used to reach the screen as a failure with nothing said.
Deliberately not fixed here, each being its own change: the save dialog's suggested name
and MIME still come from the current picker rather than from the job that ran, so a
reattached job in a non-default format is offered the default extension; a result
dismissed with "Start over" still keeps its staged file, so it can be offered again next
launch -- the file check closes that for free once the file is deleted; and
`setForeground` still sits outside `doWork`'s try, so its throw bypasses the retry
decision entirely.
Tested where it can be. 28 JVM tests cover the choice and the tag round trip, including
the ordering, the aliasing rules and a display name that looks like another tag. The
reattachment itself is instrumented: a ViewModel constructed against the real
WorkManager is the next launch, with no memory of the work. `WorkManagerTestInitHelper`
is deliberately not used -- its `setDelegate` replaces the singleton for the whole
process, which would quietly turn `ConversionWorkerTest` into a synchronous test double
depending on class order.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
cfd705af05 |
Delete staged output the user never saved, instead of waiting for the OS
Every conversion writes a full-size file into <cacheDir>/conversions/. save()
published it and deleted it, but reset() -- what the "Start over" button on the
Converted and Joined states calls -- dropped the File reference and left the file
behind. Converting something and deciding not to save it is an ordinary path
through the UI, so it leaked a full-size copy every time. cacheDir is evictable,
so this was never unbounded growth; it was the app relying on the OS to clean up
after it, and on a device under no storage pressure "eventually" means never.
OutputPublisher.clearStaging() was written for exactly this and called from
nowhere. It is NOT wired up here -- it is deleted. It emptied the directory
unconditionally, and the convert tab, the join tab and ConcatEngine's
concat_list.txt all share that directory with no per-job namespacing (D8), so a
blanket delete could take a file out from under a running job. Two narrower
methods replace it:
discardStaged(file) one file, guarded. The handle reaches the ViewModel as a
path string in WorkInfo.outputData and becomes a File with
nothing checking where it points, so this compares the
CANONICAL parent against the staging dir -- the naive
string comparison accepts conversions/../elsewhere.
sweepStaging(now) age-based, for orphans no ViewModel is left to clean up.
The cleanup handle is a ViewModel field, not something read back out of the state
machine, because the state machine cannot answer it on the path that needs it
most: a failed save lands on Failed(message), which carries no file reference at
all. On that path the file is deliberately kept -- it may be the only copy of an
hour of transcoding and the destination did not receive it, so deleting to tidy a
cache directory would destroy the work. It stays collectable by a later reset()
or by the sweep.
The sweep runs once per process from a new Application subclass, off the main
thread. The reason it cannot race a live job is the grace period, not ordering:
WorkManager initialises through androidx.startup's InitializationProvider, a
ContentProvider, so it is already up before onCreate() and can be resuming a
worker in this same process while the sweep runs. StagingSweep only collects a
file nothing has written to for 24 hours. Outputs are written continuously and
keep their own mtime fresh; concat_list.txt is the one file written once and then
only read, and a WorkManager attempt is capped by the six-hour foreground-service
budget with retries restarting doWork() from the top, so no attempt can hold a
file still for a day. sweepStaging() also re-reads each timestamp immediately
before deleting, closing the window between listing the directory and acting on
the list -- unlinking an inode a running job still holds open would end with the
job reporting success for a path that no longer exists.
The rule itself is a pure function over (name, lastModifiedMs) pairs and a clock.
Timestamps are values rather than Files so the tests measure the arithmetic --
the grace boundary, and a clock that moved backwards -- rather than the
filesystem's mtime granularity.
Tests cover the tool AND the wiring, because the wiring is where the defect was.
A pure rule test and a Robolectric test of OutputPublisher both stay green when
the discardStaged call is deleted from reset(), which would have made the number
read as coverage of a bug that was still there. So both ViewModels are driven --
through a real WorkManager, to Converted/Joined -- and then asserted on the
filesystem: Start over deletes the staged file; a successful save leaves nothing
to delete twice; a failed save keeps the file and a later reset collects it, which
pins the argued decision above rather than leaving it as a comment. Verified by
deleting the discardStaged line from both reset() methods: 4 of the 6 fail with
"reset() should have discarded exactly the staged file expected:<[...]> but
was:<[]>", and the two save-path tests correctly stay green.
Three things made that reachable, all reusing what was already here:
- Both ViewModels now resolve their publisher through ConversionDependencies,
like the workers already did. They were the only place bypassing the seam.
- MediaProbe joins that seam too. It spawns FFprobe, and FFmpegKit's loader
throws a bare java.lang.Error when the native library is absent -- which its
own `catch (e: Exception)` cannot catch, so every JVM test died on the file
pick. Instrumented tests are unaffected and still get the real probe.
That error path is a LATENT PRODUCTION HAZARD, recorded in the KDoc and
deliberately not fixed here: onInputPicked does not catch it either, so a
missing .so would surface as an uncaught error rather than the "could not read
this file" the code was written to give. It cannot fire on a device that ships
the libraries, so widening MediaProbe's catch to Throwable would change the
pick path on the strength of a condition no user meets. Its own commit.
- reset()'s cleanup dispatcher is a constructor parameter defaulting to
Dispatchers.IO, which makes the delete assertable and states the ordering --
Idle is published synchronously, the delete is dispatched -- as a decision
rather than an accident. @JvmOverloads keeps the single-argument constructor
that viewModel()'s AndroidViewModelFactory looks up reflectively.
androidx-work-testing was already in the catalog and already inside the prerelease
guard via its androidx. group, so it needed no new pinning argument.
Robolectric is added for the one assertion no pure function can make: that the
file is really gone from a real cacheDir. It is PINNED at 4.16.1 and belongs with
ktlint/detekt/jacoco rather than the floating libraries. The prerelease guard in
app/build.gradle.kts only covers androidx., junit and com.arthenica, so
org.robolectric is unguarded and a "4.+" would resolve to 4.17-beta-3; beyond
that, a bump changes which android-all jar the tests execute against, which is the
same "a tool moved under a diff that cannot explain it" failure the linters are
pinned for.
Two things Robolectric needed. testOptions did not exist in this module at all;
it now sets isIncludeAndroidResources so the merged manifest and resource table
reach the JVM tests, and grants --enable-native-access, which Java 25 otherwise
warns about four times per run when Robolectric's native runtime calls
System.load(). And robolectric.properties pins sdk=36: Robolectric defaults to the
manifest's targetSdk of 37, there is no android-all jar for 37, and the class
fails to initialise before any test body runs. 36 is where CI's emulator matrix
already stops, so this does not widen the gap -- API 37 was already a manual check
on the Pixel 10 Pro XL before each release.
Known and left alone: a conversion that fails inside the worker never reaches
Converted, so pendingStaged is never set and any partial output relies on the
sweep alone. reset()'s delete is also fire-and-forget on viewModelScope, so it is
cancelled if the Activity finishes first. The sweep is the backstop for both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
496f1c7e73 |
Apply ktlintFormat
Tool output only, no hand edits, so this is safe to read with whitespace diffing off. It is its own commit for exactly that reason: a whole-repo reformat folded into the commit that configured the formatter would have made both unreviewable. What it did, mostly: trailing commas on wrapped argument lists, signatures collapsed onto one line where they fit inside 120 columns, import order, and four genuinely unused imports removed. Unit tests pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2e0cf2f5d6 |
Let the user pick a container and codecs independently, and remux without re-encoding
OutputFormat was a closed enum of twelve (container, videoCodec, audioCodec) triples, defended on the grounds that a closed set was what made routing decidable. Two things it could not express: changing the container while copying the streams, and choosing codecs per track. OutputSpec replaces it as the vocabulary; OutputFormat stays as presets over it. Decidability moves to ContainerCapabilities, which is explicit and unit-tested rather than implicit in whichever combinations somebody enumerated. The matrix is indexed by (container, codec, trackType, mode), not one boolean. "Can MP4 carry AV1" and "can this app encode AV1" have different answers, and copy is where the difference shows: a single flag would refuse a legitimate remux or promise an encode neither engine can deliver. COPY is a codec value rather than a flag, so every exhaustive `when` in the codebase had to say what it does about copying. CopyPlanner resolves it before anything else reads the request, and inherits ConcatPlanner's rule that an unproven match is never a copy — a needless re-encode costs time, a wrong stream copy costs a file that will not play. Container now drives -f, the extension and the SAF MIME type, so Matroska without video is .mka and MP4 without video is .m4a without a preset for each. FLAC was declared as Container.MKV with a .flac extension, inert only while nothing read the container; it now has its own. Six containers added: MOV, MKV audio, MPEG-TS, AVI, FLV and WMV/ASF. Routing asks the plan, never the request. COPY belongs to none of the capability sets, so testing the request directly sends every remux to FFmpeg on the first check — and nothing notices, because -c copy produces a correct file, just on the CPU. The router also learns what Media3 can *carry* as opposed to encode: its MP4 muxer takes AAC, Opus, Vorbis and PCM but neither MP3 nor FLAC. MediaProbe now separates "no video track" from "could not parse" and reports the source container, which MediaExtractor cannot supply at all. FFprobe runs on every pick for that reason, not as a fallback. The Advanced picker shows the whole matrix and lets an impossible combination be selected on purpose, then explains it and offers alternatives. Convert is what blocks the job. ConversionWorker validates too, so a stale queued spec fails with the reason rather than being coerced into something else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
dd2fcf0a19 |
Rename the project to LibreMediaConverter
Done now rather than later: the application ID is permanent once published -- Play treats a change as an entirely different app -- so this is the last cheap moment to choose it. applicationId / namespace dev.jasonmross.mediaconverter -> org.libremediaconverter source tree java/dev/jasonmross/mediaconverter -> java/org/libremediaconverter gradle project AndroidMediaConverter -> LibreMediaConverter theme Theme.MediaConverter -> Theme.LibreMediaConverter compose theme MediaConverterTheme -> LibreMediaConverterTheme display name "Media Converter" -> "LibreMediaConverter" org.* rather than dev.jasonmross.* because "Libre" signals a project rather than a personal app, and a project-owned namespace lets maintainership move later without the identifier contradicting reality. The source trees moved with git mv so history follows the files instead of showing 42 deletions beside 42 additions. Verified after the rename: 66 unit tests, and 40 instrumented tests on an API 36 emulator, 0 failures. The built APK reports org.libremediaconverter, and no stale jasonmross, AndroidMediaConverter or MediaConverterTheme identifiers remain anywhere in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |