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

[api] Allow to set volume as a relative step count #273

Merged
merged 1 commit into from
Dec 6, 2020
Merged

[api] Allow to set volume as a relative step count #273

merged 1 commit into from
Dec 6, 2020

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Dec 6, 2020

Fixes #272 by allowing a client to increase the volume by 10 steps (or decrease it by 5 steps).

To be discussed:

  • I don't accept both volume and step being present, but we could change this to give precedence to volume instead.
  • As the player already has volume clamping in its volumeUp/volumeDown logic, I don't validate that step is in any range.
  • The player could have a unique changeVolume(int steps) method instead of overloading volumeUp and volumeDown
# Valid requests
$ curl -X POST -d "step=10" localhost:24879/player/set-volume
$ curl -X POST -d "step=-5" localhost:24879/player/set-volume
$ curl -X POST -d "volume=30000" localhost:24879/player/set-volume

# Invalid input: both present
$ curl -X POST -d "step=15&volume=30000" localhost:24879/player/set-volume
{"name":"step","reason":"Cannot be passed alongside volume"}

# Invalid input on existing volume param
$ curl -X POST -d "volume=-30000" localhost:24879/player/set-volume
{"name":"volume","reason":"Must be >= 0 and <= 65536"}

# Invalid input on new step param
$ curl -X POST -d "step=0" localhost:24879/player/set-volume
{"name":"step","reason":"Must be non zero"}
$ curl -X POST -d "step=A" localhost:24879/player/set-volume
{"name":"step","reason":"Not an integer"}

@devgianlu
Copy link
Member

  • Good
  • Good
  • I am keeping the those methods for backward compatibility

I thought you wanted to bypass the step value and step up/down the volume by an arbitrary amount, but it's perfectly fine this way too.

@devgianlu devgianlu merged commit f69d3ed into librespot-org:dev Dec 6, 2020
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.

[api] Allow better control of relative volume change
2 participants