Adjust: Allow repeat in combination with shuffle (#1561)

* fix: incorrect autoplay resolver behavior when shuffling

* refactor: store the initial track in the remote context

* adjust: shuffle repeat interaction

* chore: update .gitignore

* chore: rename internal error

* adjust: shuffle behavior to ensure consistency

* fix: prefer repeat context over autoplay

* chore: update changelog

* chore: reduce complexity of shuffle

* chore: test shuffle with first

.gitignore

@@ -1,6 +1,7 @@
 target
 .cargo
 spotify_appkey.key
+.idea/
 .vagrant/
 .project
 .history

CHANGELOG.md

@@ -11,14 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- [connect] Shuffling was adjusted, so that shuffle and repeat can be used combined
+
 ### Deprecated
 
 ### Removed
 
 ### Fixed
 
+- [connect] Repeat context will not go into autoplay anymore and triggering autoplay while shuffling shouldn't reshuffle anymore 
 - [connect] Only deletes the connect state on dealer shutdown instead on disconnecting
-- [core] Fixed a problem where in `spclient` where a http 411 error was thrown because the header were set wrong 
+- [core] Fixed a problem where in `spclient` where an http 411 error was thrown because the header were set wrong
 - [main] Use the config instead of the type default for values that are not provided by the user
 
 

connect/src/context_resolver.rs

@@ -318,8 +318,8 @@ impl ContextResolver {
         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(None)
+        } else if state.shuffling_context() && next.update == ContextType::Default {
+            state.shuffle_new()
         } 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

connect/src/shuffle_vec.rs

@@ -8,6 +8,12 @@ use std::{
 pub struct ShuffleVec<T> {
     vec: Vec<T>,
     indices: Option<Vec<usize>>,
+    /// This is primarily necessary to ensure that shuffle does not behave out of place.
+    ///
+    /// For that reason we swap the first track with the currently playing track. By that we ensure
+    /// that the shuffle state is consistent between resets of the state because the first track is
+    /// always the track with which we started playing when switching to shuffle.
+    original_first_position: Option<usize>,
 }
 
 impl<T: PartialEq> PartialEq for ShuffleVec<T> {
@@ -41,16 +47,25 @@ impl<T> IntoIterator for ShuffleVec<T> {
 
 impl<T> From<Vec<T>> for ShuffleVec<T> {
     fn from(vec: Vec<T>) -> Self {
-        Self { vec, indices: None }
+        Self {
+            vec,
+            original_first_position: None,
+            indices: None,
+        }
     }
 }
 
 impl<T> ShuffleVec<T> {
-    pub fn shuffle_with_seed(&mut self, seed: u64) {
-        self.shuffle_with_rng(SmallRng::seed_from_u64(seed))
+    pub fn shuffle_with_seed<F: Fn(&T) -> bool>(&mut self, seed: u64, is_first: F) {
+        self.shuffle_with_rng(SmallRng::seed_from_u64(seed), is_first)
+    }
+
+    pub fn shuffle_with_rng<F: Fn(&T) -> bool>(&mut self, mut rng: impl Rng, is_first: F) {
+        if self.vec.len() <= 1 {
+            info!("skipped shuffling for less or equal one item");
+            return;
         }
 
-    pub fn shuffle_with_rng(&mut self, mut rng: impl Rng) {
         if self.indices.is_some() {
             self.unshuffle()
         }
@@ -66,7 +81,12 @@ impl<T> ShuffleVec<T> {
             self.vec.swap(i, rnd_ind);
         }
 
-        self.indices = Some(indices)
+        self.indices = Some(indices);
+
+        self.original_first_position = self.vec.iter().position(is_first);
+        if let Some(first_pos) = self.original_first_position {
+            self.vec.swap(0, first_pos)
+        }
     }
 
     pub fn unshuffle(&mut self) {
@@ -75,9 +95,16 @@ impl<T> ShuffleVec<T> {
             None => return,
         };
 
+        if let Some(first_pos) = self.original_first_position {
+            self.vec.swap(0, first_pos);
+            self.original_first_position = None;
+        }
+
         for i in 1..self.vec.len() {
-            let n = indices[self.vec.len() - i - 1];
-            self.vec.swap(n, i);
+            match indices.get(self.vec.len() - i - 1) {
+                None => return,
+                Some(n) => self.vec.swap(*n, i),
+            }
         }
     }
 }
@@ -86,25 +113,86 @@ impl<T> ShuffleVec<T> {
 mod test {
     use super::*;
     use rand::Rng;
+    use std::ops::Range;
+
+    fn base(range: Range<usize>) -> (ShuffleVec<usize>, u64) {
+        let seed = rand::rng().random_range(0..10_000_000_000_000);
+
+        let vec = range.collect::<Vec<_>>();
+        (vec.into(), seed)
+    }
 
     #[test]
-    fn test_shuffle_with_seed() {
-        let seed = rand::rng().random_range(0..10000000000000);
-
-        let vec = (0..100).collect::<Vec<_>>();
-        let base_vec: ShuffleVec<i32> = vec.into();
+    fn test_shuffle_without_first() {
+        let (base_vec, seed) = base(0..100);
 
         let mut shuffled_vec = base_vec.clone();
-        shuffled_vec.shuffle_with_seed(seed);
+        shuffled_vec.shuffle_with_seed(seed, |_| false);
 
         let mut different_shuffled_vec = base_vec.clone();
-        different_shuffled_vec.shuffle_with_seed(seed);
+        different_shuffled_vec.shuffle_with_seed(seed, |_| false);
 
-        assert_eq!(shuffled_vec, different_shuffled_vec);
+        assert_eq!(
+            shuffled_vec, different_shuffled_vec,
+            "shuffling with the same seed has the same result"
+        );
 
         let mut unshuffled_vec = shuffled_vec.clone();
         unshuffled_vec.unshuffle();
 
-        assert_eq!(base_vec, unshuffled_vec);
+        assert_eq!(
+            base_vec, unshuffled_vec,
+            "unshuffle restores the original state"
+        );
+    }
+
+    #[test]
+    fn test_shuffle_with_first() {
+        const MAX_RANGE: usize = 200;
+
+        let (base_vec, seed) = base(0..MAX_RANGE);
+        let rand_first = rand::rng().random_range(0..MAX_RANGE);
+
+        let mut shuffled_with_first = base_vec.clone();
+        shuffled_with_first.shuffle_with_seed(seed, |i| i == &rand_first);
+
+        assert_eq!(
+            Some(&rand_first),
+            shuffled_with_first.first(),
+            "after shuffling the first is expected to be the given item"
+        );
+
+        let mut shuffled_without_first = base_vec.clone();
+        shuffled_without_first.shuffle_with_seed(seed, |_| false);
+
+        let mut switched_positions = Vec::with_capacity(2);
+        for (i, without_first_value) in shuffled_without_first.iter().enumerate() {
+            if without_first_value != &shuffled_with_first[i] {
+                switched_positions.push(i);
+            } else {
+                assert_eq!(
+                    without_first_value, &shuffled_with_first[i],
+                    "shuffling with the same seed has the same result"
+                );
+            }
+        }
+
+        assert_eq!(
+            switched_positions.len(),
+            2,
+            "only the switched positions should be different"
+        );
+
+        assert_eq!(
+            shuffled_with_first[switched_positions[0]],
+            shuffled_without_first[switched_positions[1]],
+            "the switched values should be equal"
+        );
+
+        assert_eq!(
+            shuffled_with_first[switched_positions[1]],
+            shuffled_without_first[switched_positions[0]],
+            "the switched values should be equal"
+        )
     }
 }

connect/src/spirc.rs

@@ -1287,7 +1287,7 @@ impl SpircTask {
             if self.context_resolver.has_next() {
                 self.connect_state.update_queue_revision()
             } else {
-                self.connect_state.shuffle(None)?;
+                self.connect_state.shuffle_new()?;
                 self.add_autoplay_resolving_when_required();
             }
         } else {

connect/src/state.rs

@@ -23,6 +23,7 @@ use crate::{
     },
     state::{
         context::{ContextType, ResetContext, StateContext},
+        options::ShuffleState,
         provider::{IsProvider, Provider},
     },
 };
@@ -55,7 +56,7 @@ pub(super) enum StateError {
     #[error("the provided context has no tracks")]
     ContextHasNoTracks,
     #[error("playback of local files is not supported")]
-    UnsupportedLocalPlayBack,
+    UnsupportedLocalPlayback,
     #[error("track uri <{0:?}> contains invalid characters")]
     InvalidTrackUri(Option<String>),
 }
@@ -69,7 +70,7 @@ impl From<StateError> for Error {
             | CanNotFindTrackInContext(_, _)
             | ContextHasNoTracks
             | InvalidTrackUri(_) => Error::failed_precondition(err),
-            CurrentlyDisallowed { .. } | UnsupportedLocalPlayBack => Error::unavailable(err),
+            CurrentlyDisallowed { .. } | UnsupportedLocalPlayback => Error::unavailable(err),
         }
     }
 }
@@ -123,7 +124,7 @@ pub(super) struct ConnectState {
     /// the context from which we play, is used to top up prev and next tracks
     context: Option<StateContext>,
     /// seed extracted in [ConnectState::handle_initial_transfer] and used in [ConnectState::finish_transfer]
-    transfer_shuffle_seed: Option<u64>,
+    transfer_shuffle: Option<ShuffleState>,
 
     /// a context to keep track of the autoplay context
     autoplay_context: Option<StateContext>,
@@ -395,7 +396,7 @@ impl ConnectState {
         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()) {
+        if !self.current_track(|t| t.is_queue() || self.is_skip_track(t, None)) {
             self.set_current_track(new_index)?;
         }
 

connect/src/state/context.rs

@@ -24,7 +24,6 @@ const SEARCH_IDENTIFIER: &str = "spotify:search";
 #[derive(Debug)]
 pub struct StateContext {
     pub tracks: ShuffleVec<ProvidedTrack>,
-    pub skip_track: Option<ProvidedTrack>,
     pub metadata: HashMap<String, String>,
     pub restrictions: Option<Restrictions>,
     /// is used to keep track which tracks are already loaded into the next_tracks
@@ -108,6 +107,7 @@ impl ConnectState {
 
         if let Ok(ctx) = self.get_context_mut(ContextType::Default) {
             ctx.remove_shuffle_seed();
+            ctx.remove_initial_track();
             ctx.tracks.unshuffle()
         }
 
@@ -194,7 +194,7 @@ impl ConnectState {
             error!("context didn't have any tracks: {context:#?}");
             Err(StateError::ContextHasNoTracks)?;
         } else if matches!(context.uri, Some(ref uri) if uri.starts_with(LOCAL_FILES_IDENTIFIER)) {
-            Err(StateError::UnsupportedLocalPlayBack)?;
+            Err(StateError::UnsupportedLocalPlayback)?;
         }
 
         let mut next_contexts = Vec::new();
@@ -377,18 +377,23 @@ impl ConnectState {
 
         StateContext {
             tracks: tracks.into(),
-            skip_track: None,
             restrictions,
             metadata,
             index: ContextIndex::new(),
         }
     }
 
-    pub fn is_skip_track(&self, track: &ProvidedTrack) -> bool {
-        self.get_context(self.active_context)
-            .ok()
-            .and_then(|t| t.skip_track.as_ref().map(|t| t.uri == track.uri))
-            .unwrap_or(false)
+    pub fn is_skip_track(&self, track: &ProvidedTrack, iteration: Option<u32>) -> bool {
+        let ctx = match self.get_context(self.active_context).ok() {
+            None => return false,
+            Some(ctx) => ctx,
+        };
+
+        if ctx.get_initial_track().is_none_or(|uri| uri != &track.uri) {
+            return false;
+        }
+
+        iteration.is_none_or(|i| i == 0)
     }
 
     pub fn merge_context(&mut self, new_page: Option<ContextPage>) -> Option<()> {

connect/src/state/handle.rs

@@ -13,7 +13,7 @@ impl ConnectState {
         self.set_shuffle(shuffle);
 
         if shuffle {
-            return self.shuffle(None);
+            return self.shuffle_new();
         }
 
         self.reset_context(ResetContext::DefaultIndex);
@@ -44,17 +44,14 @@ impl ConnectState {
         self.set_repeat_context(repeat);
 
         if repeat {
-            self.set_shuffle(false);
-            self.reset_context(ResetContext::DefaultIndex);
+            if let ContextType::Autoplay = self.fill_up_context {
+                self.fill_up_context = ContextType::Default;
+            }
+        }
 
         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
-            })?;
+        let current_track =
+            ConnectState::find_index_in_context(ctx, |t| self.current_track(|t| &t.uri) == &t.uri)?;
         self.reset_playback_to_position(Some(current_track))
-        } else {
-            self.update_restrictions();
-            Ok(())
-        }
     }
 }

connect/src/state/metadata.rs

@@ -14,6 +14,7 @@ const ITERATION: &str = "iteration";
 
 const CUSTOM_CONTEXT_INDEX: &str = "context_index";
 const CUSTOM_SHUFFLE_SEED: &str = "shuffle_seed";
+const CUSTOM_INITIAL_TRACK: &str = "initial_track";
 
 macro_rules! metadata_entry {
     ( $get:ident, $set:ident, $clear:ident ($key:ident: $entry:ident)) => {
@@ -63,6 +64,7 @@ pub(super) trait Metadata {
     metadata_entry!(get_entity_uri, set_entity_uri, remove_entity_uri (entity_uri: ENTITY_URI));
     metadata_entry!(get_iteration, set_iteration, remove_iteration (iteration: ITERATION));
     metadata_entry!(get_shuffle_seed, set_shuffle_seed, remove_shuffle_seed (shuffle_seed: CUSTOM_SHUFFLE_SEED));
+    metadata_entry!(get_initial_track, set_initial_track, remove_initial_track (initial_track: CUSTOM_INITIAL_TRACK));
 }
 
 macro_rules! impl_metadata {

connect/src/state/options.rs

@@ -10,6 +10,12 @@ use crate::{
 use protobuf::MessageField;
 use rand::Rng;
 
+#[derive(Default, Debug)]
+pub(crate) struct ShuffleState {
+    pub seed: u64,
+    pub initial_track: String,
+}
+
 impl ConnectState {
     fn add_options_if_empty(&mut self) {
         if self.player().options.is_none() {
@@ -44,7 +50,7 @@ impl ConnectState {
         self.set_repeat_context(false);
     }
 
-    pub fn shuffle(&mut self, seed: Option<u64>) -> Result<(), Error> {
+    fn validate_shuffle_allowed(&self) -> Result<(), Error> {
         if let Some(reason) = self
             .player()
             .restrictions
@@ -55,27 +61,39 @@ impl ConnectState {
                 action: "shuffle",
                 reason: reason.clone(),
             })?
+        } else {
+            Ok(())
+        }
     }
 
+    pub fn shuffle_restore(&mut self, shuffle_state: ShuffleState) -> Result<(), Error> {
+        self.validate_shuffle_allowed()?;
+
+        self.shuffle(shuffle_state.seed, &shuffle_state.initial_track)
+    }
+
+    pub fn shuffle_new(&mut self) -> Result<(), Error> {
+        self.validate_shuffle_allowed()?;
+
+        let new_seed = rand::rng().random_range(100_000_000_000..1_000_000_000_000);
+        let current_track = self.current_track(|t| t.uri.clone());
+
+        self.shuffle(new_seed, &current_track)
+    }
+
+    fn shuffle(&mut self, seed: u64, initial_track: &str) -> Result<(), Error> {
         self.clear_prev_track();
         self.clear_next_tracks();
 
-        let current_track = self.current_track(|t| t.clone().take());
-
         self.reset_context(ResetContext::DefaultIndex);
+
         let ctx = self.get_context_mut(ContextType::Default)?;
+        ctx.tracks
+            .shuffle_with_seed(seed, |f| f.uri == initial_track);
 
-        // we don't need to include the current track, because it is already being played
-        ctx.skip_track = current_track;
-
-        let seed =
-            seed.unwrap_or_else(|| rand::rng().random_range(100_000_000_000..1_000_000_000_000));
-
-        ctx.tracks.shuffle_with_seed(seed);
+        ctx.set_initial_track(initial_track);
         ctx.set_shuffle_seed(seed);
 
-        self.set_active_context(ContextType::Default);
-        self.fill_up_context = ContextType::Default;
         self.fill_up_next_tracks()?;
 
         Ok(())

connect/src/state/restrictions.rs

@@ -14,7 +14,6 @@ impl ConnectState {
     pub fn update_restrictions(&mut self) {
         const NO_PREV: &str = "no previous tracks";
         const AUTOPLAY: &str = "autoplay";
-        const ENDLESS_CONTEXT: &str = "endless_context";
 
         let prev_tracks_is_empty = self.prev_tracks().is_empty();
 
@@ -51,8 +50,6 @@ impl ConnectState {
                 restrictions.disallow_toggling_shuffle_reasons = vec![AUTOPLAY.to_string()];
                 restrictions.disallow_toggling_repeat_context_reasons = vec![AUTOPLAY.to_string()];
                 restrictions.disallow_toggling_repeat_track_reasons = vec![AUTOPLAY.to_string()];
-            } else if player.options.repeating_context {
-                restrictions.disallow_toggling_shuffle_reasons = vec![ENDLESS_CONTEXT.to_string()]
             } else {
                 restrictions.disallow_toggling_shuffle_reasons.clear();
                 restrictions

connect/src/state/tracks.rs

@@ -124,7 +124,6 @@ impl<'ct> ConnectState {
                     continue;
                 }
                 Some(next) if next.is_unavailable() => continue,
-                Some(next) if self.is_skip_track(&next) => continue,
                 other => break other,
             };
         };
@@ -297,7 +296,8 @@ impl<'ct> ConnectState {
                     delimiter
                 }
                 None if !matches!(self.fill_up_context, ContextType::Autoplay)
-                    && self.autoplay_context.is_some() =>
+                    && self.autoplay_context.is_some()
+                    && !self.repeat_context() =>
                 {
                     self.update_context_index(self.fill_up_context, new_index)?;
 
@@ -322,7 +322,11 @@ impl<'ct> ConnectState {
                     }
                 }
                 None => break,
-                Some(ct) if ct.is_unavailable() || self.is_skip_track(ct) => {
+                Some(ct) if ct.is_unavailable() || self.is_skip_track(ct, Some(iteration)) => {
+                    debug!(
+                        "skipped track {} during fillup as it's unavailable or should be skipped",
+                        ct.uri
+                    );
                     new_index += 1;
                     continue;
                 }

connect/src/state/transfer.rs

@@ -4,6 +4,7 @@ use crate::{
     state::{
         context::ContextType,
         metadata::Metadata,
+        options::ShuffleState,
         provider::{IsProvider, Provider},
         {ConnectState, StateError},
     },
@@ -54,6 +55,7 @@ impl ConnectState {
         }
 
         let mut shuffle_seed = None;
+        let mut initial_track = None;
         if let Some(session) = transfer.current_session.as_mut() {
             player.play_origin = session.play_origin.take().map(Into::into).into();
             player.suppressions = session.suppressions.take().map(Into::into).into();
@@ -72,6 +74,8 @@ impl ConnectState {
                 .get_shuffle_seed()
                 .and_then(|seed| seed.parse().ok());
 
+            initial_track = session.context.get_initial_track().cloned();
+
             if let Some(mut ctx) = session.context.take() {
                 player.restrictions = ctx.restrictions.take().map(Into::into).into();
                 for (key, value) in ctx.metadata {
@@ -89,7 +93,13 @@ impl ConnectState {
             }
         }
 
-        self.transfer_shuffle_seed = shuffle_seed;
+        self.transfer_shuffle = match (shuffle_seed, initial_track) {
+            (Some(seed), Some(initial_track)) => Some(ShuffleState {
+                seed,
+                initial_track,
+            }),
+            _ => None,
+        };
 
         self.clear_prev_track();
         self.clear_next_tracks();
@@ -163,8 +173,10 @@ impl ConnectState {
             self.set_current_track(current_index.unwrap_or_default())?;
             self.set_shuffle(true);
 
-            let previous_seed = self.transfer_shuffle_seed.take();
-            self.shuffle(previous_seed)?;
+            match self.transfer_shuffle.take() {
+                None => self.shuffle_new(),
+                Some(state) => self.shuffle_restore(state),
+            }?
         } else {
             self.reset_playback_to_position(current_index)?;
         }