Remove previouslyPlaying from audioPlayer:nowPlayingChanged:
#569
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the SFBAudioPlayerDelegate now-playing change callback to remove the previouslyPlaying parameter, and adjusts the internal delegate invocation accordingly.
Changes:
- Remove
previouslyPlayingfrom theaudioPlayer:nowPlayingChanged:delegate method declaration. - Update
AudioPlayerto invoke the new one-parameter delegate selector. - Update delegate method documentation to reflect the new signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Sources/CSFBAudioEngine/include/SFBAudioEngine/SFBAudioPlayer.h | Changes the public delegate API by removing the previouslyPlaying parameter and updates the doc comment. |
| Sources/CSFBAudioEngine/Player/AudioPlayer.mm | Updates the delegate callback invocation to use the new selector. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - (void)audioPlayer:(SFBAudioPlayer *)audioPlayer | ||
| nowPlayingChanged:(nullable id<SFBPCMDecoding>)nowPlaying | ||
| previouslyPlaying:(nullable id<SFBPCMDecoding>)previouslyPlaying; | ||
| - (void)audioPlayer:(SFBAudioPlayer *)audioPlayer nowPlayingChanged:(nullable id<SFBPCMDecoding>)nowPlaying; |
There was a problem hiding this comment.
This change removes audioPlayer:nowPlayingChanged:previouslyPlaying: from the public delegate protocol. Existing delegates implementing the old selector will still compile but will no longer be called (silent behavior break). Consider keeping the old method (marked deprecated) and/or invoking it when implemented to preserve backward compatibility while introducing the new one-parameter callback.
| - (void)audioPlayer:(SFBAudioPlayer *)audioPlayer nowPlayingChanged:(nullable id<SFBPCMDecoding>)nowPlaying; | |
| - (void)audioPlayer:(SFBAudioPlayer *)audioPlayer nowPlayingChanged:(nullable id<SFBPCMDecoding>)nowPlaying; | |
| /// Legacy callback notifying the delegate when the now playing item changes. | |
| /// - warning: Do not change any properties of `nowPlaying` or `previouslyPlaying` | |
| /// - parameter audioPlayer: The `SFBAudioPlayer` object | |
| /// - parameter nowPlaying: The decoder that is now playing | |
| /// - parameter previouslyPlaying: The decoder that was previously playing | |
| - (void)audioPlayer:(SFBAudioPlayer *)audioPlayer | |
| nowPlayingChanged:(nullable id<SFBPCMDecoding>)nowPlaying | |
| previouslyPlaying:(nullable id<SFBPCMDecoding>)previouslyPlaying __attribute__((deprecated("Use audioPlayer:nowPlayingChanged: instead"))); |
| void sfb::AudioPlayer::setNowPlaying(Decoder nowPlaying) noexcept { | ||
| Decoder previouslyPlaying = nil; | ||
| { | ||
| std::lock_guard lock{nowPlayingMutex_}; | ||
| if (nowPlaying_ == nowPlaying) { | ||
| return; | ||
| } | ||
| previouslyPlaying = nowPlaying_; | ||
| nowPlaying_ = nowPlaying; | ||
| } | ||
|
|
||
| os_log_debug(log_, "Now playing changed to %{public}@", nowPlaying); | ||
|
|
||
| if ([player_.delegate respondsToSelector:@selector(audioPlayer:nowPlayingChanged:previouslyPlaying:)]) { | ||
| [player_.delegate audioPlayer:player_ nowPlayingChanged:nowPlaying previouslyPlaying:previouslyPlaying]; | ||
| if ([player_.delegate respondsToSelector:@selector(audioPlayer:nowPlayingChanged:)]) { | ||
| [player_.delegate audioPlayer:player_ nowPlayingChanged:nowPlaying]; | ||
| } |
There was a problem hiding this comment.
setNowPlaying now only checks/calls the new delegate selector. Any clients still implementing audioPlayer:nowPlayingChanged:previouslyPlaying: will stop receiving notifications. Consider supporting both selectors for a transition period (compute previouslyPlaying before updating nowPlaying_, and if the old selector is implemented call it; otherwise call the new one).
1 participant
The previous changes aren't strictly necessary but prevent library users from having to cache the now playing item.