Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the recently added AddedToQueue player event with a new universal SetQueue event. The SetQueue event provides a comprehensive view of the queue state, including context URI, current track, next tracks, and previous tracks. This simplifies queue state management for GUI applications by emitting the complete queue state whenever it changes, rather than requiring consumers to manually track incremental changes.
Changes:
- Replaced
AddedToQueueevent and command withSetQueueevent and command - Added queue state emission when context is loaded, tracks are added to queue, or queue is set via Spotify Connect
- Updated event handler to process the new SetQueue event structure
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
playback/src/player.rs |
Replaced EmitAddedToQueueEvent command and AddedToQueue event with EmitSetQueueEvent and SetQueue, including full queue state (context URI, current track, next/prev tracks) |
connect/src/spirc.rs |
Added emit_set_queue_event() helper method and updated queue operations to emit the new SetQueue event instead of AddedToQueue |
src/player_event_handler.rs |
Updated event handler to process SetQueue event with formatted track lists instead of single track AddedToQueue event |
CHANGELOG.md |
Updated changelog entry to reflect the new SetQueue event |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I see you iterated a bit on your previous implementation hehe. I only did read your wall of text so far, but sounds interesting in itself. I will give it a more detailed look in the coming days :) On a side note. #1448 is related, but would be the |
|
Some things I have going through my mind right now:
|
|
Yes, #1448 seems related. Cloning the entire queue sounds expensive at first, but it's only 2 small strings (uri and provider) per track. Negligible on a normal computer, much less of an impact than decoding the stream I would say, but I don't know what range of resource restricted devices librespot runs on and what implications this has. I'm open to creating a separate Event channel for Spirc, or introducing a handler style system. If you direct me what direction you'd rather go, I'm happy to explore. |
2 strings times 70 is still 140 strings for each queue change. So this might not be really optimal on lower end systems. We had something similar previously with |
|
Thanks for your input! Will think about the opt in approach. |
photovoltex
left a comment
There was a problem hiding this comment.
Beside my previous comment with the opt-in for the feature, and the other comment. This seems pretty good as is.
|
@photovoltex I'm still thinking about this:
I do think it's a good idea, but I did not have the time to play with it yet. Now I wonder if we should make single events optional or if we should move this to a separate Spirc channel and make the whole channel optional. 🤔 If you have any guidance, let me know. |
hmm, I think another channel for the event's is probably a different topic then making this optional. I think it would be a good idea to move some event's to a separate spirc channel, which then is optional. But there should also be an option to disable this specific feature, as it's more resource hungry and is not always wanted, as in a standalone "just a player" use-case. to keep it clean we should probably move the channel separation into another PR later down the line and just make this new event optional in this current context^^ |
Other expensive events might be added to `OptInPlayerEvents` in the future.
9dd062a to
5bf1c5b
Compare
|
@photovoltex i made the new event opt-in in 5bf1c5b. |
|
The way you implemented it in 5bf1c5b only solves a clone which is done when sending the event (by the channel itself), but not the clone which is done when creating the event in the So probably something like adding a parameter to the |
|
Damn, you're right, I overlooked this. 😕 I refactored this 2a5f698 to use a boolean toggle |
photovoltex
left a comment
There was a problem hiding this comment.
just some minor changes, after that we can get this merged. Thanks for your patience :)
Co-authored-by: Felix Prillwitz <photovoltex@mailbox.org>
|
All requested changes implemented (I hope). Appreciate your guidance and focus on quality! |
photovoltex
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the contribution again :)
|
My pleasure! 🙏 |
What does this do?
This emits a new universal
Player::SetQueueevent.Background info: How is the Spotify Connect queue organised?
The Spotify connect queue basically has the following data structure:
Each Track has at least a track URI and a provider which can be
autoplay,context,queueorunavailable.autoplay: Added by Spotify to keep playingcontext: part of the current context identified bycontext_uriqueue: manually queued while a context was loadedunavailable: self explanatoryHere you can see an example of a queue:
Why is this event "universal"?
There are multiple actions that manipulate the queue in different ways. Examples:
context_uri,current_track,prev_tracks,next_tracksnext_tracksnext_tracksnext_tracksIf a GUI application wants to adjust its state when any of these actions are performed remote or local, it just needs to listen to this one universal event. The queue displayed in the GUI is always correct and always up to date.
Why does this remove AddToQueue that was just added in 87d37c3 ?
AddToQueueis a special case ofSetQueue, andSetQueuecovers all cases ofAddToQueueusage. Also, it's less error prone for app developers. Say a GUI developer wants to display the current queue from Spirc, and the queue looks like this:When another track is queued, it will be placed after the currently playing track:
In this case,
AddToQueuewill be emitted for Track 1, and the GUI developer will be responsible to add it in the right position of the app's state.When yet another track (or collection of tracks, e.g. an album with 2 tracks) is queued, those will be placed after the currently playing track AND after all the other
queuetracks:In this case,
AddToQueuewill be emitted with Track 2, then with Track 3. Again, the developer of the consuming app is responsible for placing the new tracks in the right position.In contrast to this,
SetQueuealways emits the whole queue with the tracks in the right order, so the developer of the consuming app cannot place the tracks in the wrong position.Also,
AddToQueuewas never in a release, so removing it is ok imho.Are there downsides?
As we're emitting ALL THE QUEUE FIELDS \o/ with every change, we do sometimes emit more than necessary. But we have to emit
next_tracksmost of the time, and the remaining queue fields are not that large compared tonext_tracks.current_trackandcontext_uriis negligible, andprev_tracksis capped at 20 tracks.How to go from here?
I'd love to hear your input on this. Do you think a universal
SetQueueevent is a good thing? Or would you rather have more fine grained events emitting less data, e.g. one forprev_tracks, one fornext_tracksone forcurrent_trackandcurrent_track? Or should we keep the universalSetQueuefor actions that manipulate all ofnext_tracks, but keepAddToTrackwhen adding tracks to the queue, manipulating only small parts ofnext_tracks?The current approach works well for me and my reactive SwiftUI app, and makes updating the queue incredibly easy. But I'm still pretty new to this and there might be use cases that I didn't think of.