diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | .superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout-report.md | 22 | ||||
| -rw-r--r-- | .superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout.png | bin | 0 -> 105109 bytes | |||
| -rw-r--r-- | app/src/main/java/org/terst/listen/fingerprint/BreakTrack.kt | 47 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt | 141 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt | 131 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt | 318 |
7 files changed, 340 insertions, 321 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index b0b26f7..94f785c 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.23 +94.15 diff --git a/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout-report.md b/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout-report.md new file mode 100644 index 0000000..8c53a09 --- /dev/null +++ b/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout-report.md @@ -0,0 +1,22 @@ +# Timeline readout report + +## Changed +- `SkipSummary.kt`: `EpisodeSkips` is now `served`, `declared`, `cut`, `status` plus derived `numbers`. No lists, no per-break strings, no intro/outro. `SkipSummary.episode(episode, ranges)` lost its `show` param. Removed `BreakLine`, `breakLines`, `offset`, `describe`, `insertedAds` (the gap of served minus declared is now what the line lets you see). Added `LEGEND_ADVERTISING`, `LEGEND_OWN_AUDIO`. +- New `BreakTrack.kt` (pure): `markAt(marks, fraction, slop)`, `trackFraction(x, width, inset)`, `describeMark`, `markStartMs`. Split out of SkipSummary because detekt TooManyFunctions fired at 14; threshold NOT raised. +- `EpisodeScreen.kt`: readout moved directly under the scrubber (above the buttons, so on screen without scrolling). `BreakMarks` is now a 28dp tappable strip under the slider (tap -> `BreakTrack.markAt` with 24px slop -> name shown below + `seekTo(start)`); colours tertiary (advertising) / secondary (own audio) / surfaceVariant (track) / onSurface (selected outline), all from MaterialTheme, no alpha. Dropped unused `show` plumbing. + +## Wording +`Served 29:34 · Declared 22:05 · Cut 6:15` (unknown parts omitted, line absent if none known). Status line only when no breaks: `Not checked yet` / `None found` / waiting text / `Can't check this copy; redownload to get the server's` (all pinned strings unchanged). Legend: `Advertising (always cut)`, `Show's own audio (follows your setting)` (each only if that kind is present). Tap text: `Ad break, 5:00 - 10:00` via `describeKind`. + +## Tests +Deliberately changed (the removed behaviour): intro/outro offset tests, per-break line tests, insertedAds tests, declared/measured/total tests -- rewritten against the new fields. Pinned describeKind/isAdvertising/markers tests unchanged. Added: three-number line, each unknown case, no-breaks, 37-break regression guard (no Collection field, no per-break text), status cases, hit-testing (inside, between, slop, overlap, none, trackFraction, describeMark). +- Red: stubbed API, `gradle-test testDebugUnitTest -- --rerun-tasks` -> 738 tests, 15 failures (assertion errors). +- Green: 738 tests, 0 failures, exit 0. detekt via gradle-report: 0 findings, exit 0. + +## Device +emulator-5554 (doot_test_api36), debug build installed, seeded one episode (Grass Apes, 1325000 declared, 1774000 served, one intro + two ad breaks) into listen.db. Screenshot: `.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout.png`. Dark theme only; numbers, strip, legend all above the fold. Tap-on-mark NOT exercised on device (canvas has no label for android-ui, blind adb taps forbidden); its logic is unit-tested only. + +## Noticed, not changed +- Seeded kind='rss' crashes the app on read (enum is PODCAST); my seed error, not a bug. +- Cold start on the emulator ANRs the first launch (114MB APK); retry worked. +- Light theme not checked by eye (colours are all theme roles). diff --git a/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout.png b/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout.png Binary files differnew file mode 100644 index 0000000..56e75d3 --- /dev/null +++ b/.superpowers/sdd/2026-10-12-break-kinds-and-furniture-toggle/timeline-readout.png diff --git a/app/src/main/java/org/terst/listen/fingerprint/BreakTrack.kt b/app/src/main/java/org/terst/listen/fingerprint/BreakTrack.kt new file mode 100644 index 0000000..576027e --- /dev/null +++ b/app/src/main/java/org/terst/listen/fingerprint/BreakTrack.kt @@ -0,0 +1,47 @@ +package org.terst.listen.fingerprint + +/** + * Hit-testing and naming for the marks on the episode screen's track. + * + * Separate from [SkipSummary] only to keep each object a size one reader can hold; the two are one + * feature. Pure, so the one thing a Canvas cannot give for free -- which mark a finger landed on -- + * is covered by tests rather than by eye. + */ +object BreakTrack { + + /** + * Which mark a tap at [fraction] of the track landed on, or null when it landed between them. + * + * [slop] widens every mark on both sides, because a break is often a few pixels wide and a + * finger is not. Where widened marks overlap, the one whose edge is nearest wins. + */ + fun markAt(marks: List<SkipSummary.BreakMarker>, fraction: Float, slop: Float): SkipSummary.BreakMarker? = + marks.map { it to distanceTo(it, fraction) } + .filter { it.second <= slop } + .minByOrNull { it.second } + ?.first + + private fun distanceTo(mark: SkipSummary.BreakMarker, fraction: Float): Float = when { + fraction < mark.start -> mark.start - fraction + fraction > mark.end -> fraction - mark.end + else -> 0f + } + + /** + * A pixel position along a track that is [inset] shorter at each end than its [width], as a + * fraction of the track -- the same inset the marks are drawn with, so a tap and a drawn mark + * agree about where 0.4 is. + */ + fun trackFraction(x: Float, width: Float, inset: Float): Float { + val usable = width - inset * 2 + return if (usable <= 0f) 0f else ((x - inset) / usable).coerceIn(0f, 1f) + } + + /** What tapping a mark says: its kind in the pinned wording, then when it runs. */ + fun describeMark(mark: SkipSummary.BreakMarker, durationMs: Long): String = + "${SkipSummary.describeKind(mark.kind)}, ${SkipSummary.clock(markStartMs(mark, durationMs))} - " + + SkipSummary.clock((mark.end * durationMs).toLong()) + + /** Where a tap on [mark] seeks to. */ + fun markStartMs(mark: SkipSummary.BreakMarker, durationMs: Long): Long = (mark.start * durationMs).toLong() +} diff --git a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt index 9c2576d..d42d2a8 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt @@ -4,34 +4,37 @@ import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity -import org.terst.listen.data.insertedAdMs import org.terst.listen.data.ShowFingerprintEntity import org.terst.listen.data.ShowMidrollEntity -import org.terst.listen.data.SubscriptionEntity /** - * What the episode screen shows about skipping. [breaks] is never empty; [insertedAds] is null - * when there is nothing to compare, which is any episode the player has not opened yet. + * What the episode screen says about skipping, as three numbers and at most one status. + * + * Deliberately no per-break text: a list of 37 timestamps hid that the detector was matching + * the whole episode, where the same breaks as marks on a track were obviously wrong. Each of + * [served], [declared] and [cut] is null when genuinely unknown -- never a zero or a dash. */ data class EpisodeSkips( - val intro: String, - val outro: String, - /** One line per detected break, or a single status line when none is detected yet. */ - val breaks: List<SkipSummary.BreakLine>, - val insertedAds: String?, - /** What the feed says the episode is, null if it does not say. */ - val declaredLength: String? = null, /** What the player measured the file to be, null until it has opened it. */ - val measuredLength: String? = null, + val served: String?, + /** What the feed says the episode is, null if it does not say. */ + val declared: String?, + /** What the detected breaks add up to, null when none are detected. */ + val cut: String?, /** - * How much of the episode the detected breaks add up to, null when none are detected. - * - * The point of showing it next to [insertedAds]: the length gap says how much advertising is - * in the file without any audio analysis, so these two should roughly agree. Far short means - * the scan is missing breaks; far over means it is matching programme. + * Why there are no breaks to show: not checked yet, none found, waiting, or unable to check. + * Null once any break is detected. "Nothing found" and "not checked yet" are different + * answers and must stay different. */ - val breaksTotal: String? = null, -) + val status: String?, +) { + /** The three numbers on one line, known ones only; null when none is known. */ + val numbers: String? = listOfNotNull( + served?.let { "Served $it" }, + declared?.let { "Declared $it" }, + cut?.let { "Cut $it" }, + ).takeIf { it.isNotEmpty() }?.joinToString(" \u00b7 ") +} /** * Says in words what the skip detection has worked out, so it stops being invisible. @@ -60,12 +63,6 @@ object SkipSummary { data class BreakMarker(val start: Float, val end: Float, val kind: String) /** - * One line of the break list on the episode screen: what to call it, from [describeKind], - * and when it falls (a clock range, or a status message when nothing has a range yet). - */ - data class BreakLine(val label: String, val range: String) - - /** * The detected breaks as positions on a 0..1 timeline, for the episode screen's scrubber. * * Here rather than in the composable because a marker drawn in the wrong place looks @@ -140,84 +137,50 @@ object SkipSummary { } /** - * The offset actually in force for one episode, and where it came from. - * - * Mirrors [org.terst.listen.playback.effectiveIntroMs]: a detected value wins over the - * manual one. A detected *zero* is a real finding -- this show turned out to have no intro - * -- and must not read as "nothing detected", or the readout would disagree with what - * playback is doing. - */ - private fun offset(detectedMs: Long?, manualMs: Long): String = when { - detectedMs != null -> "${describe(detectedMs)}, detected" - manualMs > 0 -> "${describe(manualMs)}, set by hand" - else -> "None" - } - - /** - * Detected breaks in one episode, earliest first, each named by [describeKind] rather than - * lumped under one "Ad breaks" header -- the whole point of this is that they are not all - * advertising. - */ - private fun breakLines(ranges: List<EpisodeMidrollEntity>): List<BreakLine> = - ranges.sortedBy { it.startMs }.map { - BreakLine(describeKind(it.kind), "${clock(it.startMs)} - ${clock(it.endMs)}") - } - - /** * Everything the episode screen says about skipping, decided here rather than in the * composable so the "not checked yet" case is actually covered by a test. */ fun episode( episode: EpisodeEntity, - show: SubscriptionEntity?, ranges: List<EpisodeMidrollEntity>, - ): EpisodeSkips { - val breaks = breakLines(ranges) - return EpisodeSkips( - insertedAds = episode.insertedAdMs?.let { clock(it) }, - declaredLength = episode.durationMs?.let { clock(it) }, - measuredLength = episode.actualDurationMs?.let { clock(it) }, - breaksTotal = ranges.sumOf { (it.endMs - it.startMs).coerceAtLeast(0) } - .takeIf { ranges.isNotEmpty() } - ?.let { clock(it) }, - intro = offset(episode.detectedIntroMs, show?.skipIntroMs ?: 0), - outro = offset(episode.detectedOutroMs, show?.skipOutroMs ?: 0), - breaks = when { - breaks.isNotEmpty() -> breaks - // Nothing has been downloaded yet, so the skips cannot have been checked either; - // say what is actually happening instead of "Not checked yet". - episode.downloadState == DownloadState.WAITING_FOR_SERVER -> - listOf(BreakLine("Breaks", WaitingForServerText.STATUS)) - // A download that is not the server's own copy can never be refreshed in - // place -- a show with dynamic ad insertion serves a different file on every - // request, so the server's timestamps only mean anything against its own - // audio. "Not checked yet" would read as "the server has not looked", which - // promises this will resolve on its own; it never will without a re-download, - // so it gets its own, honest status instead. A download that predates this - // column defaults to the same "not the server's" and is treated identically -- - // the conservative answer for "we don't know" is "assume it cannot be checked". - episode.downloadState == DownloadState.DOWNLOADED && !episode.audioFromServer -> - listOf(BreakLine("Breaks", CANNOT_CHECK_THIS_COPY)) - // Distinct from "none found": scanning runs nightly while charging, so a - // freshly downloaded episode has genuinely not been looked at yet. Neither - // status line names a kind, so both use a generic label rather than one of - // describeKind's four. - episode.midrollCheckedAtMs == null -> listOf(BreakLine("Breaks", "Not checked yet")) - else -> listOf(BreakLine("Breaks", "None found")) - }, - ) + ): EpisodeSkips = EpisodeSkips( + served = episode.actualDurationMs?.let { clock(it) }, + declared = episode.durationMs?.let { clock(it) }, + cut = ranges.sumOf { (it.endMs - it.startMs).coerceAtLeast(0) } + .takeIf { ranges.isNotEmpty() } + ?.let { clock(it) }, + status = if (ranges.isEmpty()) statusWithoutBreaks(episode) else null, + ) + + private fun statusWithoutBreaks(episode: EpisodeEntity): String = when { + // Nothing has been downloaded yet, so the skips cannot have been checked either; say what + // is actually happening instead of "Not checked yet". + episode.downloadState == DownloadState.WAITING_FOR_SERVER -> WaitingForServerText.STATUS + // A download that is not the server's own copy can never be refreshed in place -- a show + // with dynamic ad insertion serves a different file on every request, so the server's + // timestamps only mean anything against its own audio. "Not checked yet" would promise + // this resolves on its own; it never will without a re-download. A download that + // predates this column defaults to the same "not the server's" and is treated + // identically: the conservative answer for "we don't know" is "assume it cannot be + // checked". + episode.downloadState == DownloadState.DOWNLOADED && !episode.audioFromServer -> CANNOT_CHECK_THIS_COPY + // Distinct from "none found": scanning runs nightly while charging, so a freshly + // downloaded episode has genuinely not been looked at yet. + episode.midrollCheckedAtMs == null -> "Not checked yet" + else -> "None found" } + /** Legend text for the two colours on the track: only advertising is unconditionally cut. */ + const val LEGEND_ADVERTISING = "Advertising (always cut)" + const val LEGEND_OWN_AUDIO = "Show's own audio (follows your setting)" + /** * Shown instead of "Not checked yet" for a download that cannot be refreshed against the * server -- a status, not a transient wait, so the wording says what would actually fix it. */ const val CANNOT_CHECK_THIS_COPY = "Can't check this copy; redownload to get the server's" - private fun describe(ms: Long): String = - if (ms <= 0) "None" else "${ms / MS_PER_SECOND}s" - - private fun clock(ms: Long): String { + internal fun clock(ms: Long): String { val totalSeconds = ms / MS_PER_SECOND val seconds = totalSeconds % SECONDS_PER_MINUTE val minutes = (totalSeconds / SECONDS_PER_MINUTE) % MINUTES_PER_HOUR diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt index b3a7c96..21a9b5b 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt @@ -2,6 +2,7 @@ package org.terst.listen.ui import androidx.compose.foundation.Canvas import androidx.compose.foundation.background +import androidx.compose.foundation.gestures.detectTapGestures import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column @@ -43,6 +44,9 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip import androidx.compose.ui.geometry.Offset import androidx.compose.ui.geometry.Size +import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.drawscope.Stroke +import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage @@ -50,14 +54,12 @@ import kotlinx.coroutines.launch import org.terst.listen.data.EpisodeEntity import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.EpisodeMidrollEntity -import org.terst.listen.data.SubscriptionEntity import org.terst.listen.data.playbackDurationMs import org.terst.listen.feed.WatchLink +import org.terst.listen.fingerprint.BreakTrack import org.terst.listen.fingerprint.EpisodeSkips import org.terst.listen.fingerprint.SkipSummary import org.terst.listen.playback.NowPlaying -import org.terst.listen.playback.effectiveIntroMs -import org.terst.listen.playback.effectiveOutroMs /** * One episode in full. @@ -82,7 +84,6 @@ fun EpisodeScreen( } val scope = rememberCoroutineScope() val rules by vm.rules.collectAsState() - val shows by vm.subscriptions.collectAsState() val nowPlaying by player.nowPlaying.collectAsState() val current = episode @@ -102,7 +103,6 @@ fun EpisodeScreen( episode = current, detail = episodeDetailState( episode = current, - show = shows.firstOrNull { it.id == current.subscriptionId }, ruleName = rules.firstOrNull { rule -> rule.id == current.appliedRuleId }?.name, ranges = ranges, nowPlaying = nowPlaying, @@ -165,6 +165,8 @@ private fun EpisodeDetail( EpisodeScrubber(detail.playback, actions) + SkipReadout(detail.skips, detail.playback.breaks) + FlowRow( horizontalArrangement = Arrangement.spacedBy(8.dp), verticalArrangement = Arrangement.spacedBy(8.dp), @@ -195,8 +197,6 @@ private fun EpisodeDetail( RuleAttribution(episode, detail.ruleName) - SkipReadout(detail.skips) - episode.description?.let { Text(stripHtml(it), style = MaterialTheme.typography.bodyMedium) } @@ -232,7 +232,6 @@ data class EpisodeDetailState( */ private fun episodeDetailState( episode: EpisodeEntity, - show: SubscriptionEntity?, ruleName: String?, ranges: List<EpisodeMidrollEntity>, nowPlaying: NowPlaying, @@ -242,7 +241,7 @@ private fun episodeDetailState( val durationMs = if (isPlayingThis) nowPlaying.durationMs else episode.playbackDurationMs return EpisodeDetailState( ruleName = ruleName, - skips = SkipSummary.episode(episode, show, ranges), + skips = SkipSummary.episode(episode, ranges), playback = EpisodePlayback( positionMs = if (isPlayingThis) nowPlaying.positionMs else episode.positionMs, durationMs = durationMs, @@ -285,10 +284,6 @@ private fun EpisodeScrubber(playback: EpisodePlayback, actions: EpisodeDetailAct } Column(Modifier.weight(1f)) { Box { - // Behind the slider, so the track reads as the episode and the marks read as - // parts of it. Drawn at the track's own inset, or they would not line up with - // the thumb positions they are describing. - BreakMarks(playback.breaks, Modifier.align(Alignment.Center)) Slider( value = shown.coerceIn(0f, duration.toFloat().coerceAtLeast(0f)), onValueChange = { dragValue = it }, @@ -300,6 +295,7 @@ private fun EpisodeScrubber(playback: EpisodePlayback, actions: EpisodeDetailAct enabled = duration > 0, ) } + BreakMarks(playback.breaks, duration) { actions.seekTo(it) } Row(Modifier.fillMaxWidth(), horizontalArrangement = Arrangement.SpaceBetween) { Text(formatClock(shown.toLong()), style = MaterialTheme.typography.bodySmall) Text(formatClock(duration), style = MaterialTheme.typography.bodySmall) @@ -309,64 +305,90 @@ private fun EpisodeScrubber(playback: EpisodePlayback, actions: EpisodeDetailAct } /** - * The detected breaks as bands across the track, advertising in one colour and the show's own - * audio (intro, cue, outro) in another, decided by [SkipSummary.isAdvertising] rather than by - * comparing against [SkipSummary.describeKind]'s display string -- a renamed label must not - * silently invert which colour means advertising. A kind this version does not recognise draws - * as advertising too, matching what playback does with it. + * The detected breaks as bands on a strip under the slider, advertising in one colour and the + * show's own audio (intro, cue, outro) in another, decided by [SkipSummary.isAdvertising] rather + * than by comparing against [SkipSummary.describeKind]'s display string -- a renamed label must + * not silently invert which colour means advertising. A kind this version does not recognise + * draws as advertising too, matching what playback does with it. + * + * Tapping a band names it and seeks to its start. The strip is its own, taller target rather than + * a hairline behind the slider, because a break is a few pixels wide and a finger is not; what a + * tap hits is decided by [BreakTrack.markAt], so the composable only converts pixels. */ @Composable -private fun BreakMarks(breaks: List<SkipSummary.BreakMarker>, modifier: Modifier) { +private fun BreakMarks(breaks: List<SkipSummary.BreakMarker>, durationMs: Long, onSeek: (Long) -> Unit) { if (breaks.isEmpty()) return + var picked by remember(breaks) { mutableStateOf<SkipSummary.BreakMarker?>(null) } val adColour = MaterialTheme.colorScheme.tertiary val furnitureColour = MaterialTheme.colorScheme.secondary - Canvas(modifier.fillMaxWidth().height(BREAK_MARK_HEIGHT)) { + val trackColour = MaterialTheme.colorScheme.surfaceVariant + val outlineColour = MaterialTheme.colorScheme.onSurface + Canvas( + Modifier + .fillMaxWidth() + .height(BREAK_STRIP_HEIGHT) + .pointerInput(breaks, durationMs) { + detectTapGestures { tap -> + val width = size.width.toFloat() + val usable = (width - THUMB_RADIUS_PX * 2).coerceAtLeast(1f) + val hit = BreakTrack.markAt( + breaks, + BreakTrack.trackFraction(tap.x, width, THUMB_RADIUS_PX), + slop = TAP_SLOP_PX / usable, + ) + picked = hit + hit?.let { onSeek(BreakTrack.markStartMs(it, durationMs)) } + } + }, + ) { // Inset by the thumb's radius at each end: the slider's track is laid out inside it, so // marks drawn edge to edge would sit a thumb-width off the positions they describe. val inset = THUMB_RADIUS_PX val usable = (size.width - inset * 2).coerceAtLeast(0f) + val bandTop = (size.height - BREAK_BAND_PX) / 2 + drawRect(trackColour, Offset(inset, bandTop), Size(usable, BREAK_BAND_PX)) breaks.forEach { mark -> val left = inset + usable * mark.start val right = inset + usable * mark.end - val isAd = SkipSummary.isAdvertising(mark.kind) - drawRect( - color = if (isAd) adColour else furnitureColour, - topLeft = Offset(left, 0f), - size = Size((right - left).coerceAtLeast(MIN_BREAK_MARK_PX), size.height), - ) + val topLeft = Offset(left, bandTop) + val bandSize = Size((right - left).coerceAtLeast(MIN_BREAK_MARK_PX), BREAK_BAND_PX) + drawRect(if (SkipSummary.isAdvertising(mark.kind)) adColour else furnitureColour, topLeft, bandSize) + if (mark == picked) drawRect(outlineColour, topLeft, bandSize, style = Stroke(width = 2f)) } } + picked?.let { + Text(BreakTrack.describeMark(it, durationMs), style = MaterialTheme.typography.bodySmall) + } } /** - * What is actually being skipped in this episode, and where the numbers came from. + * Served, declared and cut, on one line, plus a key for the two colours on the track. * - * The one place the detection is visible per episode. It deliberately reports what playback - * will really do -- [effectiveIntroMs] and [effectiveOutroMs] prefer a detected value over the - * manual one, so reading the manual field here would quietly contradict the player. - * - * "Not checked yet" is a distinct state from "nothing found". Midroll scanning runs nightly - * while charging, so a freshly downloaded episode legitimately has nothing to say yet, and - * reporting that as "no ad breaks" would look like the feature failing. + * The check Peter asked for: the gap between the first two is the advertising in the file, and + * the third is what the detector took out, so a third that is far from that gap is the detector + * being wrong. When there is no cut to report, [EpisodeSkips.status] says why -- "not checked + * yet" is a different answer from "none found" and must stay one. Intro and outro are not here: + * the server does not publish those offsets yet, and when it does they are marks like any other. */ @Composable -private fun SkipReadout(skips: EpisodeSkips) { +private fun SkipReadout(skips: EpisodeSkips, breaks: List<SkipSummary.BreakMarker>) { Column(verticalArrangement = Arrangement.spacedBy(2.dp)) { - Text("Skips", style = MaterialTheme.typography.titleSmall) - // Free, and a check on the detector: whatever it finds should eventually add up to - // about this much. - // The three numbers only mean anything together: the feed's length against the file's - // says how much advertising is in it, and what the detector found says how much of that - // it has actually accounted for. Short means breaks are being missed; over means - // programme is being matched. - skips.declaredLength?.let { declared -> - ReadoutRow("Length", skips.measuredLength?.let { "$declared declared, $it measured" } ?: declared) + skips.numbers?.let { Text(it, style = MaterialTheme.typography.bodyMedium) } + skips.status?.let { Text(it, style = MaterialTheme.typography.bodySmall) } + if (breaks.any { SkipSummary.isAdvertising(it.kind) }) { + LegendEntry(MaterialTheme.colorScheme.tertiary, SkipSummary.LEGEND_ADVERTISING) + } + if (breaks.any { !SkipSummary.isAdvertising(it.kind) }) { + LegendEntry(MaterialTheme.colorScheme.secondary, SkipSummary.LEGEND_OWN_AUDIO) } - skips.insertedAds?.let { ReadoutRow("Ads inserted", "~$it") } - ReadoutRow("Intro", skips.intro) - ReadoutRow("Outro", skips.outro) - skips.breaks.forEach { line -> ReadoutRow(line.label, line.range) } - skips.breaksTotal?.let { ReadoutRow("Breaks found", it) } + } +} + +@Composable +private fun LegendEntry(colour: Color, label: String) { + Row(verticalAlignment = Alignment.CenterVertically, horizontalArrangement = Arrangement.spacedBy(6.dp)) { + Box(Modifier.size(LEGEND_SWATCH).background(colour)) + Text(label, style = MaterialTheme.typography.bodySmall) } } @@ -403,8 +425,15 @@ private fun RuleAttribution(episode: EpisodeEntity, ruleName: String?) { Text(label, style = MaterialTheme.typography.bodySmall) } -/** Thin enough to read as an annotation on the track rather than a second control. */ -private val BREAK_MARK_HEIGHT = 4.dp +/** Tall enough to tap; the band drawn inside it is what reads as an annotation on the track. */ +private val BREAK_STRIP_HEIGHT = 28.dp + +private const val BREAK_BAND_PX = 20f + +/** How far from a band a tap still counts as hitting it. */ +private const val TAP_SLOP_PX = 24f + +private val LEGEND_SWATCH = 10.dp /** Material's slider thumb radius, which the track is inset by at both ends. */ private const val THUMB_RADIUS_PX = 20f diff --git a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt index d550c69..dc9c41b 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -68,15 +68,6 @@ class SkipSummaryTest { audioFromServer = audioFromServer ?: false, ) - private fun showRow(skipIntroMs: Long = 0, skipOutroMs: Long = 0) = SubscriptionEntity( - id = "s1", - feedUrl = "https://example.com/feed.xml", - title = "A Show", - addedAtMs = 0, - skipIntroMs = skipIntroMs, - skipOutroMs = skipOutroMs, - ) - private fun candidate(id: String, confirmedCount: Int) = ShowMidrollEntity( subscriptionId = "s1", id = id, @@ -138,44 +129,101 @@ class SkipSummaryTest { assertEquals("1 confirmed", SkipSummary.midrollState(listOf(candidate("a", 3)), 3)) } - /** Detection wins over the manual offset, and the readout has to say which is in force. */ + + /** + * The three numbers Peter asked for as a sanity check, on one line: what was served, what the + * feed declared, and how much the breaks cut. Served first because it is the one measured + * against the actual file. + */ @Test - fun offsetNamesWhereTheNumberCameFrom() { - val detected = SkipSummary.episode(episodeRow(detectedIntroMs = 47_000), showRow(skipIntroMs = 30_000), emptyList()) - assertEquals("47s, detected", detected.intro) + fun `shows served, declared and cut on one line`() { + val skips = SkipSummary.episode( + episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), + listOf( + EpisodeMidrollEntity("e1", "c1", startMs = 120_000, endMs = 180_000), + EpisodeMidrollEntity("e1", "c2", startMs = 600_000, endMs = 690_000), + ), + ) - val manual = SkipSummary.episode(episodeRow(), showRow(skipIntroMs = 30_000), emptyList()) - assertEquals("30s, set by hand", manual.intro) + assertEquals("29:34", skips.served) + assertEquals("22:05", skips.declared) + assertEquals("2:30", skips.cut) + assertEquals("Served 29:34 \u00b7 Declared 22:05 \u00b7 Cut 2:30", skips.numbers) + assertEquals(null, skips.status) + } + + /** Never played, so the file has not been measured: omitted, not shown as a zero or a dash. */ + @Test + fun `an unmeasured length is omitted from the line`() { + val skips = SkipSummary.episode( + episodeRow(durationMs = 1_325_000, midrollCheckedAtMs = 1), + listOf(EpisodeMidrollEntity("e1", "c1", startMs = 120_000, endMs = 180_000)), + ) - val neither = SkipSummary.episode(episodeRow(), showRow(), emptyList()) - assertEquals("None", neither.intro) + assertEquals(null, skips.served) + assertEquals("Declared 22:05 \u00b7 Cut 1:00", skips.numbers) } - /** A detected zero is a real answer -- this show has no intro -- not a missing one. */ @Test - fun aDetectedZeroIsNotTheSameAsNothingDetected() { - val skips = SkipSummary.episode(episodeRow(detectedIntroMs = 0), showRow(skipIntroMs = 30_000), emptyList()) + fun `an undeclared length is omitted from the line`() { + val skips = SkipSummary.episode( + episodeRow(actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), + listOf(EpisodeMidrollEntity("e1", "c1", startMs = 120_000, endMs = 180_000)), + ) - assertEquals("None, detected", skips.intro) + assertEquals(null, skips.declared) + assertEquals("Served 29:34 \u00b7 Cut 1:00", skips.numbers) } - /** Earliest first, whatever order the rows come back in. */ + /** Nothing detected is not a cut of zero -- there is no figure to compare yet. */ @Test - fun rangesReadAsClockTimes() { - val ranges = listOf( - EpisodeMidrollEntity(episodeId = "e1", candidateId = "a", startMs = 750_000, endMs = 825_000), - EpisodeMidrollEntity(episodeId = "e1", candidateId = "b", startMs = 61_000, endMs = 91_500), + fun `no detected breaks means no cut figure`() { + val skips = SkipSummary.episode( + episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), + emptyList(), ) + assertEquals(null, skips.cut) + assertEquals("Served 29:34 \u00b7 Declared 22:05", skips.numbers) + } + + @Test + fun `nothing known means no line at all`() { + assertEquals(null, SkipSummary.episode(episodeRow(), emptyList()).numbers) + } + + @Test + fun `lengths past an hour keep the hour`() { + val skips = SkipSummary.episode(episodeRow(actualDurationMs = 3_723_000), emptyList()) + + assertEquals("1:02:03", skips.served) + } + + /** + * Regression guard for the 37-row screen: the readout exposes no per-break text. The marks on + * the track carry positions; if a list comes back, a field of list type has to have appeared. + */ + @Test + fun `the readout carries no per-break strings`() { + val ranges = (1..37).map { + EpisodeMidrollEntity("e1", "c$it", startMs = it * 100_000L, endMs = it * 100_000L + 5_000) + } + val skips = SkipSummary.episode( + episodeRow(durationMs = 1_325_000, actualDurationMs = 4_000_000, midrollCheckedAtMs = 1), + ranges, + ) + + val everything = skips.toString() + assertEquals(false, everything.contains("1:40")) + assertEquals(false, everything.contains("Ad break")) + assertEquals(false, everything.contains(" - ")) assertEquals( - listOf( - SkipSummary.BreakLine("Ad break", "1:01 - 1:31"), - SkipSummary.BreakLine("Ad break", "12:30 - 13:45"), - ), - SkipSummary.episode(episodeRow(), showRow(), ranges).breaks, + false, + EpisodeSkips::class.java.declaredFields.any { Collection::class.java.isAssignableFrom(it.type) }, ) } + /** * "Not checked yet" and "none found" are different answers. * @@ -185,20 +233,16 @@ class SkipSummaryTest { */ @Test fun anUncheckedEpisodeSaysSoRatherThanClaimingNoBreaks() { - val skips = SkipSummary.episode(episodeRow(), showRow(), ranges = emptyList()) + val skips = SkipSummary.episode(episodeRow(), ranges = emptyList()) - assertEquals(listOf(SkipSummary.BreakLine("Breaks", "Not checked yet")), skips.breaks) + assertEquals("Not checked yet", skips.status) } @Test fun aCheckedEpisodeWithNothingInItSaysNoneFound() { - val skips = SkipSummary.episode( - episodeRow(midrollCheckedAtMs = 1_000), - showRow(), - ranges = emptyList(), - ) + val skips = SkipSummary.episode(episodeRow(midrollCheckedAtMs = 1_000), ranges = emptyList()) - assertEquals(listOf(SkipSummary.BreakLine("Breaks", "None found")), skips.breaks) + assertEquals("None found", skips.status) } /** @@ -212,177 +256,78 @@ class SkipSummaryTest { */ @Test fun aDownloadNotSourcedFromTheServerSaysItCannotBeChecked() { - val skips = SkipSummary.episode( - downloadedEpisode(audioFromServer = false), - showRow(), - ranges = emptyList(), - ) + val skips = SkipSummary.episode(downloadedEpisode(audioFromServer = false), ranges = emptyList()) - assertEquals( - listOf(SkipSummary.BreakLine("Breaks", SkipSummary.CANNOT_CHECK_THIS_COPY)), - skips.breaks, - ) + assertEquals(SkipSummary.CANNOT_CHECK_THIS_COPY, skips.status) } /** A server-sourced download with nothing yet is still a legitimate "not checked yet", * unlike a publisher-sourced one, which can never become checked by waiting. */ @Test fun aServerSourcedDownloadWithNothingYetStillSaysNotCheckedYet() { - val skips = SkipSummary.episode( - downloadedEpisode(audioFromServer = true), - showRow(), - ranges = emptyList(), - ) + val skips = SkipSummary.episode(downloadedEpisode(audioFromServer = true), ranges = emptyList()) - assertEquals(listOf(SkipSummary.BreakLine("Breaks", "Not checked yet")), skips.breaks) + assertEquals("Not checked yet", skips.status) } - @Test - fun episodeReportsTheOffsetsActuallyInForce() { - val skips = SkipSummary.episode( - episodeRow(detectedIntroMs = 47_000, midrollCheckedAtMs = 1_000), - showRow(skipIntroMs = 30_000, skipOutroMs = 20_000), - ranges = listOf( - EpisodeMidrollEntity(episodeId = "e1", candidateId = "a", startMs = 61_000, endMs = 91_500) - ), - ) - - assertEquals("47s, detected", skips.intro) - assertEquals("20s, set by hand", skips.outro) - assertEquals(listOf(SkipSummary.BreakLine("Ad break", "1:01 - 1:31")), skips.breaks) - } + // ---- hit-testing the marks on the track ---------------------------------------------- - /** - * The point of this whole task: a break list with a mix of kinds names each one by what it - * actually is, instead of calling all of it "Ad breaks". - */ - @Test - fun `breaks of different kinds are each named for what they are`() { - val skips = SkipSummary.episode( - episodeRow(), - showRow(), - ranges = listOf( - EpisodeMidrollEntity("e1", "a", startMs = 0, endMs = 20_000, kind = "intro"), - EpisodeMidrollEntity("e1", "b", startMs = 300_000, endMs = 420_000, kind = "ad"), - EpisodeMidrollEntity("e1", "c", startMs = 500_000, endMs = 520_000, kind = "outro"), - ), - ) - - assertEquals( - listOf( - SkipSummary.BreakLine("Show intro", "0:00 - 0:20"), - SkipSummary.BreakLine("Ad break", "5:00 - 7:00"), - SkipSummary.BreakLine("Credits", "8:20 - 8:40"), - ), - skips.breaks, - ) - } + private val marks = listOf( + SkipSummary.BreakMarker(start = 0.10f, end = 0.12f, kind = "ad"), + SkipSummary.BreakMarker(start = 0.50f, end = 0.60f, kind = "intro"), + ) @Test - fun rangesPastAnHourKeepTheHour() { - val ranges = listOf( - EpisodeMidrollEntity(episodeId = "e1", candidateId = "a", startMs = 3_723_000, endMs = 3_780_000) - ) - - assertEquals( - listOf(SkipSummary.BreakLine("Ad break", "1:02:03 - 1:03:00")), - SkipSummary.episode(episodeRow(), showRow(), ranges).breaks, - ) + fun `a tap inside a mark hits it`() { + assertEquals(marks[1], BreakTrack.markAt(marks, 0.55f, slop = 0f)) } - /** - * How much advertising is in an episode, for free. - * - * The feed declares the ad-free length and the CDN serves something longer; the gap is the - * ads. Radiolab's Grass Apes: 22:05 declared, 29:34 served. - */ @Test - fun `the gap between declared and measured length is the advertising`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000), - showRow(), - emptyList(), - ) - - assertEquals("7:29", skips.insertedAds) + fun `a tap between marks hits nothing`() { + assertEquals(null, BreakTrack.markAt(marks, 0.30f, slop = 0.01f)) } - /** Nothing to compare until the player has actually opened the file. */ + /** A 4-minute break on a 2-hour file is a hair wide; a finger needs some forgiveness. */ @Test - fun `an episode never played reports no advertising figure`() { - val skips = SkipSummary.episode(episodeRow(durationMs = 1_325_000), showRow(), emptyList()) - - assertEquals(null, skips.insertedAds) + fun `slop widens a narrow mark on both sides`() { + assertEquals(marks[0], BreakTrack.markAt(marks, 0.095f, slop = 0.01f)) + assertEquals(marks[0], BreakTrack.markAt(marks, 0.125f, slop = 0.01f)) + assertEquals(null, BreakTrack.markAt(marks, 0.14f, slop = 0.01f)) } - /** A file shorter than claimed is a wrong feed, not negative advertising. */ @Test - fun `a file shorter than the feed claims reports nothing`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_774_000, actualDurationMs = 1_325_000), - showRow(), - emptyList(), + fun `where slop makes two marks overlap the nearer one wins`() { + val close = listOf( + SkipSummary.BreakMarker(0.10f, 0.11f, "ad"), + SkipSummary.BreakMarker(0.13f, 0.14f, "ad"), ) - assertEquals(null, skips.insertedAds) + assertEquals(close[0], BreakTrack.markAt(close, 0.118f, slop = 0.02f)) + assertEquals(close[1], BreakTrack.markAt(close, 0.124f, slop = 0.02f)) } - /** - * The sanity check Peter asked for, in one place: what the feed claims, what the file turned - * out to be, and how much of the gap the detector has actually accounted for. - * - * The three numbers only mean anything together. `insertedAds` says there are seven and a - * half minutes of advertising in Grass Apes; `breaksTotal` saying thirty seconds means the - * midroll scan is missing breaks, and saying nine minutes means it is matching programme. - * Either way the readout has to show both or it cannot be checked at all. - */ @Test - fun `reports the declared and measured lengths side by side`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000), - showRow(), - emptyList(), - ) - - assertEquals("22:05", skips.declaredLength) - assertEquals("29:34", skips.measuredLength) + fun `no marks means nothing to hit`() { + assertEquals(null, BreakTrack.markAt(emptyList(), 0.5f, slop = 0.05f)) } - /** An episode the player has never opened has no measured length to report. */ @Test - fun `an unplayed episode reports only the declared length`() { - val skips = SkipSummary.episode(episodeRow(durationMs = 1_325_000), showRow(), emptyList()) - - assertEquals("22:05", skips.declaredLength) - assertEquals(null, skips.measuredLength) + fun `a tap position converts to a fraction of the track inset by the thumb at each end`() { + // 1000px wide, 20px inset: the track runs 20..980. + assertEquals(0f, BreakTrack.trackFraction(x = 20f, width = 1000f, inset = 20f), 1e-6f) + assertEquals(0.5f, BreakTrack.trackFraction(x = 500f, width = 1000f, inset = 20f), 1e-6f) + assertEquals(1f, BreakTrack.trackFraction(x = 990f, width = 1000f, inset = 20f), 1e-6f) } + /** What a tap says: the pinned kind wording, and where the mark falls. */ @Test - fun `totals up the detected ad breaks so they can be checked against the inserted ads`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), - showRow(), - listOf( - EpisodeMidrollEntity("e1", "c1", startMs = 120_000, endMs = 180_000), - EpisodeMidrollEntity("e1", "c2", startMs = 600_000, endMs = 690_000), - ), - ) + fun `a mark is named with the pinned kind wording and its times`() { + val mark = SkipSummary.BreakMarker(start = 0.25f, end = 0.5f, kind = "ad") - // A minute plus ninety seconds, against the 7:29 the length gap implies. - assertEquals("2:30", skips.breaksTotal) + assertEquals("Ad break, 5:00 - 10:00", BreakTrack.describeMark(mark, durationMs = 1_200_000)) + assertEquals(300_000L, BreakTrack.markStartMs(mark, durationMs = 1_200_000)) } - /** Nothing detected is not a total of zero -- there is no figure to compare yet. */ - @Test - fun `no detected breaks means no total`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), - showRow(), - emptyList(), - ) - - assertEquals(null, skips.breaksTotal) - } /** * Where the detected breaks sit, as fractions of the episode, so the scrubber can mark them. @@ -473,16 +418,29 @@ class SkipSummaryTest { } // ---- waiting for the server's copy --------------------------------------------------- - @Test fun aWaitingEpisodeSaysItIsWaitingRatherThanNotCheckedYet() { val waiting = episodeRow().copy(downloadState = DownloadState.WAITING_FOR_SERVER) - val skips = SkipSummary.episode(waiting, showRow(), ranges = emptyList()) + val skips = SkipSummary.episode(waiting, ranges = emptyList()) - assertEquals( - listOf(SkipSummary.BreakLine("Breaks", WaitingForServerText.STATUS)), - skips.breaks, - ) + assertEquals(WaitingForServerText.STATUS, skips.status) + } + + @Test + fun `a track too narrow to have a usable span maps every tap to the start`() { + assertEquals(0f, BreakTrack.trackFraction(x = 10f, width = 30f, inset = 20f), 0f) + } + + @Test + fun `a tap outside the track clamps to its ends`() { + assertEquals(0f, BreakTrack.trackFraction(x = 0f, width = 1000f, inset = 20f), 0f) + } + + /** The two colours are explained in words that say which one always cuts. */ + @Test + fun `the legend says which colour is always cut`() { + assertEquals("Advertising (always cut)", SkipSummary.LEGEND_ADVERTISING) + assertEquals("Show's own audio (follows your setting)", SkipSummary.LEGEND_OWN_AUDIO) } } |
