Add a zapping mode. - #2045
Conversation
db0ee73 to
09acb3d
Compare
e50fed7 to
132a4dc
Compare
d8d5f18 to
f3e7762
Compare
f3e7762 to
16eac67
Compare
93a11a7 to
7c4414f
Compare
7c4414f to
b1d6093
Compare
fed1593 to
f688ec1
Compare
11bdb37 to
fd6607a
Compare
1a292c3 to
bf90b11
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:
📝 WalkthroughWalkthroughAdds two per-playlist playback window fields ( ChangesPlaylist Playback Window Feature
Sequence DiagramssequenceDiagram
participant MW as MainWindow::UpdateTrackPosition
participant Player as Player
participant Playlist as Playlist::current_item
participant Engine as Engine
Note over MW,Player: On each position tick while Playing
MW->>Player: EndPositionNext(position)
Player->>Player: if play_end_sec_ > 0 && position <= play_end_sec_
Player->>Player: Next()
Player->>Playlist: current_item(play_offset_nanosec_, play_end_sec_)
Playlist->>Playlist: compute center, start, end from half_playing_time_s_ / percent_interest_song_
Playlist-->>Player: PlaylistItemPtr + updated start_offset_ns, end_offset_s
Player->>Engine: Play(play_offset_nanosec_)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/playlist/playlistbackend.h (1)
54-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize the new playback-window fields in the default constructor.
At Line 54,
Playlist()does not initializehalf_playing_time_s_andpercent_interest_song_(Lines 66-67). Default-return paths can propagate undefined values.Suggested fix
- Playlist() : id(-1), favorite(false), last_played(0) {} + Playlist() : id(-1), favorite(false), last_played(0), half_playing_time_s_(0), percent_interest_song_(50) {}🤖 Prompt for 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. In `@src/playlist/playlistbackend.h` around lines 54 - 67, The Playlist() default constructor on line 54 initializes some member variables but does not initialize the newly added member variables half_playing_time_s_ and percent_interest_song_ declared on lines 66-67, which can result in undefined values. Add initialization for both half_playing_time_s_ and percent_interest_song_ in the Playlist() constructor's initialization list, assigning them appropriate default values (such as 0 for integer fields).
🧹 Nitpick comments (1)
src/playlist/playlist.h (1)
238-242: 💤 Low valueConsider using
ScheduleSave()instead of immediateSave()for consistency.The update methods call
Save()directly, bypassing the debouncedScheduleSave()pattern used elsewhere in this class (e.g., line 182). If the user rapidly adjusts the spinbox values, this triggers multiple immediate database writes. Consider usingScheduleSave()for consistency with the existing save pattern.♻️ Suggested change
- void UpdatePlayingTime(const int time_s) { half_playing_time_s_ = time_s; Save(); } + void UpdatePlayingTime(const int time_s) { half_playing_time_s_ = time_s; ScheduleSaveAsync(); } - void UpdatePlayingPosition(const int percent_time) { percent_interest_song_ = percent_time; Save(); } + void UpdatePlayingPosition(const int percent_time) { percent_interest_song_ = percent_time; ScheduleSaveAsync(); }🤖 Prompt for 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. In `@src/playlist/playlist.h` around lines 238 - 242, The UpdatePlayingTime() and UpdatePlayingPosition() methods currently call Save() directly, which causes immediate database writes when values change rapidly. Replace the Save() calls with ScheduleSave() calls in both methods to use the debounced save pattern that is already used elsewhere in the class, ensuring consistency and preventing multiple rapid database writes when users adjust spinbox values quickly.
🤖 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 `@data/schema/schema-23.sql`:
- Around line 1-5: The new columns half_playing_time_s and position_playing_time
are being added as nullable without defaults, which will leave migrated rows
with NULL values and break expected behavior when the feature logic reads these
values expecting the baseline (0, 50) values. Modify the ALTER TABLE statements
to set appropriate DEFAULT values on each column (with half_playing_time_s
defaulting to one baseline value and position_playing_time to the other), and
add a backfill UPDATE statement to set the default values for all existing rows
in the playlists table before updating the schema_version.
In `@src/core/mainwindow.cpp`:
- Around line 1826-1827: The EndPositionNext(position) method is being called
unconditionally from a timer that runs regardless of playback state, causing the
player to skip to the next track even while paused. Add a guard condition to
check if the player is currently in a playing state before calling
EndPositionNext(position). This ensures track advancement only occurs during
active playback, not when the player is paused.
In `@src/playlist/playlistcontainer.cpp`:
- Around line 485-490: The DisplayPlayingOption() method only toggles the
visibility of UI controls but does not disable the zapping mode when hiding
them. When the controls are hidden (visibility becomes false), also reset the
zapping state by clearing or resetting the playing_time_before_or_after value to
ensure zapping is actually disabled and not just hidden from view. This ensures
the toggle accurately reflects the actual state of the zapping feature.
In `@src/playlist/playlistcontainer.ui`:
- Around line 231-234: The user-facing text for the action
show_collection_option in the playlistcontainer.ui file (at the property
name="text" string element) currently says "Show the collection option" which
does not accurately describe what this playback-window control actually does.
Update the string element to use feature-accurate user text that accurately
describes the functionality of this playback control, ensuring the text clearly
conveys the actual purpose and behavior of toggling or managing the collection
option during playback.
---
Outside diff comments:
In `@src/playlist/playlistbackend.h`:
- Around line 54-67: The Playlist() default constructor on line 54 initializes
some member variables but does not initialize the newly added member variables
half_playing_time_s_ and percent_interest_song_ declared on lines 66-67, which
can result in undefined values. Add initialization for both half_playing_time_s_
and percent_interest_song_ in the Playlist() constructor's initialization list,
assigning them appropriate default values (such as 0 for integer fields).
---
Nitpick comments:
In `@src/playlist/playlist.h`:
- Around line 238-242: The UpdatePlayingTime() and UpdatePlayingPosition()
methods currently call Save() directly, which causes immediate database writes
when values change rapidly. Replace the Save() calls with ScheduleSave() calls
in both methods to use the debounced save pattern that is already used elsewhere
in the class, ensuring consistency and preventing multiple rapid database writes
when users adjust spinbox values quickly.
🪄 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: 016f9dc2-5a2b-48ca-9384-0a5b7a18f0d0
📒 Files selected for processing (17)
data/data.qrcdata/schema/schema-23.sqldata/schema/schema.sqlsrc/core/database.cppsrc/core/mainwindow.cppsrc/core/player.cppsrc/core/player.hsrc/playlist/playlist.cppsrc/playlist/playlist.hsrc/playlist/playlistbackend.cppsrc/playlist/playlistbackend.hsrc/playlist/playlistcontainer.cppsrc/playlist/playlistcontainer.hsrc/playlist/playlistcontainer.uisrc/playlist/playlistmanager.cppsrc/playlist/playlistmanager.hsrc/smartplaylists/smartplaylistsearchpreview.cpp
bf90b11 to
fcc09a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/playlist/playlist.h (1)
239-242: ⚡ Quick winUse debounced persistence for spinbox-driven updates.
Line 239 and Line 242 call
Save()directly, which can trigger a DB write per incremental UI value change. Switching toScheduleSave()keeps behavior but coalesces writes.[recommendation]
Suggested patch
- void UpdatePlayingTime(const int time_s) { half_playing_time_s_ = time_s; Save(); } + void UpdatePlayingTime(const int time_s) { half_playing_time_s_ = time_s; ScheduleSave(); } ... - void UpdatePlayingPosition(const int percent_time) { percent_interest_song_ = percent_time; Save(); } + void UpdatePlayingPosition(const int percent_time) { percent_interest_song_ = percent_time; ScheduleSave(); }🤖 Prompt for 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. In `@src/playlist/playlist.h` around lines 239 - 242, The UpdatePlayingTime and UpdatePlayingPosition methods in the playlist class are calling Save() directly in response to spinbox UI changes, which can cause frequent database writes for each incremental value change. Replace the direct Save() calls with ScheduleSave() in both methods to enable debounced persistence that coalesces multiple rapid updates into fewer database writes while maintaining the same overall behavior.
🤖 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 1968-1989: In the current_item method, initialize start_offset_ns
to a default value (0) immediately after resetting end_offset_s, before the
conditional logic that checks if start_time_ns should overwrite it. Currently,
start_offset_ns is only assigned when a specific condition is met at line 1981,
which allows previous values to persist across function calls and leak into
subsequent playback operations. Add an initialization line right after
end_offset_s = 0 to ensure the parameter is always reset on every invocation.
---
Nitpick comments:
In `@src/playlist/playlist.h`:
- Around line 239-242: The UpdatePlayingTime and UpdatePlayingPosition methods
in the playlist class are calling Save() directly in response to spinbox UI
changes, which can cause frequent database writes for each incremental value
change. Replace the direct Save() calls with ScheduleSave() in both methods to
enable debounced persistence that coalesces multiple rapid updates into fewer
database writes while maintaining the same overall behavior.
🪄 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: 8cc0340f-6252-40fd-bb8a-48df81590e58
📒 Files selected for processing (17)
data/data.qrcdata/schema/schema-23.sqldata/schema/schema.sqlsrc/core/database.cppsrc/core/mainwindow.cppsrc/core/player.cppsrc/core/player.hsrc/playlist/playlist.cppsrc/playlist/playlist.hsrc/playlist/playlistbackend.cppsrc/playlist/playlistbackend.hsrc/playlist/playlistcontainer.cppsrc/playlist/playlistcontainer.hsrc/playlist/playlistcontainer.uisrc/playlist/playlistmanager.cppsrc/playlist/playlistmanager.hsrc/smartplaylists/smartplaylistsearchpreview.cpp
✅ Files skipped from review due to trivial changes (1)
- data/data.qrc
🚧 Files skipped from review as they are similar to previous changes (13)
- data/schema/schema-23.sql
- src/core/mainwindow.cpp
- data/schema/schema.sql
- src/core/player.h
- src/playlist/playlistbackend.h
- src/playlist/playlistmanager.cpp
- src/playlist/playlistcontainer.h
- src/smartplaylists/smartplaylistsearchpreview.cpp
- src/playlist/playlistmanager.h
- src/playlist/playlistcontainer.cpp
- src/playlist/playlistbackend.cpp
- src/core/player.cpp
- src/core/database.cpp
a2798aa to
80ca07a
Compare
5599f00 to
96d6001
Compare
9eda5c6 to
2f7281c
Compare
f2bfaca to
bc1582d
Compare
a20fb59 to
33c39b9
Compare
b447e42 to
9dbc73b
Compare
9dbc73b to
e12efde
Compare
A zapping mode consists in selecting a time in the track and play the specified amount of seconds before and after this selected time.
If the amount of seconds is big enough to start before the beginning of the track, then the amount of seconds before the selected time is reduced to start at the beginning of the track.
Same thing for the end of the track : the amount of seconds after the selected time will be reduced to end at the end of the track.
This function is associated to the playlist itself and is hidden by default ; a button had been added at the end of the search bar in the playlist tab. This button toggle the display of these two values : one value to select the amount of seconds (0 to disable the zapping, it's the default value) before and after the time in percent of the track length.
If you choose 10 seconds around 90% of the track, on a 5mn long track, the play will start at 4mn20s and will end at 4mn40s. On a 1mn long track, the play will start at 44s and will end at the end of the track.
Technical part :
Summary by CodeRabbit