4e88de30451c16cf0de6bdfd105b362e9bd4f2e4
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
6f3966cc69 |
W3 (#156): the screen wiring, and a narrower hazard than the ticket claimed
The stateful outer composables hand `ConverterScreenContent` and `JoinScreenContent` a list of `viewModel::` references. No test in the suite had ever seen that list: the content tests build their own `ConverterActions`, so they drive the stateless inner and never touch the wiring. THE TICKET'S PREMISE WAS HALF WRONG, AND CHECKING BEAT ASSUMING. #156 was filed claiming a transposition of any two of seventeen bindings would survive the suite. Measured instead of trusted: onVideoCodec <-> onAudioCodec -> REJECTED: "Inapplicable candidate(s): fun setAudioCodec(codec: AudioCodec)" onCancel <-> onReset -> COMPILES Every typed binding -- container, both codecs, preset, suggestion, quality, engine preference -- takes a distinct parameter type, so the compiler is already the test. Writing assertions against those transpositions would have been theatre, and this file says so rather than quietly including them. WHAT IS ACTUALLY AT RISK is the `() -> Unit` bindings, which are interchangeable to the compiler: two on the converter screen (onCancel, onReset) and *three* on the join screen (onJoin, onCancel, onReset). A Cancel button that discards the finished file, a Start-over that leaves it on screen, or a Join button that cancels -- each is one wrong word and each ships. I got that wrong in the first check too: an early run reported the onCancel/ onReset swap as rejected, from a grep-and-exit-code test that misread a stale build. Re-running it properly printed BUILD SUCCESSFUL with the swap in place. THE SEAM. `converterActions(viewModel, onPickInput, onConvert, onSave)` and `joinActions(viewModel, onPickInputs, onSave)`. The launcher-backed actions stay parameters -- they need an ActivityResultLauncher, which is the part that genuinely needs a composition, and keeping them out means the rest needs none. Told apart by effect rather than by a recording double: `reset()` sets the state to Idle, `cancel()` with no active job leaves it alone (`activeWorkId?.let`, which SettingsEditsTest pins). Mutations -- every transposition caught, each by two tests: converter onCancel <-> onReset | 2 tests join onJoin <-> onCancel | 2 tests join onReset <-> onCancel | 2 tests a typed binding dropped to {} | 1 test a launcher action rerouted | 1 test The two-test symmetry is deliberate: one direction alone passes against a wiring with BOTH actions bound to the same method, which is what a copy-pasted line produces. Three guard assertions earned their place during writing -- the picks land through an injected dispatcher, and without `ParkedPickDispatcher.runAll()` all three state-based tests sat on Idle and would have asserted nothing. They failed loudly instead of passing quietly. `@UnstableApi` on both builders, per CLAUDE.md; lint caught their absence, as it did in W1. 525 -> 537 tests, 88.0% -> 88.9% line, 70.5% -> 75.4% branch. Gate green. 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-dev
|
46ad95350b |
Give both screens somewhere for a state to come from
`ConverterScreen` and `JoinScreen` each inlined their whole `when (state)` inside the public entry point, and state arrived only as `viewModel.state`. That left four of the twelve state branches across the two screens with no test that could ever reach them: driving a real ViewModel needs a WorkManager and a media probe in the constructor, and even then `Waiting` follows a denied foreground start and `Converted`/`Joined` follow a worker run that has already succeeded. So the `when` moves into `ConverterScreenContent` and `JoinScreenContent`, which take the state, the settings, the validation and an actions holder. The entry points keep the three launchers and `collectAsStateWithLifecycle` and nothing else. The callbacks travel in `ConverterActions` / `JoinActions` rather than as loose parameters because detekt's `LongParameterList` sits at its default threshold of six and `config/detekt/detekt.yml` does not relax it for `@Composable` -- `AdvancedPicker` already sits exactly on it. Twelve flat parameters would turn a clean detekt run red; data classes are exempt from the rule. Nothing else changed. The body was cut and pasted rather than retyped, so the U+2026, U+2014 and U+00B7 characters the leaf tests match on are the same bytes, and `is Idle -> Unit` in the nested `when` -- permanently unreachable, and deliberately kept -- survives the move. The diff stops above `FormatPicker` in one file and above `FileRow` in the other, which is why the leaf suites #57-#60 landed pass unedited: every one of them composes a leaf directly and none references either entry point. The two new tests are the bite, one per screen and one per direction of the seam: a `Converted` / `Joined` state renders Save, and tapping Save hands back the name the finished job chose. The state matrix itself is #62 and #63. |
||
|
JMR-devandClaude Opus 5
|
df2e42a2b3 |
Name the screen leaves, and give the tests a tag vocabulary
Kotlin `private` on a top-level declaration is file-scoped, so every leaf composable in the two screens was invisible even to the JVM test source set, which is a friend of main. The only three declarations src/test could name were ConverterScreen, JoinScreen and AppRoot -- there was nothing to write a test against, which is why #52 could not be started as filed. `internal` is the same choice MainActivity already documents for Destination: the unit tests can name it, and it stays invisible to anything outside the module. Eleven declarations in ConverterScreen and FileRow in JoinScreen. The tag table is the other half. Tests reference a symbol rather than a literal, which is what keeps the five children that follow independent: "Cancel", "Start over" and "Save file" are each rendered by both screens and by more than one state branch, so rewording one would otherwise redden several PRs at once and no diff would explain why. There were zero testTag, semantics or contentDescription calls anywhere in main before this. Every tag is applied inside main. A tag a test hands down as a Modifier proves only that the test set it -- it would survive the affordance losing its own tag entirely, which is the vacuous shape CLAUDE.md records nine of in one review. That is why FileRow and DetailRow derive theirs from data they already hold rather than taking an index from the call site. TestTags is public rather than internal, and R38.8 is the reason. It reads the table from androidTest, and whether that is a friend source set of main under AGP 9 had no in-tree answer -- nothing referenced a main internal from there. Settled by compiling one: it is a friend, so internal would work today. Public anyway, because that friendship is AGP wiring rather than something this project states, and the KDoc now carries the measurement so nobody has to repeat it. Smoke tests cover each leaf: it renders, and its tag resolves to exactly one node. Counting rather than asserting existence is deliberate, since a duplicated tag fails differently depending on which finder a later test happens to use. The state-branch tags -- Convert, Cancel, Save file, Start over, the progress bars -- have no bite yet: reaching a branch needs the state seam R38.5 extracts, and the state matrix is R38.6/R38.7 by design. Five mutations, all red on the named test alone: FORMAT_CHIPS deleted, FILE_CARD_NAME deleted, detailRow's tag no longer derived from its label, FileRow's no longer derived from its name, and JOIN_MORE given JOIN's value -- the last caught only by the uniqueness check, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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
|
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
|
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
|
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> |