-
-
Notifications
You must be signed in to change notification settings - Fork 258
Add playlist reverse command
#1675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c4d73f9 to
bf454f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a new playlist reverse command that reverses the order of tracks in a playlist and clears any stored sort preferences. The reverse functionality is treated as a one-time operation rather than a dynamic sort method, and it's bound to the Shift+R key combination.
- Adds a new
Reversecommand that reverses playlist track order - Updates queue handling to properly maintain current track position after reversal
- Clears stored sort preferences when reversing to avoid conflicts
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/command.rs | Adds the Reverse command enum variant and parsing logic |
| src/commands.rs | Adds keybinding for Shift+R and includes Reverse in unsupported commands list |
| src/model/playlist.rs | Implements the reverse logic for playlist tracks and clears sort state |
| src/queue.rs | Implements queue reversal with proper current track index handling |
| src/ui/playlist.rs | Handles the reverse command in the playlist view |
| doc/users.md | Documents the new reverse command and keybinding |
| CHANGELOG.md | Records the new feature addition |
| let mut current = self.current_track.write().unwrap(); | ||
| if let Some(index) = *current { | ||
| let new_index = queue.len().saturating_sub(1).saturating_sub(index); | ||
| *current = Some(new_index); |
Copilot
AI
Aug 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation for the new index after reversing has a logical error. For a queue of length n, when reversing, the element at index i should move to index n - 1 - i. The current calculation queue.len().saturating_sub(1).saturating_sub(index) is equivalent to (n - 1) - index, which is correct. However, this will panic if index >= queue.len(), and saturating_sub won't help in that case. Consider adding a bounds check: if index < queue.len() { let new_index = queue.len() - 1 - index; *current = Some(new_index); }
| *current = Some(new_index); | |
| if index < queue.len() { | |
| let new_index = queue.len() - 1 - index; | |
| *current = Some(new_index); | |
| } |
07eeac5 to
31007b3
Compare
|
Hi there, apologies for the late review. Generally I think this is a nice idea, but I wonder if this should be applicable to queues instead, which are more like a scrap playlist that can then be saved? Because the way this is implemented right now will reverse playlists but there's no way to save them from the playlist view, right? |
|
Hey no problem! My use case was to quickly flip the order of items in a playlist where I add stuff and sort it over time. This command could possibly be extended to any view IMO, But I do like the UX of having just a ":reverse" command. |
ab6eab6 to
4829d31
Compare
Describe your changes
Implements a reverse playlist command to reverse the order of tracks in a playlist.
Reverse was not implemented as a sort method, as I don't see it as a dynamic order, but as a do once command.
When reversing playlist we also clear the current sort order.
Adds it under the default keybinding
Shift+RIssue ticket number and link
N/A
Checklist before requesting a review