Add a sub-menu to the shuffle playlist menu to decide how to shuffle.#2037
Add a sub-menu to the shuffle playlist menu to decide how to shuffle.#2037Vinzzzze wants to merge 1 commit into
Conversation
b67c38c to
37f312f
Compare
37f312f to
547c2bb
Compare
da0abb6 to
151a5d1
Compare
9f20871 to
b40c2ea
Compare
7294324 to
32dc249
Compare
32dc249 to
0a876e4
Compare
b293551 to
c867763
Compare
c867763 to
46e9772
Compare
63f0947 to
e01dd29
Compare
0d83afe to
669ccd7
Compare
ef4b509 to
59bfc08
Compare
59bfc08 to
ba28391
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThree shuffle modes are added across the playlist API, shuffle logic, and main window menu wiring. The playlist manager now forwards a selected mode, the shuffle algorithm branches by mode, and the shuffle action opens a submenu for all, albums, or grouped tracks. ChangesMode-aware playlist shuffle with submenu UI
Sequence DiagramsequenceDiagram
participant User
participant MainWindow
participant PlaylistManager
participant Playlist
User->>MainWindow: selects shuffle submenu action
MainWindow->>MainWindow: ShufflePlaylistActionTriggered(QAction*)
MainWindow->>PlaylistManager: ShuffleCurrent(ShuffleMode)
PlaylistManager->>Playlist: Shuffle(ShuffleMode)
Playlist->>Playlist: ReshuffleIndices(virtual_items, ShuffleMode, album_keep_track_order)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/playlist/playlist.cpp`:
- Around line 2194-2196: The reference front-forcing logic has index arithmetic
issues at three locations in playlist.cpp. At lines 2194-2196, the condition
checks if reference_row is greater than base_reference but does not validate
that the adjusted index (reference_row - base_reference) is within bounds of
virtual_items; when the reference row is excluded (e.g., in ShuffleMode::All),
this can cause an out-of-bounds access. Fix this by ensuring the adjusted index
is less than the size of virtual_items. At lines 2225-2227 and 2278-2280, the
code subtracts base_reference from pos when performing the swap operations, but
pos is already relative to shuffled_*_keys, so the subtraction causes incorrect
indexing; remove the base_reference subtraction from both locations since pos
does not need adjustment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e17dc230-f4f9-405d-a6dd-4aa6f4bff545
📒 Files selected for processing (8)
src/core/mainwindow.cppsrc/core/mainwindow.hsrc/core/mainwindow.uisrc/playlist/playlist.cppsrc/playlist/playlist.hsrc/playlist/playlistmanager.cppsrc/playlist/playlistmanager.hsrc/playlist/playlistmanagerinterface.h
ba28391 to
bf6d7b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/playlist/playlist.h`:
- Around line 370-371: The ReshuffleIndices function declaration is missing a
comma between the shuffle_mode parameter and the const bool
album_keep_track_order parameter. Add a comma after shuffle_mode to fix the
syntax error in the parameter list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41daddfc-5ae5-49f0-b363-483a4ac1c5fa
📒 Files selected for processing (8)
src/core/mainwindow.cppsrc/core/mainwindow.hsrc/core/mainwindow.uisrc/playlist/playlist.cppsrc/playlist/playlist.hsrc/playlist/playlistmanager.cppsrc/playlist/playlistmanager.hsrc/playlist/playlistmanagerinterface.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/playlist/playlistmanager.h
- src/playlist/playlistmanagerinterface.h
- src/core/mainwindow.ui
- src/core/mainwindow.h
- src/core/mainwindow.cpp
f58a918 to
72d39a8
Compare
a2f6091 to
febaf7a
Compare
c2c070b to
a1942c3
Compare
d388cad to
f12f440
Compare
9a4a10e to
2d7d831
Compare
2d7d831 to
dcd0bd3
Compare
Change the menu Shuffle playlist (mainwindow ui) to offer three shuffle modes :
Correct what I consider as an issue : when playing a playlist with a mode shuffle grouped, the tracks are played in the order they have in the playlist. Grouped tracks must be played following the track number order.
Update mainwindow for updating the menus
Update playlist to make the ReshuffleIndices reusable for both Reshuffling indices when playing in shuffle mode and when shuffling the playlist from the menu. I left some commented code with a TODO mark for some code linked to another PR to remind me what to do when I will merge one branch in the other.
Update the playlistmanager and it's interface to handle the choice we can now make while shuffling a playlist.
Add the translations for the created menu
Summary by CodeRabbit