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

Author: photovoltex

.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(&mut self, mut rng: impl Rng) {
+    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;
+        }
+
         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;
 
-    #[test]
-    fn test_shuffle_with_seed() {
-        let seed = rand::rng().random_range(0..10000000000000);
+    fn base(range: Range<usize>) -> (ShuffleVec<usize>, u64) {
+        let seed = rand::rng().random_range(0..10_000_000_000_000);
 
-        let vec = (0..100).collect::<Vec<_>>();
-        let base_vec: ShuffleVec<i32> = vec.into();
+        let vec = range.collect::<Vec<_>>();
+        (vec.into(), seed)
+    }
+
+    #[test]
+    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);
-
-            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
-            })?;
-            self.reset_playback_to_position(Some(current_track))
-        } else {
-            self.update_restrictions();
-            Ok(())
+            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)?;
+        self.reset_playback_to_position(Some(current_track))
     }
 }

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 {