Stop a permission answer starting a second conversion #216
Merged
JMR-dev
merged 10 commits from 2026-09-06 01:19:20 +00:00
fix/convert-guards-on-ready into main
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
4e88de3045 |
Stop a permission answer starting a second conversion (#202)
currentInput() answered for Converting, Waiting and Converted as well as Ready. Those three arms were unreachable by tapping Convert -- the button renders only in the Ready branch -- but they were reachable through the POST_NOTIFICATIONS *result*, which ConverterScreen.kt:91 wires to convert() rather than to the button. Reaching one enqueued a SECOND job over a live one: activeWorkId was overwritten, and the first job kept running with its foreground notification orphaned and nothing left holding its id to cancel it. #202 decided to narrow rather than to test it as it stood, because a test written against the old shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode docs/coverage-read-findings.md names. currentInput() is now (_state.value as? ConversionState.Ready)?.input, which is what JoinViewModel.join() has been all along; the two screens are the same shape and only one of them was over-general. Four cold refusal arms come with it, all reached the same way -- a system callback arriving after the screen has moved on, which is what a result redelivered after process death does: ConversionViewModel.kt:513 currentInput() ?: return ConversionViewModel.kt:600 pendingSave() ?: return JoinViewModel.kt:316 (as? Ready)?.inputs ?: return JoinViewModel.kt:390 pendingSave() ?: return One fixture note worth keeping: the second case needs a real staged file in the worker's output Data. A SUCCEEDED job with no output path maps to Failed rather than Converted, so Data.EMPTY never reaches the state the case is about -- which cost a timed-out awaitState before it was spotted. Mutations, all run and restored: restore the over-general four-arm when 1 red <- the defect this change fixes currentInput()!! at :513 2 red pendingSave()!! in save() 1 red drop both join guards 1 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2db0dc65d3 |
Open the save dialog with the type the job produced (#201)
ConverterScreen.kt:80 -- state.pendingSave()?.mimeType ?: settings.spec.mimeType -- had never taken its left-hand side. Its comment records what the line is for: a retry after a failed save must open with the type its FIRST attempt used, because the fallback beside it is the current picker, which a reattached job never set. So the untested half is the fix and the tested half is the fallback it was added to stop being used. This withdraws a named exemption rather than working around it. FailedSaveRetryTest's KDoc listed the line as not asserted because "it lives in the entry point, above the ScreenContent seam, and reaching it needs a real ViewModel inside a composition". True when written; AdaptiveShellTest (#173) then established composing the real screens with real ViewModels, and #200 added the ShadowActivity mechanics for reading what a launcher launched. The reason the exemption gave no longer holds, so it is withdrawn in the same change rather than left to be taken at face value -- the shape of #141 revising #84's boundary. The job is reattached rather than run because the screen composes its own ViewModel through viewModel() and nothing can be injected into it. That is also the case the line exists for: a reattached job's spec was never in these settings at all. The test asserts the two mime types differ as well as which one is used. Without that, the assertion would pass just as well against the fallback if the fixture ever drifted onto MP4. Mutation: collapse :80 to settings.spec.mimeType -- red. Run and restored. Unrelated, and recorded because it turned up here: #159 now reproduces on this host. The full suite failed once in six runs on OutputPublisherStagingTest:184, and the isolating experiment says it is not this change -- three runs WITH the new test all passed, and the failure occurred on a run with the file removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
6334dcba34 |
Pin the launcher layer, where two callbacks share a signature (#200)
ConversionViewModel.onInputPicked(uri: Uri) and .save(destination: Uri) are both
(Uri) -> Unit, so swapping the two launcher callbacks at ConverterScreen.kt:70 and :83
compiles, renders, and passed the entire suite. Picking a file would attempt a save to it;
choosing a destination would load it as input.
That is the defect class ScreenWiringTest exists for, on the one pair it declines to cover:
it drives converterActions directly and says the launcher-backed actions stay parameters.
Right about the actions seam, and it leaves the edge above that seam unpinned. Join's
equivalents are List<Uri> and Uri, so they are not transposable and get no such test.
Two mechanics, neither used anywhere else in the suite, so both were spiked before any
assertion was written:
shadowOf(activity).nextStartedActivityForResult reads the launched Intent, EXTRA_MIME_TYPES intact
shadowOf(activity).receiveResult(...) reaches ComponentActivity's ActivityResultRegistry
and fires the rememberLauncherForActivityResult callback
createAndroidComposeRule for AdaptiveShellTest's reason: the screens compose real ViewModels
through viewModel(), and the plain rule supplies no ViewModelStoreOwner.
The picker filter rides along, since the harness is the same. ConverterScreen.kt:65-67
records why the all-types wildcard is load-bearing -- the photo picker offers no audio and
misses mkv/flac/webm -- and narrowing it would have made every audio conversion unreachable
from the picker with nothing going red.
One incidental: a KDoc cannot contain the all-types wildcard, because its second half closes
the block comment. The literal is spelled only in the assertion, and the KDoc says why.
Mutations, all run and restored:
transpose onInputPicked and save 1 red
narrow the converter picker to video only 1 red
widen the join picker to every type 1 red
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
7e09f010c7 |
Call the theme the way MainActivity calls it (#197)
ThemeColorSchemeTest resolves every branch of the `when` and always passes darkTheme explicitly, so the $default bridge is never entered and isSystemInDarkTheme() is never called. MainActivity.kt:79 is its only default-argument caller and does not execute on the JVM, which left the app's actual call shape -- no arguments at all -- the one nothing exercised. LibreMediaConverterTheme reported mi=21, mb=6, cb=12 at method level. Not #68. That issue is the two unreachable arms, DarkColorScheme and LightColorScheme, which cannot run because dynamicColor is always true and nothing can flip it; it is an open product decision and stays open. This is the reachable half. The assertion compares schemes rather than reading a luminance threshold, which would be a guess about the device palette. What is asserted is that the no-argument call resolves the SAME scheme an explicit darkTheme of the matching value does, and a different one from its opposite -- true whatever palette the platform hands back, and exactly the claim being made: the default reads the system rather than picking a side. The two assertions are also what stops the pair passing vacuously if all three resolutions were identical. Two @Config(qualifiers = ...) cases rather than two classes: qualifiers are settable per method, unlike the sdk pinning ForegroundTypeRegimeTest needed nested classes for. Mutations, all run and restored, and each reddening a different half -- which is also what shows the qualifiers take effect rather than both cases running in one mode: darkTheme defaulted to false night case red darkTheme defaulted to true light case red isSystemInDarkTheme() inverted both red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
49249be280 |
Report hardware progress to WorkManager, which nothing had checked (#196)
ConversionWorker.kt:208-210 is a second onProgress lambda at a second call site -- the one handed to engine.transcode -- and it reported ci == 0. Every test in this file drives the FFmpeg path; HardwareFallbackTest reaches runMedia3OrFallBack but its recording transcoder records the call and never invokes the callback it was handed. So the two engines' progress wiring was one tested and one not, and the untested one is the default: ConversionRouter sends everything it can to Media3, which makes this the lambda most conversions actually use. Same asymmetry argument CLAUDE.md records for ContainerCapabilities:94. It goes in this file rather than beside HardwareFallbackTest because this is where progress plumbing lives and where RecordingForegroundUpdater already is -- and because the software and hardware cases now sit side by side, which is what makes the asymmetry visible rather than merely fixed. workerReporting gains an engine-preference parameter defaulted to FORCE_SOFTWARE, so no existing case changes. AUTO with a real H.264 probe, because FORCE_SOFTWARE is exactly what keeps the other tests out of this branch, and because InputProbe() reports UNPARSEABLE -- which PERMISSIVE.canDecode refuses, sending every job to FFmpeg with no test saying why. The percentage is asserted, not merely that an update happened: publishProgress takes a display name and a percent, and replacing the percent with a constant compiles fine. Mutations, both run and restored: empty the hardware onProgress lambda 1 red report a constant percent instead of the engine's 1 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2125763ebf |
Read FFprobe's answer without spawning FFprobe (#195)
readMediaInformation was 114 missed instructions and 24 missed branches -- the second
largest block on the wave-4 report -- and exactly one line of it needed a device:
FFprobeKit.getMediaInformation(path).getMediaInformation()
Everything after it reads an ordinary object, so it moves into ffprobeInfoFrom and the edge
keeps the call and the null check. Verified JVM-safe rather than assumed: javap over the
committed AAR's runtime jar shows MediaInformation(JSONObject, List<StreamInformation>,
List<Chapter>) and StreamInformation(JSONObject) as plain public constructors whose <clinit>
does not load the native library, so a test builds its own without libffmpegkit present.
The decision worth reaching is containerFrom's SECOND argument. FFprobe reports
"matroska,webm" for both MKV and WebM because they share a demuxer, so the video codec is
the only thing separating them. containerFrom has thirty-three covered branches and not one
can notice that argument being dropped -- the mistake is at the call, not in the callee, so
every existing containerFrom test stays green while every VP9 WebM quietly becomes an MKV.
Two things the tests found rather than confirmed.
The format properties are NESTED under "format": getFormat() resolves through
getStringFormatProperty, not off the top-level object. The first fixture put the keys at the
top level and four cases failed with a null container. The helper says so now.
And one mutation SURVIVED on the first pass -- reading dimensions with
streams.firstNotNullOfOrNull { it.getWidth() } instead of video?.getWidth(). The fixture put
the dimensions on the chosen video stream, which is also the first stream carrying any, so
the two readings agreed and the test could not tell them apart. Separating them needs a
chosen video stream with NO dimensions and a later one that has them, which is a real shape:
FFprobe omits width/height for a stream it could not measure. That case is now its own test
and the mutation reddens it.
Mutations, each run and restored:
drop the video codec argument to containerFrom 1 red
take the LAST video stream instead of the first 1 red
read dimensions from any stream, not the chosen one 0 red -> 1 red after the new case
let an unparseable duration throw instead of zero 1 red
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
6d700f0014 |
Cut a seam through the codec enumeration, and say what a failed one actually does (#194)
probe() was 20 never-executed lines, the biggest single block on the report. It has been looked at twice and left out twice, and both closes were right about what they closed: #86 ruled it device-bound, and #133 re-checked that with ShadowMediaCodecList in hand and still declined, because MediaCodecInfoBuilder has no setIsAlias and no setCanonicalName -- "so the alias skip and the canonical-name dedup, the two things the class's KDoc calls out as easy to get wrong, are not reachable through it." That objection is about the shadow. It does not apply to a function taking its own entry type, which #133 did not evaluate. capabilitiesFrom(enumerate: () -> Sequence<CodecEntry>) holds every rule; the edge keeps only the mapping from MediaCodecList onto CodecEntry. The parameter is a Sequence rather than a List on purpose. runCatching has always wrapped the *iteration*, so a MediaCodecInfo whose properties throw partway leaves the codecs already read in place. A List parameter would move that throw outside the loop and turn a partial answer into an empty one -- a behaviour change smuggled in as a refactor. There is now a test for the partial case, and swapping the Sequence for an eager toList() reddens it. The behaviour change this DOES make is one line of log, and it is the reason the seam was worth cutting at all. The fallback said "Codec enumeration failed; assuming permissive" and returned empty sets -- but "video/avc" in emptySet() is false, so canEncode and canDecode answer no to everything and every job routes to FFmpeg. That is the restrictive answer, and it is the right one: FFmpeg does whatever Media3 does, only slower. The code stays; the message and the class KDoc now describe it. Seven tests. One of them was wrong first and the mutation is what said so: the alias case originally listed the alias *after* the codec it aliases and passed with the skip deleted, because canonicalName is shared and the dedup catches the second entry either way. The two rules overlap, so a fixture that does not separate them tests neither. Order separates them -- an alias arriving first claims the canonical name in `seen` and gets its own types credited, and the real codec is then dropped by the dedup. That is now the test, and it also says what the rule is worth: with a Set accumulator, an alias declaring the same types as its codec changes nothing, so the skip earns its place only when the two disagree. Mutations, each run and restored, each reddening the test that owns it: drop !isSoftwareOnly from the encoder predicate 1 red remove the alias skip 1 red (0 before the fixture was fixed) remove the canonical-name dedup 1 red remove the video/ prefix filter 1 red apply the hardware predicate to decoders too 1 red make the failure fallback permissive 2 red eager toList() instead of the lazy Sequence 1 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
da8d53851b |
Render the container row for a video nothing could name (#199)
ConverterScreen.kt:668's null arm -- DetailRow("Container", probe.container?.label ?:
"Unknown") in the VIDEO branch -- had never rendered. Every video case in FileCardTest uses
VIDEO_PROBE, which carries container = MP4.
The argument for adding it is the asymmetry, not the coverage. FileCard renders that exact
expression twice, once in AUDIO_ONLY (:660) and once in VIDEO (:668), and "an audio-only
file nothing else could describe degrades one row at a time" drives only the first. Same
expression, same fallback, one kind covered and one not -- which is the same argument
CLAUDE.md records for including ContainerCapabilities:94.
Nor is null an edge case here. InputProbe.container's own KDoc says MediaExtractor cannot
report a container at all, so it comes from FFprobe alone: any run where FFprobe did not
answer produces exactly this shape -- real codec, real dimensions, real duration, no
container. An empty value in its place would read as a rendering bug rather than as a
probe that got half its sources.
The other three rows are asserted alongside, which is what keeps this from being a copy of
the audio-only case. There, everything is unknown at once; here one field is missing from
a probe that is otherwise complete, and the rest have to be unaffected by it.
Mutation: `?: "Unknown"` -> `?: ""` at :668 only, run and restored. The AUDIO_ONLY twin at
:660 is a separate expression, and mutating that one would redden the existing test instead
-- which would prove nothing about this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
cc215195ee |
Build a command for the audio the user turned off (#198)
audioArgs' Drop arm -- `AudioPlan.Drop -> listOf("-an")` -- was ci == 0. The suite's only
-an assertion lives in "gif generates a palette to avoid banding and drops audio", and that
one comes from the image path at FFmpegCommandBuilder.kt:79/:90, which emits -an directly
and never reaches audioArgs. Two sites, one string, one tested.
It is a live path rather than defensive code. AdvancedPicker renders all of
AudioCodec.entries including NONE, ContainerCapabilities.validate permits audio-off whenever
the input has video, and MKV routes the job to FFmpeg -- so "convert this and drop the
soundtrack" is something a user can do today and nothing had built the command for.
Both halves are asserted, and the second is not padding: -an alone still passes if the arm
falls through to the else and emits an AAC encoder beside the flag, which is a file that is
silent because the flag won while carrying an encoder nobody asked for.
Three mutations, all run and restored. The third is the one that justifies the second
assertion, since the first two break -an as a side effect and so cannot show it:
Drop -> emptyList() red
Drop -> the else arm's aac encoder red (loses -an as well)
Drop -> listOf("-an", "-c:a", "aac") red -- -an intact, caught by assertFalse
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
92bcff8656 |
Make a failure that says nothing still say something (#193)
Three sites, all ci == 0 before this, and all the same rule: work/ConversionWorker.kt:316 cause.message ?: GENERIC_FAILURE_MESSAGE convert/ConversionViewModel.kt:631 e.message ?: SAVE_FAILED_MESSAGE join/JoinViewModel.kt:416 e.message ?: SAVE_FAILED_MESSAGE Every existing test throws WITH a message, so the right-hand side had never been evaluated anywhere in the suite. A Throwable carrying none is not exotic: RuntimeException(), IOException() and most platform exceptions raised without an argument all have a null message, and a native engine that dies is exactly where one comes from. The worker case needed care, and the care is the reason it survived three waves. ConversionStateMappingTest's "a failure with nothing said still says something" looks like it covers that site and does not -- it drives the READ side, map(FAILED, Data.EMPTY), and that side has a fallback of its own at ConversionViewModel.kt:147-149 which turns a blank KEY_ERROR back into the same constant. So a test asserting on the resulting Failed state stays green while the worker's fallback is broken. Measured rather than reasoned: with :316 mutated to .orEmpty(), exactly ONE of 587 tests went red, and it was the new one. Everything else, including the test that appears to cover it, stayed green. So the worker case reads KEY_ERROR off the worker's own Result, before anything downstream can repair it. The two save cases have no such second line -- both write _state.value directly -- so the state is the right thing to assert there, and both also assert that `pending` still travels: a fallback that dropped the handle would leave the file unreachable from the very screen that just said the save failed. Held in one class against the ticket's suggestion of three. They are one rule at three layers, and the masking above has to be explained once rather than three times. FailedSaveRetryTest already sets the precedent for both ViewModels in one file; this adds one worker to that shape. FORCE_SOFTWARE in the worker fixture so the failure comes straight out of runFFmpeg. AUTO would enter runMedia3OrFallBack, whose catch runs the job a second time in software: the same exception arrives, but by a path this is not about and which HardwareFallbackTest owns. Mutations, all run and restored -- each site to .orEmpty(), never to a different constant, which would only prove the test reads a constant: ConversionWorker:316 1 of 587 red (this file) ConversionViewModel:631 red JoinViewModel:416 red Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |