diff options
9 files changed, 102 insertions, 29 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index e4cbd3e..db882fc 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.42 +94.43 diff --git a/app/src/main/java/org/terst/listen/playback/AdJump.kt b/app/src/main/java/org/terst/listen/playback/AdJump.kt index 99622c1..baf09ed 100644 --- a/app/src/main/java/org/terst/listen/playback/AdJump.kt +++ b/app/src/main/java/org/terst/listen/playback/AdJump.kt @@ -7,19 +7,18 @@ import org.terst.listen.data.EpisodeMidrollEntity * How far "this is an ad" jumps. * * The press means "get me past this advertisement", so the jump is the best estimate available - * at that moment rather than a constant. If detection already found a break here, its end is a - * measured answer and is used. Otherwise the default stands in. + * at that moment: if detection already found a break here, its end is a measured answer and is + * used. * - * The size of that default is the trade between too small -- several presses to clear one pod -- - * and too large, which lands him in the programme and makes him correct constantly. Advertising - * is sold in roughly 15-second units grouped into pods, and he presses partway through one, so - * the typical *remaining* duration is what this estimates. Erring small is deliberate: a second - * press costs one tap and improves the label, while over-shooting costs him content. + * With nothing measured it falls back to the forward button's own amount, and deliberately + * introduces no timing of its own. Skipping forward is already what the listener does to an + * advertisement; this button does the same distance and additionally says why. One number to + * know, and pressing again goes the same distance again -- which also extends the label. */ object AdJump { - /** Three 15-second units. See the class comment for why it is not larger. */ - const val DEFAULT_JUMP_MS = 45_000L + /** The forward button's amount. Not a constant of this feature's own; see [SeekAmounts]. */ + const val DEFAULT_JUMP_MS = SeekAmounts.FORWARD_MS fun targetFor( ranges: List<EpisodeMidrollEntity>, diff --git a/app/src/main/java/org/terst/listen/playback/NotificationControls.kt b/app/src/main/java/org/terst/listen/playback/NotificationControls.kt index 21f3959..1dba3df 100644 --- a/app/src/main/java/org/terst/listen/playback/NotificationControls.kt +++ b/app/src/main/java/org/terst/listen/playback/NotificationControls.kt @@ -41,17 +41,18 @@ object NotificationControls { * Media3 ships numbered icons for 5, 10, 15 and 30 seconds only. * * Back is 15, so it gets the numbered icon. Forward is 25 and there is no 25 icon, so it - * gets the plain arrow rather than one captioned with a number the button doesn't do. + * gets the plain arrow rather than one captioned with a number the button doesn't do. The + * captions come from [SeekAmounts] for the same reason. */ fun mediaButtonPreferences(): List<CommandButton> = listOf( CommandButton.Builder() - .setDisplayName("Back 15 seconds") + .setDisplayName(SeekAmounts.backLabel) .setIconResId(androidx.media3.session.R.drawable.media3_icon_skip_back_15) .setSessionCommand(seekBackCommand) .setSlots(CommandButton.SLOT_BACK) .build(), CommandButton.Builder() - .setDisplayName("Forward 25 seconds") + .setDisplayName(SeekAmounts.forwardLabel) .setIconResId(androidx.media3.session.R.drawable.media3_icon_skip_forward) .setSessionCommand(seekForwardCommand) .setSlots(CommandButton.SLOT_FORWARD) diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt index 3a2543b..7af4e0a 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -66,10 +66,10 @@ class PlaybackService : MediaLibraryService() { /* handleAudioFocus = */ true, ) .setHandleAudioBecomingNoisy(true) - // These drive the notification, lock screen, Bluetooth remote and car controls. - // Asymmetric on purpose: back catches a missed sentence, forward steps over an ad. - .setSeekBackIncrementMs(SEEK_BACK_MS) - .setSeekForwardIncrementMs(SEEK_FORWARD_MS) + // These drive the notification, lock screen, Bluetooth remote and car controls -- + // and, through SeekAmounts, the in-app transport and the button captions too. + .setSeekBackIncrementMs(SeekAmounts.BACK_MS) + .setSeekForwardIncrementMs(SeekAmounts.FORWARD_MS) .build() player = exo @@ -345,8 +345,6 @@ class PlaybackService : MediaLibraryService() { private companion object { const val TAG = "ListenPlayback" const val POSITION_TICK_MS = 2_000L - const val SEEK_BACK_MS = 15_000L - const val SEEK_FORWARD_MS = 25_000L } } diff --git a/app/src/main/java/org/terst/listen/playback/SeekAmounts.kt b/app/src/main/java/org/terst/listen/playback/SeekAmounts.kt new file mode 100644 index 0000000..a02a56b --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/SeekAmounts.kt @@ -0,0 +1,24 @@ +package org.terst.listen.playback + +/** + * How far a skip goes. One definition, because there were three. + * + * The player's increments, the episode screen's transport buttons and the notification button's + * caption each carried their own copy of 15 and 25 seconds, and the "this is an ad" jump nearly + * became a fourth with a different number again. A listener has one idea of how far a skip goes; + * every control that skips uses this. + * + * Asymmetric on purpose: back catches a missed sentence, forward steps over an advertisement. + */ +object SeekAmounts { + + const val BACK_MS = 15_000L + const val FORWARD_MS = 25_000L + + /** + * Captions, derived rather than written out, so a caption cannot name a number the button + * does not do. + */ + val backLabel: String = "Back ${BACK_MS / 1_000} seconds" + val forwardLabel: String = "Forward ${FORWARD_MS / 1_000} seconds" +} diff --git a/app/src/main/java/org/terst/listen/ui/Transport.kt b/app/src/main/java/org/terst/listen/ui/Transport.kt index 80c1018..506770d 100644 --- a/app/src/main/java/org/terst/listen/ui/Transport.kt +++ b/app/src/main/java/org/terst/listen/ui/Transport.kt @@ -1,10 +1,14 @@ package org.terst.listen.ui +import org.terst.listen.playback.SeekAmounts + /** The episode screen's transport arithmetic, kept out of the composable so it can be tested. */ object Transport { - const val BACK_MS = 15_000L - const val FORWARD_MS = 25_000L + // The player's own increments, not a second opinion about them: the screen's buttons and + // the notification's have to move the playhead by the same amount. + const val BACK_MS = SeekAmounts.BACK_MS + const val FORWARD_MS = SeekAmounts.FORWARD_MS /** Where a skip lands, kept inside the episode. */ fun stepped(positionMs: Long, deltaMs: Long, durationMs: Long?): Long { diff --git a/app/src/test/java/org/terst/listen/playback/AdJumpTest.kt b/app/src/test/java/org/terst/listen/playback/AdJumpTest.kt index 3421aa2..d2b3138 100644 --- a/app/src/test/java/org/terst/listen/playback/AdJumpTest.kt +++ b/app/src/test/java/org/terst/listen/playback/AdJumpTest.kt @@ -87,7 +87,7 @@ class AdJumpTest { } @Test - fun `the default is a plausible remaining advertisement, in whole ad units`() { - assertEquals(0L, AdJump.DEFAULT_JUMP_MS % 15_000L) + fun `the default jump is the forward button's amount, not a number of its own`() { + assertEquals(SeekAmounts.FORWARD_MS, AdJump.DEFAULT_JUMP_MS) } } diff --git a/app/src/test/java/org/terst/listen/playback/SeekAmountsTest.kt b/app/src/test/java/org/terst/listen/playback/SeekAmountsTest.kt new file mode 100644 index 0000000..8d0865c --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/SeekAmountsTest.kt @@ -0,0 +1,37 @@ +package org.terst.listen.playback + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * One definition of how far a skip goes. + * + * These were duplicated in three places -- the player's increments, the episode screen's + * transport, and the notification button's caption -- and a fourth nearly appeared. + */ +class SeekAmountsTest { + + @Test + fun `the amounts are what they have always been`() { + assertEquals(15_000L, SeekAmounts.BACK_MS) + assertEquals(25_000L, SeekAmounts.FORWARD_MS) + } + + @Test + fun `the episode screen skips by the same amounts as the player`() { + assertEquals(SeekAmounts.BACK_MS, org.terst.listen.ui.Transport.BACK_MS) + assertEquals(SeekAmounts.FORWARD_MS, org.terst.listen.ui.Transport.FORWARD_MS) + } + + @Test + fun `a notification caption states the amount it actually seeks`() { + assertEquals("Back 15 seconds", SeekAmounts.backLabel) + assertEquals("Forward 25 seconds", SeekAmounts.forwardLabel) + } + + @Test + fun `back is shorter than forward -- back catches a missed sentence, forward steps over an ad`() { + assertTrue(SeekAmounts.BACK_MS < SeekAmounts.FORWARD_MS) + } +} diff --git a/docs/superpowers/plans/2026-10-20-corrections-and-signature-library.md b/docs/superpowers/plans/2026-10-20-corrections-and-signature-library.md index cd79f1e..7ec6c52 100644 --- a/docs/superpowers/plans/2026-10-20-corrections-and-signature-library.md +++ b/docs/superpowers/plans/2026-10-20-corrections-and-signature-library.md @@ -91,8 +91,11 @@ window. Headphone/Bluetooth transport keys still seek: `KEYCODE_MEDIA_NEXT` +25s **Android Auto is NOT verified** — it needs the real car. The risk is low (player commands, browse tree and custom actions are all unchanged or strictly added to) but it is not zero. -**What the default jump is.** 45s, three 15-second units, when nothing better is known; the end -of the detected break when the press lands inside one. See `AdJump`. +**What the default jump is.** The forward button's own 25s when nothing better is known; the end +of the detected break when the press lands inside one. Peter, 2026-10-20, on an earlier 45s: +*"the jump should stay 25 seconds. Don't go inventing new timing."* The amount now has one +definition (`SeekAmounts`) that the player's increments, the episode screen's transport and the +notification captions all read, so there is no second place for a number to be invented. **Why this button matters more than the popup.** They catch opposite errors: @@ -129,7 +132,7 @@ The two back cases are told apart by *where he lands*, not by guessing: inside t a correction to the end, before the original press is a rejection. Both are unambiguous from the playhead alone. -**How far to jump is an ergonomics question, and probably not a constant.** +**How far to jump — settled, after two wrong answers.** An earlier draft of this plan argued the jump must be a fixed size and deliberately unequal to the 25s forward-seek, so that a back-press would be "unambiguous". Peter challenged it and it does not @@ -142,9 +145,16 @@ hold, in either half: the correction is read from **where he lands** against a span the system recorded exactly. That works for any jump size; he never needs to know the number. -What actually sets the size is the trade between **too small** (several presses to clear one pod) -and **too large** (routinely landing in content, so he corrects constantly). It should sit near the -typical *remaining* advertisement duration from a mid-advertisement press. +**Settled: it is the forward button's 25s, and nothing new.** The first build shipped 45s, argued +from ad-unit arithmetic. Peter, 2026-10-20: *"the jump should stay 25 seconds. Don't go inventing +new timing."* Skipping forward is already what he does to an advertisement; this button goes the +same distance and additionally says why, so there is one number to know rather than two, and a +second press goes the same distance again. + +That is twice this one question has been got wrong by reasoning about it from first principles +instead of from the controls that already exist. The amount now has a single definition +(`SeekAmounts`) that the player's increments, the episode screen's transport and the notification +captions all read, so there is no second place for a number to be invented. And it need not be fixed: if a detected break is nearby, jump to its end — that position is already known. If the episode's declared-versus-served gap says minutes remain, that is informative too. |