diff --git a/CHANGELOG.md b/CHANGELOG.md index 92045b9ce..aa906e98b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,14 +16,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- [playback] Changed type of `SpotifyId` fields in `PlayerEvent` members to `SpotifyUri` (breaking) - [metadata] Changed arguments for `Metadata` trait from `&SpotifyId` to `&SpotifyUri` (breaking) -- [player] `load` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) -- [player] `preload` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) -- [spclient] `get_radio_for_track` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) +- [playback] Changed type of `SpotifyId` fields in `PlayerEvent` members to `SpotifyUri` (breaking) +- [playback] `load` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) +- [playback] `preload` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) +- [core] `get_radio_for_track` function changed from accepting a `SpotifyId` to accepting a `SpotifyUri` (breaking) ### Fixed +- [connect] Fixed failed transferring with transfer data that had an empty context uri and no tracks - [connect] Use the provided index or the first as fallback value to always play a track on loading ### Removed diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index c05988a82..429e66950 100644 --- a/connect/src/spirc.rs +++ b/connect/src/spirc.rs @@ -1087,13 +1087,17 @@ impl SpircTask { fn handle_transfer(&mut self, mut transfer: TransferState) -> Result<(), Error> { let mut ctx_uri = match transfer.current_session.context.uri { None => Err(SpircError::NoUri("transfer context"))?, - // can apparently happen when a state is transferred stared with "uris" via the api - Some(ref uri) if uri == "-" => String::new(), - Some(ref uri) => uri.clone(), + // can apparently happen when a state is transferred and was started with "uris" via the api + Some(ref uri) if uri == "-" || uri.is_empty() => None, + Some(ref uri) => Some(uri.clone()), }; - self.connect_state - .reset_context(ResetContext::WhenDifferent(&ctx_uri)); + self.connect_state.reset_context( + ctx_uri + .as_deref() + .map(ResetContext::WhenDifferent) + .unwrap_or(ResetContext::Completely), + ); match self.connect_state.current_track_from_transfer(&transfer) { Err(why) => warn!("didn't find initial track: {why}"), @@ -1105,44 +1109,47 @@ impl SpircTask { let autoplay = self.connect_state.current_track(|t| t.is_autoplay()); if autoplay { - ctx_uri = ctx_uri.replace("station:", ""); + ctx_uri = ctx_uri.map(|c| c.replace("station:", "")); } let fallback = self.connect_state.current_track(|t| &t.uri).clone(); - let load_from_context_uri = !ctx_uri.is_empty(); - - if load_from_context_uri { - self.context_resolver.add(ResolveContext::from_uri( - ctx_uri.clone(), - &fallback, - ContextType::Default, - ContextAction::Replace, - )); - } else { - self.load_context_from_tracks( - transfer + let load_from_context_uri = ctx_uri.is_some(); + + match ctx_uri { + Some(ref uri) => { + self.context_resolver.add(ResolveContext::from_uri( + uri.clone(), + &fallback, + ContextType::Default, + ContextAction::Replace, + )); + } + None => { + let all_tracks = transfer .current_session .context .pages .iter() .cloned() .flat_map(|p| p.tracks) - .collect::>(), - )? - } + .collect::>(); - self.context_resolver.add(ResolveContext::from_uri( - ctx_uri.clone(), - &fallback, - ContextType::Default, - ContextAction::Replace, - )); + if !all_tracks.is_empty() { + self.load_context_from_tracks(all_tracks)?; + } else { + warn!( + "tried to transfer with an invalid state, using fallback as ctx_uri ({fallback})" + ); + ctx_uri = Some(fallback.clone()) + } + } + }; self.handle_activate(); let timestamp = self.now_ms(); let state = &mut self.connect_state; - state.handle_initial_transfer(&mut transfer); + state.handle_initial_transfer(&mut transfer, ctx_uri.clone()); // adjust active context, so resolve knows for which context it should set up the state state.active_context = if autoplay { @@ -1166,24 +1173,34 @@ impl SpircTask { let is_playing = !transfer.playback.is_paused(); if self.connect_state.current_track(|t| t.is_autoplay()) || autoplay { - debug!("currently in autoplay context, async resolving autoplay for {ctx_uri}"); - - self.context_resolver.add(ResolveContext::from_uri( - ctx_uri, - fallback, - ContextType::Autoplay, - ContextAction::Replace, - )) + if let Some(ctx_uri) = ctx_uri { + debug!("currently in autoplay context, async resolving autoplay for {ctx_uri}"); + self.context_resolver.add(ResolveContext::from_uri( + ctx_uri, + fallback, + ContextType::Autoplay, + ContextAction::Replace, + )) + } else { + warn!("couldn't resolve autoplay context without a context uri"); + } } if load_from_context_uri { self.transfer_state = Some(transfer); } else { - let ctx = self.connect_state.get_context(ContextType::Default)?; - let idx = ConnectState::find_index_in_context(ctx, |pt| { - self.connect_state.current_track(|t| pt.uri == t.uri) - })?; - self.connect_state.reset_playback_to_position(Some(idx))?; + match self.connect_state.get_context(ContextType::Default) { + Err(why) => { + warn!("continuing transfer in an unknown state. {why}"); + self.transfer_state = Some(transfer); + } + Ok(ctx) => { + let idx = ConnectState::find_index_in_context(ctx, |pt| { + self.connect_state.current_track(|t| pt.uri == t.uri) + })?; + self.connect_state.reset_playback_to_position(Some(idx))?; + } + } } self.load_track(is_playing, position.try_into()?) @@ -1397,7 +1414,11 @@ impl SpircTask { } fn load_context_from_tracks(&mut self, tracks: impl Into) -> Result<(), Error> { + const WEB_API_URI: &str = "spotify:web-api"; let ctx = Context { + // by providing values for uri/url the player in the official client's isn't frozen + uri: Some(WEB_API_URI.into()), + url: Some(format!("context://{WEB_API_URI}")), pages: vec![tracks.into()], ..Default::default() }; diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index 8fdb21fa8..e91ec5fb9 100644 --- a/connect/src/state/transfer.rs +++ b/connect/src/state/transfer.rs @@ -17,8 +17,10 @@ impl ConnectState { transfer: &TransferState, ) -> Result { let track = if transfer.queue.is_playing_queue.unwrap_or_default() { + debug!("transfer track was used from the queue"); transfer.queue.tracks.first() } else { + debug!("transfer track was the current track"); transfer.playback.current_track.as_ref() } .ok_or(StateError::CouldNotResolveTrackFromTransfer)?; @@ -37,7 +39,11 @@ impl ConnectState { } /// handles the initially transferable data - pub fn handle_initial_transfer(&mut self, transfer: &mut TransferState) { + pub fn handle_initial_transfer( + &mut self, + transfer: &mut TransferState, + ctx_uri: Option, + ) { let current_context_metadata = self.context.as_ref().map(|c| c.metadata.clone()); let player = self.player_mut(); @@ -84,8 +90,13 @@ impl ConnectState { } } - player.context_url.clear(); - player.context_uri.clear(); + const UNKNOWN_URI: &str = "spotify:unknown"; + // it's important to always set the url/uri to a value + // so that the player doesn't go into an inactive state + let uri = ctx_uri.unwrap_or(UNKNOWN_URI.into()); + + player.context_url = format!("context://{uri}"); + player.context_uri = uri; if let Some(metadata) = current_context_metadata { for (key, value) in metadata {