fix: fixing restarting music when pausing it#21
fix: fixing restarting music when pausing it#21LeonardoFernandesContrera wants to merge 1 commit into
Conversation
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts player resume and play logic so that pausing/resuming via IPC no longer restarts the track, and only starts a new play when there is no active mpv process. Sequence diagram for PlayerService resume behavior with active ipcSocketsequenceDiagram
actor User
participant PlayerService
participant IpcSocket
User->>PlayerService: resume()
PlayerService->>PlayerService: check ipcSocket and destroyed
alt ipcSocket exists and not destroyed
PlayerService->>IpcSocket: sendIpcCommand(set_property, pause, false)
PlayerService->>PlayerService: isPlaying = true
alt currentVolume is defined
PlayerService->>PlayerService: setTimeout(100ms)
PlayerService->>IpcSocket: sendIpcCommand(set_property, volume, currentVolume)
end
end
Sequence diagram for PlayerService resume behavior when mpvProcess is not runningsequenceDiagram
actor User
participant PlayerService
participant MpvProcess
User->>PlayerService: resume()
PlayerService->>PlayerService: check ipcSocket and destroyed
alt no ipcSocket or destroyed
PlayerService->>PlayerService: check mpvProcess and currentUrl
alt mpvProcess is not running and currentUrl is set
PlayerService->>PlayerService: play(currentUrl, volume = currentVolume)
PlayerService->>MpvProcess: start playback
end
end
Updated class diagram for PlayerService play and resume logicclassDiagram
class PlayerService {
string currentTrackId
string currentUrl
number currentVolume
boolean isPlaying
any ipcSocket
any mpvProcess
play(url, options)
stop()
resume()
sendIpcCommand(args)
}
class PlayOptions {
number volume
string videoId
}
PlayerService ..> PlayOptions : uses
%% Highlighted behavior changes
%% - currentTrackId is now set after stop() in play()
%% - resume() uses ipcSocket pause property when available
%% - resume() only calls play() when mpvProcess is not running and currentUrl is set
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Warning Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file)
AnalysisThe PR fixes the issue where pressing spacebar during playback would restart the track instead of pausing/resuming from the current position. The changes are logical and well-structured:
Minor Issue: Line 532 uses Security: No secrets exposed, no module security issues found. Breaking Changes: None - this is a bug fix that improves existing behavior. Fix these issues in Kilo Cloud Reviewed by minimax-m2.5-20260211 · 214,309 tokens |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the PlayerService by reordering the currentTrackId assignment in the play method to occur after stopping existing playback, and by refining the resume method's logic to correctly set isPlaying and handle different playback scenarios. A review comment highlights a potential race condition in the resume method's use of setTimeout for reapplying volume, suggesting a more robust solution by waiting for mpv confirmation.
| setTimeout(() => { | ||
| this.sendIpcCommand(['set_property', 'volume', this.currentVolume]); | ||
| }, 100); |
There was a problem hiding this comment.
Using setTimeout with a fixed delay to reapply volume can be fragile. On a slow system or under high load, 100ms might not be enough for the unpause command to be processed by mpv, leading to a race condition where volume is set before the player is ready.
A more robust approach would be to wait for a confirmation from mpv that the player has resumed. This could be done by listening for the property-change event for pause: false before sending the volume command. This would make the resume logic more reliable.
1 participant
Description
Fix the issue where pressing the spacebar while music is playing would restart the track from the beginning instead of pausing and resuming from the current position.
Summary by Sourcery
Bug Fixes: