Makussu/subtracks
1
0
Fork 0
mirror of https://github.com/austinried/subtracks.git synced 2026-11-11 07:29:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

fix scrobble double-count and missed plays found in review

Browse Source 
Only treat a rewind as a new listen when it lands in the first few
seconds, so a small seek-back no longer re-arms and duplicates a
scrobble. Fall back to the player's resolved duration when the queue
item has none, and stop the position flow publishing a value for a
track the state has not caught up to. Submit off the collector so an
offline write cannot stall counting.
This commit is contained in:
austinried
2026-10-04 00:30:04 +00:00
parent 0379102e24
commit 153122a552
5 changed files with 99 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt
+4 -2
View File
@@ -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)
app/src/main/kotlin/com/subtracks/playback/Scrobbler.kt
+19 -8
View File
@@ -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<PlaybackState>,
positionMs: StateFlow<Long>,
) {
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) {
app/src/test/kotlin/com/subtracks/playback/ScrobblePolicyTest.kt
+49 -9
View File
@@ -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<Scrobble> {
val events = mutableListOf<Scrobble>()
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<Scrobble>()
while (position <= 300_000) {
policy.advance(item, 600_000, isPlaying = true, positionMs = position)?.let(after::add)
position += 500
}
assertEquals(emptyList<Scrobble>(), 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))
}
}
app/src/test/kotlin/com/subtracks/playback/ScrobblerTest.kt
+26
View File
@@ -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 {