From ff94263f12f278d748ff8ddd488c5270decd880a Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Mon, 6 Apr 2026 09:53:22 -0400 Subject: [PATCH 1/5] wip --- spotify_player/src/event/mod.rs | 12 +++--- spotify_player/src/event/window.rs | 63 +++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 15 deletions(-) diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index f3861e2b..2d7987f8 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -8,12 +8,12 @@ use crate::{ key::{Key, KeySequence}, state::{ ActionListItem, Album, AlbumId, Artist, ArtistFocusState, ArtistId, ArtistPopupAction, - BrowsePageUIState, Context, ContextId, ContextPageType, ContextPageUIState, DataReadGuard, - Focusable, Id, Item, ItemId, LibraryFocusState, LibraryPageUIState, PageState, PageType, - PlayableId, Playback, PlaylistCreateCurrentField, PlaylistFolderItem, PlaylistId, - PlaylistPopupAction, PopupState, SearchFocusState, SearchPageUIState, SharedState, ShowId, - Track, TrackId, TrackOrder, TracksId, UIStateGuard, USER_LIKED_TRACKS_ID, - USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, + BrowsePageUIState, Context, ContextId, ContextPageType, ContextPageUIState, CustomQueue, + DataReadGuard, Focusable, Id, Item, ItemId, LibraryFocusState, LibraryPageUIState, + PageState, PageType, PlayableId, Playback, PlaylistCreateCurrentField, PlaylistFolderItem, + PlaylistId, PlaylistPopupAction, PopupState, SearchFocusState, SearchPageUIState, + SharedState, ShowId, Track, TrackId, TrackOrder, TracksId, UIStateGuard, + USER_LIKED_TRACKS_ID, USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, }, ui::{single_line_input::LineInput, Orientation}, utils::parse_uri, diff --git a/spotify_player/src/event/window.rs b/spotify_player/src/event/window.rs index e47d1c3d..fb476c48 100644 --- a/spotify_player/src/event/window.rs +++ b/spotify_player/src/event/window.rs @@ -258,6 +258,7 @@ fn handle_playlist_modify_command( Ok(false) } +#[allow(clippy::needless_pass_by_value)] fn handle_command_for_track_table_window( command: Command, client_pub: &flume::Sender, @@ -322,19 +323,63 @@ fn handle_command_for_track_table_window( } } - let base_playback = match context_id { - None | Some(ContextId::Tracks(_)) => { - Playback::URIs(tracks.iter().map(|t| t.id.clone().into()).collect(), None) + let configs = config::get_config(); + let limit = configs.app_config.tracks_playback_limit; + + // When streaming with the custom queue enabled and the context is a + // playlist/album/artist, build a CustomQueue that holds the **full** + // track list and always use URIs-based playback so the app controls + // the play order. + #[cfg(feature = "streaming")] + let use_custom_queue = state.should_use_custom_queue() + && matches!( + context_id, + Some( + ContextId::Playlist(_) | ContextId::Album(_) | ContextId::Artist(_) + ) + ); + #[cfg(not(feature = "streaming"))] + let use_custom_queue = false; + + let base_playback = if use_custom_queue { + Playback::URIs(tracks.iter().map(|t| t.id.clone().into()).collect(), None) + } else { + // Clear any stale custom queue when not using it. + state.player.write().custom_queue = None; + match &context_id { + None | Some(ContextId::Tracks(_)) => { + Playback::URIs( + tracks.iter().map(|t| t.id.clone().into()).collect(), + None, + ) + } + Some(ContextId::Show(_)) => unreachable!( + "show context should be handled by handle_command_for_episode_table_window" + ), + Some(context_id) => Playback::Context(context_id.clone(), None), } - Some(ContextId::Show(_)) => unreachable!( - "show context should be handled by handle_command_for_episode_table_window" - ), - Some(context_id) => Playback::Context(context_id, None), }; + #[cfg(feature = "streaming")] + if use_custom_queue { + let track_ids: Vec> = + tracks.iter().map(|t| t.id.clone().into()).collect(); + let start_position = track_ids + .iter() + .position(|t| t.uri() == uri) + .unwrap_or(0); + let queue = CustomQueue::new( + track_ids, + start_position, + limit, + context_id.clone(), + false, // autoplay: wired up in Phase 5 + ); + state.player.write().custom_queue = Some(queue); + } + client_pub.send(ClientRequest::Player(PlayerRequest::StartPlayback( - base_playback - .uri_offset(uri, config::get_config().app_config.tracks_playback_limit), + base_playback.uri_offset(uri, limit), None, )))?; } From d18173bb8ce5fc0c81fb637866c55da5ef255518 Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Mon, 6 Apr 2026 17:38:02 -0400 Subject: [PATCH 2/5] handle EndOfTrack batch transitions --- spotify_player/src/client/mod.rs | 18 +++++++++++- spotify_player/src/event/mod.rs | 12 ++++---- spotify_player/src/event/window.rs | 16 ++++------- spotify_player/src/main.rs | 3 +- spotify_player/src/streaming.rs | 44 ++++++++++++++++++++++++++++-- 5 files changed, 72 insertions(+), 21 deletions(-) diff --git a/spotify_player/src/client/mod.rs b/spotify_player/src/client/mod.rs index dca20f4b..2082d17e 100644 --- a/spotify_player/src/client/mod.rs +++ b/spotify_player/src/client/mod.rs @@ -52,6 +52,9 @@ pub struct AppClient { user_client: Option, #[cfg(feature = "streaming")] stream_conn: Arc>>, + /// Sender half of the client request channel, used to send requests from + /// the streaming player-event task (e.g. batch transitions on `EndOfTrack`). + client_pub: Option>, } impl Deref for AppClient { @@ -108,9 +111,17 @@ impl AppClient { #[cfg(feature = "streaming")] stream_conn: Arc::new(Mutex::new(None)), + client_pub: None, }) } + /// Set the client request sender. Must be called before starting a + /// streaming connection so that the player-event task can send + /// `ClientRequest`s (e.g. batch transitions). + pub fn set_client_pub(&mut self, sender: flume::Sender) { + self.client_pub = Some(sender); + } + async fn token(&self) -> Result { self.auto_reauth().await?; Ok(self @@ -235,8 +246,13 @@ impl AppClient { session: librespot_core::Session, creds: librespot_core::authentication::Credentials, ) -> Result<()> { + let client_pub = self + .client_pub + .clone() + .context("client_pub must be set before creating a streaming connection")?; let new_conn = - crate::streaming::new_connection(self.clone(), state, session, creds).await?; + crate::streaming::new_connection(self.clone(), state, session, creds, client_pub) + .await?; let mut stream_conn = self.stream_conn.lock(); // shutdown old streaming connection and replace it with a new connection if let Some(conn) = stream_conn.as_ref() { diff --git a/spotify_player/src/event/mod.rs b/spotify_player/src/event/mod.rs index 2d7987f8..f3861e2b 100644 --- a/spotify_player/src/event/mod.rs +++ b/spotify_player/src/event/mod.rs @@ -8,12 +8,12 @@ use crate::{ key::{Key, KeySequence}, state::{ ActionListItem, Album, AlbumId, Artist, ArtistFocusState, ArtistId, ArtistPopupAction, - BrowsePageUIState, Context, ContextId, ContextPageType, ContextPageUIState, CustomQueue, - DataReadGuard, Focusable, Id, Item, ItemId, LibraryFocusState, LibraryPageUIState, - PageState, PageType, PlayableId, Playback, PlaylistCreateCurrentField, PlaylistFolderItem, - PlaylistId, PlaylistPopupAction, PopupState, SearchFocusState, SearchPageUIState, - SharedState, ShowId, Track, TrackId, TrackOrder, TracksId, UIStateGuard, - USER_LIKED_TRACKS_ID, USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, + BrowsePageUIState, Context, ContextId, ContextPageType, ContextPageUIState, DataReadGuard, + Focusable, Id, Item, ItemId, LibraryFocusState, LibraryPageUIState, PageState, PageType, + PlayableId, Playback, PlaylistCreateCurrentField, PlaylistFolderItem, PlaylistId, + PlaylistPopupAction, PopupState, SearchFocusState, SearchPageUIState, SharedState, ShowId, + Track, TrackId, TrackOrder, TracksId, UIStateGuard, USER_LIKED_TRACKS_ID, + USER_RECENTLY_PLAYED_TRACKS_ID, USER_TOP_TRACKS_ID, }, ui::{single_line_input::LineInput, Orientation}, utils::parse_uri, diff --git a/spotify_player/src/event/window.rs b/spotify_player/src/event/window.rs index fb476c48..1738a31c 100644 --- a/spotify_player/src/event/window.rs +++ b/spotify_player/src/event/window.rs @@ -1,5 +1,7 @@ use super::page::handle_navigation_command; use super::*; +#[cfg(feature = "streaming")] +use crate::state::CustomQueue; use crate::{ command::{ construct_album_actions, construct_artist_actions, construct_playlist_actions, @@ -334,9 +336,7 @@ fn handle_command_for_track_table_window( let use_custom_queue = state.should_use_custom_queue() && matches!( context_id, - Some( - ContextId::Playlist(_) | ContextId::Album(_) | ContextId::Artist(_) - ) + Some(ContextId::Playlist(_) | ContextId::Album(_) | ContextId::Artist(_)) ); #[cfg(not(feature = "streaming"))] let use_custom_queue = false; @@ -348,10 +348,7 @@ fn handle_command_for_track_table_window( state.player.write().custom_queue = None; match &context_id { None | Some(ContextId::Tracks(_)) => { - Playback::URIs( - tracks.iter().map(|t| t.id.clone().into()).collect(), - None, - ) + Playback::URIs(tracks.iter().map(|t| t.id.clone().into()).collect(), None) } Some(ContextId::Show(_)) => unreachable!( "show context should be handled by handle_command_for_episode_table_window" @@ -364,10 +361,7 @@ fn handle_command_for_track_table_window( if use_custom_queue { let track_ids: Vec> = tracks.iter().map(|t| t.id.clone().into()).collect(); - let start_position = track_ids - .iter() - .position(|t| t.uri() == uri) - .unwrap_or(0); + let start_position = track_ids.iter().position(|t| t.uri() == uri).unwrap_or(0); let queue = CustomQueue::new( track_ids, start_position, diff --git a/spotify_player/src/main.rs b/spotify_player/src/main.rs index 91a6e3ab..aaf73711 100644 --- a/spotify_player/src/main.rs +++ b/spotify_player/src/main.rs @@ -137,9 +137,10 @@ async fn start_app(state: &state::SharedState) -> Result<()> { } // create a Spotify API client - let client = client::AppClient::new() + let mut client = client::AppClient::new() .await .context("construct app client")?; + client.set_client_pub(client_pub.clone()); client .new_session(Some(state), true) .await diff --git a/spotify_player/src/streaming.rs b/spotify_player/src/streaming.rs index dc1bc940..eab6886d 100644 --- a/spotify_player/src/streaming.rs +++ b/spotify_player/src/streaming.rs @@ -1,4 +1,8 @@ -use crate::{client::AppClient, config, state::SharedState}; +use crate::{ + client::{AppClient, ClientRequest, PlayerRequest}, + config, + state::{AdvanceResult, Playback, SharedState}, +}; use anyhow::Context; use librespot_connect::{ConnectConfig, Spirc}; use librespot_core::authentication::Credentials; @@ -145,6 +149,7 @@ pub async fn new_connection( state: SharedState, session: Session, creds: Credentials, + client_pub: flume::Sender, ) -> anyhow::Result { let configs = config::get_config(); let device = &configs.app_config.device; @@ -243,7 +248,42 @@ pub async fn new_connection( bands.lock().is_active = false; } } - _ => {} + PlayerEvent::EndOfTrack { .. } => { + let mut player = state.player.write(); + if let Some(ref mut queue) = player.custom_queue { + match queue.advance() { + AdvanceResult::SameBatch => { + // librespot handles intra-batch transitions + } + AdvanceResult::NewBatch(batch) => { + let playback = Playback::URIs(batch, None); + // Drop the write lock before sending to avoid deadlock + drop(player); + if let Err(err) = + client_pub.send(ClientRequest::Player( + PlayerRequest::StartPlayback(playback, None), + )) + { + tracing::error!( + "Failed to send batch transition request: {err:#}" + ); + } + } + AdvanceResult::NeedsRadioTracks => { + // Phase 5 — for now, playback stops + tracing::info!( + "Custom queue exhausted, autoplay requested but not yet implemented" + ); + } + AdvanceResult::EndOfQueue => { + tracing::info!( + "Custom queue fully exhausted, playback will stop" + ); + } + } + } + } + PlayerEvent::Changed { .. } => {} } client.update_playback(&state); From 6b56a649e6da846e3f572a79b78dc2d38276b9bf Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Mon, 6 Apr 2026 18:52:51 -0400 Subject: [PATCH 3/5] clear custom queue on context transitions and add queue consistency check --- spotify_player/src/cli/client.rs | 7 +++- spotify_player/src/client/mod.rs | 69 ++++++++++++++++++++++++++++++- spotify_player/src/state/queue.rs | 14 ++++++- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/spotify_player/src/cli/client.rs b/spotify_player/src/cli/client.rs index 724cf282..ae87615c 100644 --- a/spotify_player/src/cli/client.rs +++ b/spotify_player/src/cli/client.rs @@ -463,7 +463,10 @@ async fn handle_playback_request( let client = client.clone(); let state = state.clone(); async move { - match client.handle_player_request(player_request, playback).await { + match client + .handle_player_request(Some(&state), player_request, playback) + .await + { Ok(playback) => { // update application's states state.player.write().buffered_playback = playback; @@ -480,7 +483,7 @@ async fn handle_playback_request( } else { // Handles the player request synchronously client - .handle_player_request(player_request, playback) + .handle_player_request(None, player_request, playback) .await?; } Ok(()) diff --git a/spotify_player/src/client/mod.rs b/spotify_player/src/client/mod.rs index 2082d17e..9bce33f0 100644 --- a/spotify_player/src/client/mod.rs +++ b/spotify_player/src/client/mod.rs @@ -267,6 +267,7 @@ impl AppClient { /// Handle a player request, return a new playback metadata on success pub async fn handle_player_request( &self, + state: Option<&SharedState>, request: PlayerRequest, mut playback: Option, ) -> Result> { @@ -277,6 +278,11 @@ impl AppClient { // because `TransferPlayback` doesn't require an active playback self.transfer_playback(&device_id, Some(force_play)).await?; tracing::info!("Transferred playback to device with id={}", device_id); + // Transferring playback leaves the custom queue's context; + // clear it so stale state doesn't interfere. + if let Some(state) = state { + state.player.write().custom_queue = None; + } return Ok(None); } PlayerRequest::StartPlayback(p, shuffle) => { @@ -417,7 +423,9 @@ impl AppClient { } ClientRequest::Player(request) => { let playback = state.player.read().buffered_playback.clone(); - let playback = self.handle_player_request(request, playback).await?; + let playback = self + .handle_player_request(Some(state), request, playback) + .await?; state.player.write().buffered_playback = playback; self.update_playback(state); } @@ -602,7 +610,34 @@ impl AppClient { } ClientRequest::GetCurrentUserQueue => { let queue = self.current_user_queue().await?; - state.player.write().queue = Some(queue); + let mut player = state.player.write(); + + // Queue consistency check — safety net that detects silent + // divergence between the custom queue and Spotify's actual + // queue state. + if let Some(custom_queue) = &mut player.custom_queue { + let in_cooldown = custom_queue.in_batch_cooldown(); + + if !in_cooldown { + if let Some(expected) = custom_queue.expected_next_track() { + let expected_uri = expected.uri(); + let found = queue + .queue + .iter() + .any(|item| item.id().is_some_and(|id| id.uri() == expected_uri)); + if !found { + tracing::warn!( + "Custom queue consistency check failed: \ + expected next track not found in Spotify queue. \ + Forcing re-sync." + ); + custom_queue.truncate_batch_to_current(); + } + } + } + } + + player.queue = Some(queue); } ClientRequest::ReorderPlaylistItems { playlist_id, @@ -1632,6 +1667,36 @@ impl AppClient { player.playback = playback; player.playback_last_updated_time = Some(std::time::Instant::now()); + // Clear custom_queue when there is no playback at all. + if player.playback.is_none() { + player.custom_queue = None; + } else if let Some(ref cq) = player.custom_queue { + // If Spotify reports a non-None context whose URI differs from + // the queue's source context, another context has taken over. + // context: None is expected for URIs playback — don't clear. + // Skip during cooldown after a batch transition to avoid + // premature clearing while Spotify's API catches up. + let in_cooldown = cq.in_batch_cooldown(); + if !in_cooldown { + if let Some(ref spotify_ctx) = player.playback.as_ref().unwrap().context { + let queue_ctx_uri = cq.source_context().map(ContextId::uri); + let spotify_uri = crate::utils::parse_uri(&spotify_ctx.uri); + let matches = queue_ctx_uri + .as_deref() + .is_some_and(|u| u == spotify_uri.as_ref()); + if !matches { + tracing::info!( + "Spotify context changed (reported={}, expected={:?}); \ + clearing custom queue", + spotify_ctx.uri, + queue_ctx_uri, + ); + player.custom_queue = None; + } + } + } + } + let curr_item = player.currently_playing(); let curr_name = match curr_item { diff --git a/spotify_player/src/state/queue.rs b/spotify_player/src/state/queue.rs index c8dc9a00..62a20266 100644 --- a/spotify_player/src/state/queue.rs +++ b/spotify_player/src/state/queue.rs @@ -1,8 +1,12 @@ use rand::seq::SliceRandom; -use std::time::Instant; +use std::time::{Duration, Instant}; use super::model::{ContextId, PlayableId}; +/// How long after a batch transition to suppress queue consistency checks +/// and context-change clearing, giving Spotify's API time to catch up. +pub const BATCH_COOLDOWN: Duration = Duration::from_secs(3); + /// Result of advancing the queue by one track. #[derive(Clone, Debug, PartialEq, Eq)] #[allow(dead_code)] @@ -187,6 +191,14 @@ impl CustomQueue { self.last_batch_transition } + /// Returns `true` if a batch transition occurred recently enough that + /// Spotify's API state may not yet reflect the new batch. Used to + /// suppress premature queue-clearing / consistency checks. + pub fn in_batch_cooldown(&self) -> bool { + self.last_batch_transition + .is_some_and(|t| t.elapsed() < BATCH_COOLDOWN) + } + /// The expected next track in the play order (if any and within the batch). /// Used for queue consistency checking. pub fn expected_next_track(&self) -> Option<&PlayableId<'static>> { From ae6103ca18643ab755e4fa7d1d9708a4cf50bef7 Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Tue, 7 Apr 2026 18:29:46 -0400 Subject: [PATCH 4/5] update advance logic, remove retreat --- examples/app.toml | 2 +- spotify_player/src/config/mod.rs | 2 +- spotify_player/src/state/queue.rs | 225 ++++++++++++++++-------------- spotify_player/src/streaming.rs | 4 +- 4 files changed, 125 insertions(+), 108 deletions(-) diff --git a/examples/app.toml b/examples/app.toml index f13e302f..1cc7755d 100644 --- a/examples/app.toml +++ b/examples/app.toml @@ -25,7 +25,7 @@ cover_img_length = 9 cover_img_width = 5 cover_img_pixels = 16 seek_duration_secs = 5 -custom_queue = true +custom_queue = false [device] name = "spotify-player" diff --git a/spotify_player/src/config/mod.rs b/spotify_player/src/config/mod.rs index f6edfc9f..0fb38281 100644 --- a/spotify_player/src/config/mod.rs +++ b/spotify_player/src/config/mod.rs @@ -390,7 +390,7 @@ impl Default for AppConfig { volume_scroll_step: 5, enable_mouse_scroll_volume: true, - custom_queue: true, + custom_queue: false, } } } diff --git a/spotify_player/src/state/queue.rs b/spotify_player/src/state/queue.rs index 62a20266..b81b8135 100644 --- a/spotify_player/src/state/queue.rs +++ b/spotify_player/src/state/queue.rs @@ -23,18 +23,6 @@ pub enum AdvanceResult { EndOfQueue, } -/// Result of retreating the queue by one track. -#[derive(Clone, Debug, PartialEq, Eq)] -#[allow(dead_code)] -pub enum RetreatResult { - /// The previous track is still within the current batch. - SameBatch, - /// Need to load the previous batch to reach the previous track. - PreviousBatch(Vec>), - /// Already at the very beginning of the queue. - BeginningOfQueue, -} - /// Shuffle mode for the custom queue. #[derive(Clone, Debug, Default, PartialEq, Eq)] #[allow(dead_code)] @@ -215,25 +203,46 @@ impl CustomQueue { self.position + 1 >= self.batch_end } - /// Whether the current track is the first in the current batch. - pub fn is_at_batch_start(&self) -> bool { - self.position == self.batch_start - } - // ── Mutations ────────────────────────────────────────────────────── - /// Advance to the next track. Returns what action the caller should take. - /// - /// This is called from the `EndOfTrack` handler — it is the **sole** - /// mechanism for advancing position. - pub fn advance(&mut self) -> AdvanceResult { - let next = self.position + 1; + /// Search `play_order[position..batch_end]` for the first element whose URI + /// matches `id`. Returns the absolute index, or `None` if not found (e.g. + /// the track was user-queued and is not part of the custom queue). + fn find_forward_in_batch(&self, id: &PlayableId<'static>) -> Option { + self.play_order[self.position..self.batch_end] + .iter() + .position(|t| t == id) + .map(|offset| self.position + offset) + } + /// Advance the queue in response to an `EndOfTrack` event. + /// + /// `ended_id` is the track that just finished playing. The method looks it + /// up in the current batch (searching forward from `position`) to determine + /// the real position. This handles: + /// + /// - **Normal playback**: `ended_id` is at `position` → next = position + 1. + /// - **Skips** (`NextTrack` fires `Changed`, not `EndOfTrack`): `ended_id` is + /// found ahead of `position` → position jumps forward, self-correcting. + /// - **User-queued tracks**: `ended_id` is not in `play_order` → position + /// unchanged, returns `SameBatch`. + pub fn advance(&mut self, ended_id: &PlayableId<'static>) -> AdvanceResult { // RepeatState::Track — don't advance; librespot loops the track. if self.repeat == rspotify::model::RepeatState::Track { return AdvanceResult::SameBatch; } + // Find where the ended track sits in the current batch. + let Some(idx) = self.find_forward_in_batch(ended_id) else { + // Track not in our batch — user-queued track played through. + // Don't touch position; librespot will resume with the next + // batch track automatically. + tracing::debug!("EndOfTrack for track not in batch (user-queued?): {ended_id:?}"); + return AdvanceResult::SameBatch; + }; + + let next = idx + 1; + if next < self.batch_end { // Still within the current batch. self.position = next; @@ -261,35 +270,6 @@ impl CustomQueue { } } - /// Retreat to the previous track. Returns what action the caller should take. - pub fn retreat(&mut self) -> RetreatResult { - if self.position == 0 { - if self.repeat == rspotify::model::RepeatState::Context { - // Wrap to end of queue. - self.position = self.play_order.len().saturating_sub(1); - self.batch_end = self.play_order.len(); - self.batch_start = self.batch_end.saturating_sub(self.max_batch_size); - self.mark_batch_transition(); - RetreatResult::PreviousBatch(self.current_batch().to_vec()) - } else { - RetreatResult::BeginningOfQueue - } - } else { - let prev = self.position - 1; - if prev >= self.batch_start { - self.position = prev; - RetreatResult::SameBatch - } else { - // Need to load the previous batch. - self.position = prev; - self.batch_end = self.batch_start; - self.batch_start = self.batch_end.saturating_sub(self.max_batch_size); - self.mark_batch_transition(); - RetreatResult::PreviousBatch(self.current_batch().to_vec()) - } - } - } - /// Truncate the current batch so that the current track is the last entry. /// /// After calling this, the next `EndOfTrack` event will trigger a batch @@ -458,12 +438,21 @@ mod tests { assert_eq!(q.current_batch().len(), 3); } + /// Helper to call advance with the track at position `pos` in the tracks list. + fn advance_with( + q: &mut CustomQueue, + tracks: &[PlayableId<'static>], + pos: usize, + ) -> AdvanceResult { + q.advance(&tracks[pos]) + } + #[test] fn advance_within_batch() { let tracks = make_tracks(10); let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); - assert_eq!(q.advance(), AdvanceResult::SameBatch); + assert_eq!(advance_with(&mut q, &tracks, 0), AdvanceResult::SameBatch); assert_eq!(q.position(), 1); assert_eq!(*q.current_track(), tracks[1]); } @@ -471,16 +460,16 @@ mod tests { #[test] fn advance_across_batch_boundary() { let tracks = make_tracks(10); - let mut q = CustomQueue::new(tracks, 0, 5, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); // Advance to position 4 (last in batch [0..5)). - for _ in 0..4 { - assert_eq!(q.advance(), AdvanceResult::SameBatch); + for i in 0..4 { + assert_eq!(advance_with(&mut q, &tracks, i), AdvanceResult::SameBatch); } assert_eq!(q.position(), 4); // Next advance should trigger a new batch. - let result = q.advance(); + let result = advance_with(&mut q, &tracks, 4); assert!(matches!(result, AdvanceResult::NewBatch(_))); assert_eq!(q.position(), 5); assert_eq!(q.batch_start(), 5); @@ -490,23 +479,24 @@ mod tests { #[test] fn advance_end_of_queue() { let tracks = make_tracks(3); - let mut q = CustomQueue::new(tracks, 0, 10, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, false); - for _ in 0..2 { - q.advance(); - } - assert_eq!(q.advance(), AdvanceResult::EndOfQueue); + advance_with(&mut q, &tracks, 0); + advance_with(&mut q, &tracks, 1); + assert_eq!(advance_with(&mut q, &tracks, 2), AdvanceResult::EndOfQueue); } #[test] fn advance_needs_radio_tracks() { let tracks = make_tracks(3); - let mut q = CustomQueue::new(tracks, 0, 10, None, true); + let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, true); - for _ in 0..2 { - q.advance(); - } - assert_eq!(q.advance(), AdvanceResult::NeedsRadioTracks); + advance_with(&mut q, &tracks, 0); + advance_with(&mut q, &tracks, 1); + assert_eq!( + advance_with(&mut q, &tracks, 2), + AdvanceResult::NeedsRadioTracks + ); } #[test] @@ -515,10 +505,9 @@ mod tests { let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, false); q.set_repeat(rspotify::model::RepeatState::Context); - for _ in 0..2 { - q.advance(); - } - let result = q.advance(); + advance_with(&mut q, &tracks, 0); + advance_with(&mut q, &tracks, 1); + let result = advance_with(&mut q, &tracks, 2); assert!(matches!(result, AdvanceResult::NewBatch(_))); assert_eq!(q.position(), 0); assert_eq!(*q.current_track(), tracks[0]); @@ -530,59 +519,87 @@ mod tests { let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, false); q.set_repeat(rspotify::model::RepeatState::Track); - assert_eq!(q.advance(), AdvanceResult::SameBatch); + // ended_id doesn't matter for repeat-track; position stays. + assert_eq!(advance_with(&mut q, &tracks, 0), AdvanceResult::SameBatch); assert_eq!(q.position(), 0); // Didn't move. assert_eq!(*q.current_track(), tracks[0]); } #[test] - fn retreat_within_batch() { + fn advance_user_queued_track_ignored() { let tracks = make_tracks(10); let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); - // Advance to position 2 (still within batch [0..5)). - q.advance(); - q.advance(); - assert_eq!(q.position(), 2); + // Simulate a user-queued track (ID not in play_order) ending. + let unknown = make_track_id(999); + assert_eq!(q.advance(&unknown), AdvanceResult::SameBatch); + assert_eq!(q.position(), 0); // Position unchanged. + } - assert_eq!(q.retreat(), RetreatResult::SameBatch); - assert_eq!(q.position(), 1); - assert_eq!(*q.current_track(), tracks[1]); + #[test] + fn advance_user_queued_at_batch_boundary() { + let tracks = make_tracks(10); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); + + // Move to last track in batch. + for i in 0..4 { + advance_with(&mut q, &tracks, i); + } + assert_eq!(q.position(), 4); + assert!(q.is_at_batch_end()); + + // User-queued track ends — should NOT trigger batch transition. + let unknown = make_track_id(999); + assert_eq!(q.advance(&unknown), AdvanceResult::SameBatch); + assert_eq!(q.position(), 4); // Still at batch end, waiting for real track. } #[test] - fn retreat_at_beginning() { + fn advance_skip_corrects_position() { let tracks = make_tracks(10); - let mut q = CustomQueue::new(tracks, 0, 5, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); - assert_eq!(q.retreat(), RetreatResult::BeginningOfQueue); - assert_eq!(q.position(), 0); + // User skipped tracks 0, 1, 2 (which fire Changed, not EndOfTrack). + // Track 3 finishes naturally. + let result = advance_with(&mut q, &tracks, 3); + assert_eq!(result, AdvanceResult::SameBatch); + assert_eq!(q.position(), 4); // Jumped from 0 to 4. } #[test] - fn retreat_across_batch_boundary() { + fn advance_skip_to_last_in_batch_triggers_new_batch() { let tracks = make_tracks(10); - let mut q = CustomQueue::new(tracks, 0, 5, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); - // Advance into the second batch. - for _ in 0..4 { - q.advance(); - } - q.advance(); // Triggers new batch at position 5. + // Skip to position 4 (last in batch [0..5)), it ends naturally. + let result = advance_with(&mut q, &tracks, 4); + assert!(matches!(result, AdvanceResult::NewBatch(_))); + assert_eq!(q.position(), 5); + assert_eq!(q.batch_start(), 5); + assert_eq!(q.batch_end(), 10); + } - // Now retreat back across the boundary. - let result = q.retreat(); - assert!(matches!(result, RetreatResult::PreviousBatch(_))); - assert_eq!(q.position(), 4); + #[test] + fn advance_duplicate_ids_takes_nearest() { + // play_order: [A, B, A, C] — duplicate A at positions 0 and 2. + let a = make_track_id(0); + let b = make_track_id(1); + let c = make_track_id(3); + let tracks = vec![a.clone(), b, a.clone(), c]; + let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, false); + + // Track A ends — should match at position 0 (nearest forward), next = 1. + assert_eq!(q.advance(&a), AdvanceResult::SameBatch); + assert_eq!(q.position(), 1); } #[test] fn truncate_batch_to_current() { let tracks = make_tracks(10); - let mut q = CustomQueue::new(tracks, 0, 5, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); - q.advance(); // position = 1 - q.advance(); // position = 2 + advance_with(&mut q, &tracks, 0); // position = 1 + advance_with(&mut q, &tracks, 1); // position = 2 q.truncate_batch_to_current(); assert_eq!(q.batch_end(), 3); // position + 1 @@ -601,9 +618,9 @@ mod tests { #[test] fn remaining_tracks_at_end() { let tracks = make_tracks(3); - let mut q = CustomQueue::new(tracks, 0, 10, None, false); - q.advance(); - q.advance(); + let mut q = CustomQueue::new(tracks.clone(), 0, 10, None, false); + advance_with(&mut q, &tracks, 0); + advance_with(&mut q, &tracks, 1); assert!(q.remaining_tracks().is_empty()); } @@ -619,11 +636,11 @@ mod tests { #[test] fn expected_next_track_at_batch_end() { let tracks = make_tracks(10); - let mut q = CustomQueue::new(tracks, 0, 5, None, false); + let mut q = CustomQueue::new(tracks.clone(), 0, 5, None, false); // Advance to position 4 (last in batch). - for _ in 0..4 { - q.advance(); + for i in 0..4 { + advance_with(&mut q, &tracks, i); } assert_eq!(q.expected_next_track(), None); // Next is outside batch. } diff --git a/spotify_player/src/streaming.rs b/spotify_player/src/streaming.rs index eab6886d..b10d6613 100644 --- a/spotify_player/src/streaming.rs +++ b/spotify_player/src/streaming.rs @@ -248,10 +248,10 @@ pub async fn new_connection( bands.lock().is_active = false; } } - PlayerEvent::EndOfTrack { .. } => { + PlayerEvent::EndOfTrack { ref playable_id } => { let mut player = state.player.write(); if let Some(ref mut queue) = player.custom_queue { - match queue.advance() { + match queue.advance(playable_id) { AdvanceResult::SameBatch => { // librespot handles intra-batch transitions } From bb66f726a49a0c1ffe652c90c2629ce2c4638b65 Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Tue, 7 Apr 2026 18:44:52 -0400 Subject: [PATCH 5/5] update queue consistency check to clear the queue on failure --- spotify_player/src/client/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spotify_player/src/client/mod.rs b/spotify_player/src/client/mod.rs index 9bce33f0..1b07937d 100644 --- a/spotify_player/src/client/mod.rs +++ b/spotify_player/src/client/mod.rs @@ -614,11 +614,11 @@ impl AppClient { // Queue consistency check — safety net that detects silent // divergence between the custom queue and Spotify's actual - // queue state. - if let Some(custom_queue) = &mut player.custom_queue { - let in_cooldown = custom_queue.in_batch_cooldown(); - - if !in_cooldown { + // queue state. If the expected next track is missing from + // Spotify's queue, our local position is unreliable — clear + // the custom queue and fall back to Spotify-managed playback. + if let Some(custom_queue) = &player.custom_queue { + if !custom_queue.in_batch_cooldown() { if let Some(expected) = custom_queue.expected_next_track() { let expected_uri = expected.uri(); let found = queue @@ -628,10 +628,10 @@ impl AppClient { if !found { tracing::warn!( "Custom queue consistency check failed: \ - expected next track not found in Spotify queue. \ - Forcing re-sync." + expected next track {expected_uri} not found in Spotify queue; \ + clearing custom queue." ); - custom_queue.truncate_batch_to_current(); + player.custom_queue = None; } } }