From eff5ca3294246a0863a23d7fb71c865498e66378 Mon Sep 17 00:00:00 2001 From: Felix Prillwitz Date: Sun, 31 Aug 2025 20:32:01 +0200 Subject: [PATCH] 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 + CHANGELOG.md | 5 +- connect/src/context_resolver.rs | 4 +- connect/src/shuffle_vec.rs | 120 ++++++++++++++++++++++++++---- connect/src/spirc.rs | 2 +- connect/src/state.rs | 9 ++- connect/src/state/context.rs | 21 ++++-- connect/src/state/handle.rs | 21 +++--- connect/src/state/metadata.rs | 2 + connect/src/state/options.rs | 42 ++++++++--- connect/src/state/restrictions.rs | 3 - connect/src/state/tracks.rs | 10 ++- connect/src/state/transfer.rs | 18 ++++- 13 files changed, 193 insertions(+), 65 deletions(-) diff --git a/.gitignore b/.gitignore index c9a8b06b..aa13aaf2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ target .cargo spotify_appkey.key +.idea/ .vagrant/ .project .history diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a35e9c6..215a5486 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/connect/src/context_resolver.rs b/connect/src/context_resolver.rs index 29336f0a..1bc0c163 100644 --- a/connect/src/context_resolver.rs +++ b/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 diff --git a/connect/src/shuffle_vec.rs b/connect/src/shuffle_vec.rs index ca73eb77..595a392b 100644 --- a/connect/src/shuffle_vec.rs +++ b/connect/src/shuffle_vec.rs @@ -8,6 +8,12 @@ use std::{ pub struct ShuffleVec { vec: Vec, indices: Option>, + /// 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, } impl PartialEq for ShuffleVec { @@ -41,16 +47,25 @@ impl IntoIterator for ShuffleVec { impl From> for ShuffleVec { fn from(vec: Vec) -> Self { - Self { vec, indices: None } + Self { + vec, + original_first_position: None, + indices: None, + } } } impl ShuffleVec { - pub fn shuffle_with_seed(&mut self, seed: u64) { - self.shuffle_with_rng(SmallRng::seed_from_u64(seed)) + pub fn shuffle_with_seed 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 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 ShuffleVec { 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 ShuffleVec { 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 ShuffleVec { mod test { use super::*; use rand::Rng; + use std::ops::Range; + + fn base(range: Range) -> (ShuffleVec, u64) { + let seed = rand::rng().random_range(0..10_000_000_000_000); + + let vec = range.collect::>(); + (vec.into(), seed) + } #[test] - fn test_shuffle_with_seed() { - let seed = rand::rng().random_range(0..10000000000000); - - let vec = (0..100).collect::>(); - let base_vec: ShuffleVec = 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" + ) } } diff --git a/connect/src/spirc.rs b/connect/src/spirc.rs index 6bb9685b..087384e9 100644 --- a/connect/src/spirc.rs +++ b/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 { diff --git a/connect/src/state.rs b/connect/src/state.rs index 9e73d861..a84de234 100644 --- a/connect/src/state.rs +++ b/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), } @@ -69,7 +70,7 @@ impl From 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, /// seed extracted in [ConnectState::handle_initial_transfer] and used in [ConnectState::finish_transfer] - transfer_shuffle_seed: Option, + transfer_shuffle: Option, /// a context to keep track of the autoplay context autoplay_context: Option, @@ -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)?; } diff --git a/connect/src/state/context.rs b/connect/src/state/context.rs index eb04fe5f..7f0fc640 100644 --- a/connect/src/state/context.rs +++ b/connect/src/state/context.rs @@ -24,7 +24,6 @@ const SEARCH_IDENTIFIER: &str = "spotify:search"; #[derive(Debug)] pub struct StateContext { pub tracks: ShuffleVec, - pub skip_track: Option, pub metadata: HashMap, pub restrictions: Option, /// 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) -> 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) -> Option<()> { diff --git a/connect/src/state/handle.rs b/connect/src/state/handle.rs index acf8bdcb..e031410a 100644 --- a/connect/src/state/handle.rs +++ b/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)) } } diff --git a/connect/src/state/metadata.rs b/connect/src/state/metadata.rs index 8212c276..82318dab 100644 --- a/connect/src/state/metadata.rs +++ b/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 { diff --git a/connect/src/state/options.rs b/connect/src/state/options.rs index 97288b7e..efb83764 100644 --- a/connect/src/state/options.rs +++ b/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) -> 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, ¤t_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(()) diff --git a/connect/src/state/restrictions.rs b/connect/src/state/restrictions.rs index e1f78094..29d8d475 100644 --- a/connect/src/state/restrictions.rs +++ b/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 diff --git a/connect/src/state/tracks.rs b/connect/src/state/tracks.rs index afaf7c00..94bde92b 100644 --- a/connect/src/state/tracks.rs +++ b/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; } diff --git a/connect/src/state/transfer.rs b/connect/src/state/transfer.rs index 1e2f40cf..8fdb21fa 100644 --- a/connect/src/state/transfer.rs +++ b/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)?; }