diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt index 49f0cdc5..eb8c52bd 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/MinstrelForwardingPlayer.kt @@ -17,6 +17,7 @@ import com.fabledsword.minstrel.player.output.ActiveUpnpHolder import com.fabledsword.minstrel.player.output.upnp.SoapFaultException import com.fabledsword.minstrel.player.output.upnp.TransportState import java.io.IOException +import kotlin.math.abs import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job @@ -58,8 +59,10 @@ import timber.log.Timber * native queue via ClearQueue + AddURIToQueue, then points the * transport at x-rincon-queue:#0. Skip/prev/seekTo delegate to * AVTransport Next/Previous/SeekToTrack so Sonos manages gap-free - * advance natively. PollLoop syncs the local cursor by comparing the - * 1-based Track index from GetPositionInfo. + * advance natively. PollLoop keeps the local cursor aligned to the + * track the renderer is actually playing, matched by track identity + * (the id in GetPositionInfo's TrackURI) so it survives queue-reload + * index drift -- see [syncLocalCursorToRemote]. */ class MinstrelForwardingPlayer( private val delegate: Player, @@ -526,18 +529,8 @@ class MinstrelForwardingPlayer( /** * One poll tick: read position + transport state from Sonos, apply to - * [remoteState], and forward-sync the local cursor to Sonos's Track - * index when not in queue load. - * - * Cursor sync is gated on `holder.target == null` (= not loading) - * because during load Sonos reports Track=1 while we're still - * appending, and syncing would race the SetAV+Seek that lands - * after. Outside load, forward sync catches Sonos auto-advances - * (queue end-of-track), Sonos-app driven Next presses, and any - * drift after a brief poll-failure burst that didn't trip the - * drop threshold. Forward-only because a Next override we just - * issued can race with a poll still reporting the prior Track -- - * the next poll catches up safely. + * [remoteState], and sync the local cursor to the track the renderer is + * actually playing (see [syncLocalCursorToRemote]). */ private suspend fun pollOnce(active: ActiveUpnp) { val info = active.avTransport.getPositionInfo() @@ -553,7 +546,7 @@ class MinstrelForwardingPlayer( trackUri = info.trackUri, trackNumber = info.track, ) - maybeSyncLocalCursor(info.track) + syncLocalCursorToRemote(sonosTrack = info.track, trackUri = info.trackUri) val transport = active.avTransport.getTransportInfo() when (transport.state) { TransportState.PLAYING -> { @@ -577,23 +570,95 @@ class MinstrelForwardingPlayer( notifyRemoteStateChanged() } - private fun maybeSyncLocalCursor(sonosTrack: Int) { - if (holder.target.value != null) return - if (sonosTrack <= 0) return - val sonosIdx = sonosTrack - 1 + /** + * Align the paused local delegate cursor to the track the renderer is + * actually playing, so the un-overridden current-item getters + * (getCurrentMediaItem/Index -- what both the in-app player UI via + * PlayerController.onEvents AND the MediaSession notification read) always + * resolve to that track. This is THE single source of truth for "what's + * playing" during a cast. + * + * Identity-first: match the track-id embedded in the renderer's current URI + * against the delegate's MediaItem.mediaId (PlayerController sets mediaId = + * TrackRef.id), so the index-offset wobble across queue reloads / re-casts + * can never park the cursor on a stale (pre-cast) track. Falls back to the + * 1-based Sonos Track index only when the URI can't be resolved to a queue + * item. Unlike the old forward-only sync this moves the cursor in BOTH + * directions -- a renderer Previous / re-cast to a lower track pulls it back + * too. Runs on the application looper (delegate access contract). + * + * Suppressed while: + * - loading ([isLoadingUpnp]) -- the native Sonos queue is still being + * (re)uploaded, so its Track index is meaningless; and + * - a user transport (next/prev/seekTo idx) is pending Sonos's ack -- + * the override already advanced the delegate optimistically, and Sonos + * still reports the OLD track for a beat, so syncing now would undo the + * user's press. The pending deadline is honoured directly so a missed + * clear can't wedge the sync. + */ + private fun syncLocalCursorToRemote(sonosTrack: Int, trackUri: String) { + if (isLoadingUpnp()) return + val pendingDeadline = remoteState.pendingTransportDeadlineMs + if (pendingDeadline > 0L && SystemClock.elapsedRealtime() < pendingDeadline) return handler.post { - val localIdx = delegate.currentMediaItemIndex - if (sonosIdx > localIdx && sonosIdx < delegate.mediaItemCount) { + val target = resolveRemoteCursorIndex(sonosTrack, trackUri) ?: return@post + if (target != delegate.currentMediaItemIndex) { Timber.w( - "UPnP cursor catch-up: local=%d -> sonos=%d", - localIdx, sonosIdx, + "UPnP cursor sync: local=%d -> %d", + delegate.currentMediaItemIndex, target, ) - delegate.seekTo(sonosIdx, 0L) + delegate.seekTo(target, 0L) } } } + /** + * Most-authoritative -> most-degraded (project rule #48): the track-id + * parsed from the renderer's current URI wins; else the 1-based Sonos Track + * index; else null (leave the cursor put rather than surface a wrong track). + * Must run on the application looper -- reads the delegate timeline. + */ + private fun resolveRemoteCursorIndex(sonosTrack: Int, trackUri: String): Int? { + val byId = trackIdFromStreamUri(trackUri)?.let { id -> + nearestIndexWithMediaId(id, preferNear = sonosTrack - 1) + } + return byId ?: (sonosTrack - 1).takeIf { it in 0 until delegate.mediaItemCount } + } + + /** + * Index of the delegate MediaItem whose mediaId == [mediaId], preferring the + * occurrence nearest [preferNear] so a track that appears twice in the queue + * doesn't snap the cursor across the queue. null if the id isn't present. + */ + private fun nearestIndexWithMediaId(mediaId: String, preferNear: Int): Int? { + var best: Int? = null + var bestDist = Int.MAX_VALUE + for (i in 0 until delegate.mediaItemCount) { + if (delegate.getMediaItemAt(i).mediaId == mediaId) { + val dist = abs(i - preferNear) + if (dist < bestDist) { + best = i + bestDist = dist + } + } + } + return best + } + + /** Extract `{id}` from a `.../api/tracks/{id}/stream...` stream URI. */ + private fun trackIdFromStreamUri(uri: String): String? { + val start = uri.indexOf(TRACKS_PATH_MARKER) + if (start < 0) return null + val idStart = start + TRACKS_PATH_MARKER.length + val idEnd = uri.indexOf('/', idStart) + return if (idEnd > idStart) uri.substring(idStart, idEnd) else null + } + private companion object { + // Path segment that precedes the track-id in a stream URI + // (…/api/tracks/{id}/stream…) — used to map the renderer's current URI + // back to a delegate MediaItem by identity. + const val TRACKS_PATH_MARKER = "/api/tracks/" const val POLL_INTERVAL_MS = 1_000L const val NON_PLAYING_CONFIRM = 2 const val SEEK_ACK_WINDOW_MS = 2_000L diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 5e200171..6af12b3c 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -598,18 +598,19 @@ class PlayerController @Inject constructor( } /** - * One position-polling tick. Owns track-change detection too: when UPnP - * is active and the wrapped ExoPlayer is paused, `delegate.seekTo` from - * `maybeSyncLocalCursor` may not fire `onMediaItemTransition`, leaving - * uiState.queueIndex stuck on the old track even after Sonos has - * advanced. So the tick reads Sonos's reported Track as the source of - * truth, rebuilds the index/title fields itself, and force-syncs the - * wrapped player as defense in depth. + * One position-polling tick. Patches ONLY the smoothly-changing transport + * fields (position / duration / play-pause / buffer) via `.copy()`. The + * queue index + current track are owned solely by [onEvents], which reads + * the authoritative delegate cursor: during a cast, + * [MinstrelForwardingPlayer.syncLocalCursorToRemote] keeps that cursor on + * the track the renderer is actually playing (matched by identity), so there + * is exactly one writer of "what's playing" and a stale tick can never + * revert it to the old track. (Previously the tick was a second index writer + * that fought onEvents — the flicker-to-stale-track bug.) */ private fun tickPositionPoll(controller: MediaController) { val upnpActive = activeUpnpHolder.active.value != null resolvePendingTransport(controller, upnpActive) - val pendingTransport = upnpActive && remoteState.pendingTransportDeadlineMs > 0L val effectiveIsPlaying = if (upnpActive) remoteState.isPlaying else controller.isPlaying val effectivePosition = if (upnpActive) { @@ -617,27 +618,29 @@ class PlayerController @Inject constructor( } else { controller.currentPosition } - val desiredIdx = desiredQueueIndex(controller, upnpActive) + val idx = controller.currentMediaItemIndex val current = uiStateInternal.value val newPos = effectivePosition.coerceAtLeast(0) val newDur = effectiveDuration( upnpActive, remoteState.durationMs, controller.duration, - desiredIdx = desiredIdx, - controllerIdx = controller.currentMediaItemIndex, + desiredIdx = idx, + controllerIdx = idx, ) val newBuf = controller.bufferedPosition.coerceAtLeast(0) - // Track adjustments are forward-only AND suppressed while a user - // transport press is pending Sonos confirmation. Together those keep - // either direction of user input from being undone by a stale poll. - val trackChanged = !pendingTransport && - desiredIdx > current.queueIndex && - desiredIdx in queueRefs.indices - publishTickIfChanged( - current, trackChanged, desiredIdx, - effectiveIsPlaying, newPos, newDur, newBuf, - ) + val somethingChanged = current.isPlaying != effectiveIsPlaying || + current.positionMs != newPos || + current.durationMs != newDur || + current.bufferedPositionMs != newBuf + if (somethingChanged) { + uiStateInternal.value = current.copy( + isPlaying = effectiveIsPlaying, + positionMs = newPos, + durationMs = newDur, + bufferedPositionMs = newBuf, + ) + } } /** @@ -654,48 +657,6 @@ class PlayerController @Inject constructor( } } - @Suppress("LongParameterList") // assembled at one tick call site; refactor would cost clarity - private fun publishTickIfChanged( - current: PlayerUiState, - trackChanged: Boolean, - desiredIdx: Int, - effectiveIsPlaying: Boolean, - newPos: Long, - newDur: Long, - newBuf: Long, - ) { - val somethingChanged = trackChanged || - current.isPlaying != effectiveIsPlaying || - current.positionMs != newPos || - current.durationMs != newDur - if (!somethingChanged) return - val newTrack = if (trackChanged) queueRefs[desiredIdx] else current.currentTrack - val newIdx = if (trackChanged) desiredIdx else current.queueIndex - uiStateInternal.value = current.copy( - currentTrack = newTrack, - queueIndex = newIdx, - isPlaying = effectiveIsPlaying, - positionMs = newPos, - durationMs = newDur, - bufferedPositionMs = newBuf, - ) - // Intentionally do NOT call controller.seekTo here. That would route - // through MinstrelForwardingPlayer's seekTo override and re-issue - // AVTransport.SeekToTrack to Sonos -- which seeks Sonos back to the - // start of the same track it's already playing, restarting the song. - // The wrapped player's index is kept in sync by maybeSyncLocalCursor's - // delegate.seekTo (which bypasses the override). If it lags briefly, - // the next pollOnce catches up; the uiState above already reflects - // Sonos's truth for the user. - } - - private fun desiredQueueIndex(controller: MediaController, upnpActive: Boolean): Int = - if (upnpActive) { - (remoteState.trackNumber - 1).coerceAtLeast(0) - } else { - controller.currentMediaItemIndex - } - // ── Remote position interpolation state ────────────────────────────── // remoteState.positionMs is only refreshed by ForwardingPlayer's 1Hz // SOAP poll (and only when the round-trip completes -- screen-off WiFi diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlayerViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlayerViewModel.kt index d7a4952a..e20a3407 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlayerViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/PlayerViewModel.kt @@ -1,11 +1,16 @@ package com.fabledsword.minstrel.player.ui import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope +import com.fabledsword.minstrel.likes.data.LikesRepository import com.fabledsword.minstrel.player.PlayerController import com.fabledsword.minstrel.player.PlayerUiState import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.SharedFlow +import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.launch import javax.inject.Inject /** @@ -22,11 +27,26 @@ import javax.inject.Inject @HiltViewModel class PlayerViewModel @Inject constructor( private val controller: PlayerController, + private val likes: LikesRepository, ) : ViewModel() { val uiState: StateFlow = controller.uiState val dropEvents: SharedFlow = controller.dropEvents + /** + * Reactive set of liked track ids — the set-based pattern the queue + * (and playlist/album/artist detail) uses to color each row's + * [com.fabledsword.minstrel.shared.widgets.LikeButton] without a + * per-row Flow subscription. + */ + val likedTrackIds: StateFlow> = + likes.observeLikedTrackIds() + .stateIn( + scope = viewModelScope, + started = SharingStarted.WhileSubscribed(SHARE_STOP_TIMEOUT_MS), + initialValue = emptySet(), + ) + fun play() = controller.play() fun pause() = controller.pause() fun seekTo(positionMs: Long) = controller.seekTo(positionMs) @@ -35,4 +55,13 @@ class PlayerViewModel @Inject constructor( fun seekToIndex(index: Int) = controller.seekToIndex(index) fun toggleShuffle() = controller.toggleShuffle() fun cycleRepeat() = controller.cycleRepeat() + + fun toggleLikeTrack(trackId: String) { + val desired = trackId !in likedTrackIds.value + viewModelScope.launch { + likes.toggleLike(LikesRepository.ENTITY_TRACK, trackId, desired) + } + } } + +private const val SHARE_STOP_TIMEOUT_MS = 5_000L diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/QueueScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/QueueScreen.kt index 71e2f64d..683c430e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ui/QueueScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ui/QueueScreen.kt @@ -36,6 +36,7 @@ import com.composables.icons.lucide.Volume2 import com.fabledsword.minstrel.models.TrackRef import com.fabledsword.minstrel.shared.formatDuration import com.fabledsword.minstrel.shared.widgets.EmptyState +import com.fabledsword.minstrel.shared.widgets.LikeButton @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -44,6 +45,7 @@ fun QueueScreen( viewModel: PlayerViewModel = hiltViewModel(), ) { val state by viewModel.uiState.collectAsStateWithLifecycle() + val likedTrackIds by viewModel.likedTrackIds.collectAsStateWithLifecycle() Scaffold( modifier = Modifier.fillMaxSize(), topBar = { @@ -67,7 +69,9 @@ fun QueueScreen( QueueList( tracks = state.queue, currentIndex = state.queueIndex, + likedTrackIds = likedTrackIds, onJumpTo = viewModel::seekToIndex, + onToggleLike = viewModel::toggleLikeTrack, ) } } @@ -78,14 +82,18 @@ fun QueueScreen( private fun QueueList( tracks: List, currentIndex: Int, + likedTrackIds: Set, onJumpTo: (Int) -> Unit, + onToggleLike: (String) -> Unit, ) { LazyColumn(modifier = Modifier.fillMaxSize()) { itemsIndexed(items = tracks, key = { _, track -> track.id }) { index, track -> QueueRow( track = track, isCurrent = index == currentIndex, + liked = track.id in likedTrackIds, onClick = { onJumpTo(index) }, + onToggleLike = { onToggleLike(track.id) }, ) HorizontalDivider() } @@ -93,7 +101,13 @@ private fun QueueList( } @Composable -private fun QueueRow(track: TrackRef, isCurrent: Boolean, onClick: () -> Unit) { +private fun QueueRow( + track: TrackRef, + isCurrent: Boolean, + liked: Boolean, + onClick: () -> Unit, + onToggleLike: () -> Unit, +) { val highlight = if (isCurrent) { MaterialTheme.colorScheme.primary.copy(alpha = HIGHLIGHT_ALPHA) } else { @@ -142,6 +156,7 @@ private fun QueueRow(track: TrackRef, isCurrent: Boolean, onClick: () -> Unit) { color = MaterialTheme.colorScheme.onSurfaceVariant, ) } + LikeButton(liked = liked, onToggle = onToggleLike) } } diff --git a/web/src/lib/components/QueueDrawer.test.ts b/web/src/lib/components/QueueDrawer.test.ts index b95a22d2..ec82c755 100644 --- a/web/src/lib/components/QueueDrawer.test.ts +++ b/web/src/lib/components/QueueDrawer.test.ts @@ -1,9 +1,17 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/svelte'; -import QueueDrawer from './QueueDrawer.svelte'; +import { emptyLikesMock } from '../../test-utils/mocks/likes'; import type { TrackRef } from '$lib/api/types'; import { makeTrack } from '$test-utils/fixtures/track'; +// QueueTrackRow (rendered inside QueueDrawer → QueueList) now renders a +// LikeButton, which reads createLikedIdsQuery. Stub the likes API so the +// rows don't need a real QueryClient in context. Both mocks — and the +// emptyLikesMock import — must precede the QueueDrawer import below, since +// loading the component evaluates the mocked modules and the hoisted +// factories need their referenced bindings already initialized. +vi.mock('$lib/api/likes', () => emptyLikesMock()); + const closeQueueDrawer = vi.fn(); let queueValue: TrackRef[] = []; let indexValue = 0; @@ -23,6 +31,8 @@ vi.mock('$lib/player/store.svelte', () => ({ moveQueueItem: vi.fn() })); +import QueueDrawer from './QueueDrawer.svelte'; + const t = (id: string): TrackRef => makeTrack({ id, title: `Song ${id}` }); describe('QueueDrawer', () => { diff --git a/web/src/lib/components/QueueTrackRow.svelte b/web/src/lib/components/QueueTrackRow.svelte index 6580ab9c..d6d043d4 100644 --- a/web/src/lib/components/QueueTrackRow.svelte +++ b/web/src/lib/components/QueueTrackRow.svelte @@ -4,6 +4,7 @@ import type { TrackRef } from '$lib/api/types'; import { playFromQueueIndex, removeFromQueue, moveQueueItem } from '$lib/player/store.svelte'; import { offsetToDelta } from './queue-row-math'; + import LikeButton from './LikeButton.svelte'; let { track, index, isCurrent } = $props<{ track: TrackRef; @@ -80,6 +81,8 @@
{track.artist_name}
+ +