diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index b93f536c..27edc44c 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -1213,7 +1213,6 @@ class PlaybackController( player.durationMs.takeIf { it > 0 } ?: player.currentItem?.durationMs?.takeIf { it > 0 } ?: 0L - _positionMs.value = player.currentPositionMs _state.value = PlaybackState( item = player.currentItem, @@ -1228,6 +1227,7 @@ class PlaybackController( repeat = repeatMode, layout = layoutVersion, ) + _positionMs.value = player.currentPositionMs if (player.isPlaying) startPositionTicker() else stopPositionTicker() } @@ -1239,7 +1239,9 @@ class PlaybackController( delay(POSITION_TICK_MS) val player = player ?: break val positionMs = player.currentPositionMs - _positionMs.value = positionMs + if (player.currentItem?.id == _state.value.item?.id) { + _positionMs.value = positionMs + } if (positionMs - lastSavedPositionMs >= POSITION_SAVE_INTERVAL_MS || positionMs < lastSavedPositionMs) { lastSavedPositionMs = positionMs queueRepository.setPosition(positionMs) diff --git a/app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt b/app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt index e901d490..323dc58a 100644 --- a/app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt +++ b/app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt @@ -5,11 +5,12 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock sealed interface Scrobble { val songId: String @@ -28,6 +29,7 @@ class ScrobblePolicy( private val now: () -> Long = System::currentTimeMillis, ) { private var songId: String? = null + private var lastItemId: String? = null private var durationMs = 0L private var startedAt = 0L private var playedMs = 0L @@ -37,6 +39,7 @@ class ScrobblePolicy( fun reset() { songId = null + lastItemId = null playedMs = 0 lastPositionMs = 0 nowPlayingSent = false @@ -45,13 +48,18 @@ class ScrobblePolicy( fun advance( item: QueueItem?, + durationMs: Long, isPlaying: Boolean, positionMs: Long, ): Scrobble? { + if (durationMs > 0) this.durationMs = durationMs val previousPosition = lastPositionMs - if (item?.id != songId || positionMs < previousPosition) { + val restarted = + item?.id != songId || + (item != null && item.id == lastItemId && positionMs < previousPosition && positionMs <= RESTART_POSITION_MS) + if (restarted) { songId = item?.id - durationMs = item?.durationMs ?: 0L + lastItemId = null playedMs = 0 lastPositionMs = positionMs nowPlayingSent = false @@ -62,6 +70,7 @@ class ScrobblePolicy( return Scrobble.NowPlaying(item.id) } lastPositionMs = positionMs + lastItemId = item?.id val id = songId ?: return null if (!isPlaying) return null if (!nowPlayingSent) { @@ -85,6 +94,7 @@ class ScrobblePolicy( const val MIN_DURATION_MS = 30_000L const val MAX_THRESHOLD_MS = 4 * 60_000L const val MAX_STEP_MS = 2_000L + const val RESTART_POSITION_MS = 3_000L } } @@ -94,25 +104,26 @@ class Scrobbler( private val scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Default), private val policy: ScrobblePolicy = ScrobblePolicy(), ) { + private val submitLock = Mutex() + fun attach( state: StateFlow, positionMs: StateFlow, ) { scope.launch { - combine(state, positionMs, enabled) { current, position, on -> Triple(current, position, on) } + combine(state, positionMs, enabled, ::Triple) .collect { (current, position, on) -> if (!on) { policy.reset() return@collect } - val event = policy.advance(current.item, current.isPlaying, position) ?: return@collect - submit(event) + val durationMs = current.item?.durationMs?.takeIf { it > 0 } ?: current.durationMs + val event = policy.advance(current.item, durationMs, current.isPlaying, position) ?: return@collect + scope.launch { submitLock.withLock { submit(event) } } } } } - fun close() = scope.cancel() - private suspend fun submit(event: Scrobble) { try { when (event) { diff --git a/app/src/test/kotlin/com/subtracks/playback/ScrobblePolicyTest.kt b/app/src/test/kotlin/com/subtracks/playback/ScrobblePolicyTest.kt index 80f7c6b5..d1a22faf 100644 --- a/app/src/test/kotlin/com/subtracks/playback/ScrobblePolicyTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/ScrobblePolicyTest.kt @@ -2,6 +2,7 @@ package com.subtracks.playback import org.junit.Assert.assertEquals import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test class ScrobblePolicyTest { @@ -16,13 +17,14 @@ class ScrobblePolicyTest { private fun play( item: QueueItem?, toPositionMs: Long, + durationMs: Long = item?.durationMs ?: 0L, stepMs: Long = 500, playing: Boolean = true, ): List { val events = mutableListOf() var position = 0L while (position <= toPositionMs) { - policy.advance(item, playing, position)?.let(events::add) + policy.advance(item, durationMs, playing, position)?.let(events::add) position += stepMs } return events @@ -37,9 +39,9 @@ class ScrobblePolicyTest { @Test fun waitsForPlaybackBeforeNowPlaying() { val item = track() - assertNull(policy.advance(item, isPlaying = false, positionMs = 0)) + assertNull(policy.advance(item, 200_000, isPlaying = false, positionMs = 0)) clock = 5_000L - assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, isPlaying = true, positionMs = 0)) + assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, 200_000, isPlaying = true, positionMs = 0)) } @Test @@ -62,6 +64,18 @@ class ScrobblePolicyTest { assertEquals(Scrobble.Submission("s1", time = 1_000L), events.last()) } + @Test + fun usesTheKnownDurationWhenTheQueueItemHasNone() { + val events = play(track(durationMs = 0), toPositionMs = 90_000, durationMs = 180_000) + assertEquals(Scrobble.Submission("s1", time = 1_000L), events.last()) + } + + @Test + fun anUnknownDurationSubmitsAtFourMinutes() { + val events = play(track(durationMs = 0), toPositionMs = 4 * 60_000, durationMs = 0) + assertEquals(Scrobble.Submission("s1", time = 1_000L), events.last()) + } + @Test fun doesNotSubmitShortTracks() { val events = play(track(durationMs = 20_000), toPositionMs = 20_000) @@ -77,17 +91,32 @@ class ScrobblePolicyTest { @Test fun seekingAheadDoesNotCountAsPlayback() { val item = track(durationMs = 200_000) - assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, isPlaying = true, positionMs = 0)) - assertNull(policy.advance(item, isPlaying = true, positionMs = 180_000)) - assertNull(policy.advance(item, isPlaying = true, positionMs = 180_500)) + assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, 200_000, isPlaying = true, positionMs = 0)) + assertNull(policy.advance(item, 200_000, isPlaying = true, positionMs = 180_000)) + assertNull(policy.advance(item, 200_000, isPlaying = true, positionMs = 180_500)) + } + + @Test + fun seekingBackAfterASubmissionDoesNotSubmitAgain() { + val item = track(durationMs = 600_000) + assertTrue(play(item, toPositionMs = 240_000).any { it is Scrobble.Submission }) + + assertNull(policy.advance(item, 600_000, isPlaying = true, positionMs = 235_000)) + var position = 235_500L + val after = mutableListOf() + while (position <= 300_000) { + policy.advance(item, 600_000, isPlaying = true, positionMs = position)?.let(after::add) + position += 500 + } + assertEquals(emptyList(), after) } @Test fun pausedTimeIsNotCounted() { val item = track(durationMs = 200_000) - assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, isPlaying = true, positionMs = 0)) - assertNull(policy.advance(item, isPlaying = false, positionMs = 120_000)) - assertNull(policy.advance(item, isPlaying = true, positionMs = 120_500)) + assertEquals(Scrobble.NowPlaying("s1"), policy.advance(item, 200_000, isPlaying = true, positionMs = 0)) + assertNull(policy.advance(item, 200_000, isPlaying = false, positionMs = 120_000)) + assertNull(policy.advance(item, 200_000, isPlaying = true, positionMs = 120_500)) } @Test @@ -109,4 +138,15 @@ class ScrobblePolicyTest { assertEquals(Scrobble.NowPlaying("s1"), repeat.first()) assertEquals(Scrobble.Submission("s1", 120_000L), repeat.last()) } + + @Test + fun aTransitionDoesNotReannounceTheNewTrackOnAStaleLowPosition() { + val first = track(id = "s1") + val second = track(id = "s2") + policy.advance(first, 200_000, isPlaying = true, positionMs = 0) + policy.advance(first, 200_000, isPlaying = true, positionMs = 100_000) + + assertEquals(Scrobble.NowPlaying("s2"), policy.advance(second, 200_000, isPlaying = true, positionMs = 199_000)) + assertNull(policy.advance(second, 200_000, isPlaying = true, positionMs = 0)) + } } diff --git a/app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt b/app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt index 69e094ad..9d3e8612 100644 --- a/app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt @@ -80,6 +80,32 @@ class ScrobblerTest { assertEquals(2, sink.calls.size) } + @Test + fun fallsBackToThePlayerDurationWhenTheQueueItemHasNone() = + runTest { + val sink = RecordingSink() + val scrobbler = Scrobbler(sink = sink, enabled = flowOf(true), scope = testScope(), policy = ScrobblePolicy(now = { 1_000L })) + val state = MutableStateFlow(PlaybackState()) + val position = MutableStateFlow(0L) + scrobbler.attach(state, position) + val item = QueueItem("s1", "Title", "Artist", "Album", null, durationMs = null) + + state.value = PlaybackState(item = item, isPlaying = true, durationMs = 30_000) + var tick = 500L + while (tick <= 15_000L) { + position.value = tick + tick += 500 + } + + assertEquals( + listOf( + Call.NowPlaying("s1"), + Call.Scrobble("s1", 1_000L), + ), + sink.calls, + ) + } + @Test fun disabledSendsNothing() = runTest { diff --git a/docs/architecture.md b/docs/architecture.md index 59e571b1..bc54c538 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -92,7 +92,7 @@ Stream URLs come from `SourceRepository.streamUri`, which asks the active `Subso The window is also the queue the media notification and Android Auto can browse — a deliberate trade-off, since the alternative is holding an entire large library in memory. The queue view resolves positions lazily too: it loads the queue around the cursor in chunks and extends by a chunk when you scroll near either end, so it never materializes the whole queue. Dragging uses `sh.calvin.reorderable`, which floats the lifted row and reorders the loaded list live (animating with `animateItem`), so editing is what a drag needs: the view reorders its loaded list while dragging and commits one move on drop. `queue_entries` is rewritten to edit: removing a position splits its entry into up to two ranges, and moving one turns the moved track into an explicit song entry inserted at the target index, so an edit costs a couple of rows rather than a copy of the queue. When the edit falls inside the player's window the controller mirrors it with `removeMediaItem`/`moveMediaItem` so playback is not interrupted; removing the playing track, or a move that crosses the window boundary, reloads the window around the cursor. The controller keeps the pre-edit entries and cursor, so the queue view can offer a one-step undo. The up-next block is fully editable in the same view (drag reorder and delete), while the context is read-only while shuffled. Timed lyrics and gapless format preferences are not wired yet. -Scrobbling is a `Scrobbler` that collects the controller's `state` and `positionMs` and drives the Subsonic `scrobble` endpoint: a now-playing notification (`submission=false`) when a track starts playing, and one submission (`submission=true`, with the playback start time) when the track passes half its duration or four minutes, whichever comes first. That split is the endpoint's own `submission` flag, and the threshold is the Last.fm rule the server forwards to; servers such as Navidrome trust the client and apply no threshold themselves. Tracks under 30 seconds are never submitted, position jumps are treated as seeks rather than listened time, and the policy is a pure state machine with no Android dependencies. A Settings toggle (`UserPreferences.scrobbling`, on by default) gates both calls. +Scrobbling is a `Scrobbler` that collects the controller's `state` and `positionMs` and drives the Subsonic `scrobble` endpoint: a now-playing notification (`submission=false`) when a track starts playing, and one submission (`submission=true`, with the playback start time) when the track passes half its duration or four minutes, whichever comes first. That split is the endpoint's own `submission` flag, and the threshold is the Last.fm rule the server forwards to; servers such as Navidrome trust the client and apply no threshold themselves. Tracks under 30 seconds are never submitted, and only time actually played counts: forward and backward seeks are ignored, and a rewind only starts a fresh listen when it lands in the track's first few seconds (a repeat or an explicit restart). The duration is the player's resolved value, falling back to the library row when the player has not reported one. Sending is decoupled from the collector so a slow or offline write cannot stall position tracking, while a lock keeps the writes in order. The policy is a pure state machine with no Android dependencies. A Settings toggle (`UserPreferences.scrobbling`, on by default) gates both calls. Server writes — now-playing, scrobbles and stars — go through a `ServerActionSink`. The only implementation today is `NetworkServerActionSink`, which resolves the active source and calls it; a persistent, ordered outbox can replace it later so plays and stars taken offline are queued and replayed when the server is reachable. Now-playing is deliberately not durable (Last.fm says failed now-playing requests must not be retried), while submissions and stars carry everything a queue would need (the song or entity id and, for a play, the time it was listened).