Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue load #26

Merged
merged 8 commits into from
May 5, 2024
Merged

Queue load #26

merged 8 commits into from
May 5, 2024

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Mar 4, 2024

I've added various features, mostly relating to queues, serializing and deserializing media messages in more detail.
Three commits add queue-related commands or fields to commands, three commits improve status deserialisation (including removing all uses of unwrap in that area), two commits add impls of common traits to public media types.

Looking at the new extended statuses, I've stayed close to the message format, but it might be more user-friendly to add extended states to the original PlayerState enum; the next release will already break compat by adding stuff to the error enum anyway.

g2p added 8 commits March 19, 2024 14:30
It is possible to get by with matches!, but this is more convenient.
On the JS sender side at least, Session.queueLoad is deprecated
in favour of Session.load with a queueData set on the loaded item.
This is useful to know when the player appears to be idle
but is in fact loading the next track from a playlist.
Also, introduce the TryFrom trait and new error types.
This removes all uses of unwrap from media.rs.
Queue item currently playing, loading, preloaded.
@azasypkin
Copy link
Owner

@g2p I see you're making changes to this PR from time to time, so I assume you're still actively working on it. Once you need any input or review, just tag me, and I'll circle back as soon as I can. Thanks!

@g2p
Copy link
Contributor Author

g2p commented Mar 31, 2024

This one is good for review if you're up to it!
The recent push was just rebasing it on top of your current branch to make it easier to merge.
Please don't let GitHub squash the commits though.

I have more queue-related work in another branch, but it's interspersed with async stuff. I've published the app that motivated the changes: https://github.com/g2p/joujou. So, I'll go clean up the history and maybe ask for input on the async branch, because it would be easier for me to have the rest of the media queue work land on top of the async-related refactoring.

@azasypkin
Copy link
Owner

This one is good for review if you're up to it!

Roger that!

I've published the app that motivated the changes: https://github.com/g2p/joujou.

Oh, nice, thanks for sharing!

So, I'll go clean up the history and maybe ask for input on the async branch, because it would be easier for me to have the rest of the media queue work land on top of the async-related refactoring.

Did you have a chance to chat with @fluffysquirrels to figure out how we can best proceed with the two competing "async conversion" PRs? (See context in #23 (comment)). I hope you folks can find a way to collaborate on that work in one way or another, since you both seem to have an interest in the project.

Copy link
Owner

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks a lot for your contributions, @g2p!

I'll handle the conflicts with the #30 and try to release a new version today or tomorrow.

@azasypkin azasypkin merged commit fa8fd0e into azasypkin:main May 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants