diff options
7 files changed, 128 insertions, 8 deletions
diff --git a/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt b/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt index d88414f..14e7f4f 100644 --- a/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt +++ b/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt @@ -135,6 +135,18 @@ class PlayerConnection(private val context: Context) { publish() } + /** + * "This is an ad": records the claim and jumps past it. + * + * Sent as the same session command the notification button sends, rather than reimplemented + * here. The gesture is four meanings deep -- press, press again, come back inside the jump, + * come back past it -- and two copies of that would drift. The service owns it; see + * [AdReporter]. + */ + fun reportAd() { + controller?.sendCustomCommand(NotificationControls.reportAdCommand, android.os.Bundle.EMPTY) + } + /** Called on a UI tick, since position changes without any callback firing. */ fun publish() { val player = controller diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt b/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt index b4e9d8a..18385d1 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt @@ -22,6 +22,11 @@ data class EpisodeDetailActions( /** Skips back or forward by a step, from wherever this episode currently is. */ val skip: (deltaMs: Long) -> Unit, val setSpeed: (Float) -> Unit, + /** + * "This is an ad": skips past it and records the claim, the same gesture the notification + * button carries. See [org.terst.listen.playback.AdReportGesture]. + */ + val reportAd: () -> Unit = {}, /** Stops playback after this many minutes, fading over the last twenty seconds. */ val armSleepTimer: (minutes: Long) -> Unit = {}, val cancelSleepTimer: () -> Unit = {}, diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeFollowing.kt b/app/src/main/java/org/terst/listen/ui/EpisodeFollowing.kt new file mode 100644 index 0000000..19844f4 --- /dev/null +++ b/app/src/main/java/org/terst/listen/ui/EpisodeFollowing.kt @@ -0,0 +1,23 @@ +package org.terst.listen.ui + +/** + * Whether the episode screen moves with the player. + * + * The screen and the player are one thing now, which is the whole point of the merge -- and it + * created this: when the queue advanced, the screen stayed on the episode that had finished. Its + * transport then belonged to an episode nobody was listening to, so pressing play there restarted + * it over the one that was. + * + * It follows only when it was already showing the playing episode. Opening an older episode while + * something else plays is a deliberate act, and a screen that snatched itself away mid-read would + * be a worse bug than the one this fixes. + */ +object EpisodeFollowing { + + /** The episode the screen should show. */ + fun shown(current: String, wasPlaying: String?, nowPlaying: String?): String { + if (nowPlaying == null || nowPlaying == current) return current + // Was this screen the player's own? Only then does the player get to move it. + return if (wasPlaying == current) nowPlaying else current + } +} 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 7ad3740..cb36d64 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt @@ -64,8 +64,21 @@ fun EpisodeScreen( var episode by remember { mutableStateOf<EpisodeEntity?>(null) } var ranges by remember { mutableStateOf<List<EpisodeMidrollEntity>>(emptyList()) } var speed by remember { mutableFloatStateOf(PlaybackSpeed.DEFAULT) } - LaunchedEffect(episodeId) { - EpisodeOpening.open(episodeId, ScreenSource(vm, player))?.let { + val nowPlaying by player.nowPlaying.collectAsState() + val sleepRemaining by player.sleepRemainingMs.collectAsState() + + // What the screen shows, which is the route's episode until the player moves off it. The + // route argument cannot be the answer on its own: the queue advances without any navigation, + // and the screen is the player now. See EpisodeFollowing. + var shownId by remember(episodeId) { mutableStateOf(episodeId) } + var wasPlaying by remember(episodeId) { mutableStateOf(nowPlaying.episodeId) } + LaunchedEffect(nowPlaying.episodeId) { + shownId = EpisodeFollowing.shown(shownId, wasPlaying, nowPlaying.episodeId) + wasPlaying = nowPlaying.episodeId + } + + LaunchedEffect(shownId) { + EpisodeOpening.open(shownId, ScreenSource(vm, player))?.let { episode = it.episode ranges = it.ranges speed = it.speed @@ -73,9 +86,7 @@ fun EpisodeScreen( } val scope = rememberCoroutineScope() val rules by vm.rules.collectAsState() - val judging = rememberBreakJudging(vm, episodeId, ranges) - val nowPlaying by player.nowPlaying.collectAsState() - val sleepRemaining by player.sleepRemainingMs.collectAsState() + val judging = rememberBreakJudging(vm, shownId, ranges) val showTitle = vm.subscriptions.collectAsState().value .firstOrNull { it.id == episode?.subscriptionId }?.title @@ -83,7 +94,7 @@ fun EpisodeScreen( val hooks = EpisodeHooks( onSpeed = { speed = it }, onEpisode = { episode = it }, - reload = { scope.launch { episode = vm.episode(episodeId) } }, + reload = { scope.launch { episode = vm.episode(shownId) } }, ) Scaffold( topBar = { @@ -95,7 +106,7 @@ fun EpisodeScreen( }, // The playing episode's own screen already has the transport, so the bar steps aside // there -- two sets of play controls on one screen would be the drift this merge removed. - bottomBar = { PlayerBar(player, nav.openEpisode, nav.openQueue, hideBarFor = episodeId) }, + bottomBar = { PlayerBar(player, nav.openEpisode, nav.openQueue, hideBarFor = shownId) }, ) { padding -> if (current == null) { Text("Episode not found", Modifier.padding(padding).padding(24.dp)) @@ -177,6 +188,7 @@ private fun episodeActions( else -> player.skipForward() } }, + reportAd = player::reportAd, armSleepTimer = player::armSleepTimer, cancelSleepTimer = player::cancelSleepTimer, setSpeed = { chosen -> diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeTransport.kt b/app/src/main/java/org/terst/listen/ui/EpisodeTransport.kt index 3a56c9d..3c4cd05 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeTransport.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeTransport.kt @@ -10,6 +10,7 @@ import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Check +import androidx.compose.material.icons.filled.Block import androidx.compose.material.icons.filled.FastForward import androidx.compose.material.icons.filled.FastRewind import androidx.compose.material.icons.filled.Pause @@ -32,11 +33,15 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.unit.dp /** - * The one transport: back, the single primary control, forward, and the speed. + * The one transport: back, the single primary control, forward, the ad button, and the speed. * * The primary button's wording says what tapping it will do (Play, Resume, Pause) and it is the * only play control on the screen. Speed sits behind a tap showing the current value, so a row * of four chips no longer takes a full line. + * + * The ad button is the same gesture the notification carries, and it is here for the same reason + * it is there: detection accounts for roughly 38% of the advertising the feeds imply, so most of + * what gets heard was never found, and this is the only moment anyone notices. */ @Composable fun EpisodeTransport(playback: EpisodePlayback, speed: Float, actions: EpisodeDetailActions) { @@ -59,6 +64,11 @@ fun EpisodeTransport(playback: EpisodePlayback, speed: Float, actions: EpisodeDe IconButton(onClick = { actions.skip(Transport.FORWARD_MS) }) { Icon(Icons.Default.FastForward, contentDescription = "Forward 25 seconds") } + // A circle-and-slash, the same mark the notification uses, so the two read as one + // control in two places rather than two features. + IconButton(onClick = actions.reportAd, enabled = playback.isPlaying) { + Icon(Icons.Default.Block, contentDescription = "This is an ad") + } SpeedChip(speed, actions.setSpeed) } } diff --git a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt index a32351b..b785763 100644 --- a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt @@ -165,6 +165,9 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { /** The scrubber, committed once the drag ends rather than on every move. */ fun seekTo(positionMs: Long) = connection.seekTo(positionMs) + /** "This is an ad": skips past it and says so, the same as the notification's button. */ + fun reportAd() = connection.reportAd() + /** * Sets the speed for the show behind [episode], persisted so it survives to the next episode * from that show, and applies it at once when that episode is the one playing. This is what diff --git a/app/src/test/java/org/terst/listen/ui/EpisodeFollowingTest.kt b/app/src/test/java/org/terst/listen/ui/EpisodeFollowingTest.kt new file mode 100644 index 0000000..5faa2ff --- /dev/null +++ b/app/src/test/java/org/terst/listen/ui/EpisodeFollowingTest.kt @@ -0,0 +1,55 @@ +package org.terst.listen.ui + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Which episode the merged screen shows when the player moves on. + * + * Reported live: "I end up with a different episode playing than shown because it doesn't + * follow what's playing." The screen is the player now, so when the queue advances off the + * episode you are watching, the screen has to go with it -- otherwise its transport belongs to + * an episode nobody is listening to, and pressing play there restarts it over the one that is. + * + * It only follows when it was already showing the playing episode. Opening an older episode + * while something else plays is a deliberate act, and hijacking that screen would be worse than + * the bug. + */ +class EpisodeFollowingTest { + + @Test + fun `follows when the queue advances off the episode being watched`() { + assertEquals("b", EpisodeFollowing.shown(current = "a", wasPlaying = "a", nowPlaying = "b")) + } + + @Test + fun `keeps following as the queue keeps moving`() { + val first = EpisodeFollowing.shown(current = "a", wasPlaying = "a", nowPlaying = "b") + assertEquals("c", EpisodeFollowing.shown(current = first, wasPlaying = "b", nowPlaying = "c")) + } + + @Test + fun `stays put when you opened something other than what is playing`() { + assertEquals("old", EpisodeFollowing.shown(current = "old", wasPlaying = "a", nowPlaying = "b")) + } + + @Test + fun `stays put when playback stops rather than following nothing`() { + assertEquals("a", EpisodeFollowing.shown(current = "a", wasPlaying = "a", nowPlaying = null)) + } + + @Test + fun `stays put when nothing was playing to follow`() { + assertEquals("a", EpisodeFollowing.shown(current = "a", wasPlaying = null, nowPlaying = "b")) + } + + @Test + fun `no move while the same episode keeps playing`() { + assertEquals("a", EpisodeFollowing.shown(current = "a", wasPlaying = "a", nowPlaying = "a")) + } + + @Test + fun `starting playback of the episode being watched is not a move`() { + assertEquals("a", EpisodeFollowing.shown(current = "a", wasPlaying = null, nowPlaying = "a")) + } +} |