From 3c054ed1ec22b1cf9f1dea5d0a2403e6407fbdfa Mon Sep 17 00:00:00 2001 From: salmon Date: Wed, 12 Aug 2026 02:17:02 +0200 Subject: queue logic fixes --- src/queue.rs | 53 +++++++++++++++------------------ tests/queue.rs | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 29 deletions(-) diff --git a/src/queue.rs b/src/queue.rs index 1f2f0fc..c3f8dbe 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -202,7 +202,6 @@ pub struct Queue { position: usize, shuffle: ShuffleMode, shuffled_indices: Vec, - original_tracks: Vec, } impl Default for Queue { @@ -218,7 +217,6 @@ impl Queue { position: 0, shuffle: ShuffleMode::None, shuffled_indices: Vec::new(), - original_tracks: Vec::new(), } } @@ -248,62 +246,59 @@ impl Queue { } pub fn set_shuffle(&mut self, mode: ShuffleMode) { - match mode { + self.shuffle = mode; + match self.shuffle { ShuffleMode::None => { - self.shuffle = ShuffleMode::None; self.restore_from_original(); } ShuffleMode::All => { - self.original_tracks = self.tracks.clone(); - self.shuffle = ShuffleMode::All; self.update_shuffle_indices(); } ShuffleMode::Album => { - self.original_tracks = self.tracks.clone(); - self.shuffle = ShuffleMode::Album; self.update_shuffle_indices_album(); } ShuffleMode::Albums => { - self.original_tracks = self.tracks.clone(); - self.shuffle = ShuffleMode::Albums; self.update_shuffle_indices_albums(); } } } fn restore_from_original(&mut self) { - if self.original_tracks.is_empty() { + // Restore linear (unshuffled) order. `tracks` is never reordered by + // shuffle — only `shuffled_indices` is a permutation — so the live + // list already carries the original insertion order. Keep the played + // segment (positions 0..=current in shuffled order) in the order it + // was played, then append the unplayed entries in original order. + // Entries are tracked by identity, so duplicate paths are preserved + // and entries added during shuffle survive. + if self.tracks.is_empty() { + self.position = 0; self.shuffled_indices.clear(); return; } - let current_pos = self.position; + let current_pos = self.position.min(self.tracks.len().saturating_sub(1)); - // Find which tracks have been played (positions 0..=current_pos in shuffled order) - let mut played_track_paths: Vec = Vec::new(); + // Played entries, in played order (positions 0..=current_pos in shuffled order). + let mut played: Vec = Vec::new(); + let mut played_idx: Vec = Vec::new(); for i in 0..=current_pos { - if let Some(&idx) = self.shuffled_indices.get(i) - && let Some(track) = self.tracks.get(idx) - { - played_track_paths.push(track.path.clone()); + if let Some(&idx) = self.shuffled_indices.get(i) { + if let Some(track) = self.tracks.get(idx) { + played.push(track.clone()); + played_idx.push(idx); + } } } - // Keep the played tracks in their current order - let mut new_tracks: Vec = Vec::new(); - for path in &played_track_paths { - if let Some(track) = self.tracks.iter().find(|t| &t.path == path) { + // Unplayed entries keep their original (insertion) order. + let mut new_tracks = played; + for (idx, track) in self.tracks.iter().enumerate() { + if !played_idx.contains(&idx) { new_tracks.push(track.clone()); } } - // Add remaining tracks from original in original order - for orig_track in &self.original_tracks { - let is_played = played_track_paths.iter().any(|p| p == &orig_track.path); - if !is_played { - new_tracks.push(orig_track.clone()); - } - } self.tracks = new_tracks; self.position = current_pos; self.shuffled_indices.clear(); diff --git a/tests/queue.rs b/tests/queue.rs index 2eaa6f3..d338f2a 100644 --- a/tests/queue.rs +++ b/tests/queue.rs @@ -474,6 +474,99 @@ fn test_clear_resets_shuffle_indices() { assert!(q.shuffled_indices().is_empty()); } +// ---------- Regression tests ---------- +// These encode invariants the queue must uphold across shuffle transitions. +// They previously failed and now pin the fixed behavior. + +#[test] +fn test_add_during_shuffle_survives_shuffle_off() { + // A track added while Shuffle::All is active must survive turning shuffle + // off: `restore_from_original` rebuilds the queue from the live track + // list (entries, not paths), so late additions are kept. + // (src/queue.rs: add_track 223, restore_from_original 266) + let mut q = Queue::new(); + add_track(&mut q, "/music/a.mp3"); + add_track(&mut q, "/music/b.mp3"); + add_track(&mut q, "/music/c.mp3"); + q.set_shuffle(ShuffleMode::All); + + // User adds a track while shuffle is active (e.g. `rsp play --next`). + add_track(&mut q, "/music/d.mp3"); + assert_eq!(q.len(), 4); + + q.set_shuffle(ShuffleMode::None); + + // d.mp3 must still be in the queue. + assert_eq!( + q.len(), + 4, + "queue lost a track after shuffle-off; len = {}", + q.len() + ); + assert!( + q.tracks().iter().any(|t| t.path == PathBuf::from("/music/d.mp3")), + "track added during shuffle was dropped when shuffle was turned off" + ); +} + +#[test] +fn test_clear_then_shuffle_off_does_not_resurrect_queue() { + // A cleared queue must stay empty across a later shuffle-off: `clear()` + // wipes tracks/position/shuffled_indices, and `restore_from_original` + // must not resurrect entries from any lingering state. + // (src/queue.rs: clear 546 / restore_from_original 266; + // src/daemon.rs: stop 815, queue-end 353-354) + let mut q = Queue::new(); + add_track(&mut q, "/music/a.mp3"); + add_track(&mut q, "/music/b.mp3"); + add_track(&mut q, "/music/c.mp3"); + q.set_shuffle(ShuffleMode::All); + + q.clear(); + assert!(q.is_empty()); + assert_eq!(q.len(), 0); + + // Later the user toggles shuffle off: the queue must stay empty. + q.set_shuffle(ShuffleMode::None); + assert!( + q.is_empty(), + "cleared queue was resurrected from the stale shuffle snapshot (len = {})", + q.len() + ); + assert_eq!(q.len(), 0); +} + +#[test] +fn test_shuffle_off_keeps_unplayed_duplicate_entries() { + // Duplicates are distinct queue entries: `restore_from_original` rebuilds + // by entry identity (played indices vs. the rest), never by path, so an + // unplayed duplicate of an already-played path must survive shuffle-off. + // (src/queue.rs: restore_from_original 266) + let mut q = Queue::new(); + add_track(&mut q, "/music/a.mp3"); + add_track(&mut q, "/music/b.mp3"); + add_track(&mut q, "/music/a.mp3"); // duplicate entry (same path) + + // Play the first two entries (position 1). The third entry is unplayed. + q.next(); + assert_eq!(q.position(), 1); + + // Position is 1 with len 3: the shuffled tail is a single element, so the + // shuffled indices stay identity and the restore is deterministic. + q.set_shuffle(ShuffleMode::All); + q.set_shuffle(ShuffleMode::None); + + // All three entries must survive shuffle-off (duplicates are distinct + // queue entries, not a single path). + assert_eq!( + q.len(), + 3, + "unplayed duplicate entry was dropped on shuffle-off (len = {})", + q.len() + ); + assert_eq!(q.position(), 1); +} + // ---------- Track struct ---------- #[test] -- cgit v1.2.3