Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4d21996735 | ||
|
|
264b8027e4 | ||
|
|
6a8cc01862 |
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
|
|||||||
import androidx.work.Data
|
import androidx.work.Data
|
||||||
import androidx.work.ForegroundInfo
|
import androidx.work.ForegroundInfo
|
||||||
import androidx.work.OneTimeWorkRequestBuilder
|
import androidx.work.OneTimeWorkRequestBuilder
|
||||||
|
import androidx.work.OutOfQuotaPolicy
|
||||||
import androidx.work.WorkerParameters
|
import androidx.work.WorkerParameters
|
||||||
import androidx.work.hasKeyWithValueOfType
|
import androidx.work.hasKeyWithValueOfType
|
||||||
import androidx.work.workDataOf
|
import androidx.work.workDataOf
|
||||||
@@ -27,6 +28,10 @@ import org.libremediaconverter.model.OutputFormat
|
|||||||
* Progress is not reported. FFmpeg's statistics callback gives a timestamp against a
|
* Progress is not reported. FFmpeg's statistics callback gives a timestamp against a
|
||||||
* single input's duration, which is meaningless once several files are being
|
* single input's duration, which is meaningless once several files are being
|
||||||
* concatenated; showing a fabricated percentage would be worse than showing none.
|
* concatenated; showing a fabricated percentage would be worse than showing none.
|
||||||
|
*
|
||||||
|
* Enqueued as **expedited** work for the same reasons, and with the same caveats, as
|
||||||
|
* [ConversionWorker] — its class KDoc carries both, and a join is user-initiated in exactly the
|
||||||
|
* way a conversion is.
|
||||||
*/
|
*/
|
||||||
@UnstableApi
|
@UnstableApi
|
||||||
class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) {
|
class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) {
|
||||||
@@ -68,13 +73,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
|||||||
// which is where a WorkManager restart after process death always begins -- used to
|
// which is where a WorkManager restart after process death always begins -- used to
|
||||||
// throw straight past this catch, taking the retry, the error message and the delete
|
// throw straight past this catch, taking the retry, the error message and the delete
|
||||||
// with it. See ConversionWorker.doWork and FailureOutcome.
|
// with it. See ConversionWorker.doWork and FailureOutcome.
|
||||||
setForeground(
|
//
|
||||||
ForegroundInfo(
|
// Posted through getForegroundInfo() rather than built here a second time -- see that
|
||||||
NOTIFICATION_ID,
|
// override, and its twin in ConversionWorker.
|
||||||
notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true),
|
setForeground(getForegroundInfo())
|
||||||
ConversionForegroundType.current(),
|
|
||||||
),
|
|
||||||
)
|
|
||||||
|
|
||||||
val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format)
|
val result = ConversionDependencies.concat(applicationContext).join(uris, staged, format)
|
||||||
Result.success(
|
Result.success(
|
||||||
@@ -129,11 +131,29 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
|||||||
return publisher.hasSpaceFor(bytes)
|
return publisher.hasSpaceFor(bytes)
|
||||||
}
|
}
|
||||||
|
|
||||||
override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo(
|
/**
|
||||||
NOTIFICATION_ID,
|
* The notification a starting join posts, and now the only definition of it.
|
||||||
notifications.build(id, "Joining files", 0, indeterminate = true),
|
*
|
||||||
ConversionForegroundType.current(),
|
* WorkManager's hook for expedited work, which **will not call this on any device this app
|
||||||
)
|
* supports** — see [ConversionWorker.getForegroundInfo] for the measurement and for why
|
||||||
|
* `setExpedited` alone would have left these lines exactly as cold as they were. What makes
|
||||||
|
* them live is [doWork] posting this instead of building its own copy.
|
||||||
|
*
|
||||||
|
* It counts the inputs itself rather than being handed the number, so that it is still answerable
|
||||||
|
* before [doWork] has parsed anything — which is the contract WorkManager's own caller wants.
|
||||||
|
* The count is read from the same key, so the two cannot disagree. The `?: 0` arm is
|
||||||
|
* unreachable and named rather than covered: [doWork] refuses a job with no URI array several
|
||||||
|
* lines above this call, and nothing else calls it. It is the shape `docs/coverage-read-findings.md`
|
||||||
|
* calls F4 — a second line of defence that cannot be provoked.
|
||||||
|
*/
|
||||||
|
override suspend fun getForegroundInfo(): ForegroundInfo {
|
||||||
|
val inputCount = inputData.getStringArray(KEY_INPUT_URIS)?.size ?: 0
|
||||||
|
return ForegroundInfo(
|
||||||
|
NOTIFICATION_ID,
|
||||||
|
notifications.build(id, joiningTitle(inputCount), 0, indeterminate = true),
|
||||||
|
ConversionForegroundType.current(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
companion object {
|
companion object {
|
||||||
/**
|
/**
|
||||||
@@ -204,6 +224,16 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
|||||||
*/
|
*/
|
||||||
fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}"
|
fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}"
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the progress notification says while a join runs.
|
||||||
|
*
|
||||||
|
* Named once, for the convention #158 established about strings the user can see. It was
|
||||||
|
* two strings until 2026-09-06 — `"Joining N files"` built inline in [doWork] and a
|
||||||
|
* countless `"Joining files"` in [getForegroundInfo] — for one notification that only ever
|
||||||
|
* had one job, and the copy nothing executed was free to drift from the one that did.
|
||||||
|
*/
|
||||||
|
fun joiningTitle(inputCount: Int): String = "Joining $inputCount files"
|
||||||
|
|
||||||
private const val NOTIFICATION_ID = 1002
|
private const val NOTIFICATION_ID = 1002
|
||||||
private const val TAG = "ConcatWorker"
|
private const val TAG = "ConcatWorker"
|
||||||
|
|
||||||
@@ -215,6 +245,10 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
|||||||
*/
|
*/
|
||||||
fun request(inputs: List<Uri>, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) =
|
fun request(inputs: List<Uri>, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) =
|
||||||
OneTimeWorkRequestBuilder<ConcatWorker>()
|
OneTimeWorkRequestBuilder<ConcatWorker>()
|
||||||
|
// Expedited, exactly as ConversionWorker.request is and for the same reasons; that
|
||||||
|
// one's comment and class KDoc carry them. Nothing here sets an initial delay or a
|
||||||
|
// constraint, which is what makes it legal for `build()` to accept.
|
||||||
|
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
|
||||||
.addTag(JobTags.inputCount(inputs.size))
|
.addTag(JobTags.inputCount(inputs.size))
|
||||||
.setInputData(
|
.setInputData(
|
||||||
Data.Builder()
|
Data.Builder()
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import androidx.work.CoroutineWorker
|
|||||||
import androidx.work.Data
|
import androidx.work.Data
|
||||||
import androidx.work.ForegroundInfo
|
import androidx.work.ForegroundInfo
|
||||||
import androidx.work.OneTimeWorkRequestBuilder
|
import androidx.work.OneTimeWorkRequestBuilder
|
||||||
|
import androidx.work.OutOfQuotaPolicy
|
||||||
import androidx.work.WorkerParameters
|
import androidx.work.WorkerParameters
|
||||||
import androidx.work.hasKeyWithValueOfType
|
import androidx.work.hasKeyWithValueOfType
|
||||||
import androidx.work.workDataOf
|
import androidx.work.workDataOf
|
||||||
@@ -40,8 +41,28 @@ import java.io.File
|
|||||||
* observe. That durability is what makes the six-hour foreground-service timeout
|
* observe. That durability is what makes the six-hour foreground-service timeout
|
||||||
* recoverable instead of fatal.
|
* recoverable instead of fatal.
|
||||||
*
|
*
|
||||||
* Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs
|
* Enqueued as **expedited** work, with `RUN_AS_NON_EXPEDITED_WORK_REQUEST`. This paragraph said
|
||||||
* with a short quota, which is the wrong shape for a multi-minute transcode.
|
* the opposite until 2026-09-06 — "deliberately *not* used… the wrong shape for a multi-minute
|
||||||
|
* transcode" — and the quota it named does not reach a transcode the way it reads:
|
||||||
|
*
|
||||||
|
* - The quota belongs to the *JobScheduler* job, and `SystemJobInfoConverter:135` in
|
||||||
|
* work-runtime 2.11.2 sets `JobInfo.setExpedited(true)` only when `!isRetry && !isDelayed`.
|
||||||
|
* A retry is therefore scheduled exactly as every job is scheduled today.
|
||||||
|
* - A job the system stops mid-run does not get its answer from [FailureOutcome].
|
||||||
|
* `WorkerWrapper.interrupt` cancels the worker's coroutine with a `WorkerStoppedException`,
|
||||||
|
* which its `launch` resolves as `ResetWorkerStatus` — the worker's own `Result` is discarded
|
||||||
|
* and the work re-enqueued with backoff, whatever it returned. So a quota stop is a retry, and
|
||||||
|
* the `CancellationException` arm in [doWork] is what deletes the partial on the way through.
|
||||||
|
*
|
||||||
|
* What it buys is narrower than "conversions start sooner", and the narrowness is the honest part:
|
||||||
|
* `GreedyScheduler` starts unconstrained, undelayed work in-process the moment it is enqueued and
|
||||||
|
* carries no `expedited` branch at all, so a conversion begun from the open app runs exactly when
|
||||||
|
* it ran before — the common case does not move. The flag is for the job that has to go *through*
|
||||||
|
* JobScheduler because no process is left to start it: one still enqueued when the app died.
|
||||||
|
* `SystemJobScheduler.schedule` re-converts the spec every time it schedules, so such a job is
|
||||||
|
* expedited on the way back in, and a retried one is not. `RUN_AS_NON_EXPEDITED_WORK_REQUEST`
|
||||||
|
* rather than `DROP_WORK_REQUEST`: an invisible quota is no reason to throw a user's conversion
|
||||||
|
* away, and `SystemJobScheduler:198` degrades it to an ordinary job instead.
|
||||||
*
|
*
|
||||||
* That durability is not free, and the queue surviving is not the same as the job surviving.
|
* That durability is not free, and the queue surviving is not the same as the job surviving.
|
||||||
* When WorkManager recovers a job after process death the app is by definition in the background,
|
* When WorkManager recovers a job after process death the app is by definition in the background,
|
||||||
@@ -60,7 +81,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
|||||||
override suspend fun doWork(): Result {
|
override suspend fun doWork(): Result {
|
||||||
val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
|
val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse)
|
||||||
?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))
|
?: return Result.failure(workDataOf(KEY_ERROR to "No input file."))
|
||||||
val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input"
|
val displayName = displayName()
|
||||||
// Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what
|
// Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what
|
||||||
// made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free".
|
// made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free".
|
||||||
val declaredSize = inputData
|
val declaredSize = inputData
|
||||||
@@ -100,7 +121,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
|||||||
// process death is. With it above the try that throw escaped doWork() entirely: no
|
// process death is. With it above the try that throw escaped doWork() entirely: no
|
||||||
// retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below
|
// retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below
|
||||||
// was outside for the same reason and had the same problem.
|
// was outside for the same reason and had the same problem.
|
||||||
setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true))
|
//
|
||||||
|
// Posted through getForegroundInfo() rather than built here a second time -- see that
|
||||||
|
// override for what the duplicate cost.
|
||||||
|
setForeground(getForegroundInfo())
|
||||||
|
|
||||||
// Through the seam rather than MediaProbe directly. The seam already existed for the
|
// Through the seam rather than MediaProbe directly. The seam already existed for the
|
||||||
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
|
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
|
||||||
@@ -339,8 +363,32 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
|||||||
return OutputSpec(container, video, audio)
|
return OutputSpec(container, video, audio)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What this job's input is called, or [InputQuery.FALLBACK_DISPLAY_NAME] when nothing named it.
|
||||||
|
*
|
||||||
|
* One read rather than the two copies of `?: "input"` that [doWork] and [getForegroundInfo]
|
||||||
|
* each carried, and against `InputQuery`'s constant rather than a third literal of the same
|
||||||
|
* string: it is the same fallback the picker uses, and it reaches the save dialog as
|
||||||
|
* `input_converted.mp4`.
|
||||||
|
*/
|
||||||
|
private fun displayName(): String = inputData.getString(KEY_DISPLAY_NAME) ?: InputQuery.FALLBACK_DISPLAY_NAME
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The notification a starting conversion posts, and now the only definition of it.
|
||||||
|
*
|
||||||
|
* This is WorkManager's hook for expedited work, and **it will not be called on any device
|
||||||
|
* this app supports.** `WorkForeground.kt:38` in work-runtime 2.11.2 opens with
|
||||||
|
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's
|
||||||
|
* only caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So #252's premise — that
|
||||||
|
* enqueueing expedited work would make these lines live — is false, and `setExpedited` alone
|
||||||
|
* would have left them exactly as cold as the first instrumented coverage read found them.
|
||||||
|
*
|
||||||
|
* What makes them live is [doWork] posting *this* instead of building its own copy. The two
|
||||||
|
* were identical — same title, `percent = 0`, `indeterminate = true` — so one was a duplicate
|
||||||
|
* that could drift, and the one nothing executed is the one that would have drifted silently.
|
||||||
|
*/
|
||||||
override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
|
override suspend fun getForegroundInfo(): ForegroundInfo = foregroundInfo(
|
||||||
inputData.getString(KEY_DISPLAY_NAME) ?: "input",
|
displayName(),
|
||||||
percent = 0,
|
percent = 0,
|
||||||
indeterminate = true,
|
indeterminate = true,
|
||||||
)
|
)
|
||||||
@@ -416,6 +464,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
|||||||
quality: QualityTier = QualityTier.FAST,
|
quality: QualityTier = QualityTier.FAST,
|
||||||
enginePreference: EnginePreference = EnginePreference.AUTO,
|
enginePreference: EnginePreference = EnginePreference.AUTO,
|
||||||
) = OneTimeWorkRequestBuilder<ConversionWorker>()
|
) = OneTimeWorkRequestBuilder<ConversionWorker>()
|
||||||
|
// Expedited, so the jobs that do go through JobScheduler are treated as the
|
||||||
|
// user-initiated work they are -- see the class KDoc for what that is and is not worth.
|
||||||
|
// Safe to set here and only because of what this builder does not do: `build()` refuses
|
||||||
|
// an expedited request carrying an initial delay or any constraint but network and
|
||||||
|
// storage, and none of the three is set below.
|
||||||
|
.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)
|
||||||
.addTag(JobTags.displayName(displayName))
|
.addTag(JobTags.displayName(displayName))
|
||||||
// Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has
|
// Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has
|
||||||
// no null, so the absence of the key *is* the unknown — and a tag reading
|
// no null, so the absence of the key *is* the unknown — and a tag reading
|
||||||
|
|||||||
@@ -0,0 +1,102 @@
|
|||||||
|
package org.libremediaconverter.work
|
||||||
|
|
||||||
|
import android.net.Uri
|
||||||
|
import androidx.media3.common.util.UnstableApi
|
||||||
|
import androidx.work.Constraints
|
||||||
|
import androidx.work.OutOfQuotaPolicy
|
||||||
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Assert.assertTrue
|
||||||
|
import org.junit.Test
|
||||||
|
import org.junit.runner.RunWith
|
||||||
|
import org.robolectric.RobolectricTestRunner
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Both workers enqueue **expedited** work, and stay legal doing it.
|
||||||
|
*
|
||||||
|
* The two questions are separate and only one of them is about the flag.
|
||||||
|
*
|
||||||
|
* - **Is it set.** `expedited` is `false` by default, so `assertTrue` here is what a deleted
|
||||||
|
* `setExpedited(...)` reddens. That mutation was run.
|
||||||
|
* - **Is it legal.** `WorkRequest.Builder.build()` refuses an expedited request that carries an
|
||||||
|
* initial delay or any constraint but network and storage — `require(workSpec.initialDelay <= 0)
|
||||||
|
* { "Expedited jobs cannot be delayed" }` in work-runtime 2.11.2. Neither `request` sets either
|
||||||
|
* today, so both `build()` calls pass and the `IllegalArgumentException` is a *future* hazard
|
||||||
|
* rather than a current one. The delay and constraints assertions below are what name it: add a
|
||||||
|
* delay to either builder and this class fails on the throw, in the same second, instead of the
|
||||||
|
* app failing to enqueue a conversion on a device.
|
||||||
|
*
|
||||||
|
* **The policy assertion bites less than it reads, and that is worth writing down rather than
|
||||||
|
* leaving to be rediscovered.** `WorkSpec.outOfQuotaPolicy` *defaults* to
|
||||||
|
* `RUN_AS_NON_EXPEDITED_WORK_REQUEST`, so it is already this value on a request that was never
|
||||||
|
* expedited at all — deleting `setExpedited` does not redden it. What it does pin is the one
|
||||||
|
* alternative: `DROP_WORK_REQUEST` throws a user's conversion away because an invisible quota ran
|
||||||
|
* out, and that mutation *is* red here.
|
||||||
|
*
|
||||||
|
* The delay is not hypothetical either. Three tests deliberately build a delayed request to hold a
|
||||||
|
* job in `ENQUEUED` — `NotificationCancelActionTest`, `ReattachOnLaunchTest` and
|
||||||
|
* `CancelReachesWorkManagerTest` — and every one of them builds its own
|
||||||
|
* `OneTimeWorkRequestBuilder` rather than adding a delay to what `request` returns. That is why
|
||||||
|
* making these expedited broke none of them; the one that starts from `request` takes only
|
||||||
|
* `base.workSpec.input` from it.
|
||||||
|
*/
|
||||||
|
@UnstableApi
|
||||||
|
@RunWith(RobolectricTestRunner::class)
|
||||||
|
class ExpeditedRequestTest {
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a conversion is enqueued as expedited work`() {
|
||||||
|
val spec = ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec
|
||||||
|
|
||||||
|
assertTrue("a conversion the user asked for has to be expedited work", spec.expedited)
|
||||||
|
assertEquals(
|
||||||
|
"a quota nobody can see is no reason to drop a conversion",
|
||||||
|
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
|
||||||
|
spec.outOfQuotaPolicy,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a join is enqueued as expedited work`() {
|
||||||
|
val spec = ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec
|
||||||
|
|
||||||
|
assertTrue("a join the user asked for has to be expedited work", spec.expedited)
|
||||||
|
assertEquals(
|
||||||
|
"a quota nobody can see is no reason to drop a join",
|
||||||
|
OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST,
|
||||||
|
spec.outOfQuotaPolicy,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The two properties that keep `build()` from throwing, asserted on both requests at once
|
||||||
|
* because the rule is WorkManager's rather than either worker's.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `neither expedited request carries what would make it illegal`() {
|
||||||
|
val requests = listOf(
|
||||||
|
ConversionWorker.request(INPUT, DISPLAY_NAME, INPUT_BYTES).workSpec,
|
||||||
|
ConcatWorker.request(listOf(INPUT, SECOND_INPUT), TOTAL_BYTES).workSpec,
|
||||||
|
)
|
||||||
|
|
||||||
|
requests.forEach { spec ->
|
||||||
|
assertEquals(
|
||||||
|
"expedited work cannot be delayed: ${spec.workerClassName}",
|
||||||
|
0L,
|
||||||
|
spec.initialDelay,
|
||||||
|
)
|
||||||
|
assertEquals(
|
||||||
|
"expedited work takes only network and storage constraints: ${spec.workerClassName}",
|
||||||
|
Constraints.NONE,
|
||||||
|
spec.constraints,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private companion object {
|
||||||
|
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
|
||||||
|
val SECOND_INPUT: Uri = Uri.parse("file:///tmp/holiday2.mp4")
|
||||||
|
const val DISPLAY_NAME = "holiday.mp4"
|
||||||
|
const val INPUT_BYTES = 1_024L
|
||||||
|
const val TOTAL_BYTES = 2_048L
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,203 @@
|
|||||||
|
package org.libremediaconverter.work
|
||||||
|
|
||||||
|
import android.app.Application
|
||||||
|
import android.app.Notification
|
||||||
|
import android.net.Uri
|
||||||
|
import androidx.media3.common.util.UnstableApi
|
||||||
|
import androidx.work.Data
|
||||||
|
import androidx.work.testing.TestListenableWorkerBuilder
|
||||||
|
import androidx.work.workDataOf
|
||||||
|
import kotlinx.coroutines.runBlocking
|
||||||
|
import org.junit.After
|
||||||
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Assert.assertTrue
|
||||||
|
import org.junit.Before
|
||||||
|
import org.junit.Test
|
||||||
|
import org.junit.runner.RunWith
|
||||||
|
import org.libremediaconverter.convert.ConcatJoiner
|
||||||
|
import org.libremediaconverter.convert.ConversionDependencies
|
||||||
|
import org.libremediaconverter.convert.installTestWorkManager
|
||||||
|
import org.libremediaconverter.ffmpeg.ConcatEngine
|
||||||
|
import org.libremediaconverter.model.ConcatStrategy
|
||||||
|
import org.libremediaconverter.model.DeviceCodecs
|
||||||
|
import org.libremediaconverter.model.EnginePreference
|
||||||
|
import org.libremediaconverter.model.InputProbe
|
||||||
|
import org.libremediaconverter.model.OutputFormat
|
||||||
|
import org.robolectric.RobolectricTestRunner
|
||||||
|
import org.robolectric.RuntimeEnvironment
|
||||||
|
import java.io.File
|
||||||
|
import java.util.UUID
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The first thing either worker posts is what its own `getForegroundInfo()` builds.
|
||||||
|
*
|
||||||
|
* **Both overrides were dead code until 2026-09-06, and #252 is where that was found** — the first
|
||||||
|
* instrumented coverage read reported `ConversionWorker:342-346` and `ConcatWorker:132-136` among
|
||||||
|
* the 32 lines *neither* suite reaches. The ticket's premise was that enqueueing expedited work
|
||||||
|
* would make them live, since `getForegroundInfo()` is WorkManager's expedited-work hook.
|
||||||
|
*
|
||||||
|
* **That premise is false at this `minSdk`, which is the finding underneath the fix.**
|
||||||
|
* `WorkForeground.kt:38` in work-runtime 2.11.2 opens `workForeground` with
|
||||||
|
* `if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return`, that function is the library's only
|
||||||
|
* caller of `getForegroundInfoAsync()`, and `minSdk` is 33. So `setExpedited` alone would have left
|
||||||
|
* both overrides exactly as cold as the read found them, and a test written to drive them through
|
||||||
|
* WorkManager would be testing a code path no device this app supports can take — E1's failure
|
||||||
|
* mode, where a test asserts and never reaches.
|
||||||
|
*
|
||||||
|
* What makes them live is a single-definition change instead. Each worker had **two** definitions
|
||||||
|
* of one notification: the override, and an identical `ForegroundInfo` built inline in `doWork`.
|
||||||
|
* `doWork` now posts the override's, so the copy nothing executed is gone and the one that remains
|
||||||
|
* runs on every job.
|
||||||
|
*
|
||||||
|
* These tests are what hold that wiring. Each asserts the notification's *contents* against
|
||||||
|
* constants rather than against `worker.getForegroundInfo()` — comparing the two would move
|
||||||
|
* together under every mutation and stay green — and the mutations that redden them are named on
|
||||||
|
* each test.
|
||||||
|
*/
|
||||||
|
@UnstableApi
|
||||||
|
@RunWith(RobolectricTestRunner::class)
|
||||||
|
class ForegroundNotificationTest {
|
||||||
|
|
||||||
|
private lateinit var app: Application
|
||||||
|
private lateinit var updater: RecordingForegroundUpdater
|
||||||
|
|
||||||
|
@Before
|
||||||
|
fun setUp() {
|
||||||
|
app = RuntimeEnvironment.getApplication()
|
||||||
|
updater = RecordingForegroundUpdater()
|
||||||
|
ConversionDependencies.publisher = { AlwaysRoomPublisher(app) }
|
||||||
|
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||||
|
ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE }
|
||||||
|
ConversionDependencies.software = { WritingTranscoder }
|
||||||
|
ConversionDependencies.concat = { WritingJoiner }
|
||||||
|
// The notification carries a WorkManager cancel PendingIntent, so without this the worker
|
||||||
|
// fails building the notification rather than on anything these tests are about.
|
||||||
|
installTestWorkManager(app, Data.EMPTY)
|
||||||
|
}
|
||||||
|
|
||||||
|
@After
|
||||||
|
fun tearDown() {
|
||||||
|
ConversionDependencies.reset()
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Mutation that must go red, and did: inside `ConversionWorker.getForegroundInfo`, replace
|
||||||
|
* `displayName()` with a literal, or `percent = 0` with anything else. Both are in the override's
|
||||||
|
* own body, so a red here is proof `doWork` executes it rather than a copy of it.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `a conversion's first foreground post is the one getForegroundInfo builds`() {
|
||||||
|
runBlocking { conversionWorker().doWork() }
|
||||||
|
|
||||||
|
val first = updater.infos.first()
|
||||||
|
val extras = first.notification.extras
|
||||||
|
assertEquals(
|
||||||
|
"the notification has to name the file the user picked",
|
||||||
|
DISPLAY_NAME,
|
||||||
|
extras.getString(Notification.EXTRA_TITLE),
|
||||||
|
)
|
||||||
|
assertEquals("a conversion starts at zero", 0, extras.getInt(Notification.EXTRA_PROGRESS))
|
||||||
|
assertTrue(
|
||||||
|
"nothing is known about the length of the job yet, so the bar is indeterminate",
|
||||||
|
extras.getBoolean(Notification.EXTRA_PROGRESS_INDETERMINATE),
|
||||||
|
)
|
||||||
|
assertEquals(
|
||||||
|
"the foreground service type is the regime's, not zero",
|
||||||
|
ConversionForegroundType.current(),
|
||||||
|
first.foregroundServiceType,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The count is the point.
|
||||||
|
*
|
||||||
|
* `getForegroundInfo` said `"Joining files"` and `doWork` said `"Joining N files"` — one
|
||||||
|
* notification with two texts, and the one nothing ran was free to drift. Now there is one,
|
||||||
|
* and it reads the input array itself so it can still answer before `doWork` has parsed
|
||||||
|
* anything.
|
||||||
|
*
|
||||||
|
* Mutation that must go red, and did: replace the array read in `ConcatWorker.getForegroundInfo`
|
||||||
|
* with a constant `0`, which yields `"Joining 0 files"`. Asserting merely that the title starts
|
||||||
|
* with "Joining" would survive that, which is why the whole string is pinned.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `a join's first foreground post counts the files it was given`() {
|
||||||
|
runBlocking { joinWorker().doWork() }
|
||||||
|
|
||||||
|
val first = updater.infos.first()
|
||||||
|
assertEquals(
|
||||||
|
"the notification has to say how many files are being joined",
|
||||||
|
ConcatWorker.joiningTitle(INPUTS.size),
|
||||||
|
first.notification.extras.getString(Notification.EXTRA_TITLE),
|
||||||
|
)
|
||||||
|
assertEquals(
|
||||||
|
"the foreground service type is the regime's, not zero",
|
||||||
|
ConversionForegroundType.current(),
|
||||||
|
first.foregroundServiceType,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* And the title is really the file's name rather than any string at all.
|
||||||
|
*
|
||||||
|
* [ConcatWorker.joiningTitle] is asserted above through the constant the worker itself uses, so
|
||||||
|
* that assertion cannot catch the sentence being reworded — deliberately, since the wording is
|
||||||
|
* not what the test is about. This one can: two files, two names, one worker each.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `two conversions of differently named files post differently named notifications`() {
|
||||||
|
runBlocking { conversionWorker(displayName = OTHER_NAME).doWork() }
|
||||||
|
|
||||||
|
assertEquals(
|
||||||
|
OTHER_NAME,
|
||||||
|
updater.infos.first().notification.extras.getString(Notification.EXTRA_TITLE),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
private fun conversionWorker(displayName: String = DISPLAY_NAME) = TestListenableWorkerBuilder<ConversionWorker>(
|
||||||
|
context = app,
|
||||||
|
inputData = workDataOf(
|
||||||
|
ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
|
||||||
|
ConversionWorker.KEY_DISPLAY_NAME to displayName,
|
||||||
|
ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
|
||||||
|
// FORCE_SOFTWARE is the one preference that decides without consulting the input,
|
||||||
|
// and a file:// URI keeps the worker out of FFmpegKit's native SAF bridge.
|
||||||
|
ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name,
|
||||||
|
),
|
||||||
|
runAttemptCount = 0,
|
||||||
|
).setId(JOB_ID)
|
||||||
|
.setForegroundUpdater(updater)
|
||||||
|
.build()
|
||||||
|
|
||||||
|
private fun joinWorker() = TestListenableWorkerBuilder<ConcatWorker>(
|
||||||
|
context = app,
|
||||||
|
inputData = workDataOf(
|
||||||
|
ConcatWorker.KEY_INPUT_URIS to INPUTS.map(Uri::toString).toTypedArray(),
|
||||||
|
ConcatWorker.KEY_TOTAL_BYTES to TOTAL_BYTES,
|
||||||
|
ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name,
|
||||||
|
),
|
||||||
|
runAttemptCount = 0,
|
||||||
|
).setId(JOB_ID)
|
||||||
|
.setForegroundUpdater(updater)
|
||||||
|
.build()
|
||||||
|
|
||||||
|
private companion object {
|
||||||
|
val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4")
|
||||||
|
val INPUTS: List<Uri> = listOf(INPUT, Uri.parse("file:///tmp/holiday2.mp4"))
|
||||||
|
const val DISPLAY_NAME = "holiday.mp4"
|
||||||
|
const val OTHER_NAME = "birthday.mkv"
|
||||||
|
const val INPUT_BYTES = 1_024L
|
||||||
|
const val TOTAL_BYTES = 2_048L
|
||||||
|
val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000252")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/** A joiner that writes an output and reports a strategy; nothing here is about the engine. */
|
||||||
|
private object WritingJoiner : ConcatJoiner {
|
||||||
|
override suspend fun join(inputs: List<Uri>, output: File, format: OutputFormat): ConcatEngine.Result {
|
||||||
|
output.writeBytes(ByteArray(OUTPUT_BYTES))
|
||||||
|
return ConcatEngine.Result(ConcatStrategy.STREAM_COPY, output)
|
||||||
|
}
|
||||||
|
|
||||||
|
private const val OUTPUT_BYTES = 512
|
||||||
|
}
|
||||||
@@ -3,16 +3,12 @@ package org.libremediaconverter.work
|
|||||||
import android.app.Application
|
import android.app.Application
|
||||||
import android.app.Notification
|
import android.app.Notification
|
||||||
import android.app.NotificationManager
|
import android.app.NotificationManager
|
||||||
import android.content.Context
|
|
||||||
import android.net.Uri
|
import android.net.Uri
|
||||||
import androidx.media3.common.util.UnstableApi
|
import androidx.media3.common.util.UnstableApi
|
||||||
import androidx.work.Data
|
import androidx.work.Data
|
||||||
import androidx.work.ForegroundInfo
|
|
||||||
import androidx.work.WorkInfo
|
import androidx.work.WorkInfo
|
||||||
import androidx.work.testing.TestForegroundUpdater
|
|
||||||
import androidx.work.testing.TestListenableWorkerBuilder
|
import androidx.work.testing.TestListenableWorkerBuilder
|
||||||
import androidx.work.workDataOf
|
import androidx.work.workDataOf
|
||||||
import com.google.common.util.concurrent.ListenableFuture
|
|
||||||
import kotlinx.coroutines.runBlocking
|
import kotlinx.coroutines.runBlocking
|
||||||
import org.junit.After
|
import org.junit.After
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
@@ -224,26 +220,6 @@ class ProgressNotificationTest {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
|
|
||||||
*
|
|
||||||
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
|
|
||||||
* worker awaits what this returns, so a future that never completes would hang the initial
|
|
||||||
* `setForeground` rather than test anything.
|
|
||||||
*/
|
|
||||||
private class RecordingForegroundUpdater : TestForegroundUpdater() {
|
|
||||||
val infos = mutableListOf<ForegroundInfo>()
|
|
||||||
|
|
||||||
override fun setForegroundAsync(
|
|
||||||
context: Context,
|
|
||||||
id: UUID,
|
|
||||||
foregroundInfo: ForegroundInfo,
|
|
||||||
): ListenableFuture<Void> {
|
|
||||||
infos += foregroundInfo
|
|
||||||
return super.setForegroundAsync(context, id, foregroundInfo)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/** An engine that reports whatever [report] wants reported, then writes an output. */
|
/** An engine that reports whatever [report] wants reported, then writes an output. */
|
||||||
private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder {
|
private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder {
|
||||||
override suspend fun run(
|
override suspend fun run(
|
||||||
|
|||||||
@@ -1,11 +1,14 @@
|
|||||||
package org.libremediaconverter.work
|
package org.libremediaconverter.work
|
||||||
|
|
||||||
import android.content.Context
|
import android.content.Context
|
||||||
|
import androidx.work.ForegroundInfo
|
||||||
|
import androidx.work.testing.TestForegroundUpdater
|
||||||
import com.google.common.util.concurrent.ListenableFuture
|
import com.google.common.util.concurrent.ListenableFuture
|
||||||
import org.libremediaconverter.convert.OutputPublisher
|
import org.libremediaconverter.convert.OutputPublisher
|
||||||
import org.libremediaconverter.convert.SoftwareTranscoder
|
import org.libremediaconverter.convert.SoftwareTranscoder
|
||||||
import org.libremediaconverter.model.ConversionRequest
|
import org.libremediaconverter.model.ConversionRequest
|
||||||
import java.io.File
|
import java.io.File
|
||||||
|
import java.util.UUID
|
||||||
import java.util.concurrent.ExecutionException
|
import java.util.concurrent.ExecutionException
|
||||||
import java.util.concurrent.Executor
|
import java.util.concurrent.Executor
|
||||||
import java.util.concurrent.TimeUnit
|
import java.util.concurrent.TimeUnit
|
||||||
@@ -94,3 +97,27 @@ internal class FailedFuture(private val failure: Throwable) : ListenableFuture<V
|
|||||||
override fun get(): Void = throw ExecutionException(failure)
|
override fun get(): Void = throw ExecutionException(failure)
|
||||||
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
|
override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default.
|
||||||
|
*
|
||||||
|
* Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture<Void>`: the
|
||||||
|
* worker awaits what this returns, so a future that never completes would hang the initial
|
||||||
|
* `setForeground` rather than test anything.
|
||||||
|
*
|
||||||
|
* Shared scaffolding since #252 moved it here out of `ProgressNotificationTest`, which asks what a
|
||||||
|
* *running* worker publishes; `ForegroundNotificationTest` asks what its *first* post is, and both
|
||||||
|
* questions need the same recorder. `infos.first()` is that first post in either.
|
||||||
|
*/
|
||||||
|
internal class RecordingForegroundUpdater : TestForegroundUpdater() {
|
||||||
|
val infos = mutableListOf<ForegroundInfo>()
|
||||||
|
|
||||||
|
override fun setForegroundAsync(
|
||||||
|
context: Context,
|
||||||
|
id: UUID,
|
||||||
|
foregroundInfo: ForegroundInfo,
|
||||||
|
): ListenableFuture<Void> {
|
||||||
|
infos += foregroundInfo
|
||||||
|
return super.setForegroundAsync(context, id, foregroundInfo)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -403,6 +403,27 @@ They are still correct to keep: `ForegroundInfo` is required by the `CoroutineWo
|
|||||||
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
|
named — a `getForegroundInfo` that starts branching — plus one more: the day anything calls
|
||||||
`setExpedited`.
|
`setExpedited`.
|
||||||
|
|
||||||
|
**Updated 2026-09-06 (#252, and the sentence above is half wrong).** "WorkManager calls
|
||||||
|
`getForegroundInfoAsync()` only for expedited work" is true and *not sufficient*, and the missing
|
||||||
|
half is what made the reopening trigger wrong. `WorkForeground.kt:38` in work-runtime 2.11.2 opens
|
||||||
|
the library's only caller with
|
||||||
|
|
||||||
|
```kotlin
|
||||||
|
if (!spec.expedited || Build.VERSION.SDK_INT >= 31) return
|
||||||
|
```
|
||||||
|
|
||||||
|
and `minSdk` is 33. So calling `setExpedited` reopens nothing: on **every** device this app
|
||||||
|
supports, WorkManager does not consult `getForegroundInfo()` whether the work is expedited or not.
|
||||||
|
#252 was filed on the trigger as this entry stated it, and its acceptance criterion — "a request
|
||||||
|
now carries `setExpedited` and the existing worker tests drive them" — cannot be met that way.
|
||||||
|
|
||||||
|
What made the lines live instead was that each worker held **two** definitions of one notification:
|
||||||
|
the override, and an identical `ForegroundInfo` built inline in `doWork`. `doWork` now posts the
|
||||||
|
override's, so the duplicate is gone and what remains runs on every job. The general lesson is the
|
||||||
|
one E1 states from the other side: *check that the mechanism you are relying on actually fires on
|
||||||
|
the machine that runs it* — here the mechanism was a library early-return two source lines long,
|
||||||
|
and four waves of reading had taken the API summary's word for it.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
|
## F10 — Three arms that are reachable, uncovered, and cannot be made to bite
|
||||||
@@ -452,7 +473,7 @@ the cheaper order.
|
|||||||
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
|
| F6 | Four more unreachable arms; `ConversionRouter:214-217`'s KDoc is false | low | confirmed by inspection; each traced to its upstream guard | **no action**, except the one-line KDoc fix |
|
||||||
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
|
| F7 | `probeWithExtractor`'s catch is unreachable, as `probeForConcat`'s is | n/a | measured across four URI shapes (recorded in `CLAUDE.md`) | **no action** — device-only, now written down for both sites |
|
||||||
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
|
| F8 | Three more dead members and six unused defaults | low | confirmed by inspection; grep per member | delete or keep knowingly — **not** a test gap |
|
||||||
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **no action** — sharpens #88's close |
|
| F9 | Both `getForegroundInfo` overrides are dead: expedited work is never used | n/a | confirmed by inspection; `grep setExpedited` returns nothing | **closed 2026-09-06 by #252** — and its stated reopening trigger was wrong; see the update on the entry |
|
||||||
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
|
| F10 | Three reachable arms where no mutation bites | n/a | confirmed by inspection; each mutation traced to its masking guard | **no action** — recorded to stop the next read re-picking them |
|
||||||
|
|
||||||
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
|
Order, if these are acted on: **F1 and F5 first, separately.** They are the two with a possible
|
||||||
|
|||||||
@@ -495,7 +495,7 @@ Every one was read. **None of them is an e2e test gap**, which is the result:
|
|||||||
| lines | where | classification |
|
| lines | where | classification |
|
||||||
|---|---|---|
|
|---|---|---|
|
||||||
| 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` |
|
| 9 | `Transcoders` ×3, `ConversionViewModel`, `ConverterScreen`, `JoinViewModel`, `JoinScreen`, `MainActivity`, `Reattachment` | **compiler-generated** — default-arg `$default` bridges, coroutine completion, the synthetic `NoWhenBranchMatchedException` arm of a `when` over `Destination` |
|
||||||
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252** |
|
| 10 | `ConversionWorker:342-346`, `ConcatWorker:132-136` | `getForegroundInfo()` — WorkManager's **expedited-work** hook, and nothing here enqueues expedited work. The live path is `setForeground(foregroundInfo(...))`, which is covered. **#252 — closed 2026-09-06, and not the way this row expects.** Expedited work is now enqueued, but that is *not* what covers these lines: `WorkForeground.kt:38` returns before the hook whenever `SDK_INT >= 31`, and `minSdk` is 33. What covers them is `doWork` posting the override instead of a second copy of the same notification. See `coverage-read-findings.md` F9's update |
|
||||||
| 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record |
|
| 3 | `ConversionNotifications:60-62` | **F5** — `areEnabled()` has no callers. Already on record |
|
||||||
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
|
| 3 | `CopyPlanner:28`, `OutputFormat:222-223` | public members with no callers. **#253**, with F5 |
|
||||||
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
|
| 3 | `MediaProbe:210-212` | `probeWithFFprobe`'s `catch` — **F7's sibling, and now measured**. See below |
|
||||||
|
|||||||
Reference in New Issue
Block a user