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

feat: option for audio format #1082

Merged
merged 2 commits into from
Mar 23, 2023
Merged

feat: option for audio format #1082

merged 2 commits into from
Mar 23, 2023

Conversation

sapristi
Copy link
Contributor

Add support for audio-format configuration.

Closes #1081

eladyn
eladyn previously approved these changes Apr 19, 2022
Copy link
Member

@eladyn eladyn 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 and works for me:

$ target/debug/spotifyd --no-daemon --audio-format "S32"
Loading config from "/home/user/.config/spotifyd/spotifyd.conf"
No password specified. Checking password_cmd
No password_cmd specified
No proxy specified
Using software volume controller.
Connecting to AP "ap.spotify.com:443"
Authenticated as "user" !
Using Alsa sink with format: S32
Country: "DE"
...

Now we'll just have to wait for maintainers to come around and review this. ☺️

src/main_loop.rs Outdated Show resolved Hide resolved
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Just two additional nitpicks. Looking over config.rs I have some additional improvements in mind like switching to clap or resurrecting this PR, but those are more general and definitely out-of-scope for this PR.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
@sapristi sapristi force-pushed the master branch 2 times, most recently from 9c510cc to cf52e80 Compare April 20, 2022 14:51
eladyn
eladyn previously approved these changes Apr 20, 2022
@sapristi
Copy link
Contributor Author

sapristi commented Apr 20, 2022

In the derive block, at least Deserialize is necessary. Otherwise I really don't have an opinion on this :)

Also not much used to github PR, how the hell is there no comment thread ? Or am I missing something ?

@eladyn
Copy link
Member

eladyn commented Sep 9, 2022

@sapristi With the recent PRs that have been merged, this PR now has merge conflicts. If you could resolve them, maybe we can get that into the next release?

@slondr slondr added the waiting for feedback Issues that need user feedback for e.g. log files label Mar 3, 2023
@eladyn eladyn force-pushed the master branch 2 times, most recently from fc84cfd to 62bee6d Compare March 22, 2023 01:34
@eladyn
Copy link
Member

eladyn commented Mar 22, 2023

I took the liberty to take this PR and rebase it on the current master. So this could potentially also go into the next release.

@slondr slondr added this to the v0.3.5 milestone Mar 22, 2023
@slondr slondr added enhancement A new feature that would improve Spotifyd and removed waiting for feedback Issues that need user feedback for e.g. log files labels Mar 22, 2023
slondr
slondr previously approved these changes Mar 22, 2023
Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

awesome

@slondr slondr merged commit 08fedd2 into Spotifyd:master Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature that would improve Spotifyd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify Alsa sink format
3 participants