From 2f2460618063a2a84d0646b89cdc7ea3d0a9b2a9 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 17:37:50 +0100 Subject: [PATCH 01/30] connect: simplify `handle_command` for SpircCommand --- connect/src/spirc.rs | 115 ++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 79 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index b9240851c..8e7890b78 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -612,88 +612,45 @@ impl SpircTask { } async fn handle_command(&mut self, cmd: SpircCommand) -> Result<(), Error> { - if matches!(cmd, SpircCommand::Shutdown) { - trace!("Received SpircCommand::Shutdown"); - self.handle_disconnect().await?; - self.shutdown = true; - if let Some(rx) = self.commands.as_mut() { - rx.close() - } - Ok(()) - } else if self.connect_state.is_active() { - trace!("Received SpircCommand::{:?}", cmd); - match cmd { - SpircCommand::Play => { - self.handle_play(); - self.notify().await - } - SpircCommand::PlayPause => { - self.handle_play_pause(); - self.notify().await - } - SpircCommand::Pause => { - self.handle_pause(); - self.notify().await - } - SpircCommand::Prev => { - self.handle_prev()?; - self.notify().await - } - SpircCommand::Next => { - self.handle_next(None)?; - self.notify().await - } - SpircCommand::VolumeUp => { - self.handle_volume_up(); - self.notify().await - } - SpircCommand::VolumeDown => { - self.handle_volume_down(); - self.notify().await - } - SpircCommand::Disconnect => { - self.handle_disconnect().await?; - self.notify().await - } - SpircCommand::Shuffle(shuffle) => { - self.connect_state.handle_shuffle(shuffle)?; - self.notify().await - } - SpircCommand::Repeat(repeat) => { - self.connect_state.set_repeat_context(repeat); - self.notify().await + trace!("Received SpircCommand::{:?}", cmd); + match cmd { + SpircCommand::Shutdown => { + trace!("Received SpircCommand::Shutdown"); + self.handle_disconnect().await?; + self.shutdown = true; + if let Some(rx) = self.commands.as_mut() { + rx.close() } - SpircCommand::RepeatTrack(repeat) => { - self.connect_state.set_repeat_track(repeat); - self.notify().await - } - SpircCommand::SetPosition(position) => { - self.handle_seek(position); - self.notify().await - } - SpircCommand::SetVolume(volume) => { - self.set_volume(volume); - self.notify().await - } - SpircCommand::Load(command) => { - self.handle_load(command, None).await?; - self.notify().await - } - _ => Ok(()), } - } else { - match cmd { - SpircCommand::Activate => { - trace!("Received SpircCommand::{:?}", cmd); - self.handle_activate(); - self.notify().await - } - _ => { - warn!("SpircCommand::{:?} will be ignored while Not Active", cmd); - Ok(()) - } + SpircCommand::Activate if !self.connect_state.is_active() => { + trace!("Received SpircCommand::{:?}", cmd); + self.handle_activate(); + return self.notify().await; } - } + SpircCommand::Activate => warn!( + "SpircCommand::{:?} will be ignored while already active", + cmd + ), + _ if !self.connect_state.is_active() => { + warn!("SpircCommand::{:?} will be ignored while Not Active", cmd) + } + SpircCommand::Play => self.handle_play(), + SpircCommand::PlayPause => self.handle_play_pause(), + SpircCommand::Pause => self.handle_pause(), + SpircCommand::Prev => self.handle_prev()?, + SpircCommand::Next => self.handle_next(None)?, + SpircCommand::VolumeUp => self.handle_volume_up(), + SpircCommand::VolumeDown => self.handle_volume_down(), + SpircCommand::Disconnect => self.handle_disconnect().await?, + SpircCommand::Shuffle(shuffle) => self.connect_state.handle_shuffle(shuffle)?, + SpircCommand::Repeat(repeat) => self.connect_state.set_repeat_context(repeat), + SpircCommand::RepeatTrack(repeat) => self.connect_state.set_repeat_track(repeat), + SpircCommand::SetPosition(position) => self.handle_seek(position), + SpircCommand::SetVolume(volume) => self.set_volume(volume), + SpircCommand::Load(command) => self.handle_load(command, None).await?, + }; + + self.notify().await } async fn handle_player_event(&mut self, event: PlayerEvent) -> Result<(), Error> { From e0f11f12c901fa3ffbdb674cb46e23602c03ec73 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 18:03:23 +0100 Subject: [PATCH 02/30] connect: simplify `handle_player_event` --- connect/src/spirc.rs | 211 ++++++++++++++++++++----------------------- 1 file changed, 99 insertions(+), 112 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 8e7890b78..5751be42d 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -664,122 +664,119 @@ impl SpircTask { self.play_request_id = Some(play_request_id); return Ok(()); } + + let is_current_track = matches! { + (event.get_play_request_id(), self.play_request_id), + (Some(event_id), Some(current_id)) if event_id == current_id + }; // we only process events if the play_request_id matches. If it doesn't, it is // an event that belongs to a previous track and only arrives now due to a race // condition. In this case we have updated the state already and don't want to // mess with it. - if let Some(play_request_id) = event.get_play_request_id() { - if Some(play_request_id) == self.play_request_id { - match event { - PlayerEvent::EndOfTrack { .. } => self.handle_end_of_track().await, - PlayerEvent::Loading { .. } => { - match self.play_status { - SpircPlayStatus::LoadingPlay { position_ms } => { - self.connect_state - .update_position(position_ms, self.now_ms()); - trace!("==> kPlayStatusPlay"); - } - SpircPlayStatus::LoadingPause { position_ms } => { - self.connect_state - .update_position(position_ms, self.now_ms()); - trace!("==> kPlayStatusPause"); - } - _ => { - self.connect_state.update_position(0, self.now_ms()); - trace!("==> kPlayStatusLoading"); - } - } - self.notify().await - } - PlayerEvent::Playing { position_ms, .. } - | PlayerEvent::PositionCorrection { position_ms, .. } - | PlayerEvent::Seeked { position_ms, .. } => { - trace!("==> kPlayStatusPlay"); - let new_nominal_start_time = self.now_ms() - position_ms as i64; - match self.play_status { - SpircPlayStatus::Playing { - ref mut nominal_start_time, - .. - } => { - if (*nominal_start_time - new_nominal_start_time).abs() > 100 { - *nominal_start_time = new_nominal_start_time; - self.connect_state - .update_position(position_ms, self.now_ms()); - self.notify().await - } else { - Ok(()) - } - } - SpircPlayStatus::LoadingPlay { .. } - | SpircPlayStatus::LoadingPause { .. } => { - self.connect_state - .update_position(position_ms, self.now_ms()); - self.play_status = SpircPlayStatus::Playing { - nominal_start_time: new_nominal_start_time, - preloading_of_next_track_triggered: false, - }; - self.notify().await - } - _ => Ok(()), - } - } - PlayerEvent::Paused { - position_ms: new_position_ms, + if !is_current_track { + return Ok(()); + } + + match event { + PlayerEvent::EndOfTrack { .. } => { + let next_track = self + .connect_state + .repeat_track() + .then(|| self.connect_state.current_track(|t| t.uri.clone())); + + self.handle_next(next_track)? + } + PlayerEvent::Loading { .. } => match self.play_status { + SpircPlayStatus::LoadingPlay { position_ms } => { + self.connect_state + .update_position(position_ms, self.now_ms()); + trace!("==> kPlayStatusPlay"); + } + SpircPlayStatus::LoadingPause { position_ms } => { + self.connect_state + .update_position(position_ms, self.now_ms()); + trace!("==> kPlayStatusPause"); + } + _ => { + self.connect_state.update_position(0, self.now_ms()); + trace!("==> kPlayStatusLoading"); + } + }, + PlayerEvent::Playing { position_ms, .. } + | PlayerEvent::PositionCorrection { position_ms, .. } + | PlayerEvent::Seeked { position_ms, .. } => { + trace!("==> kPlayStatusPlay"); + let new_nominal_start_time = self.now_ms() - position_ms as i64; + match self.play_status { + SpircPlayStatus::Playing { + ref mut nominal_start_time, .. } => { - trace!("==> kPlayStatusPause"); - match self.play_status { - SpircPlayStatus::Paused { .. } | SpircPlayStatus::Playing { .. } => { - self.connect_state - .update_position(new_position_ms, self.now_ms()); - self.play_status = SpircPlayStatus::Paused { - position_ms: new_position_ms, - preloading_of_next_track_triggered: false, - }; - self.notify().await - } - SpircPlayStatus::LoadingPlay { .. } - | SpircPlayStatus::LoadingPause { .. } => { - self.connect_state - .update_position(new_position_ms, self.now_ms()); - self.play_status = SpircPlayStatus::Paused { - position_ms: new_position_ms, - preloading_of_next_track_triggered: false, - }; - self.notify().await - } - _ => Ok(()), + if (*nominal_start_time - new_nominal_start_time).abs() > 100 { + *nominal_start_time = new_nominal_start_time; + self.connect_state + .update_position(position_ms, self.now_ms()); + } else { + return Ok(()); } } - PlayerEvent::Stopped { .. } => { - trace!("==> kPlayStatusStop"); - match self.play_status { - SpircPlayStatus::Stopped => Ok(()), - _ => { - self.play_status = SpircPlayStatus::Stopped; - self.notify().await - } - } + SpircPlayStatus::LoadingPlay { .. } | SpircPlayStatus::LoadingPause { .. } => { + self.connect_state + .update_position(position_ms, self.now_ms()); + self.play_status = SpircPlayStatus::Playing { + nominal_start_time: new_nominal_start_time, + preloading_of_next_track_triggered: false, + }; } - PlayerEvent::TimeToPreloadNextTrack { .. } => { - self.handle_preload_next_track(); - Ok(()) + _ => return Ok(()), + } + } + PlayerEvent::Paused { + position_ms: new_position_ms, + .. + } => { + trace!("==> kPlayStatusPause"); + match self.play_status { + SpircPlayStatus::Paused { .. } | SpircPlayStatus::Playing { .. } => { + self.connect_state + .update_position(new_position_ms, self.now_ms()); + self.play_status = SpircPlayStatus::Paused { + position_ms: new_position_ms, + preloading_of_next_track_triggered: false, + }; } - PlayerEvent::Unavailable { track_id, .. } => { - self.handle_unavailable(track_id)?; - if self.connect_state.current_track(|t| &t.uri) == &track_id.to_uri()? { - self.handle_next(None)?; - } - self.notify().await + SpircPlayStatus::LoadingPlay { .. } | SpircPlayStatus::LoadingPause { .. } => { + self.connect_state + .update_position(new_position_ms, self.now_ms()); + self.play_status = SpircPlayStatus::Paused { + position_ms: new_position_ms, + preloading_of_next_track_triggered: false, + }; } - _ => Ok(()), + _ => return Ok(()), } - } else { - Ok(()) } - } else { - Ok(()) + PlayerEvent::Stopped { .. } => { + trace!("==> kPlayStatusStop"); + match self.play_status { + SpircPlayStatus::Stopped => return Ok(()), + _ => self.play_status = SpircPlayStatus::Stopped, + } + } + PlayerEvent::TimeToPreloadNextTrack { .. } => { + self.handle_preload_next_track(); + return Ok(()); + } + PlayerEvent::Unavailable { track_id, .. } => { + self.handle_unavailable(track_id)?; + if self.connect_state.current_track(|t| &t.uri) == &track_id.to_uri()? { + self.handle_next(None)? + } + } + _ => return Ok(()), } + + self.notify().await } async fn handle_connection_id_update(&mut self, connection_id: String) -> Result<(), Error> { @@ -1454,16 +1451,6 @@ impl SpircTask { self.set_volume(volume); } - async fn handle_end_of_track(&mut self) -> Result<(), Error> { - let next_track = self - .connect_state - .repeat_track() - .then(|| self.connect_state.current_track(|t| t.uri.clone())); - - self.handle_next(next_track)?; - self.notify().await - } - fn handle_playlist_modification( &mut self, playlist_modification_info: PlaylistModificationInfo, From c8131beebbaf94c7c7abe5c66473de7678d2ba09 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 18:05:04 +0100 Subject: [PATCH 03/30] connect: `handle_player_event` update log entries --- connect/src/spirc.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 5751be42d..240beda72 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -690,22 +690,22 @@ impl SpircTask { SpircPlayStatus::LoadingPlay { position_ms } => { self.connect_state .update_position(position_ms, self.now_ms()); - trace!("==> kPlayStatusPlay"); + trace!("==> LoadingPlay"); } SpircPlayStatus::LoadingPause { position_ms } => { self.connect_state .update_position(position_ms, self.now_ms()); - trace!("==> kPlayStatusPause"); + trace!("==> LoadingPause"); } _ => { self.connect_state.update_position(0, self.now_ms()); - trace!("==> kPlayStatusLoading"); + trace!("==> Loading"); } }, PlayerEvent::Playing { position_ms, .. } | PlayerEvent::PositionCorrection { position_ms, .. } | PlayerEvent::Seeked { position_ms, .. } => { - trace!("==> kPlayStatusPlay"); + trace!("==> Playing"); let new_nominal_start_time = self.now_ms() - position_ms as i64; match self.play_status { SpircPlayStatus::Playing { @@ -735,7 +735,7 @@ impl SpircTask { position_ms: new_position_ms, .. } => { - trace!("==> kPlayStatusPause"); + trace!("==> Paused"); match self.play_status { SpircPlayStatus::Paused { .. } | SpircPlayStatus::Playing { .. } => { self.connect_state @@ -757,7 +757,7 @@ impl SpircTask { } } PlayerEvent::Stopped { .. } => { - trace!("==> kPlayStatusStop"); + trace!("==> Stopped"); match self.play_status { SpircPlayStatus::Stopped => return Ok(()), _ => self.play_status = SpircPlayStatus::Stopped, From 29cd9b3da35fc08db4539fe1ef12100fd4c066c9 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 18:29:37 +0100 Subject: [PATCH 04/30] connect: set `playback_speed` according to player state --- connect/src/state.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/connect/src/state.rs b/connect/src/state.rs index 28e57dadd..8bfea2a31 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -293,6 +293,12 @@ impl ConnectState { | SpircPlayStatus::Stopped ); + if player.is_paused { + player.playback_speed = 0.; + } else { + player.playback_speed = 1.; + } + // desktop and mobile require all 'states' set to true, when we are paused, // otherwise the play button (desktop) is grayed out or the preview (mobile) can't be opened player.is_buffering = player.is_paused From d8969dab0cca17a75e04fc768ee4dde8194937bd Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 18:51:52 +0100 Subject: [PATCH 05/30] connect: reduce/group state updates by delaying them slightly --- connect/src/spirc.rs | 48 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 240beda72..0f819c8ff 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -111,6 +111,10 @@ struct SpircTask { /// when no other future resolves, otherwise resets the delay update_volume: bool, + /// when set to true, it will update the volume after [UPDATE_STATE_DELAY], + /// when no other future resolves, otherwise resets the delay + update_state: bool, + spirc_id: usize, } @@ -141,11 +145,13 @@ const CONTEXT_FETCH_THRESHOLD: usize = 2; const VOLUME_STEP_SIZE: u16 = 1024; // (u16::MAX + 1) / VOLUME_STEPS // delay to resolve a bundle of context updates, delaying the update prevents duplicate context updates of the same type -const RESOLVE_CONTEXT_DELAY: Duration = Duration::from_millis(500); +const RESOLVE_CONTEXT_DELAY: Duration = Duration::from_millis(600); // time after which an unavailable context is retried const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600); // delay to update volume after a certain amount of time, instead on each update request const VOLUME_UPDATE_DELAY: Duration = Duration::from_secs(2); +// to reduce updates to remote, we group some request by waiting for a set amount of time +const UPDATE_STATE_DELAY: Duration = Duration::from_millis(400); pub struct Spirc { commands: mpsc::UnboundedSender, @@ -250,6 +256,7 @@ impl Spirc { unavailable_contexts: HashMap::new(), transfer_state: None, update_volume: false, + update_state: false, spirc_id, }; @@ -418,11 +425,16 @@ impl SpircTask { error!("could not dispatch player event: {}", e); } }, - _ = async { sleep(RESOLVE_CONTEXT_DELAY).await }, if !self.resolve_context.is_empty() => { - if let Err(why) = self.handle_resolve_context().await { + _ = async { sleep(UPDATE_STATE_DELAY).await }, if self.update_state => { + self.update_state = false; + + if let Err(why) = self.notify().await { error!("ContextError: {why}") } }, + _ = async { sleep(RESOLVE_CONTEXT_DELAY).await }, if !self.resolve_context.is_empty() => { + self.handle_resolve_context().await + }, _ = async { sleep(VOLUME_UPDATE_DELAY).await }, if self.update_volume => { self.update_volume = false; @@ -455,7 +467,7 @@ impl SpircTask { self.session.dealer().close().await; } - async fn handle_resolve_context(&mut self) -> Result<(), Error> { + async fn handle_resolve_context(&mut self) { let mut last_resolve = None::; while let Some(resolve) = self.resolve_context.pop() { if matches!(last_resolve, Some(ref last_resolve) if last_resolve == &resolve) { @@ -468,7 +480,7 @@ impl SpircTask { Some(resolve) => resolve, None => { warn!("tried to resolve context without resolve_uri: {resolve}"); - return Ok(()); + return; } }; @@ -494,24 +506,30 @@ impl SpircTask { } if let Some(transfer_state) = self.transfer_state.take() { - self.connect_state.finish_transfer(transfer_state)? + if let Err(why) = self.connect_state.finish_transfer(transfer_state) { + error!("finishing setup of transfer failed: {why}") + } } if matches!(self.connect_state.active_context, ContextType::Default) { let ctx = self.connect_state.context.as_ref(); if matches!(ctx, Some(ctx) if ctx.tracks.is_empty()) { self.connect_state.clear_next_tracks(true); - self.handle_next(None)?; + // skip to the next queued track, otherwise it should stop + let _ = self.handle_next(None); } } - self.connect_state.fill_up_next_tracks()?; + if let Err(why) = self.connect_state.fill_up_next_tracks() { + error!("fill up of next tracks failed after updating contexts: {why}") + } + self.connect_state.update_restrictions(); self.connect_state.update_queue_revision(); self.preload_autoplay_when_required(); - self.notify().await + self.update_state = true; } async fn resolve_context( @@ -542,10 +560,6 @@ impl SpircTask { error!("resolving context should only update the tracks, but had no page, or track. {ctx:#?}"); }; - if let Err(why) = self.notify().await { - error!("failed to update connect state, after updating the context: {why}") - } - return Ok(()); } @@ -776,7 +790,8 @@ impl SpircTask { _ => return Ok(()), } - self.notify().await + self.update_state = true; + Ok(()) } async fn handle_connection_id_update(&mut self, connection_id: String) -> Result<(), Error> { @@ -909,7 +924,7 @@ impl SpircTask { // fixme: workaround fix, because of missing information why it behaves like it does // background: when another device sends a connect-state update, some player's position de-syncs // tried: providing session_id, playback_id, track-metadata "track_player" - self.notify().await?; + self.update_state = true; } } else if self.connect_state.is_active() { self.connect_state.became_inactive(&self.session).await?; @@ -1038,7 +1053,8 @@ impl SpircTask { Resume(_) => self.handle_play(), } - self.notify().await + self.update_state = true; + Ok(()) } fn handle_transfer(&mut self, mut transfer: TransferState) -> Result<(), Error> { From 28588c4a8ee68abcda9cca0c3a1c7d88a7d151bc Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 21:34:54 +0100 Subject: [PATCH 06/30] connect: load entire context at once --- connect/src/model.rs | 47 +--------------- connect/src/spirc.rs | 102 ++++++++++++++++++----------------- connect/src/state.rs | 5 +- connect/src/state/context.rs | 84 +++++++++++++++++------------ 4 files changed, 105 insertions(+), 133 deletions(-) diff --git a/connect/src/model.rs b/connect/src/model.rs index f9165eaee..a7ff6e224 100644 --- a/connect/src/model.rs +++ b/connect/src/model.rs @@ -64,12 +64,6 @@ pub(super) struct ResolveContext { context: Context, fallback: Option, autoplay: bool, - /// if `true` updates the entire context, otherwise only fills the context from the next - /// retrieve page, it is usually used when loading the next page of an already established context - /// - /// like for example: - /// - playing an artists profile - update: bool, } impl ResolveContext { @@ -82,7 +76,6 @@ impl ResolveContext { }, fallback: (!fallback_uri.is_empty()).then_some(fallback_uri), autoplay, - update: true, } } @@ -91,35 +84,6 @@ impl ResolveContext { context, fallback: None, autoplay, - update: true, - } - } - - // expected page_url: hm://artistplaycontext/v1/page/spotify/album/5LFzwirfFwBKXJQGfwmiMY/km_artist - pub fn from_page_url(page_url: String) -> Self { - let split = if let Some(rest) = page_url.strip_prefix("hm://") { - rest.split('/') - } else { - warn!("page_url didn't started with hm://. got page_url: {page_url}"); - page_url.split('/') - }; - - let uri = split - .skip_while(|s| s != &"spotify") - .take(3) - .collect::>() - .join(":"); - - trace!("created an ResolveContext from page_url <{page_url}> as uri <{uri}>"); - - Self { - context: Context { - uri, - ..Default::default() - }, - fallback: None, - update: false, - autoplay: false, } } @@ -140,21 +104,16 @@ impl ResolveContext { pub fn autoplay(&self) -> bool { self.autoplay } - - pub fn update(&self) -> bool { - self.update - } } impl Display for ResolveContext { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, - "resolve_uri: <{:?}>, context_uri: <{}>, autoplay: <{}>, update: <{}>", + "resolve_uri: <{:?}>, context_uri: <{}>, autoplay: <{}>", self.resolve_uri(), self.context.uri, self.autoplay, - self.update ) } } @@ -164,9 +123,8 @@ impl PartialEq for ResolveContext { let eq_context = self.context_uri() == other.context_uri(); let eq_resolve = self.resolve_uri() == other.resolve_uri(); let eq_autoplay = self.autoplay == other.autoplay; - let eq_update = self.update == other.update; - eq_context && eq_resolve && eq_autoplay && eq_update + eq_context && eq_resolve && eq_autoplay } } @@ -177,7 +135,6 @@ impl Hash for ResolveContext { self.context_uri().hash(state); self.resolve_uri().hash(state); self.autoplay.hash(state); - self.update.hash(state); } } diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 0f819c8ff..150b1331b 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -27,7 +27,7 @@ use crate::{ use crate::{ model::{ResolveContext, SpircPlayStatus}, state::{ - context::{ContextType, LoadNext, UpdateContext}, + context::{ContextType, UpdateContext}, provider::IsProvider, {ConnectState, ConnectStateConfig}, }, @@ -488,12 +488,7 @@ impl SpircTask { // the autoplay endpoint can return a 404, when it tries to retrieve an // autoplay context for an empty playlist as it seems if let Err(why) = self - .resolve_context( - resolve_uri, - resolve.context_uri(), - resolve.autoplay(), - resolve.update(), - ) + .resolve_context(resolve_uri, resolve.context_uri(), resolve.autoplay()) .await { error!("failed resolving context <{resolve}>: {why}"); @@ -537,37 +532,29 @@ impl SpircTask { resolve_uri: &str, context_uri: &str, autoplay: bool, - update: bool, ) -> Result<(), Error> { if !autoplay { let mut ctx = self.session.spclient().get_context(resolve_uri).await?; + ctx.uri = context_uri.to_string(); + ctx.url = format!("context://{context_uri}"); - if update { - ctx.uri = context_uri.to_string(); - ctx.url = format!("context://{context_uri}"); - - self.connect_state - .update_context(ctx, UpdateContext::Default)? - } else if matches!(ctx.pages.first(), Some(p) if !p.tracks.is_empty()) { - debug!( - "update context from single page, context {} had {} pages", - ctx.uri, - ctx.pages.len() - ); - self.connect_state - .fill_context_from_page(ctx.pages.remove(0))?; - } else { - error!("resolving context should only update the tracks, but had no page, or track. {ctx:#?}"); - }; + if let Some(remaining) = self + .connect_state + .update_context(ctx, UpdateContext::Default)? + { + self.try_resolve_remaining(remaining).await; + } return Ok(()); } + // refuse resolve of not supported autoplay context if resolve_uri.contains("spotify:show:") || resolve_uri.contains("spotify:episode:") { // autoplay is not supported for podcasts Err(SpircError::NotAllowedContext(resolve_uri.to_string()))? } + // resolve autoplay let previous_tracks = self.connect_state.prev_autoplay_track_uris(); debug!( @@ -587,8 +574,40 @@ impl SpircTask { .get_autoplay_context(&ctx_request) .await?; - self.connect_state - .update_context(context, UpdateContext::Autoplay) + if let Some(remaining) = self + .connect_state + .update_context(context, UpdateContext::Autoplay)? + { + self.try_resolve_remaining(remaining).await; + } + + Ok(()) + } + + async fn try_resolve_remaining(&mut self, remaining: Vec) { + for resolve_uri in remaining { + let mut ctx = match self.session.spclient().get_context(&resolve_uri).await { + Ok(ctx) => ctx, + Err(why) => { + warn!("failed to retrieve context for remaining <{resolve_uri}>: {why}"); + continue; + } + }; + + if ctx.pages.len() > 1 { + warn!("context contained more page then expected: {ctx:#?}"); + continue; + } + + debug!("appending context from single page, adding: <{}>", ctx.uri); + + if let Err(why) = self + .connect_state + .fill_context_from_page(ctx.pages.remove(0)) + { + warn!("failed appending context <{resolve_uri}>: {why}"); + } + } } fn add_resolve_context(&mut self, resolve: ResolveContext) { @@ -1193,7 +1212,7 @@ impl SpircTask { debug!("context <{current_context_uri}> didn't change, no resolving required") } else { debug!("resolving context for load command"); - self.resolve_context(&fallback, &cmd.context_uri, false, true) + self.resolve_context(&fallback, &cmd.context_uri, false) .await?; } @@ -1366,33 +1385,18 @@ impl SpircTask { fn preload_autoplay_when_required(&mut self) { let require_load_new = !self .connect_state - .has_next_tracks(Some(CONTEXT_FETCH_THRESHOLD)); + .has_next_tracks(Some(CONTEXT_FETCH_THRESHOLD)) + && self.session.autoplay(); if !require_load_new { return; } - match self.connect_state.try_load_next_context() { - Err(why) => error!("failed loading next context: {why}"), - Ok(next) => { - match next { - LoadNext::Done => info!("loaded next context"), - LoadNext::PageUrl(page_url) => { - self.add_resolve_context(ResolveContext::from_page_url(page_url)) - } - LoadNext::Empty if self.session.autoplay() => { - let current_context = self.connect_state.context_uri(); - let fallback = self.connect_state.current_track(|t| &t.uri); - let resolve = ResolveContext::from_uri(current_context, fallback, true); + let current_context = self.connect_state.context_uri(); + let fallback = self.connect_state.current_track(|t| &t.uri); + let resolve = ResolveContext::from_uri(current_context, fallback, true); - self.add_resolve_context(resolve) - } - LoadNext::Empty => { - debug!("next context is empty and autoplay isn't enabled, no preloading required") - } - } - } - } + self.add_resolve_context(resolve); } fn is_playing(&self) -> bool { diff --git a/connect/src/state.rs b/connect/src/state.rs index 8bfea2a31..b9cf37d62 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -20,8 +20,7 @@ use librespot_protocol::connect::{ Capabilities, Device, DeviceInfo, MemberType, PutStateReason, PutStateRequest, }; use librespot_protocol::player::{ - ContextIndex, ContextPage, ContextPlayerOptions, PlayOrigin, PlayerState, ProvidedTrack, - Suppressions, + ContextIndex, ContextPlayerOptions, PlayOrigin, PlayerState, ProvidedTrack, Suppressions, }; use log::LevelFilter; use protobuf::{EnumOrUnknown, MessageField}; @@ -112,8 +111,6 @@ pub struct ConnectState { /// the context from which we play, is used to top up prev and next tracks pub context: Option, - /// upcoming contexts, directly provided by the context-resolver - next_contexts: Vec, /// a context to keep track of our shuffled context, /// should be only available when `player.option.shuffling_context` is true diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 3e9d720e6..378da66cd 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -27,12 +27,6 @@ pub enum ContextType { Autoplay, } -pub enum LoadNext { - Done, - PageUrl(String), - Empty, -} - #[derive(Debug)] pub enum UpdateContext { Default, @@ -45,6 +39,27 @@ pub enum ResetContext<'s> { WhenDifferent(&'s str), } +/// Extracts the spotify uri from a given page_url +/// +/// Just extracts "spotify/album/5LFzwirfFwBKXJQGfwmiMY" and replaces the slash's with colon's +/// +/// Expected `page_url` should look something like the following: +/// `hm://artistplaycontext/v1/page/spotify/album/5LFzwirfFwBKXJQGfwmiMY/km_artist` +fn page_url_to_uri(page_url: &str) -> String { + let split = if let Some(rest) = page_url.strip_prefix("hm://") { + rest.split('/') + } else { + warn!("page_url didn't started with hm://. got page_url: {page_url}"); + page_url.split('/') + }; + + split + .skip_while(|s| s != &"spotify") + .take(3) + .collect::>() + .join(":") +} + impl ConnectState { pub fn find_index_in_context bool>( context: Option<&StateContext>, @@ -86,7 +101,6 @@ impl ConnectState { ResetContext::Completely => { self.context = None; self.autoplay_context = None; - self.next_contexts.clear(); } ResetContext::WhenDifferent(_) => debug!("context didn't change, no reset"), ResetContext::DefaultIndex => { @@ -142,7 +156,11 @@ impl ConnectState { } } - pub fn update_context(&mut self, mut context: Context, ty: UpdateContext) -> Result<(), Error> { + pub fn update_context( + &mut self, + mut context: Context, + ty: UpdateContext, + ) -> Result>, Error> { if context.pages.iter().all(|p| p.tracks.is_empty()) { error!("context didn't have any tracks: {context:#?}"); return Err(StateError::ContextHasNoTracks.into()); @@ -150,16 +168,13 @@ impl ConnectState { return Err(StateError::UnsupportedLocalPlayBack.into()); } - if matches!(ty, UpdateContext::Default) { - self.next_contexts.clear(); - } - + let mut next_contexts = Vec::new(); let mut first_page = None; for page in context.pages { if first_page.is_none() && !page.tracks.is_empty() { first_page = Some(page); } else { - self.next_contexts.push(page) + next_contexts.push(page) } } @@ -234,7 +249,27 @@ impl ConnectState { } } - Ok(()) + if next_contexts.is_empty() { + return Ok(None); + } + + // load remaining contexts + let next_contexts = next_contexts + .into_iter() + .flat_map(|page| { + if !page.tracks.is_empty() { + self.fill_context_from_page(page).ok()?; + None + } else if !page.page_url.is_empty() { + Some(page_url_to_uri(&page.page_url)) + } else { + warn!("unhandled context page: {page:#?}"); + None + } + }) + .collect(); + + Ok(Some(next_contexts)) } fn state_context_from_page( @@ -391,25 +426,4 @@ impl ConnectState { Ok(()) } - - pub fn try_load_next_context(&mut self) -> Result { - let next = match self.next_contexts.first() { - None => return Ok(LoadNext::Empty), - Some(_) => self.next_contexts.remove(0), - }; - - if next.tracks.is_empty() { - if next.page_url.is_empty() { - Err(StateError::NoContext(ContextType::Default))? - } - - self.update_current_index(|i| i.page += 1); - return Ok(LoadNext::PageUrl(next.page_url)); - } - - self.fill_context_from_page(next)?; - self.fill_up_next_tracks()?; - - Ok(LoadNext::Done) - } } From c73871c516b7edd9e27f0ea2f5cd22e4b00002ec Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 10 Dec 2024 21:38:36 +0100 Subject: [PATCH 07/30] connect: use is_playing from connect_state --- connect/src/spirc.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 150b1331b..e19d7b2a1 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1399,15 +1399,8 @@ impl SpircTask { self.add_resolve_context(resolve); } - fn is_playing(&self) -> bool { - matches!( - self.play_status, - SpircPlayStatus::Playing { .. } | SpircPlayStatus::LoadingPlay { .. } - ) - } - fn handle_next(&mut self, track_uri: Option) -> Result<(), Error> { - let continue_playing = self.is_playing(); + let continue_playing = self.connect_state.player().is_playing; let current_uri = self.connect_state.current_track(|t| &t.uri); let mut has_next_track = @@ -1450,7 +1443,7 @@ impl SpircTask { self.connect_state.reset_playback_to_position(None)?; self.handle_stop() } - Some(_) => self.load_track(self.is_playing(), 0)?, + Some(_) => self.load_track(self.connect_state.player().is_playing, 0)?, } } else { self.handle_seek(0); @@ -1573,7 +1566,7 @@ impl SpircTask { async fn notify(&mut self) -> Result<(), Error> { self.connect_state.set_status(&self.play_status); - if self.is_playing() { + if self.connect_state.player().is_playing { self.connect_state .update_position_in_relation(self.now_ms()); } From d2f1dee43c46bba6eed04ee573fcae7b7fbc78c8 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Wed, 11 Dec 2024 17:36:33 +0100 Subject: [PATCH 08/30] connect: move `ResolveContext` in own file --- connect/src/context_resolver.rs | 91 +++++++++++++++++++++++++++++++++ connect/src/lib.rs | 1 + connect/src/model.rs | 89 -------------------------------- connect/src/spirc.rs | 17 +++--- 4 files changed, 101 insertions(+), 97 deletions(-) create mode 100644 connect/src/context_resolver.rs diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs new file mode 100644 index 000000000..bcc4a8df1 --- /dev/null +++ b/connect/src/context_resolver.rs @@ -0,0 +1,91 @@ +use crate::state::ConnectState; +use librespot_protocol::player::Context; +use std::{ + fmt::{Display, Formatter}, + hash::{Hash, Hasher}, +}; + +#[derive(Debug, Clone)] +pub(super) struct ResolveContext { + context: Context, + fallback: Option, + autoplay: bool, +} + +impl ResolveContext { + pub fn from_uri(uri: impl Into, fallback: impl Into, autoplay: bool) -> Self { + let fallback_uri = fallback.into(); + Self { + context: Context { + uri: uri.into(), + ..Default::default() + }, + fallback: (!fallback_uri.is_empty()).then_some(fallback_uri), + autoplay, + } + } + + pub fn from_context(context: Context, autoplay: bool) -> Self { + Self { + context, + fallback: None, + autoplay, + } + } + + /// the uri which should be used to resolve the context, might not be the context uri + pub fn resolve_uri(&self) -> Option<&String> { + // it's important to call this always, or at least for every ResolveContext + // otherwise we might not even check if we need to fallback and just use the fallback uri + ConnectState::get_context_uri_from_context(&self.context) + .and_then(|s| (!s.is_empty()).then_some(s)) + .or(self.fallback.as_ref()) + } + + /// the actual context uri + pub fn context_uri(&self) -> &str { + &self.context.uri + } + + pub fn autoplay(&self) -> bool { + self.autoplay + } +} + +impl Display for ResolveContext { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "resolve_uri: <{:?}>, context_uri: <{}>, autoplay: <{}>", + self.resolve_uri(), + self.context.uri, + self.autoplay, + ) + } +} + +impl PartialEq for ResolveContext { + fn eq(&self, other: &Self) -> bool { + let eq_context = self.context_uri() == other.context_uri(); + let eq_resolve = self.resolve_uri() == other.resolve_uri(); + let eq_autoplay = self.autoplay == other.autoplay; + + eq_context && eq_resolve && eq_autoplay + } +} + +impl Eq for ResolveContext {} + +impl Hash for ResolveContext { + fn hash(&self, state: &mut H) { + self.context_uri().hash(state); + self.resolve_uri().hash(state); + self.autoplay.hash(state); + } +} + +impl From for Context { + fn from(value: ResolveContext) -> Self { + value.context + } +} diff --git a/connect/src/lib.rs b/connect/src/lib.rs index 3cfbbca19..11a651863 100644 --- a/connect/src/lib.rs +++ b/connect/src/lib.rs @@ -5,6 +5,7 @@ use librespot_core as core; use librespot_playback as playback; use librespot_protocol as protocol; +mod context_resolver; mod model; pub mod spirc; pub mod state; diff --git a/connect/src/model.rs b/connect/src/model.rs index a7ff6e224..63d48efe4 100644 --- a/connect/src/model.rs +++ b/connect/src/model.rs @@ -1,8 +1,4 @@ -use crate::state::ConnectState; use librespot_core::dealer::protocol::SkipTo; -use librespot_protocol::player::Context; -use std::fmt::{Display, Formatter}; -use std::hash::{Hash, Hasher}; #[derive(Debug)] pub struct SpircLoadCommand { @@ -58,88 +54,3 @@ pub(super) enum SpircPlayStatus { preloading_of_next_track_triggered: bool, }, } - -#[derive(Debug, Clone)] -pub(super) struct ResolveContext { - context: Context, - fallback: Option, - autoplay: bool, -} - -impl ResolveContext { - pub fn from_uri(uri: impl Into, fallback: impl Into, autoplay: bool) -> Self { - let fallback_uri = fallback.into(); - Self { - context: Context { - uri: uri.into(), - ..Default::default() - }, - fallback: (!fallback_uri.is_empty()).then_some(fallback_uri), - autoplay, - } - } - - pub fn from_context(context: Context, autoplay: bool) -> Self { - Self { - context, - fallback: None, - autoplay, - } - } - - /// the uri which should be used to resolve the context, might not be the context uri - pub fn resolve_uri(&self) -> Option<&String> { - // it's important to call this always, or at least for every ResolveContext - // otherwise we might not even check if we need to fallback and just use the fallback uri - ConnectState::get_context_uri_from_context(&self.context) - .and_then(|s| (!s.is_empty()).then_some(s)) - .or(self.fallback.as_ref()) - } - - /// the actual context uri - pub fn context_uri(&self) -> &str { - &self.context.uri - } - - pub fn autoplay(&self) -> bool { - self.autoplay - } -} - -impl Display for ResolveContext { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!( - f, - "resolve_uri: <{:?}>, context_uri: <{}>, autoplay: <{}>", - self.resolve_uri(), - self.context.uri, - self.autoplay, - ) - } -} - -impl PartialEq for ResolveContext { - fn eq(&self, other: &Self) -> bool { - let eq_context = self.context_uri() == other.context_uri(); - let eq_resolve = self.resolve_uri() == other.resolve_uri(); - let eq_autoplay = self.autoplay == other.autoplay; - - eq_context && eq_resolve && eq_autoplay - } -} - -impl Eq for ResolveContext {} - -impl Hash for ResolveContext { - fn hash(&self, state: &mut H) { - self.context_uri().hash(state); - self.resolve_uri().hash(state); - self.autoplay.hash(state); - } -} - -impl From for Context { - fn from(value: ResolveContext) -> Self { - value.context - } -} diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index e19d7b2a1..a6bc482d2 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1,5 +1,14 @@ pub use crate::model::{PlayingTrack, SpircLoadCommand}; use crate::state::{context::ResetContext, metadata::Metadata}; +use crate::{ + context_resolver::ResolveContext, + model::SpircPlayStatus, + state::{ + context::{ContextType, UpdateContext}, + provider::IsProvider, + {ConnectState, ConnectStateConfig}, + }, +}; use crate::{ core::{ authentication::Credentials, @@ -24,14 +33,6 @@ use crate::{ user_attributes::UserAttributesMutation, }, }; -use crate::{ - model::{ResolveContext, SpircPlayStatus}, - state::{ - context::{ContextType, UpdateContext}, - provider::IsProvider, - {ConnectState, ConnectStateConfig}, - }, -}; use futures_util::StreamExt; use protobuf::MessageField; use std::collections::HashMap; From 673ded3d5754d61a75cd05f65932a5f77c7e8764 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Wed, 11 Dec 2024 20:49:00 +0100 Subject: [PATCH 09/30] connect: handle metadata correct --- connect/src/state/context.rs | 18 +++++++++++++----- connect/src/state/transfer.rs | 2 ++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 378da66cd..8aad6463d 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -202,6 +202,7 @@ impl ConnectState { UpdateContext::Default => { let mut new_context = self.state_context_from_page( page, + context.metadata, context.restrictions.take(), Some(&context.uri), None, @@ -242,6 +243,7 @@ impl ConnectState { UpdateContext::Autoplay => { self.autoplay_context = Some(self.state_context_from_page( page, + context.metadata, context.restrictions.take(), Some(&context.uri), Some(Provider::Autoplay), @@ -275,6 +277,7 @@ impl ConnectState { fn state_context_from_page( &mut self, page: ContextPage, + metadata: HashMap, restrictions: Option, new_context_uri: Option<&str>, provider: Option, @@ -285,8 +288,12 @@ impl ConnectState { .tracks .iter() .flat_map(|track| { - match self.context_to_provided_track(track, Some(new_context_uri), provider.clone()) - { + match self.context_to_provided_track( + track, + Some(new_context_uri), + Some(&page.metadata), + provider.clone(), + ) { Ok(t) => Some(t), Err(why) => { error!("couldn't convert {track:#?} into ProvidedTrack: {why}"); @@ -299,7 +306,7 @@ impl ConnectState { StateContext { tracks, restrictions, - metadata: page.metadata, + metadata, index: ContextIndex::new(), } } @@ -358,6 +365,7 @@ impl ConnectState { &self, ctx_track: &ContextTrack, context_uri: Option<&str>, + page_metadata: Option<&HashMap>, provider: Option, ) -> Result { let id = if !ctx_track.uri.is_empty() { @@ -388,7 +396,7 @@ impl ConnectState { ctx_track.uid.to_string() }; - let mut metadata = HashMap::new(); + let mut metadata = page_metadata.cloned().unwrap_or_default(); for (k, v) in &ctx_track.metadata { metadata.insert(k.to_string(), v.to_string()); } @@ -414,7 +422,7 @@ impl ConnectState { } pub fn fill_context_from_page(&mut self, page: ContextPage) -> Result<(), Error> { - let context = self.state_context_from_page(page, None, None, None); + let context = self.state_context_from_page(page, HashMap::new(), None, None, None); let ctx = self .context .as_mut() diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index c310e0b9c..feafb6983 100644 --- a/connect/src/state/transfer.rs +++ b/connect/src/state/transfer.rs @@ -21,6 +21,7 @@ impl ConnectState { self.context_to_provided_track( track, Some(&transfer.current_session.context.uri), + None, transfer.queue.is_playing_queue.then_some(Provider::Queue), ) } @@ -125,6 +126,7 @@ impl ConnectState { if let Ok(queued_track) = self.context_to_provided_track( track, Some(self.context_uri()), + None, Some(Provider::Queue), ) { self.add_to_queue(queued_track, false); From d3ae5c0820e76315cb5cc577f154c07dcfa1b4f4 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Wed, 11 Dec 2024 22:12:05 +0100 Subject: [PATCH 10/30] connect: resolve context rework - resolved contexts independently, by that we don't block our main loop - move resolve logic into own file - polish handling for play and transfer --- connect/src/context_resolver.rs | 315 ++++++++++++++++++++++++++--- connect/src/spirc.rs | 338 ++++++++++++-------------------- connect/src/state.rs | 10 +- connect/src/state/context.rs | 48 ++--- connect/src/state/handle.rs | 13 +- connect/src/state/options.rs | 8 +- connect/src/state/tracks.rs | 37 ++-- connect/src/state/transfer.rs | 4 +- 8 files changed, 472 insertions(+), 301 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index bcc4a8df1..2746d5ab0 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -1,54 +1,92 @@ -use crate::state::ConnectState; -use librespot_protocol::player::Context; +use crate::{ + core::{Error, Session}, + protocol::{ + autoplay_context_request::AutoplayContextRequest, + player::{Context, TransferState}, + }, + state::{context::UpdateContext, ConnectState}, +}; use std::{ + collections::{HashMap, VecDeque}, fmt::{Display, Formatter}, hash::{Hash, Hasher}, + time::Duration, }; +use thiserror::Error as ThisError; +use tokio::time::Instant; + +#[derive(Debug, Clone)] +enum Resolve { + Uri(String), + Context(Context), +} + +#[derive(Debug, Clone)] +pub(super) enum ContextAction { + Append, + Replace, +} #[derive(Debug, Clone)] pub(super) struct ResolveContext { - context: Context, + resolve: Resolve, fallback: Option, - autoplay: bool, + update: UpdateContext, + action: ContextAction, } impl ResolveContext { - pub fn from_uri(uri: impl Into, fallback: impl Into, autoplay: bool) -> Self { + fn append_context(uri: impl Into) -> Self { + Self { + resolve: Resolve::Uri(uri.into()), + fallback: None, + update: UpdateContext::Default, + action: ContextAction::Append, + } + } + + pub fn from_uri( + uri: impl Into, + fallback: impl Into, + update: UpdateContext, + action: ContextAction, + ) -> Self { let fallback_uri = fallback.into(); Self { - context: Context { - uri: uri.into(), - ..Default::default() - }, + resolve: Resolve::Uri(uri.into()), fallback: (!fallback_uri.is_empty()).then_some(fallback_uri), - autoplay, + update, + action, } } - pub fn from_context(context: Context, autoplay: bool) -> Self { + pub fn from_context(context: Context, update: UpdateContext, action: ContextAction) -> Self { Self { - context, + resolve: Resolve::Context(context), fallback: None, - autoplay, + update, + action, } } /// the uri which should be used to resolve the context, might not be the context uri - pub fn resolve_uri(&self) -> Option<&String> { + fn resolve_uri(&self) -> Option<&str> { // it's important to call this always, or at least for every ResolveContext // otherwise we might not even check if we need to fallback and just use the fallback uri - ConnectState::get_context_uri_from_context(&self.context) - .and_then(|s| (!s.is_empty()).then_some(s)) - .or(self.fallback.as_ref()) + match self.resolve { + Resolve::Uri(ref uri) => ConnectState::valid_resolve_uri(uri), + Resolve::Context(ref ctx) => { + ConnectState::get_context_uri_from_context(ctx).or(self.fallback.as_deref()) + } + } } /// the actual context uri - pub fn context_uri(&self) -> &str { - &self.context.uri - } - - pub fn autoplay(&self) -> bool { - self.autoplay + fn context_uri(&self) -> &str { + match self.resolve { + Resolve::Uri(ref uri) => uri, + Resolve::Context(ref ctx) => &ctx.uri, + } } } @@ -56,10 +94,10 @@ impl Display for ResolveContext { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, - "resolve_uri: <{:?}>, context_uri: <{}>, autoplay: <{}>", + "resolve_uri: <{:?}>, context_uri: <{}>, update: <{:?}>", self.resolve_uri(), - self.context.uri, - self.autoplay, + self.context_uri(), + self.update, ) } } @@ -68,7 +106,7 @@ impl PartialEq for ResolveContext { fn eq(&self, other: &Self) -> bool { let eq_context = self.context_uri() == other.context_uri(); let eq_resolve = self.resolve_uri() == other.resolve_uri(); - let eq_autoplay = self.autoplay == other.autoplay; + let eq_autoplay = self.update == other.update; eq_context && eq_resolve && eq_autoplay } @@ -80,12 +118,227 @@ impl Hash for ResolveContext { fn hash(&self, state: &mut H) { self.context_uri().hash(state); self.resolve_uri().hash(state); - self.autoplay.hash(state); + self.update.hash(state); + } +} + +#[derive(Debug, ThisError)] +enum ContextResolverError { + #[error("no next context to resolve")] + NoNext, + #[error("tried appending context with {0} pages")] + UnexpectedPagesSize(usize), + #[error("tried resolving not allowed context: {0:?}")] + NotAllowedContext(String), +} + +impl From for Error { + fn from(value: ContextResolverError) -> Self { + Error::failed_precondition(value) } } -impl From for Context { - fn from(value: ResolveContext) -> Self { - value.context +pub struct ContextResolver { + session: Session, + queue: VecDeque, + unavailable_contexts: HashMap, +} + +// time after which an unavailable context is retried +const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600); + +impl ContextResolver { + pub fn new(session: Session) -> Self { + Self { + session, + queue: VecDeque::new(), + unavailable_contexts: HashMap::new(), + } + } + + pub fn add(&mut self, resolve: ResolveContext) { + let last_try = self + .unavailable_contexts + .get(&resolve) + .map(|i| i.duration_since(Instant::now())); + + let last_try = if matches!(last_try, Some(last_try) if last_try > RETRY_UNAVAILABLE) { + let _ = self.unavailable_contexts.remove(&resolve); + debug!( + "context was requested {}s ago, trying again to resolve the requested context", + last_try.expect("checked by condition").as_secs() + ); + None + } else { + last_try + }; + + if last_try.is_some() { + debug!("tried loading unavailable context: {resolve}"); + return; + } else if self.queue.contains(&resolve) { + debug!("update for {resolve} is already added"); + return; + } + + self.queue.push_back(resolve) + } + + pub fn add_list(&mut self, resolve: Vec) { + for resolve in resolve { + self.add(resolve) + } + } + + pub fn remove_used_and_invalid(&mut self) { + if let Some((_, _, remove)) = self.find_next() { + for _ in 0..remove { + let _ = self.queue.pop_front(); + } + } + self.queue.pop_front(); + } + + pub fn clear(&mut self) { + self.queue = VecDeque::new() + } + + fn find_next(&self) -> Option<(&ResolveContext, &str, usize)> { + let mut idx = 0; + loop { + let next = self.queue.front()?; + match next.resolve_uri() { + None => { + warn!("skipped {idx} because of no valid resolve_uri: {next}"); + idx += 1; + continue; + } + Some(uri) => break Some((next, uri, idx)), + } + } + } + + pub fn has_next(&self) -> bool { + self.find_next().is_some() + } + + pub async fn get_next_context( + &self, + recent_track_uri: impl Fn() -> Vec, + ) -> Result { + let (next, resolve_uri, _) = self.find_next().ok_or(ContextResolverError::NoNext)?; + + match next.update { + UpdateContext::Default => { + let mut ctx = self.session.spclient().get_context(resolve_uri).await; + if let Ok(ctx) = ctx.as_mut() { + ctx.uri = next.context_uri().to_string(); + ctx.url = format!("context://{}", ctx.uri); + } + + ctx + } + UpdateContext::Autoplay => { + if resolve_uri.contains("spotify:show:") || resolve_uri.contains("spotify:episode:") + { + // autoplay is not supported for podcasts + Err(ContextResolverError::NotAllowedContext( + resolve_uri.to_string(), + ))? + } + + let request = AutoplayContextRequest { + context_uri: Some(resolve_uri.to_string()), + recent_track_uri: recent_track_uri(), + ..Default::default() + }; + self.session.spclient().get_autoplay_context(&request).await + } + } + } + + pub fn mark_next_unavailable(&mut self) { + if let Some((next, _, _)) = self.find_next() { + self.unavailable_contexts + .insert(next.clone(), Instant::now()); + } + } + + pub fn handle_next_context( + &self, + state: &mut ConnectState, + mut context: Context, + ) -> Result>, Error> { + let (next, _, _) = self.find_next().ok_or(ContextResolverError::NoNext)?; + + let remaining = match next.action { + ContextAction::Append if context.pages.len() == 1 => state + .fill_context_from_page(context.pages.remove(0)) + .map(|_| None), + ContextAction::Replace => { + let remaining = state.update_context(context, next.update); + if let Resolve::Context(ref ctx) = next.resolve { + state.merge_context(Some(ctx.clone())); + } + + remaining + } + ContextAction::Append => { + warn!("unexpected page size: {context:#?}"); + Err(ContextResolverError::UnexpectedPagesSize(context.pages.len()).into()) + } + }?; + + Ok(remaining.map(|remaining| { + remaining + .into_iter() + .map(ResolveContext::append_context) + .collect::>() + })) + } + + pub fn try_finish( + &self, + state: &mut ConnectState, + transfer_state: &mut Option, + ) -> bool { + let (next, _, _) = match self.find_next() { + None => return false, + Some(next) => next, + }; + + // when there is only one update type, we are the last of our kind, so we should update the state + if self + .queue + .iter() + .filter(|resolve| resolve.update == next.update) + .count() + != 1 + { + return false; + } + + debug!("last item of type <{:?}> finishing state", next.update); + + if let Some(transfer_state) = transfer_state.take() { + if let Err(why) = state.finish_transfer(transfer_state) { + error!("finishing setup of transfer failed: {why}") + } + } + + let res = if state.shuffling_context() { + state.shuffle() + } else { + state.reset_playback_to_position(Some(state.player().index.track as usize)) + }; + + if let Err(why) = res { + error!("setting up state failed after updating contexts: {why}") + } + + state.update_restrictions(); + state.update_queue_revision(); + + true } } diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index a6bc482d2..958ea74fa 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1,15 +1,6 @@ pub use crate::model::{PlayingTrack, SpircLoadCommand}; -use crate::state::{context::ResetContext, metadata::Metadata}; -use crate::{ - context_resolver::ResolveContext, - model::SpircPlayStatus, - state::{ - context::{ContextType, UpdateContext}, - provider::IsProvider, - {ConnectState, ConnectStateConfig}, - }, -}; use crate::{ + context_resolver::{ContextAction, ContextResolver, ResolveContext}, core::{ authentication::Credentials, dealer::{ @@ -19,12 +10,12 @@ use crate::{ session::UserAttributes, Error, Session, SpotifyId, }, + model::SpircPlayStatus, playback::{ mixer::Mixer, player::{Player, PlayerEvent, PlayerEventChannel}, }, protocol::{ - autoplay_context_request::AutoplayContextRequest, connect::{Cluster, ClusterUpdate, LogoutCommand, SetVolumeCommand}, explicit_content_pubsub::UserAttributesUpdate, player::{Context, TransferState}, @@ -32,11 +23,17 @@ use crate::{ social_connect_v2::{session::_host_active_device_id, SessionUpdate}, user_attributes::UserAttributesMutation, }, + state::{ + context::{ + ResetContext, {ContextType, UpdateContext}, + }, + metadata::Metadata, + provider::IsProvider, + {ConnectState, ConnectStateConfig}, + }, }; use futures_util::StreamExt; use protobuf::MessageField; -use std::collections::HashMap; -use std::time::Instant; use std::{ future::Future, sync::atomic::{AtomicUsize, Ordering}, @@ -94,17 +91,11 @@ struct SpircTask { commands: Option>, player_events: Option, + context_resolver: ContextResolver, + shutdown: bool, session: Session, - /// the list of contexts to resolve - resolve_context: Vec, - - /// contexts may not be resolvable at the moment so we should ignore any further request - /// - /// an unavailable context is retried after [RETRY_UNAVAILABLE] - unavailable_contexts: HashMap, - /// is set when transferring, and used after resolving the contexts to finish the transfer pub transfer_state: Option, @@ -145,14 +136,10 @@ const CONTEXT_FETCH_THRESHOLD: usize = 2; const VOLUME_STEP_SIZE: u16 = 1024; // (u16::MAX + 1) / VOLUME_STEPS -// delay to resolve a bundle of context updates, delaying the update prevents duplicate context updates of the same type -const RESOLVE_CONTEXT_DELAY: Duration = Duration::from_millis(600); -// time after which an unavailable context is retried -const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600); // delay to update volume after a certain amount of time, instead on each update request const VOLUME_UPDATE_DELAY: Duration = Duration::from_secs(2); // to reduce updates to remote, we group some request by waiting for a set amount of time -const UPDATE_STATE_DELAY: Duration = Duration::from_millis(400); +const UPDATE_STATE_DELAY: Duration = Duration::from_millis(300); pub struct Spirc { commands: mpsc::UnboundedSender, @@ -250,11 +237,11 @@ impl Spirc { commands: Some(cmd_rx), player_events: Some(player_events), + context_resolver: ContextResolver::new(session.clone()), + shutdown: false, session, - resolve_context: Vec::new(), - unavailable_contexts: HashMap::new(), transfer_state: None, update_volume: false, update_state: false, @@ -360,6 +347,10 @@ impl SpircTask { let commands = self.commands.as_mut(); let player_events = self.player_events.as_mut(); + // when state and volume update have a higher priority than context resolving + // because of that the context resolving has to wait, so that the other tasks can finish + let allow_context_resolving = !self.update_state && !self.update_volume; + tokio::select! { // startup of the dealer requires a connection_id, which is retrieved at the very beginning connection_id_update = self.connection_id_update.next() => unwrap! { @@ -422,7 +413,7 @@ impl SpircTask { } }, event = async { player_events?.recv().await }, if player_events.is_some() => if let Some(event) = event { - if let Err(e) = self.handle_player_event(event).await { + if let Err(e) = self.handle_player_event(event) { error!("could not dispatch player event: {}", e); } }, @@ -430,12 +421,9 @@ impl SpircTask { self.update_state = false; if let Err(why) = self.notify().await { - error!("ContextError: {why}") + error!("state update: {why}") } }, - _ = async { sleep(RESOLVE_CONTEXT_DELAY).await }, if !self.resolve_context.is_empty() => { - self.handle_resolve_context().await - }, _ = async { sleep(VOLUME_UPDATE_DELAY).await }, if self.update_volume => { self.update_volume = false; @@ -451,6 +439,21 @@ impl SpircTask { error!("error updating connect state for volume update: {why}") } }, + // context resolver handling, the idea/reason behind it the following: + // + // when we request a context that has multiple pages (for example an artist) + // resolving all pages at once can take around ~1-30sec, when we resolve + // everything at once that would block our main loop for that time + // + // to circumvent this behavior, we request each context separately here and + // finish after we received our last item of a type + next_context = async { + self.context_resolver.get_next_context(|| { + self.connect_state.prev_autoplay_track_uris() + }).await + }, if allow_context_resolving && self.context_resolver.has_next() => { + self.handle_context(next_context) + }, else => break } } @@ -468,175 +471,43 @@ impl SpircTask { self.session.dealer().close().await; } - async fn handle_resolve_context(&mut self) { - let mut last_resolve = None::; - while let Some(resolve) = self.resolve_context.pop() { - if matches!(last_resolve, Some(ref last_resolve) if last_resolve == &resolve) { - debug!("did already update the context for {resolve}"); - continue; - } else { - last_resolve = Some(resolve.clone()); - - let resolve_uri = match resolve.resolve_uri() { - Some(resolve) => resolve, - None => { - warn!("tried to resolve context without resolve_uri: {resolve}"); - return; - } - }; - - debug!("resolving: {resolve}"); - // the autoplay endpoint can return a 404, when it tries to retrieve an - // autoplay context for an empty playlist as it seems - if let Err(why) = self - .resolve_context(resolve_uri, resolve.context_uri(), resolve.autoplay()) - .await - { - error!("failed resolving context <{resolve}>: {why}"); - self.unavailable_contexts.insert(resolve, Instant::now()); - continue; - } - - self.connect_state.merge_context(Some(resolve.into())); - } - } - - if let Some(transfer_state) = self.transfer_state.take() { - if let Err(why) = self.connect_state.finish_transfer(transfer_state) { - error!("finishing setup of transfer failed: {why}") - } - } - - if matches!(self.connect_state.active_context, ContextType::Default) { - let ctx = self.connect_state.context.as_ref(); - if matches!(ctx, Some(ctx) if ctx.tracks.is_empty()) { - self.connect_state.clear_next_tracks(true); - // skip to the next queued track, otherwise it should stop - let _ = self.handle_next(None); - } - } - - if let Err(why) = self.connect_state.fill_up_next_tracks() { - error!("fill up of next tracks failed after updating contexts: {why}") - } - - self.connect_state.update_restrictions(); - self.connect_state.update_queue_revision(); - - self.preload_autoplay_when_required(); - - self.update_state = true; - } - - async fn resolve_context( - &mut self, - resolve_uri: &str, - context_uri: &str, - autoplay: bool, - ) -> Result<(), Error> { - if !autoplay { - let mut ctx = self.session.spclient().get_context(resolve_uri).await?; - ctx.uri = context_uri.to_string(); - ctx.url = format!("context://{context_uri}"); - - if let Some(remaining) = self - .connect_state - .update_context(ctx, UpdateContext::Default)? - { - self.try_resolve_remaining(remaining).await; + fn handle_context(&mut self, next_context: Result) { + let next_context = match next_context { + Err(why) => { + self.context_resolver.mark_next_unavailable(); + self.context_resolver.remove_used_and_invalid(); + error!("{why}"); + return; } - - return Ok(()); - } - - // refuse resolve of not supported autoplay context - if resolve_uri.contains("spotify:show:") || resolve_uri.contains("spotify:episode:") { - // autoplay is not supported for podcasts - Err(SpircError::NotAllowedContext(resolve_uri.to_string()))? - } - - // resolve autoplay - let previous_tracks = self.connect_state.prev_autoplay_track_uris(); - - debug!( - "requesting autoplay context <{resolve_uri}> with {} previous tracks", - previous_tracks.len() - ); - - let ctx_request = AutoplayContextRequest { - context_uri: Some(resolve_uri.to_string()), - recent_track_uri: previous_tracks, - ..Default::default() + Ok(ctx) => ctx, }; - let context = self - .session - .spclient() - .get_autoplay_context(&ctx_request) - .await?; - - if let Some(remaining) = self - .connect_state - .update_context(context, UpdateContext::Autoplay)? + match self + .context_resolver + .handle_next_context(&mut self.connect_state, next_context) { - self.try_resolve_remaining(remaining).await; - } - - Ok(()) - } - - async fn try_resolve_remaining(&mut self, remaining: Vec) { - for resolve_uri in remaining { - let mut ctx = match self.session.spclient().get_context(&resolve_uri).await { - Ok(ctx) => ctx, - Err(why) => { - warn!("failed to retrieve context for remaining <{resolve_uri}>: {why}"); - continue; + Ok(remaining) => { + if let Some(remaining) = remaining { + self.context_resolver.add_list(remaining) } - }; - - if ctx.pages.len() > 1 { - warn!("context contained more page then expected: {ctx:#?}"); - continue; } - - debug!("appending context from single page, adding: <{}>", ctx.uri); - - if let Err(why) = self - .connect_state - .fill_context_from_page(ctx.pages.remove(0)) - { - warn!("failed appending context <{resolve_uri}>: {why}"); + Err(why) => { + error!("{why}") } } - } - fn add_resolve_context(&mut self, resolve: ResolveContext) { - let last_try = self - .unavailable_contexts - .get(&resolve) - .map(|i| i.duration_since(Instant::now())); - - let last_try = if matches!(last_try, Some(last_try) if last_try > RETRY_UNAVAILABLE) { - let _ = self.unavailable_contexts.remove(&resolve); - debug!( - "context was requested {}s ago, trying again to resolve the requested context", - last_try.expect("checked by condition").as_secs() - ); - None - } else { - last_try - }; - - if last_try.is_none() { - debug!("add resolve request: {resolve}"); - self.resolve_context.push(resolve); - } else { - debug!("tried loading unavailable context: {resolve}") + if self + .context_resolver + .try_finish(&mut self.connect_state, &mut self.transfer_state) + { + self.add_autoplay_resolving_when_required(); + self.update_state = true; } + + self.context_resolver.remove_used_and_invalid(); } - // todo: time_delta still necessary? + // todo: is the time_delta still necessary? fn now_ms(&self) -> i64 { let dur = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -687,7 +558,7 @@ impl SpircTask { self.notify().await } - async fn handle_player_event(&mut self, event: PlayerEvent) -> Result<(), Error> { + fn handle_player_event(&mut self, event: PlayerEvent) -> Result<(), Error> { if let PlayerEvent::TrackChanged { audio_item } = event { self.connect_state.update_duration(audio_item.duration_ms); return Ok(()); @@ -910,7 +781,7 @@ impl SpircTask { self.player .emit_auto_play_changed_event(matches!(new_value, "1")); - self.preload_autoplay_when_required() + self.add_autoplay_resolving_when_required() } } else { trace!( @@ -993,9 +864,10 @@ impl SpircTask { update_context.context.uri, self.connect_state.context_uri() ) } else { - self.add_resolve_context(ResolveContext::from_context( + self.context_resolver.add(ResolveContext::from_context( update_context.context, - false, + super::state::context::UpdateContext::Default, + ContextAction::Replace, )) } return Ok(()); @@ -1100,7 +972,12 @@ impl SpircTask { let fallback = self.connect_state.current_track(|t| &t.uri).clone(); - self.add_resolve_context(ResolveContext::from_uri(ctx_uri.clone(), &fallback, false)); + self.context_resolver.add(ResolveContext::from_uri( + ctx_uri.clone(), + &fallback, + UpdateContext::Default, + ContextAction::Replace, + )); let timestamp = self.now_ms(); let state = &mut self.connect_state; @@ -1123,7 +1000,12 @@ impl SpircTask { if self.connect_state.current_track(|t| t.is_autoplay()) || autoplay { debug!("currently in autoplay context, async resolving autoplay for {ctx_uri}"); - self.add_resolve_context(ResolveContext::from_uri(ctx_uri, fallback, true)) + self.context_resolver.add(ResolveContext::from_uri( + ctx_uri, + fallback, + UpdateContext::Autoplay, + ContextAction::Replace, + )) } self.transfer_state = Some(transfer); @@ -1132,6 +1014,7 @@ impl SpircTask { } async fn handle_disconnect(&mut self) -> Result<(), Error> { + self.context_resolver.clear(); self.handle_stop(); self.play_status = SpircPlayStatus::Stopped {}; @@ -1191,6 +1074,8 @@ impl SpircTask { cmd: SpircLoadCommand, context: Option, ) -> Result<(), Error> { + self.context_resolver.clear(); + self.connect_state .reset_context(ResetContext::WhenDifferent(&cmd.context_uri)); @@ -1206,15 +1091,20 @@ impl SpircTask { } } else { &cmd.context_uri - } - .clone(); + }; if current_context_uri == &cmd.context_uri && fallback == cmd.context_uri { debug!("context <{current_context_uri}> didn't change, no resolving required") } else { debug!("resolving context for load command"); - self.resolve_context(&fallback, &cmd.context_uri, false) - .await?; + self.context_resolver.add(ResolveContext::from_uri( + &cmd.context_uri, + fallback, + UpdateContext::Default, + ContextAction::Replace, + )); + let context = self.context_resolver.get_next_context(Vec::new).await; + self.handle_context(context); } // for play commands with skip by uid, the context of the command contains @@ -1228,11 +1118,11 @@ impl SpircTask { let index = match cmd.playing_track { PlayingTrack::Index(i) => i as usize, PlayingTrack::Uri(uri) => { - let ctx = self.connect_state.context.as_ref(); + let ctx = self.connect_state.get_context(ContextType::Default)?; ConnectState::find_index_in_context(ctx, |t| t.uri == uri)? } PlayingTrack::Uid(uid) => { - let ctx = self.connect_state.context.as_ref(); + let ctx = self.connect_state.get_context(ContextType::Default)?; ConnectState::find_index_in_context(ctx, |t| t.uid == uid)? } }; @@ -1244,18 +1134,21 @@ impl SpircTask { self.connect_state.set_shuffle(cmd.shuffle); self.connect_state.set_repeat_context(cmd.repeat); + self.connect_state.set_repeat_track(cmd.repeat_track); if cmd.shuffle { - self.connect_state.set_current_track(index)?; - self.connect_state.shuffle()?; + self.connect_state.set_current_track_random()?; + + if self.context_resolver.has_next() { + self.connect_state.update_queue_revision() + } else { + self.connect_state.shuffle()?; + } } else { - // manually overwrite a possible current queued track self.connect_state.set_current_track(index)?; self.connect_state.reset_playback_to_position(Some(index))?; } - self.connect_state.set_repeat_track(cmd.repeat_track); - if self.connect_state.current_track(MessageField::is_some) { self.load_track(cmd.start_playing, cmd.seek_to)?; } else { @@ -1263,8 +1156,6 @@ impl SpircTask { self.handle_stop() } - self.preload_autoplay_when_required(); - Ok(()) } @@ -1383,7 +1274,7 @@ impl SpircTask { Ok(()) } - fn preload_autoplay_when_required(&mut self) { + fn add_autoplay_resolving_when_required(&mut self) { let require_load_new = !self .connect_state .has_next_tracks(Some(CONTEXT_FETCH_THRESHOLD)) @@ -1395,9 +1286,25 @@ impl SpircTask { let current_context = self.connect_state.context_uri(); let fallback = self.connect_state.current_track(|t| &t.uri); - let resolve = ResolveContext::from_uri(current_context, fallback, true); - self.add_resolve_context(resolve); + let has_tracks = self + .connect_state + .get_context(ContextType::Autoplay) + .map(|c| !c.tracks.is_empty()) + .unwrap_or_default(); + + let resolve = ResolveContext::from_uri( + current_context, + fallback, + UpdateContext::Autoplay, + if has_tracks { + ContextAction::Append + } else { + ContextAction::Replace + }, + ); + + self.context_resolver.add(resolve); } fn handle_next(&mut self, track_uri: Option) -> Result<(), Error> { @@ -1420,7 +1327,7 @@ impl SpircTask { }; }; - self.preload_autoplay_when_required(); + self.add_autoplay_resolving_when_required(); if has_next_track { self.load_track(continue_playing, 0) @@ -1478,10 +1385,11 @@ impl SpircTask { } debug!("playlist modification for current context: {uri}"); - self.add_resolve_context(ResolveContext::from_uri( + self.context_resolver.add(ResolveContext::from_uri( uri, self.connect_state.current_track(|t| &t.uri), - false, + UpdateContext::Default, + ContextAction::Replace, )); Ok(()) diff --git a/connect/src/state.rs b/connect/src/state.rs index b9cf37d62..6366388f6 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -100,17 +100,17 @@ pub struct ConnectState { unavailable_uri: Vec, - pub active_since: Option, + active_since: Option, queue_count: u64, // separation is necessary because we could have already loaded // the autoplay context but are still playing from the default context /// to update the active context use [switch_active_context](ConnectState::set_active_context) - pub active_context: ContextType, - pub fill_up_context: ContextType, + active_context: ContextType, + fill_up_context: ContextType, /// the context from which we play, is used to top up prev and next tracks - pub context: Option, + context: Option, /// a context to keep track of our shuffled context, /// should be only available when `player.option.shuffling_context` is true @@ -359,7 +359,7 @@ impl ConnectState { self.clear_prev_track(); if new_index > 0 { - let context = self.get_context(&self.active_context)?; + let context = self.get_context(self.active_context)?; let before_new_track = context.tracks.len() - new_index; self.player_mut().prev_tracks = context diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 8aad6463d..a9881ec46 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -1,7 +1,9 @@ -use crate::state::{metadata::Metadata, provider::Provider, ConnectState, StateError}; -use librespot_core::{Error, SpotifyId}; -use librespot_protocol::player::{ - Context, ContextIndex, ContextPage, ContextTrack, ProvidedTrack, Restrictions, +use crate::{ + core::{Error, SpotifyId}, + protocol::player::{ + Context, ContextIndex, ContextPage, ContextTrack, ProvidedTrack, Restrictions, + }, + state::{metadata::Metadata, provider::Provider, ConnectState, StateError}, }; use protobuf::MessageField; use std::collections::HashMap; @@ -27,7 +29,7 @@ pub enum ContextType { Autoplay, } -#[derive(Debug)] +#[derive(Debug, PartialEq, Hash, Copy, Clone)] pub enum UpdateContext { Default, Autoplay, @@ -62,26 +64,22 @@ fn page_url_to_uri(page_url: &str) -> String { impl ConnectState { pub fn find_index_in_context bool>( - context: Option<&StateContext>, + ctx: &StateContext, f: F, ) -> Result { - let ctx = context - .as_ref() - .ok_or(StateError::NoContext(ContextType::Default))?; - ctx.tracks .iter() .position(f) .ok_or(StateError::CanNotFindTrackInContext(None, ctx.tracks.len())) } - pub(super) fn get_context(&self, ty: &ContextType) -> Result<&StateContext, StateError> { + pub fn get_context(&self, ty: ContextType) -> Result<&StateContext, StateError> { match ty { ContextType::Default => self.context.as_ref(), ContextType::Shuffle => self.shuffle_context.as_ref(), ContextType::Autoplay => self.autoplay_context.as_ref(), } - .ok_or(StateError::NoContext(*ty)) + .ok_or(StateError::NoContext(ty)) } pub fn context_uri(&self) -> &String { @@ -117,21 +115,24 @@ impl ConnectState { self.update_restrictions() } - pub fn get_context_uri_from_context(context: &Context) -> Option<&String> { - if !context.uri.starts_with(SEARCH_IDENTIFIER) { - return Some(&context.uri); - } + pub fn valid_resolve_uri(uri: &str) -> Option<&str> { + (!uri.starts_with(SEARCH_IDENTIFIER)).then_some(uri) + } - context - .pages - .first() - .and_then(|p| p.tracks.first().map(|t| &t.uri)) + pub fn get_context_uri_from_context(context: &Context) -> Option<&str> { + match Self::valid_resolve_uri(&context.uri) { + Some(uri) => Some(uri), + None => context + .pages + .first() + .and_then(|p| p.tracks.first().map(|t| t.uri.as_ref())), + } } pub fn set_active_context(&mut self, new_context: ContextType) { self.active_context = new_context; - let ctx = match self.get_context(&new_context) { + let ctx = match self.get_context(new_context) { Err(why) => { debug!("couldn't load context info because: {why}"); return; @@ -213,7 +214,7 @@ impl ConnectState { if !self.context_uri().contains(SEARCH_IDENTIFIER) && self.context_uri() == &context.uri { - match Self::find_index_in_context(Some(&new_context), |t| { + match Self::find_index_in_context(&new_context, |t| { self.current_track(|t| &t.uri) == &t.uri }) { Ok(new_pos) => { @@ -326,12 +327,11 @@ impl ConnectState { } if let Ok(position) = - Self::find_index_in_context(Some(current_context), |t| t.uri == new_track.uri) + Self::find_index_in_context(current_context, |t| t.uri == new_track.uri) { let context_track = current_context.tracks.get_mut(position)?; for (key, value) in new_track.metadata { - warn!("merging metadata {key} {value}"); context_track.metadata.insert(key, value); } diff --git a/connect/src/state/handle.rs b/connect/src/state/handle.rs index a69e1ebe1..1c1a4b325 100644 --- a/connect/src/state/handle.rs +++ b/connect/src/state/handle.rs @@ -1,5 +1,10 @@ -use crate::state::{context::ResetContext, ConnectState}; -use librespot_core::{dealer::protocol::SetQueueCommand, Error}; +use crate::{ + core::{dealer::protocol::SetQueueCommand, Error}, + state::{ + context::{ContextType, ResetContext}, + ConnectState, + }, +}; use protobuf::MessageField; impl ConnectState { @@ -16,7 +21,7 @@ impl ConnectState { return Ok(()); } - let ctx = self.context.as_ref(); + let ctx = self.get_context(ContextType::Default)?; let current_index = ConnectState::find_index_in_context(ctx, |c| self.current_track(|t| c.uri == t.uri))?; @@ -52,7 +57,7 @@ impl ConnectState { self.set_shuffle(false); self.reset_context(ResetContext::DefaultIndex); - let ctx = self.context.as_ref(); + let ctx = self.get_context(ContextType::Default)?; let current_track = ConnectState::find_index_in_context(ctx, |t| { self.current_track(|t| &t.uri) == &t.uri })?; diff --git a/connect/src/state/options.rs b/connect/src/state/options.rs index b6bc331c9..12040d3d4 100644 --- a/connect/src/state/options.rs +++ b/connect/src/state/options.rs @@ -51,12 +51,8 @@ impl ConnectState { let current_uri = self.current_track(|t| &t.uri); - let ctx = self - .context - .as_ref() - .ok_or(StateError::NoContext(ContextType::Default))?; - - let current_track = Self::find_index_in_context(Some(ctx), |t| &t.uri == current_uri)?; + let ctx = self.get_context(ContextType::Default)?; + let current_track = Self::find_index_in_context(ctx, |t| &t.uri == current_uri)?; let mut shuffle_context = ctx.clone(); // we don't need to include the current track, because it is already being played diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index 2dc1b9af4..700bfec5a 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -1,12 +1,15 @@ -use crate::state::{ - context::ContextType, - metadata::Metadata, - provider::{IsProvider, Provider}, - ConnectState, StateError, SPOTIFY_MAX_NEXT_TRACKS_SIZE, SPOTIFY_MAX_PREV_TRACKS_SIZE, +use crate::{ + core::{Error, SpotifyId}, + protocol::player::ProvidedTrack, + state::{ + context::ContextType, + metadata::Metadata, + provider::{IsProvider, Provider}, + ConnectState, StateError, SPOTIFY_MAX_NEXT_TRACKS_SIZE, SPOTIFY_MAX_PREV_TRACKS_SIZE, + }, }; -use librespot_core::{Error, SpotifyId}; -use librespot_protocol::player::ProvidedTrack; use protobuf::MessageField; +use rand::Rng; // identifier used as part of the uid pub const IDENTIFIER_DELIMITER: &str = "delimiter"; @@ -64,8 +67,14 @@ impl<'ct> ConnectState { &self.player().next_tracks } + pub fn set_current_track_random(&mut self) -> Result<(), Error> { + let max_tracks = self.get_context(self.active_context)?.tracks.len(); + let rng_track = rand::thread_rng().gen_range(0..max_tracks); + self.set_current_track(rng_track) + } + pub fn set_current_track(&mut self, index: usize) -> Result<(), Error> { - let context = self.get_context(&self.active_context)?; + let context = self.get_context(self.active_context)?; let new_track = context .tracks @@ -77,8 +86,8 @@ impl<'ct> ConnectState { debug!( "set track to: {} at {} of {} tracks", - index, new_track.uri, + index, context.tracks.len() ); @@ -132,7 +141,7 @@ impl<'ct> ConnectState { self.set_active_context(ContextType::Autoplay); None } else { - let ctx = self.context.as_ref(); + let ctx = self.get_context(ContextType::Default)?; let new_index = Self::find_index_in_context(ctx, |c| c.uri == new_track.uri); match new_index { Ok(new_index) => Some(new_index as u32), @@ -272,12 +281,12 @@ impl<'ct> ConnectState { } pub fn fill_up_next_tracks(&mut self) -> Result<(), StateError> { - let ctx = self.get_context(&self.fill_up_context)?; + let ctx = self.get_context(self.fill_up_context)?; let mut new_index = ctx.index.track as usize; let mut iteration = ctx.index.page; while self.next_tracks().len() < SPOTIFY_MAX_NEXT_TRACKS_SIZE { - let ctx = self.get_context(&self.fill_up_context)?; + let ctx = self.get_context(self.fill_up_context)?; let track = match ctx.tracks.get(new_index) { None if self.repeat_context() => { let delimiter = Self::new_delimiter(iteration.into()); @@ -292,14 +301,14 @@ impl<'ct> ConnectState { // transition to autoplay as fill up context self.fill_up_context = ContextType::Autoplay; - new_index = self.get_context(&ContextType::Autoplay)?.index.track as usize; + new_index = self.get_context(ContextType::Autoplay)?.index.track as usize; // add delimiter to only display the current context Self::new_delimiter(iteration.into()) } None if self.autoplay_context.is_some() => { match self - .get_context(&ContextType::Autoplay)? + .get_context(ContextType::Autoplay)? .tracks .get(new_index) { diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index feafb6983..384087c77 100644 --- a/connect/src/state/transfer.rs +++ b/connect/src/state/transfer.rs @@ -86,7 +86,7 @@ impl ConnectState { self.set_active_context(context_ty); self.fill_up_context = context_ty; - let ctx = self.get_context(&self.active_context).ok(); + let ctx = self.get_context(self.active_context)?; let current_index = if track.is_queue() { Self::find_index_in_context(ctx, |c| c.uid == transfer.current_session.current_uid) @@ -99,7 +99,7 @@ impl ConnectState { "active track is <{}> with index {current_index:?} in {:?} context, has {} tracks", track.uri, self.active_context, - ctx.map(|c| c.tracks.len()).unwrap_or_default() + ctx.tracks.len() ); if self.player().track.is_none() { From 11040d994e87cae73d007089c416225c196fe832 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Wed, 11 Dec 2024 23:35:59 +0100 Subject: [PATCH 11/30] connect: rework aftermath --- connect/src/context_resolver.rs | 33 ++++++++++++++++++++++----- connect/src/spirc.rs | 14 ++++++++++-- connect/src/state.rs | 2 +- connect/src/state/context.rs | 40 ++++++++++++++++++++------------- connect/src/state/tracks.rs | 4 +++- 5 files changed, 68 insertions(+), 25 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index 2746d5ab0..c66d2f122 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -1,3 +1,4 @@ +use crate::state::context::ContextType; use crate::{ core::{Error, Session}, protocol::{ @@ -6,6 +7,7 @@ use crate::{ }, state::{context::UpdateContext, ConnectState}, }; +use std::cmp::PartialEq; use std::{ collections::{HashMap, VecDeque}, fmt::{Display, Formatter}, @@ -75,10 +77,9 @@ impl ResolveContext { // otherwise we might not even check if we need to fallback and just use the fallback uri match self.resolve { Resolve::Uri(ref uri) => ConnectState::valid_resolve_uri(uri), - Resolve::Context(ref ctx) => { - ConnectState::get_context_uri_from_context(ctx).or(self.fallback.as_deref()) - } + Resolve::Context(ref ctx) => ConnectState::get_context_uri_from_context(ctx), } + .or(self.fallback.as_deref()) } /// the actual context uri @@ -147,6 +148,8 @@ pub struct ContextResolver { // time after which an unavailable context is retried const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600); +const CONCERNING_AMOUNT_OF_SKIPS: usize = 1_000; + impl ContextResolver { pub fn new(session: Session) -> Self { Self { @@ -208,6 +211,8 @@ impl ContextResolver { loop { let next = self.queue.front()?; match next.resolve_uri() { + // this is here to prevent an endless amount of skips + None if idx > CONCERNING_AMOUNT_OF_SKIPS => unreachable!(), None => { warn!("skipped {idx} because of no valid resolve_uri: {next}"); idx += 1; @@ -318,7 +323,16 @@ impl ContextResolver { return false; } - debug!("last item of type <{:?}> finishing state", next.update); + debug!("last item of type <{:?}>, finishing state", next.update); + + match (next.update, state.active_context) { + (UpdateContext::Default, ContextType::Default) => {} + (UpdateContext::Default, _) => { + debug!("skipped finishing default, because it isn't the active context"); + return false; + } + (UpdateContext::Autoplay, _) => {} + } if let Some(transfer_state) = transfer_state.take() { if let Err(why) = state.finish_transfer(transfer_state) { @@ -328,8 +342,17 @@ impl ContextResolver { let res = if state.shuffling_context() { state.shuffle() + } else if let Ok(ctx) = state.get_context(state.active_context) { + let idx = ConnectState::find_index_in_context(ctx, |t| { + state.current_track(|c| t.uri == c.uri) + }) + .ok(); + + state + .reset_playback_to_position(idx) + .and_then(|_| state.fill_up_next_tracks()) } else { - state.reset_playback_to_position(Some(state.player().index.track as usize)) + state.fill_up_next_tracks() }; if let Err(why) = res { diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 958ea74fa..bd8fc9628 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -985,6 +985,13 @@ impl SpircTask { state.set_active(true); state.handle_initial_transfer(&mut transfer); + // adjust active context, so resolve knows for which context it should set up the state + state.active_context = if autoplay { + ContextType::Autoplay + } else { + ContextType::Default + }; + // update position if the track continued playing let position = if transfer.playback.is_paused { transfer.playback.position_as_of_timestamp.into() @@ -1110,6 +1117,8 @@ impl SpircTask { // for play commands with skip by uid, the context of the command contains // tracks with uri and uid, so we merge the new context with the resolved/existing context self.connect_state.merge_context(context); + + // load here, so that we clear the queue only after we definitely retrieved a new context self.connect_state.clear_next_tracks(false); self.connect_state.clear_restrictions(); @@ -1143,10 +1152,12 @@ impl SpircTask { self.connect_state.update_queue_revision() } else { self.connect_state.shuffle()?; + self.add_autoplay_resolving_when_required(); } } else { self.connect_state.set_current_track(index)?; self.connect_state.reset_playback_to_position(Some(index))?; + self.add_autoplay_resolving_when_required(); } if self.connect_state.current_track(MessageField::is_some) { @@ -1327,9 +1338,8 @@ impl SpircTask { }; }; - self.add_autoplay_resolving_when_required(); - if has_next_track { + self.add_autoplay_resolving_when_required(); self.load_track(continue_playing, 0) } else { info!("Not playing next track because there are no more tracks left in queue."); diff --git a/connect/src/state.rs b/connect/src/state.rs index 6366388f6..a132e4798 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -106,7 +106,7 @@ pub struct ConnectState { // separation is necessary because we could have already loaded // the autoplay context but are still playing from the default context /// to update the active context use [switch_active_context](ConnectState::set_active_context) - active_context: ContextType, + pub active_context: ContextType, fill_up_context: ContextType, /// the context from which we play, is used to top up prev and next tracks diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index a9881ec46..c0f95b69d 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -7,6 +7,7 @@ use crate::{ }; use protobuf::MessageField; use std::collections::HashMap; +use std::ops::Deref; use uuid::Uuid; const LOCAL_FILES_IDENTIFIER: &str = "spotify:local-files"; @@ -21,7 +22,7 @@ pub struct StateContext { pub index: ContextIndex, } -#[derive(Default, Debug, Copy, Clone)] +#[derive(Default, Debug, Copy, Clone, PartialEq)] pub enum ContextType { #[default] Default, @@ -35,6 +36,17 @@ pub enum UpdateContext { Autoplay, } +impl Deref for UpdateContext { + type Target = ContextType; + + fn deref(&self) -> &Self::Target { + match self { + UpdateContext::Default => &ContextType::Default, + UpdateContext::Autoplay => &ContextType::Autoplay, + } + } +} + pub enum ResetContext<'s> { Completely, DefaultIndex, @@ -86,11 +98,16 @@ impl ConnectState { &self.player().context_uri } + fn different_context_uri(&self, uri: &str) -> bool { + // search identifier is always different + self.context_uri() != uri || uri.starts_with(SEARCH_IDENTIFIER) + } + pub fn reset_context(&mut self, mut reset_as: ResetContext) { self.set_active_context(ContextType::Default); self.fill_up_context = ContextType::Default; - if matches!(reset_as, ResetContext::WhenDifferent(ctx) if self.context_uri() != ctx) { + if matches!(reset_as, ResetContext::WhenDifferent(ctx) if self.different_context_uri(ctx)) { reset_as = ResetContext::Completely } self.shuffle_context = None; @@ -134,7 +151,7 @@ impl ConnectState { let ctx = match self.get_context(new_context) { Err(why) => { - debug!("couldn't load context info because: {why}"); + warn!("couldn't load context info because: {why}"); return; } Ok(ctx) => ctx, @@ -184,17 +201,8 @@ impl ConnectState { Some(p) => p, }; - let prev_context = match ty { - UpdateContext::Default => self.context.as_ref(), - UpdateContext::Autoplay => self.autoplay_context.as_ref(), - }; - debug!( - "updated context {ty:?} from <{}> ({} tracks) to <{}> ({} tracks)", - self.context_uri(), - prev_context - .map(|c| c.tracks.len().to_string()) - .unwrap_or_else(|| "-".to_string()), + "updated context {ty:?} to <{}> ({} tracks)", context.uri, page.tracks.len() ); @@ -210,8 +218,8 @@ impl ConnectState { ); // when we update the same context, we should try to preserve the previous position - // otherwise we might load the entire context twice - if !self.context_uri().contains(SEARCH_IDENTIFIER) + // otherwise we might load the entire context twice, unless it's the search context + if !self.context_uri().starts_with(SEARCH_IDENTIFIER) && self.context_uri() == &context.uri { match Self::find_index_in_context(&new_context, |t| { @@ -234,7 +242,7 @@ impl ConnectState { self.context = Some(new_context); - if !context.url.contains(SEARCH_IDENTIFIER) { + if !context.url.starts_with(SEARCH_IDENTIFIER) { self.player_mut().context_url = context.url; } else { self.player_mut().context_url.clear() diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index 700bfec5a..81dd38786 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -128,6 +128,8 @@ impl<'ct> ConnectState { }; }; + debug!("next track is {new_track:#?}"); + let new_track = match new_track { None => return Ok(None), Some(t) => t, @@ -280,7 +282,7 @@ impl<'ct> ConnectState { } } - pub fn fill_up_next_tracks(&mut self) -> Result<(), StateError> { + pub fn fill_up_next_tracks(&mut self) -> Result<(), Error> { let ctx = self.get_context(self.fill_up_context)?; let mut new_index = ctx.index.track as usize; let mut iteration = ctx.index.page; From cd9f822c900586ff2d55ee5eabd43758ec8225d1 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Wed, 11 Dec 2024 23:52:53 +0100 Subject: [PATCH 12/30] general logging and comment fixups --- connect/src/state/tracks.rs | 2 -- core/src/dealer/protocol.rs | 2 ++ core/src/spclient.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index 81dd38786..fae36bc82 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -128,8 +128,6 @@ impl<'ct> ConnectState { }; }; - debug!("next track is {new_track:#?}"); - let new_track = match new_track { None => return Ok(None), Some(t) => t, diff --git a/core/src/dealer/protocol.rs b/core/src/dealer/protocol.rs index e6b7f2dc3..d450dfa78 100644 --- a/core/src/dealer/protocol.rs +++ b/core/src/dealer/protocol.rs @@ -175,6 +175,8 @@ fn handle_transfer_encoding( let encoding = headers.get("Transfer-Encoding").map(String::as_str); if let Some(encoding) = encoding { trace!("message was send with {encoding} encoding "); + } else { + trace!("message was send with no encoding "); } if !matches!(encoding, Some("gzip")) { diff --git a/core/src/spclient.rs b/core/src/spclient.rs index c818570ac..5af77394d 100644 --- a/core/src/spclient.rs +++ b/core/src/spclient.rs @@ -820,7 +820,7 @@ impl SpClient { /// **will** contain the query /// - artists /// - returns 2 pages with tracks: 10 most popular tracks and latest/popular album - /// - remaining pages are albums of the artists and are only provided as page_url + /// - remaining pages are artist albums sorted by popularity (only provided as page_url) /// - search /// - is massively influenced by the provided query /// - the query result shown by the search expects no query at all From b13ab3aa1b713e2a586890791050be6e11f4a746 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 12 Dec 2024 17:33:15 +0100 Subject: [PATCH 13/30] connect: fix incorrect stopping --- connect/src/spirc.rs | 3 +-- connect/src/state.rs | 6 ++++++ connect/src/state/tracks.rs | 5 +++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index bd8fc9628..1d7f7a634 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1042,7 +1042,7 @@ impl SpircTask { self.connect_state.update_position(0, self.now_ms()); self.connect_state.clear_next_tracks(true); - if let Err(why) = self.connect_state.fill_up_next_tracks() { + if let Err(why) = self.connect_state.reset_playback_to_position(None) { warn!("failed filling up next_track during stopping: {why}") } } @@ -1343,7 +1343,6 @@ impl SpircTask { self.load_track(continue_playing, 0) } else { info!("Not playing next track because there are no more tracks left in queue."); - self.connect_state.reset_playback_to_position(None)?; self.handle_stop(); Ok(()) } diff --git a/connect/src/state.rs b/connect/src/state.rs index a132e4798..2c1bf9449 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -348,9 +348,15 @@ impl ConnectState { } pub fn reset_playback_to_position(&mut self, new_index: Option) -> Result<(), Error> { + debug!( + "reset_playback with active ctx <{:?}> fill_up ctx <{:?}>", + self.active_context, self.fill_up_context + ); + let new_index = new_index.unwrap_or(0); self.update_current_index(|i| i.track = new_index as u32); self.update_context_index(self.active_context, new_index + 1)?; + self.fill_up_context = self.active_context; if !self.current_track(|t| t.is_queue()) { self.set_current_track(new_index)?; diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index fae36bc82..499446bb0 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -333,6 +333,11 @@ impl<'ct> ConnectState { self.next_tracks_mut().push(track); } + debug!( + "finished filling up next_tracks ({})", + self.next_tracks().len() + ); + self.update_context_index(self.fill_up_context, new_index)?; // the web-player needs a revision update, otherwise the queue isn't updated in the ui From 932c6fa25f72dda5b4b8a1e1c2ff59a615ed86cf Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 12 Dec 2024 17:52:19 +0100 Subject: [PATCH 14/30] connect: always handle player seek event --- connect/src/spirc.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 1d7f7a634..5b6ff91b7 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -607,9 +607,13 @@ impl SpircTask { trace!("==> Loading"); } }, + PlayerEvent::Seeked { position_ms, .. } => { + trace!("==> Seeked"); + self.connect_state + .update_position(position_ms, self.now_ms()) + } PlayerEvent::Playing { position_ms, .. } - | PlayerEvent::PositionCorrection { position_ms, .. } - | PlayerEvent::Seeked { position_ms, .. } => { + | PlayerEvent::PositionCorrection { position_ms, .. } => { trace!("==> Playing"); let new_nominal_start_time = self.now_ms() - position_ms as i64; match self.play_status { From 0752b8d7533bf00ad88dc2367c7156fffc1ce74f Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Sat, 14 Dec 2024 16:13:09 +0100 Subject: [PATCH 15/30] connect: adjust behavior - rename `handle_context` to `handle_next_context` - disconnect should only pause the playback - find_next should not exceed queue length --- connect/src/context_resolver.rs | 8 ++------ connect/src/spirc.rs | 8 ++++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index c66d2f122..01bc8c2a2 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -148,8 +148,6 @@ pub struct ContextResolver { // time after which an unavailable context is retried const RETRY_UNAVAILABLE: Duration = Duration::from_secs(3600); -const CONCERNING_AMOUNT_OF_SKIPS: usize = 1_000; - impl ContextResolver { pub fn new(session: Session) -> Self { Self { @@ -211,14 +209,12 @@ impl ContextResolver { loop { let next = self.queue.front()?; match next.resolve_uri() { - // this is here to prevent an endless amount of skips - None if idx > CONCERNING_AMOUNT_OF_SKIPS => unreachable!(), - None => { + None if idx < self.queue.len() => { warn!("skipped {idx} because of no valid resolve_uri: {next}"); idx += 1; continue; } - Some(uri) => break Some((next, uri, idx)), + value => break value.map(|uri| (next, uri, idx)), } } } diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 5b6ff91b7..f6ad7e111 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -452,7 +452,7 @@ impl SpircTask { self.connect_state.prev_autoplay_track_uris() }).await }, if allow_context_resolving && self.context_resolver.has_next() => { - self.handle_context(next_context) + self.handle_next_context(next_context) }, else => break } @@ -471,7 +471,7 @@ impl SpircTask { self.session.dealer().close().await; } - fn handle_context(&mut self, next_context: Result) { + fn handle_next_context(&mut self, next_context: Result) { let next_context = match next_context { Err(why) => { self.context_resolver.mark_next_unavailable(); @@ -1026,7 +1026,7 @@ impl SpircTask { async fn handle_disconnect(&mut self) -> Result<(), Error> { self.context_resolver.clear(); - self.handle_stop(); + self.handle_pause(); self.play_status = SpircPlayStatus::Stopped {}; self.connect_state @@ -1115,7 +1115,7 @@ impl SpircTask { ContextAction::Replace, )); let context = self.context_resolver.get_next_context(Vec::new).await; - self.handle_context(context); + self.handle_next_context(context); } // for play commands with skip by uid, the context of the command contains From 08624ddf36ca9cbc0c1cebfd0b496409a907d51b Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 16 Dec 2024 00:19:21 +0100 Subject: [PATCH 16/30] fix typo and `find_next` --- connect/src/context_resolver.rs | 2 +- connect/src/state/context.rs | 2 +- core/src/dealer/protocol.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index 01bc8c2a2..7a8b56f13 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -207,7 +207,7 @@ impl ContextResolver { fn find_next(&self) -> Option<(&ResolveContext, &str, usize)> { let mut idx = 0; loop { - let next = self.queue.front()?; + let next = self.queue.get(idx)?; match next.resolve_uri() { None if idx < self.queue.len() => { warn!("skipped {idx} because of no valid resolve_uri: {next}"); diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index c0f95b69d..27527c685 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -63,7 +63,7 @@ fn page_url_to_uri(page_url: &str) -> String { let split = if let Some(rest) = page_url.strip_prefix("hm://") { rest.split('/') } else { - warn!("page_url didn't started with hm://. got page_url: {page_url}"); + warn!("page_url didn't start with hm://. got page_url: {page_url}"); page_url.split('/') }; diff --git a/core/src/dealer/protocol.rs b/core/src/dealer/protocol.rs index d450dfa78..c774a1194 100644 --- a/core/src/dealer/protocol.rs +++ b/core/src/dealer/protocol.rs @@ -174,9 +174,9 @@ fn handle_transfer_encoding( ) -> Result, Error> { let encoding = headers.get("Transfer-Encoding").map(String::as_str); if let Some(encoding) = encoding { - trace!("message was send with {encoding} encoding "); + trace!("message was sent with {encoding} encoding "); } else { - trace!("message was send with no encoding "); + trace!("message was sent with no encoding "); } if !matches!(encoding, Some("gzip")) { From 710e2fc7be8cdf570ffad47c2b236dc5506009f1 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 16 Dec 2024 18:58:26 +0100 Subject: [PATCH 17/30] connect: fixes for context and transfer - fix context_metadata and restriction incorrect reset - do some state updates earlier - add more logging --- connect/src/context_resolver.rs | 36 +++++++++++---------------------- connect/src/spirc.rs | 31 +++++++++++++++++++--------- connect/src/state/context.rs | 12 +++++------ connect/src/state/transfer.rs | 1 + 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index 7a8b56f13..eb7ff3552 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -180,6 +180,11 @@ impl ContextResolver { } else if self.queue.contains(&resolve) { debug!("update for {resolve} is already added"); return; + } else { + trace!( + "added {} to resolver queue", + resolve.resolve_uri().unwrap_or(resolve.context_uri()) + ) } self.queue.push_back(resolve) @@ -265,7 +270,7 @@ impl ContextResolver { } } - pub fn handle_next_context( + pub fn apply_next_context( &self, state: &mut ConnectState, mut context: Context, @@ -319,15 +324,17 @@ impl ContextResolver { return false; } - debug!("last item of type <{:?}>, finishing state", next.update); - match (next.update, state.active_context) { - (UpdateContext::Default, ContextType::Default) => {} + (UpdateContext::Default, ContextType::Default) | (UpdateContext::Autoplay, _) => { + debug!( + "last item of type <{:?}>, finishing state setup", + next.update + ); + } (UpdateContext::Default, _) => { debug!("skipped finishing default, because it isn't the active context"); return false; } - (UpdateContext::Autoplay, _) => {} } if let Some(transfer_state) = transfer_state.take() { @@ -336,25 +343,6 @@ impl ContextResolver { } } - let res = if state.shuffling_context() { - state.shuffle() - } else if let Ok(ctx) = state.get_context(state.active_context) { - let idx = ConnectState::find_index_in_context(ctx, |t| { - state.current_track(|c| t.uri == c.uri) - }) - .ok(); - - state - .reset_playback_to_position(idx) - .and_then(|_| state.fill_up_next_tracks()) - } else { - state.fill_up_next_tracks() - }; - - if let Err(why) = res { - error!("setting up state failed after updating contexts: {why}") - } - state.update_restrictions(); state.update_queue_revision(); diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index f6ad7e111..874a003ba 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -139,7 +139,7 @@ const VOLUME_STEP_SIZE: u16 = 1024; // (u16::MAX + 1) / VOLUME_STEPS // delay to update volume after a certain amount of time, instead on each update request const VOLUME_UPDATE_DELAY: Duration = Duration::from_secs(2); // to reduce updates to remote, we group some request by waiting for a set amount of time -const UPDATE_STATE_DELAY: Duration = Duration::from_millis(300); +const UPDATE_STATE_DELAY: Duration = Duration::from_millis(200); pub struct Spirc { commands: mpsc::UnboundedSender, @@ -452,7 +452,12 @@ impl SpircTask { self.connect_state.prev_autoplay_track_uris() }).await }, if allow_context_resolving && self.context_resolver.has_next() => { - self.handle_next_context(next_context) + let update_state = self.handle_next_context(next_context); + if update_state { + if let Err(why) = self.notify().await { + error!("update after context resolving failed: {why}") + } + } }, else => break } @@ -471,20 +476,22 @@ impl SpircTask { self.session.dealer().close().await; } - fn handle_next_context(&mut self, next_context: Result) { + fn handle_next_context(&mut self, next_context: Result) -> bool { let next_context = match next_context { Err(why) => { self.context_resolver.mark_next_unavailable(); self.context_resolver.remove_used_and_invalid(); error!("{why}"); - return; + return false; } Ok(ctx) => ctx, }; + debug!("handling next context {}", next_context.uri); + match self .context_resolver - .handle_next_context(&mut self.connect_state, next_context) + .apply_next_context(&mut self.connect_state, next_context) { Ok(remaining) => { if let Some(remaining) = remaining { @@ -496,15 +503,18 @@ impl SpircTask { } } - if self + let update_state = if self .context_resolver .try_finish(&mut self.connect_state, &mut self.transfer_state) { self.add_autoplay_resolving_when_required(); - self.update_state = true; - } + true + } else { + false + }; self.context_resolver.remove_used_and_invalid(); + update_state } // todo: is the time_delta still necessary? @@ -561,6 +571,7 @@ impl SpircTask { fn handle_player_event(&mut self, event: PlayerEvent) -> Result<(), Error> { if let PlayerEvent::TrackChanged { audio_item } = event { self.connect_state.update_duration(audio_item.duration_ms); + self.update_state = true; return Ok(()); } @@ -574,6 +585,7 @@ impl SpircTask { (event.get_play_request_id(), self.play_request_id), (Some(event_id), Some(current_id)) if event_id == current_id }; + // we only process events if the play_request_id matches. If it doesn't, it is // an event that belongs to a previous track and only arrives now due to a race // condition. In this case we have updated the state already and don't want to @@ -878,7 +890,8 @@ impl SpircTask { } // modification and update of the connect_state Transfer(transfer) => { - self.handle_transfer(transfer.data.expect("by condition checked"))? + self.handle_transfer(transfer.data.expect("by condition checked"))?; + return self.notify().await; } Play(play) => { let shuffle = play diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 27527c685..5b9621b0f 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -104,9 +104,6 @@ impl ConnectState { } pub fn reset_context(&mut self, mut reset_as: ResetContext) { - self.set_active_context(ContextType::Default); - self.fill_up_context = ContextType::Default; - if matches!(reset_as, ResetContext::WhenDifferent(ctx) if self.different_context_uri(ctx)) { reset_as = ResetContext::Completely } @@ -129,6 +126,8 @@ impl ConnectState { } } + self.fill_up_context = ContextType::Default; + self.set_active_context(ContextType::Default); self.update_restrictions() } @@ -149,6 +148,10 @@ impl ConnectState { pub fn set_active_context(&mut self, new_context: ContextType) { self.active_context = new_context; + let player = self.player_mut(); + player.context_metadata.clear(); + player.restrictions.clear(); + let ctx = match self.get_context(new_context) { Err(why) => { warn!("couldn't load context info because: {why}"); @@ -162,9 +165,6 @@ impl ConnectState { let player = self.player_mut(); - player.context_metadata.clear(); - player.restrictions.clear(); - if let Some(restrictions) = restrictions.take() { player.restrictions = MessageField::some(restrictions); } diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index 384087c77..777a29069 100644 --- a/connect/src/state/transfer.rs +++ b/connect/src/state/transfer.rs @@ -68,6 +68,7 @@ impl ConnectState { self.clear_prev_track(); self.clear_next_tracks(false); + self.update_queue_revision() } /// completes the transfer, loading the queue and updating metadata From e6fe2ac00eb0913117e775ab6800ab9e32e7ec9e Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 16 Dec 2024 23:24:53 +0100 Subject: [PATCH 18/30] revert removal of state setup --- connect/src/context_resolver.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index eb7ff3552..bc55907ba 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -341,6 +341,25 @@ impl ContextResolver { if let Err(why) = state.finish_transfer(transfer_state) { error!("finishing setup of transfer failed: {why}") } + } else { + let res = if state.shuffling_context() { + state.shuffle() + } else if let Ok(ctx) = state.get_context(state.active_context) { + let idx = ConnectState::find_index_in_context(ctx, |t| { + state.current_track(|c| t.uri == c.uri) + }) + .ok(); + + state + .reset_playback_to_position(idx) + .and_then(|_| state.fill_up_next_tracks()) + } else { + state.fill_up_next_tracks() + }; + + if let Err(why) = res { + error!("setting up state failed after updating contexts: {why}") + } } state.update_restrictions(); From f59b43b9c449f26358ffe43249aeca219d948848 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 17 Dec 2024 17:45:26 +0100 Subject: [PATCH 19/30] `clear_next_tracks` should never clear queued items just mimics official client behavior --- connect/src/spirc.rs | 4 ++-- connect/src/state.rs | 2 +- connect/src/state/context.rs | 2 +- connect/src/state/options.rs | 2 +- connect/src/state/tracks.rs | 7 +------ connect/src/state/transfer.rs | 2 +- 6 files changed, 7 insertions(+), 12 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 874a003ba..94681c62d 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1057,7 +1057,7 @@ impl SpircTask { fn handle_stop(&mut self) { self.player.stop(); self.connect_state.update_position(0, self.now_ms()); - self.connect_state.clear_next_tracks(true); + self.connect_state.clear_next_tracks(); if let Err(why) = self.connect_state.reset_playback_to_position(None) { warn!("failed filling up next_track during stopping: {why}") @@ -1136,7 +1136,7 @@ impl SpircTask { self.connect_state.merge_context(context); // load here, so that we clear the queue only after we definitely retrieved a new context - self.connect_state.clear_next_tracks(false); + self.connect_state.clear_next_tracks(); self.connect_state.clear_restrictions(); debug!("play track <{:?}>", cmd.playing_track); diff --git a/connect/src/state.rs b/connect/src/state.rs index 2c1bf9449..0d9e89b96 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -380,7 +380,7 @@ impl ConnectState { debug!("has {} prev tracks", self.prev_tracks().len()) } - self.clear_next_tracks(true); + self.clear_next_tracks(); self.fill_up_next_tracks()?; self.update_restrictions(); diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 5b9621b0f..b38045614 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -237,7 +237,7 @@ impl ConnectState { Err(_) => {} } // enforce reloading the context - self.clear_next_tracks(true); + self.clear_next_tracks(); } self.context = Some(new_context); diff --git a/connect/src/state/options.rs b/connect/src/state/options.rs index 12040d3d4..383573087 100644 --- a/connect/src/state/options.rs +++ b/connect/src/state/options.rs @@ -47,7 +47,7 @@ impl ConnectState { } self.clear_prev_track(); - self.clear_next_tracks(true); + self.clear_next_tracks(); let current_uri = self.current_track(|t| &t.uri); diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index 499446bb0..c3c26b708 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -260,12 +260,7 @@ impl<'ct> ConnectState { self.prev_tracks_mut().clear() } - pub fn clear_next_tracks(&mut self, keep_queued: bool) { - if !keep_queued { - self.next_tracks_mut().clear(); - return; - } - + pub fn clear_next_tracks(&mut self) { // respect queued track and don't throw them out of our next played tracks let first_non_queued_track = self .next_tracks() diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index 777a29069..098a9904b 100644 --- a/connect/src/state/transfer.rs +++ b/connect/src/state/transfer.rs @@ -67,7 +67,7 @@ impl ConnectState { } self.clear_prev_track(); - self.clear_next_tracks(false); + self.clear_next_tracks(); self.update_queue_revision() } From dfbac73136a1d805d20e32575d979382f168b515 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 17 Dec 2024 21:01:35 +0100 Subject: [PATCH 20/30] connect: make `playing_track` optional and handle it correctly --- connect/src/model.rs | 23 ++++++++++++++--------- connect/src/spirc.rs | 24 ++++++++++++++++-------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/connect/src/model.rs b/connect/src/model.rs index 63d48efe4..d73cadabc 100644 --- a/connect/src/model.rs +++ b/connect/src/model.rs @@ -9,7 +9,11 @@ pub struct SpircLoadCommand { pub shuffle: bool, pub repeat: bool, pub repeat_track: bool, - pub playing_track: PlayingTrack, + /// Decides the starting position in the given context + /// + /// ## Remarks: + /// If none is provided and shuffle true, a random track is played, otherwise the first + pub playing_track: Option, } #[derive(Debug)] @@ -19,19 +23,20 @@ pub enum PlayingTrack { Uid(String), } -impl From for PlayingTrack { - fn from(value: SkipTo) -> Self { +impl TryFrom for PlayingTrack { + type Error = (); + + fn try_from(value: SkipTo) -> Result { // order of checks is important, as the index can be 0, but still has an uid or uri provided, // so we only use the index as last resort if let Some(uri) = value.track_uri { - PlayingTrack::Uri(uri) + Ok(PlayingTrack::Uri(uri)) } else if let Some(uid) = value.track_uid { - PlayingTrack::Uid(uid) + Ok(PlayingTrack::Uid(uid)) + } else if let Some(index) = value.track_index { + Ok(PlayingTrack::Index(index)) } else { - PlayingTrack::Index(value.track_index.unwrap_or_else(|| { - warn!("SkipTo didn't provided any point to skip to, falling back to index 0"); - 0 - })) + Err(()) } } } diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 94681c62d..5b175e61e 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -918,7 +918,7 @@ impl SpircTask { context_uri: play.context.uri.clone(), start_playing: true, seek_to: play.options.seek_to.unwrap_or_default(), - playing_track: play.options.skip_to.into(), + playing_track: play.options.skip_to.try_into().ok(), shuffle, repeat, repeat_track, @@ -1141,16 +1141,21 @@ impl SpircTask { debug!("play track <{:?}>", cmd.playing_track); - let index = match cmd.playing_track { - PlayingTrack::Index(i) => i as usize, + let index = cmd.playing_track.map(|p| match p { + PlayingTrack::Index(i) => Ok(i as usize), PlayingTrack::Uri(uri) => { let ctx = self.connect_state.get_context(ContextType::Default)?; - ConnectState::find_index_in_context(ctx, |t| t.uri == uri)? + ConnectState::find_index_in_context(ctx, |t| t.uri == uri) } PlayingTrack::Uid(uid) => { let ctx = self.connect_state.get_context(ContextType::Default)?; - ConnectState::find_index_in_context(ctx, |t| t.uid == uid)? + ConnectState::find_index_in_context(ctx, |t| t.uid == uid) } + }); + + let index = match index { + Some(value) => Some(value?), + None => None, }; debug!( @@ -1163,7 +1168,9 @@ impl SpircTask { self.connect_state.set_repeat_track(cmd.repeat_track); if cmd.shuffle { - self.connect_state.set_current_track_random()?; + if index.is_none() { + self.connect_state.set_current_track_random()?; + } if self.context_resolver.has_next() { self.connect_state.update_queue_revision() @@ -1172,8 +1179,9 @@ impl SpircTask { self.add_autoplay_resolving_when_required(); } } else { - self.connect_state.set_current_track(index)?; - self.connect_state.reset_playback_to_position(Some(index))?; + self.connect_state + .set_current_track(index.unwrap_or_default())?; + self.connect_state.reset_playback_to_position(index)?; self.add_autoplay_resolving_when_required(); } From 0dfced80babbc617c1adf192f76235c3b3124479 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 17 Dec 2024 21:02:50 +0100 Subject: [PATCH 21/30] connect: adjust finish of context resolving --- connect/src/context_resolver.rs | 42 +++++++++--------- connect/src/state/context.rs | 79 ++++++++++++++++++++++++++------- 2 files changed, 84 insertions(+), 37 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index bc55907ba..d2f3c83da 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -337,29 +337,27 @@ impl ContextResolver { } } - if let Some(transfer_state) = transfer_state.take() { - if let Err(why) = state.finish_transfer(transfer_state) { - error!("finishing setup of transfer failed: {why}") - } + let active_ctx = state.get_context(state.active_context); + let res = if let Some(transfer_state) = transfer_state.take() { + state.finish_transfer(transfer_state) + } else if state.shuffling_context() { + state.shuffle() + } else if matches!(active_ctx, Ok(ctx) if ctx.index.track == 0) { + // has context, and context is not touched + // when the index is not zero, the next index was already evaluated elsewhere + let ctx = active_ctx.expect("checked by precondition"); + let idx = ConnectState::find_index_in_context(ctx, |t| { + state.current_track(|c| t.uri == c.uri) + }) + .ok(); + + state.reset_playback_to_position(idx) } else { - let res = if state.shuffling_context() { - state.shuffle() - } else if let Ok(ctx) = state.get_context(state.active_context) { - let idx = ConnectState::find_index_in_context(ctx, |t| { - state.current_track(|c| t.uri == c.uri) - }) - .ok(); - - state - .reset_playback_to_position(idx) - .and_then(|_| state.fill_up_next_tracks()) - } else { - state.fill_up_next_tracks() - }; - - if let Err(why) = res { - error!("setting up state failed after updating contexts: {why}") - } + state.fill_up_next_tracks() + }; + + if let Err(why) = res { + error!("setup of state failed: {why}, last used resolve {next:#?}") } state.update_restrictions(); diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index b38045614..8954d0301 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -3,7 +3,11 @@ use crate::{ protocol::player::{ Context, ContextIndex, ContextPage, ContextTrack, ProvidedTrack, Restrictions, }, - state::{metadata::Metadata, provider::Provider, ConnectState, StateError}, + state::{ + metadata::Metadata, + provider::{IsProvider, Provider}, + ConnectState, StateError, SPOTIFY_MAX_NEXT_TRACKS_SIZE, + }, }; use protobuf::MessageField; use std::collections::HashMap; @@ -222,22 +226,21 @@ impl ConnectState { if !self.context_uri().starts_with(SEARCH_IDENTIFIER) && self.context_uri() == &context.uri { - match Self::find_index_in_context(&new_context, |t| { - self.current_track(|t| &t.uri) == &t.uri - }) { - Ok(new_pos) => { - debug!("found new index of current track, updating new_context index to {new_pos}"); - new_context.index.track = (new_pos + 1) as u32; - } - // the track isn't anymore in the context - Err(_) if matches!(self.active_context, ContextType::Default) => { - warn!("current track was removed, setting pos to last known index"); - new_context.index.track = self.player().index.track + if let Some(new_index) = self.find_last_index_in_new_context(&new_context) { + new_context.index.track = match new_index { + Ok(i) => i, + Err(i) => { + self.player_mut().index = MessageField::none(); + i + } + }; + + // enforce reloading the context + if let Some(autoplay_ctx) = self.autoplay_context.as_mut() { + autoplay_ctx.index.track = 0 } - Err(_) => {} + self.clear_next_tracks(); } - // enforce reloading the context - self.clear_next_tracks(); } self.context = Some(new_context); @@ -283,6 +286,52 @@ impl ConnectState { Ok(Some(next_contexts)) } + fn find_first_prev_track_index(&self, ctx: &StateContext) -> Option { + let prev_tracks = self.prev_tracks(); + for i in (0..prev_tracks.len()).rev() { + let prev_track = prev_tracks.get(i)?; + if let Ok(idx) = Self::find_index_in_context(ctx, |t| prev_track.uri == t.uri) { + return Some(idx); + } + } + None + } + + fn find_last_index_in_new_context( + &self, + new_context: &StateContext, + ) -> Option> { + let ctx = self.context.as_ref()?; + + let is_queued_item = self.current_track(|t| t.is_queue() || t.is_from_queue()); + + let new_index = if ctx.index.track as usize >= SPOTIFY_MAX_NEXT_TRACKS_SIZE { + Some(ctx.index.track as usize - SPOTIFY_MAX_NEXT_TRACKS_SIZE) + } else if is_queued_item { + self.find_first_prev_track_index(new_context) + } else { + Self::find_index_in_context(new_context, |current| { + self.current_track(|t| t.uri == current.uri) + }) + .ok() + } + .map(|i| i as u32 + 1); + + Some(new_index.ok_or_else(|| { + info!( + "couldn't distinguish index from current or previous tracks in the updated context" + ); + let fallback_index = self + .player() + .index + .as_ref() + .map(|i| i.track) + .unwrap_or_default(); + info!("falling back to index {fallback_index}"); + fallback_index + })) + } + fn state_context_from_page( &mut self, page: ContextPage, From fbe1657040e7426874fce5521bda7484d7b2b9a9 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 19 Dec 2024 16:41:32 +0100 Subject: [PATCH 22/30] connect: set track position when shuffling --- connect/src/spirc.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 79220530b..c2c3d06b7 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1148,21 +1148,19 @@ impl SpircTask { debug!("play track <{:?}>", cmd.playing_track); - let index = cmd.playing_track.map(|p| match p { - PlayingTrack::Index(i) => Ok(i as usize), - PlayingTrack::Uri(uri) => { - let ctx = self.connect_state.get_context(ContextType::Default)?; - ConnectState::find_index_in_context(ctx, |t| t.uri == uri) - } - PlayingTrack::Uid(uid) => { - let ctx = self.connect_state.get_context(ContextType::Default)?; - ConnectState::find_index_in_context(ctx, |t| t.uid == uid) - } - }); - - let index = match index { - Some(value) => Some(value?), + let index = match cmd.playing_track { None => None, + Some(playing_track) => Some(match playing_track { + PlayingTrack::Index(i) => i as usize, + PlayingTrack::Uri(uri) => { + let ctx = self.connect_state.get_context(ContextType::Default)?; + ConnectState::find_index_in_context(ctx, |t| t.uri == uri)? + } + PlayingTrack::Uid(uid) => { + let ctx = self.connect_state.get_context(ContextType::Default)?; + ConnectState::find_index_in_context(ctx, |t| t.uid == uid)? + } + }), }; debug!( @@ -1175,7 +1173,9 @@ impl SpircTask { self.connect_state.set_repeat_track(cmd.repeat_track); if cmd.shuffle { - if index.is_none() { + if let Some(index) = index { + self.connect_state.set_current_track(index)?; + } else { self.connect_state.set_current_track_random()?; } From 67ceecda882418d765dedb21eca91fd1d608ea65 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 19 Dec 2024 17:21:01 +0100 Subject: [PATCH 23/30] example: adjust to model change --- examples/play_connect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/play_connect.rs b/examples/play_connect.rs index 9a033da23..60cf631fb 100644 --- a/examples/play_connect.rs +++ b/examples/play_connect.rs @@ -84,7 +84,7 @@ async fn main() { repeat: false, repeat_track: false, // the index specifies which track in the context starts playing, in this case the first in the album - playing_track: PlayingTrack::Index(0), + playing_track: PlayingTrack::Index(0).into(), }) .unwrap(); }); From 7d7e7861bdd9e8f3ee9a566f2f58b77b32eaad41 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 19 Dec 2024 18:00:15 +0100 Subject: [PATCH 24/30] connect: remove duplicate track --- connect/src/state/context.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 8954d0301..994dbcf97 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -185,9 +185,9 @@ impl ConnectState { ) -> Result>, Error> { if context.pages.iter().all(|p| p.tracks.is_empty()) { error!("context didn't have any tracks: {context:#?}"); - return Err(StateError::ContextHasNoTracks.into()); + Err(StateError::ContextHasNoTracks)?; } else if context.uri.starts_with(LOCAL_FILES_IDENTIFIER) { - return Err(StateError::UnsupportedLocalPlayBack.into()); + Err(StateError::UnsupportedLocalPlayBack)?; } let mut next_contexts = Vec::new(); @@ -200,7 +200,7 @@ impl ConnectState { } } - let page = match first_page { + let mut page = match first_page { None => Err(StateError::ContextHasNoTracks)?, Some(p) => p, }; @@ -245,7 +245,7 @@ impl ConnectState { self.context = Some(new_context); - if !context.url.starts_with(SEARCH_IDENTIFIER) { + if !context.url.contains(SEARCH_IDENTIFIER) { self.player_mut().context_url = context.url; } else { self.player_mut().context_url.clear() @@ -253,6 +253,17 @@ impl ConnectState { self.player_mut().context_uri = context.uri; } UpdateContext::Autoplay => { + if matches!(self.context.as_ref(), Some(ctx) if ctx.tracks.len() == 1) { + if let Some(position) = page + .tracks + .iter() + .position(|p| self.current_track(|t| t.uri == p.uri)) + { + debug!("removing track (of single track context) from autoplay context"); + page.tracks.remove(position); + } + } + self.autoplay_context = Some(self.state_context_from_page( page, context.metadata, From 309ca8de86016b35f9499b95066dd3ee6540058f Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Thu, 19 Dec 2024 20:33:55 +0100 Subject: [PATCH 25/30] connect: provide all recently played tracks to autoplay request - removes previously added workaround --- connect/src/spirc.rs | 5 ++--- connect/src/state/context.rs | 13 +------------ connect/src/state/tracks.rs | 9 +++------ 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index c2c3d06b7..adc24d226 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -449,7 +449,7 @@ impl SpircTask { // finish after we received our last item of a type next_context = async { self.context_resolver.get_next_context(|| { - self.connect_state.prev_autoplay_track_uris() + self.connect_state.recent_track_uris() }).await }, if allow_context_resolving && self.context_resolver.has_next() => { let update_state = self.handle_next_context(next_context); @@ -1105,8 +1105,6 @@ impl SpircTask { cmd: SpircLoadCommand, context: Option, ) -> Result<(), Error> { - self.context_resolver.clear(); - self.connect_state .reset_context(ResetContext::WhenDifferent(&cmd.context_uri)); @@ -1128,6 +1126,7 @@ impl SpircTask { debug!("context <{current_context_uri}> didn't change, no resolving required") } else { debug!("resolving context for load command"); + self.context_resolver.clear(); self.context_resolver.add(ResolveContext::from_uri( &cmd.context_uri, fallback, diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 994dbcf97..4ac9b3236 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -200,7 +200,7 @@ impl ConnectState { } } - let mut page = match first_page { + let page = match first_page { None => Err(StateError::ContextHasNoTracks)?, Some(p) => p, }; @@ -253,17 +253,6 @@ impl ConnectState { self.player_mut().context_uri = context.uri; } UpdateContext::Autoplay => { - if matches!(self.context.as_ref(), Some(ctx) if ctx.tracks.len() == 1) { - if let Some(position) = page - .tracks - .iter() - .position(|p| self.current_track(|t| t.uri == p.uri)) - { - debug!("removing track (of single track context) from autoplay context"); - page.tracks.remove(position); - } - } - self.autoplay_context = Some(self.state_context_from_page( page, context.metadata, diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index c3c26b708..14e3abcc2 100644 --- a/connect/src/state/tracks.rs +++ b/connect/src/state/tracks.rs @@ -359,17 +359,14 @@ impl<'ct> ConnectState { } } - pub fn prev_autoplay_track_uris(&self) -> Vec { + pub fn recent_track_uris(&self) -> Vec { let mut prev = self .prev_tracks() .iter() - .flat_map(|t| t.is_autoplay().then_some(t.uri.clone())) + .map(|t| t.uri.clone()) .collect::>(); - if self.current_track(|t| t.is_autoplay()) { - prev.push(self.current_track(|t| t.uri.clone())); - } - + prev.push(self.current_track(|t| t.uri.clone())); prev } From 3cf4197002151520c2aad6555d047b3e1ad7375a Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 23 Dec 2024 15:07:32 +0100 Subject: [PATCH 26/30] connect: apply review suggestions - use drain instead of for with pop - use for instead of loop - use or_else instead of match - use Self::Error instead of the value - free memory for metadata and restrictions --- connect/src/context_resolver.rs | 17 +++++++---------- connect/src/model.rs | 2 +- connect/src/state/context.rs | 14 +++++++------- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index d2f3c83da..22673741d 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -198,11 +198,9 @@ impl ContextResolver { pub fn remove_used_and_invalid(&mut self) { if let Some((_, _, remove)) = self.find_next() { - for _ in 0..remove { - let _ = self.queue.pop_front(); - } + let _ = self.queue.drain(0..remove); // remove invalid } - self.queue.pop_front(); + self.queue.pop_front(); // remove used } pub fn clear(&mut self) { @@ -210,18 +208,17 @@ impl ContextResolver { } fn find_next(&self) -> Option<(&ResolveContext, &str, usize)> { - let mut idx = 0; - loop { + for idx in 0..self.queue.len() { let next = self.queue.get(idx)?; match next.resolve_uri() { - None if idx < self.queue.len() => { - warn!("skipped {idx} because of no valid resolve_uri: {next}"); - idx += 1; + None => { + warn!("skipped {idx} because of invalid resolve_uri: {next}"); continue; } - value => break value.map(|uri| (next, uri, idx)), + Some(uri) => return Some((next, uri, idx)), } } + None } pub fn has_next(&self) -> bool { diff --git a/connect/src/model.rs b/connect/src/model.rs index d73cadabc..2fe79547b 100644 --- a/connect/src/model.rs +++ b/connect/src/model.rs @@ -26,7 +26,7 @@ pub enum PlayingTrack { impl TryFrom for PlayingTrack { type Error = (); - fn try_from(value: SkipTo) -> Result { + fn try_from(value: SkipTo) -> Result { // order of checks is important, as the index can be 0, but still has an uid or uri provided, // so we only use the index as last resort if let Some(uri) = value.track_uri { diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index 4ac9b3236..b6276bc1d 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -140,21 +140,21 @@ impl ConnectState { } pub fn get_context_uri_from_context(context: &Context) -> Option<&str> { - match Self::valid_resolve_uri(&context.uri) { - Some(uri) => Some(uri), - None => context + Self::valid_resolve_uri(&context.uri).or_else(|| { + context .pages .first() - .and_then(|p| p.tracks.first().map(|t| t.uri.as_ref())), - } + .and_then(|p| p.tracks.first().map(|t| t.uri.as_ref())) + }) } pub fn set_active_context(&mut self, new_context: ContextType) { self.active_context = new_context; let player = self.player_mut(); - player.context_metadata.clear(); - player.restrictions.clear(); + + player.context_metadata = Default::default(); + player.restrictions = Some(Default::default()).into(); let ctx = match self.get_context(new_context) { Err(why) => { From eb14e46d737c7c983d730344edf12afebc749a79 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 23 Dec 2024 15:35:42 +0100 Subject: [PATCH 27/30] connect: impl trait for player context --- connect/src/context_resolver.rs | 34 ++++++----------------- connect/src/state/context.rs | 2 +- protocol/src/impl_trait.rs | 1 + protocol/src/impl_trait/player_context.rs | 13 +++++++++ protocol/src/lib.rs | 2 ++ 5 files changed, 25 insertions(+), 27 deletions(-) create mode 100644 protocol/src/impl_trait.rs create mode 100644 protocol/src/impl_trait/player_context.rs diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index 22673741d..982a51b5f 100644 --- a/connect/src/context_resolver.rs +++ b/connect/src/context_resolver.rs @@ -1,35 +1,37 @@ -use crate::state::context::ContextType; use crate::{ core::{Error, Session}, protocol::{ autoplay_context_request::AutoplayContextRequest, player::{Context, TransferState}, }, - state::{context::UpdateContext, ConnectState}, + state::{ + context::{ContextType, UpdateContext}, + ConnectState, + }, }; use std::cmp::PartialEq; use std::{ collections::{HashMap, VecDeque}, fmt::{Display, Formatter}, - hash::{Hash, Hasher}, + hash::Hash, time::Duration, }; use thiserror::Error as ThisError; use tokio::time::Instant; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Hash, PartialEq, Eq)] enum Resolve { Uri(String), Context(Context), } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Hash, PartialEq, Eq)] pub(super) enum ContextAction { Append, Replace, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Hash, PartialEq, Eq)] pub(super) struct ResolveContext { resolve: Resolve, fallback: Option, @@ -103,26 +105,6 @@ impl Display for ResolveContext { } } -impl PartialEq for ResolveContext { - fn eq(&self, other: &Self) -> bool { - let eq_context = self.context_uri() == other.context_uri(); - let eq_resolve = self.resolve_uri() == other.resolve_uri(); - let eq_autoplay = self.update == other.update; - - eq_context && eq_resolve && eq_autoplay - } -} - -impl Eq for ResolveContext {} - -impl Hash for ResolveContext { - fn hash(&self, state: &mut H) { - self.context_uri().hash(state); - self.resolve_uri().hash(state); - self.update.hash(state); - } -} - #[derive(Debug, ThisError)] enum ContextResolverError { #[error("no next context to resolve")] diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index b6276bc1d..57f92db86 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -34,7 +34,7 @@ pub enum ContextType { Autoplay, } -#[derive(Debug, PartialEq, Hash, Copy, Clone)] +#[derive(Debug, Hash, Copy, Clone, PartialEq, Eq)] pub enum UpdateContext { Default, Autoplay, diff --git a/protocol/src/impl_trait.rs b/protocol/src/impl_trait.rs new file mode 100644 index 000000000..197ffdad1 --- /dev/null +++ b/protocol/src/impl_trait.rs @@ -0,0 +1 @@ +mod player_context; diff --git a/protocol/src/impl_trait/player_context.rs b/protocol/src/impl_trait/player_context.rs new file mode 100644 index 000000000..5886b81ca --- /dev/null +++ b/protocol/src/impl_trait/player_context.rs @@ -0,0 +1,13 @@ +use crate::player::Context; +use protobuf::Message; +use std::hash::{Hash, Hasher}; + +impl Hash for Context { + fn hash(&self, state: &mut H) { + if let Ok(ctx) = self.write_to_bytes() { + ctx.hash(state) + } + } +} + +impl Eq for Context {} diff --git a/protocol/src/lib.rs b/protocol/src/lib.rs index 224043e76..c905ceb87 100644 --- a/protocol/src/lib.rs +++ b/protocol/src/lib.rs @@ -1,4 +1,6 @@ // This file is parsed by build.rs // Each included module will be compiled from the matching .proto definition. +mod impl_trait; + include!(concat!(env!("OUT_DIR"), "/mod.rs")); From acc2b39d322105380a78eb488ce5d2fa5285259c Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Sun, 22 Dec 2024 15:51:49 +0100 Subject: [PATCH 28/30] connect: fix incorrect playing and paused --- connect/src/spirc.rs | 6 +++--- connect/src/state.rs | 16 ++++++++++++++++ connect/src/state/restrictions.rs | 8 ++++++-- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index adc24d226..53a362c8e 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1350,7 +1350,7 @@ impl SpircTask { } fn handle_next(&mut self, track_uri: Option) -> Result<(), Error> { - let continue_playing = self.connect_state.player().is_playing; + let continue_playing = self.connect_state.is_playing(); let current_uri = self.connect_state.current_track(|t| &t.uri); let mut has_next_track = @@ -1391,7 +1391,7 @@ impl SpircTask { self.connect_state.reset_playback_to_position(None)?; self.handle_stop() } - Some(_) => self.load_track(self.connect_state.player().is_playing, 0)?, + Some(_) => self.load_track(self.connect_state.is_playing(), 0)?, } } else { self.handle_seek(0); @@ -1515,7 +1515,7 @@ impl SpircTask { async fn notify(&mut self) -> Result<(), Error> { self.connect_state.set_status(&self.play_status); - if self.connect_state.player().is_playing { + if self.connect_state.is_playing() { self.connect_state .update_position_in_relation(self.now_ms()); } diff --git a/connect/src/state.rs b/connect/src/state.rs index 0d9e89b96..60691534b 100644 --- a/connect/src/state.rs +++ b/connect/src/state.rs @@ -233,6 +233,22 @@ impl ConnectState { self.request.is_active } + /// Returns the `is_playing` value as perceived by other connect devices + /// + /// see [ConnectState::set_status] + pub fn is_playing(&self) -> bool { + let player = self.player(); + player.is_playing && !player.is_paused + } + + /// Returns the `is_paused` state value as perceived by other connect devices + /// + /// see [ConnectState::set_status] + pub fn is_pause(&self) -> bool { + let player = self.player(); + player.is_playing && player.is_paused && player.is_buffering + } + pub fn set_volume(&mut self, volume: u32) { self.device_mut() .device_info diff --git a/connect/src/state/restrictions.rs b/connect/src/state/restrictions.rs index a0f269331..03495c680 100644 --- a/connect/src/state/restrictions.rs +++ b/connect/src/state/restrictions.rs @@ -17,14 +17,18 @@ impl ConnectState { const ENDLESS_CONTEXT: &str = "endless_context"; let prev_tracks_is_empty = self.prev_tracks().is_empty(); + + let is_paused = self.is_pause(); + let is_playing = self.is_playing(); + let player = self.player_mut(); if let Some(restrictions) = player.restrictions.as_mut() { - if player.is_playing { + if is_playing { restrictions.disallow_pausing_reasons.clear(); restrictions.disallow_resuming_reasons = vec!["not_paused".to_string()] } - if player.is_paused { + if is_paused { restrictions.disallow_resuming_reasons.clear(); restrictions.disallow_pausing_reasons = vec!["not_playing".to_string()] } From 771f9e9f3c9e39bd962693124290a5b60e75ab7b Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Mon, 23 Dec 2024 17:17:20 +0100 Subject: [PATCH 29/30] connect: apply options as official clients --- connect/src/spirc.rs | 8 +++++--- connect/src/state/options.rs | 6 ++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 53a362c8e..85c4b5872 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -901,19 +901,19 @@ impl SpircTask { .player_options_override .as_ref() .map(|o| o.shuffling_context) - .unwrap_or_else(|| self.connect_state.shuffling_context()); + .unwrap_or_default(); let repeat = play .options .player_options_override .as_ref() .map(|o| o.repeating_context) - .unwrap_or_else(|| self.connect_state.repeat_context()); + .unwrap_or_default(); let repeat_track = play .options .player_options_override .as_ref() .map(|o| o.repeating_track) - .unwrap_or_else(|| self.connect_state.repeat_track()); + .unwrap_or_default(); self.handle_load( SpircLoadCommand { @@ -1108,6 +1108,8 @@ impl SpircTask { self.connect_state .reset_context(ResetContext::WhenDifferent(&cmd.context_uri)); + self.connect_state.reset_options(); + if !self.connect_state.is_active() { self.handle_activate(); } diff --git a/connect/src/state/options.rs b/connect/src/state/options.rs index 383573087..18d10e7dc 100644 --- a/connect/src/state/options.rs +++ b/connect/src/state/options.rs @@ -33,6 +33,12 @@ impl ConnectState { } } + pub fn reset_options(&mut self) { + self.set_shuffle(false); + self.set_repeat_track(false); + self.set_repeat_context(false); + } + pub fn shuffle(&mut self) -> Result<(), Error> { if let Some(reason) = self .player() From ba7fed39a40b83062ed40b50e06e6bc7a6fe5a21 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Tue, 24 Dec 2024 12:04:29 +0100 Subject: [PATCH 30/30] protocol: move trait impls into impl_trait mod --- protocol/src/impl_trait.rs | 3 ++- protocol/src/impl_trait/{player_context.rs => context.rs} | 0 protocol/src/{conversion.rs => impl_trait/player.rs} | 0 protocol/src/lib.rs | 2 -- 4 files changed, 2 insertions(+), 3 deletions(-) rename protocol/src/impl_trait/{player_context.rs => context.rs} (100%) rename protocol/src/{conversion.rs => impl_trait/player.rs} (100%) diff --git a/protocol/src/impl_trait.rs b/protocol/src/impl_trait.rs index 197ffdad1..c936a5f0b 100644 --- a/protocol/src/impl_trait.rs +++ b/protocol/src/impl_trait.rs @@ -1 +1,2 @@ -mod player_context; +mod context; +mod player; diff --git a/protocol/src/impl_trait/player_context.rs b/protocol/src/impl_trait/context.rs similarity index 100% rename from protocol/src/impl_trait/player_context.rs rename to protocol/src/impl_trait/context.rs diff --git a/protocol/src/conversion.rs b/protocol/src/impl_trait/player.rs similarity index 100% rename from protocol/src/conversion.rs rename to protocol/src/impl_trait/player.rs diff --git a/protocol/src/lib.rs b/protocol/src/lib.rs index 84414e365..c905ceb87 100644 --- a/protocol/src/lib.rs +++ b/protocol/src/lib.rs @@ -3,6 +3,4 @@ mod impl_trait; -mod conversion; - include!(concat!(env!("OUT_DIR"), "/mod.rs"));