Skip to content

Initial support for Websockets connection to Snapcast Server #60

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

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

arpena
Copy link

@arpena arpena commented Nov 17, 2023

Hi, I've added the support for Websockets to your wonderfully simple snapcast control interface. I'm currently using it on my not yet public snapvolctl project, a daemon to control a soundbar or other component using snapcast volume and playing updates.
Hope you find useful.

@happyleavesaoc
Copy link
Owner

Hi, this is an interesting PR! Please add a few tests, and restore the setup.py named fields, and we should be good to merge. After that I'll cut and push a new release.

@arpena
Copy link
Author

arpena commented Nov 24, 2023

Thanks ! It is working quite well in my environment and it is very stable.
For the merge, I've restored the setup.py and corrected some pylint errors issued by tox on a newer commit. Should I create a new pull requests using that?

I'm not familiar with module testing on python, but from what I see in your tests, there is no connection to an actual server to test the client... so I believe that my modifications hadn't change the tests behavior.
Is there some test that you have in mind that would test better the connection interaction?

@happyleavesaoc
Copy link
Owner

You can just push the changes to this branch and it should update this PR.

The tests mock the server (basically intercepting requests that would hit the server, and returning known data). It might be a pain to test websocket_handler that way, but SnapcastWebSocketProtocol should testable using the same pattern.

@luar123
Copy link
Contributor

luar123 commented Apr 23, 2024

Hi, thanks for catching up and providing tests. Could you please create a new PR from this? I have some small comments, but GitHub shows now all changes including those of already merged commits, which makes it hard to review.
Next time maybe use rebase instead, keeps the history clean. (Here is a good explanation: https://developers.home-assistant.io/docs/development_catching_up)
And just out of couriosity: what is the benefit of websockets compared the RPC?

@SantiagoSotoC
Copy link
Contributor

Hi, thanks for catching up and providing tests. Could you please create a new PR from this? I have some small comments, but GitHub shows now all changes including those of already merged commits, which makes it hard to review. Next time maybe use rebase instead, keeps the history clean. (Here is a good explanation: https://developers.home-assistant.io/docs/development_catching_up) And just out of couriosity: what is the benefit of websockets compared the RPC?

I think there is no advantage of using websockets vs a raw tcp socket unless you are in the browser.
https://medium.com/kifi-engineering/websockets-vs-regular-sockets-b3b8e7ea0708

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.

4 participants