queue: make the Insert *command* mean what the primitive now means

The previous commit fixed QueueManager::insert_tracks to insert at an index
but left the paste path unchanged, so pasting behaved exactly as before.
PlaybackCommand::Insert built a ResolveKind::InsertAfter(position), and that
op compensated with a + 1 — which cancelled the fix on the one path paste
actually takes. Every test I had written drove either the primitive or the
op directly, so nothing caught it.

ResolveKind::InsertAfter is now InsertAt: the index insert_tracks takes, no
offset. The Insert command passes its position straight through, and the two
callers that mean "after" add the 1 themselves — play-next (`L`) passes
current + 1, and the op's chunk arithmetic is unchanged.

Three tests now cover the level that was missing: the Insert command at an
interior index pushes the row that was there down, the Insert command at 0
reaches the front (what `P` on the first row needs), and play-next still
lands right after the current track rather than on top of it. All three
would have failed before this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Test User 2026-07-26 12:10:31 +02:00
parent 7fe4326923
commit 1b2f01a2b6
2 changed files with 116 additions and 20 deletions

View File

@ -255,10 +255,12 @@ pub enum ResolveKind {
Replace, Replace,
/// Every chunk appends at the end. /// Every chunk appends at the end.
Append, Append,
/// Chunks insert after the given position, each advancing the cursor so /// Chunks insert **at** the given index, each advancing it so the resolved
/// the resolved collection stays contiguous and in order. `Queue` /// collection stays contiguous and in order. The index is the one
/// (play-after-current) is an `InsertAfter` at the current position. /// `QueueManager::insert_tracks` takes: whatever sits there shifts down,
InsertAfter(u32), /// `0` is the front. `Queue` (play-after-current) is an `InsertAt` of the
/// current position **+ 1**.
InsertAt(u32),
} }
/// The playback loop's bookkeeping for one in-flight resolve operation. /// The playback loop's bookkeeping for one in-flight resolve operation.
@ -340,13 +342,12 @@ impl PendingResolve {
queue.replace_with_tracks(tracks) queue.replace_with_tracks(tracks)
} }
ResolveKind::Append => queue.append_tracks(tracks), ResolveKind::Append => queue.append_tracks(tracks),
ResolveKind::InsertAfter(position) => { ResolveKind::InsertAt(index) => {
// Advance the cursor so this op's next chunk lands right // Advance so this op's next chunk lands right behind this one,
// behind this one, keeping the collection contiguous. // keeping the collection contiguous. `insert_tracks` clamps
// `insert_tracks` inserts *at* an index and clamps past the // past the end.
// end, so "after N" is "at N + 1". self.kind = ResolveKind::InsertAt(index + tracks.len() as u32);
self.kind = ResolveKind::InsertAfter(position + tracks.len() as u32); queue.insert_tracks(index, tracks)
queue.insert_tracks(position + 1, tracks)
} }
}; };
if started.is_some() { if started.is_some() {
@ -599,8 +600,7 @@ impl QueueManager {
/// what a client needs to paste *before* the first row. An empty queue is /// what a client needs to paste *before* the first row. An empty queue is
/// a replace, which starts playback. /// a replace, which starts playback.
/// ///
/// Callers wanting "after track N" (play-next, `ResolveKind::InsertAfter`) /// Callers wanting "after track N" (play-next) pass `N + 1`.
/// pass `N + 1`.
pub fn insert_tracks(&mut self, position: u32, tracks: &[Track]) -> Option<Track> { pub fn insert_tracks(&mut self, position: u32, tracks: &[Track]) -> Option<Track> {
let len = self.tracks.len(); let len = self.tracks.len();
if len == 0 { if len == 0 {
@ -815,9 +815,10 @@ mod tests {
} }
#[test] #[test]
fn insert_after_op_keeps_chunks_contiguous_and_in_order() { fn insert_op_keeps_chunks_contiguous_and_in_order() {
let mut q = queue_with(3); // playing track 0 let mut q = queue_with(3); // playing track 0
let mut op = PendingResolve::new(ResolveKind::InsertAfter(0)); // "after track 0" is "at index 1".
let mut op = PendingResolve::new(ResolveKind::InsertAt(1));
assert!(op.apply_chunk(&mut q, &[track(10), track(11)]).is_none()); assert!(op.apply_chunk(&mut q, &[track(10), track(11)]).is_none());
assert!(op.apply_chunk(&mut q, &[track(12)]).is_none()); assert!(op.apply_chunk(&mut q, &[track(12)]).is_none());
// Both chunks sit as one contiguous run right after the current // Both chunks sit as one contiguous run right after the current
@ -829,9 +830,9 @@ mod tests {
} }
#[test] #[test]
fn insert_after_op_clamps_past_the_end() { fn insert_op_clamps_past_the_end() {
let mut q = queue_with(1); let mut q = queue_with(1);
let mut op = PendingResolve::new(ResolveKind::InsertAfter(99)); let mut op = PendingResolve::new(ResolveKind::InsertAt(99));
assert!(op.apply_chunk(&mut q, &[track(10)]).is_none()); assert!(op.apply_chunk(&mut q, &[track(10)]).is_none());
assert!(op.apply_chunk(&mut q, &[track(11)]).is_none()); assert!(op.apply_chunk(&mut q, &[track(11)]).is_none());
assert_eq!(titles(&q), vec!["track 0", "track 10", "track 11"]); assert_eq!(titles(&q), vec!["track 0", "track 10", "track 11"]);

View File

@ -192,7 +192,8 @@ impl Playback {
}; };
queue.current_position() as u32 queue.current_position() as u32
}; };
self.start_resolve(ResolveKind::InsertAfter(position), paths); // Play-next means "right after the current track".
self.start_resolve(ResolveKind::InsertAt(position + 1), paths);
} }
PlaybackCommand::Append { paths } => { PlaybackCommand::Append { paths } => {
@ -238,7 +239,9 @@ impl Playback {
} }
PlaybackCommand::Insert { position, paths } => { PlaybackCommand::Insert { position, paths } => {
self.start_resolve(ResolveKind::InsertAfter(position), paths); // The RPC's `position` is the index to insert *at*: whatever
// sits there shifts down, 0 is the front. No offset here.
self.start_resolve(ResolveKind::InsertAt(position), paths);
} }
PlaybackCommand::Clear { exclude_current } => { PlaybackCommand::Clear { exclude_current } => {
@ -904,7 +907,12 @@ mod tests {
) )
} }
#[cfg(feature = "fs")] /// Queue titles in queue order, for order assertions.
fn queue_titles(playback: &Playback) -> Vec<String> {
let proto: ProtoQueue = playback.queue.lock().expect("queue lock").clone().into();
proto.tracks.iter().map(|t| t.title.clone()).collect()
}
fn fill_queue(playback: &Playback, n: usize) { fn fill_queue(playback: &Playback, n: usize) {
let tracks: Vec<Track> = (0..n).map(track).collect(); let tracks: Vec<Track> = (0..n).map(track).collect();
let mut queue = playback.queue.lock().expect("queue lock"); let mut queue = playback.queue.lock().expect("queue lock");
@ -1055,4 +1063,91 @@ mod tests {
let snapshot = rx.borrow().clone().expect("snapshot sent"); let snapshot = rx.borrow().clone().expect("snapshot sent");
assert_eq!(snapshot.tracks.len(), 1); assert_eq!(snapshot.tracks.len(), 1);
} }
/// The gap that let the paste bug through: every previous test drove
/// either `insert_tracks` or `PendingResolve` directly, so none of them
/// pinned what the **`Insert` command** does with its position. It is an
/// index — the row there shifts down — and 0 is the front.
#[tokio::test]
async fn insert_command_places_tracks_at_the_given_index() {
let playback = playback_with(
#[cfg(feature = "fs")]
None,
);
fill_queue(&playback, 3);
// The first op registered gets id 0; the resolve itself goes nowhere
// in tests (no provider), so the chunk is applied by hand.
playback
.handle_command(PlaybackCommand::Insert {
position: 1,
paths: vec!["/x".to_string()],
})
.await;
playback
.handle_command(PlaybackCommand::ApplyResolvedChunk {
op_id: 0,
tracks: vec![track(9)],
})
.await;
assert_eq!(
queue_titles(&playback),
vec!["track 0", "track 9", "track 1", "track 2"],
"position 1 must push the row that was there down"
);
}
/// Play-next (`L`) must still land right *after* the current track, not
/// on top of it — the same command-level check for the other caller of
/// the insert op.
#[tokio::test]
async fn queue_command_places_tracks_after_the_current_track() {
let playback = playback_with(
#[cfg(feature = "fs")]
None,
);
fill_queue(&playback, 3); // playing track 0
playback
.handle_command(PlaybackCommand::Queue {
paths: vec!["/x".to_string()],
})
.await;
playback
.handle_command(PlaybackCommand::ApplyResolvedChunk {
op_id: 0,
tracks: vec![track(9)],
})
.await;
assert_eq!(
queue_titles(&playback),
vec!["track 0", "track 9", "track 1", "track 2"]
);
}
/// And position 0 reaches the very front — what `P` on the first row needs.
#[tokio::test]
async fn insert_command_at_zero_reaches_the_front() {
let playback = playback_with(
#[cfg(feature = "fs")]
None,
);
fill_queue(&playback, 2);
playback
.handle_command(PlaybackCommand::Insert {
position: 0,
paths: vec!["/x".to_string()],
})
.await;
playback
.handle_command(PlaybackCommand::ApplyResolvedChunk {
op_id: 0,
tracks: vec![track(9)],
})
.await;
assert_eq!(
queue_titles(&playback),
vec!["track 9", "track 0", "track 1"]
);
}
} }