connect: rework aftermath

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 {

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.");

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

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()

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;