Fix: Harden the transfer flow for unexpected data (#1581)

* fix: transfer data has an invalid uri and no tracks
* fix: play/pause button being disabled on transfer

Author: photovoltex

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

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::<Vec<_>>(),
-            )?
-        }
+                    .collect::<Vec<_>>();
 
-        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<ContextPage>) -> 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()
         };

connect/src/state/transfer.rs

@@ -17,8 +17,10 @@ impl ConnectState {
         transfer: &TransferState,
     ) -> Result<ProvidedTrack, Error> {
         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<String>,
+    ) {
         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 {