Update pulsectl.py

[davinellulinvega]

When I tried to use the volume_change_all_chans, I did not check if the volume of all channels was greater than the increment. This resulted in the code trying to set a negative value for the volume. It made the whole script crash without any real error message. Thus, to avoid that it might be interesting to make sure that all your values are within the correct range. I know that 0.0 is the minimum, but i am unsure about 1.0 being the maximum. You might be able to set a higher value than one resulting in a soft-boost.

[mk-fg]

I think negative value for volume coefficient makes no sense, and code that passes it must have some error, and correct way to resolve such case is to not to hide error here, but fix the code that passes the thing. Same as if code e.g. passes a string there, or whatever other nonsense.

Guess it can be documented how this value works though, as it doesn't correspond to one in libpulse exactly.

[mk-fg]

Copied https://github.com/mk-fg/python-pulse-control/pull/1#issuecomment-206317987 to the README here: https://github.com/mk-fg/python-pulse-control/#volume Hope it'd help to avoid same confusion for the next person looking at that code.

Didn't add any special handling for negative values, as mentioned, since I don't think they should be allowed and silently modified to mean "mute" without raising errors.

[mk-fg]

It randomly occured to me that for volume_change thing, it'd make perfect sense to cap min volume at 0, and that's what you actually mentioned in the commit message (which I kinda skimmed-over and missed initially), while I only considered the volume_set case (probably from looking at the code diff). Guess better late than never.

[davinellulinvega]

Sorry for the late answer but I was quite busy this week. I agree with both your first and third comments. My solution was incomplete and maybe a little bit rushed. The change_all_chans should definitely contain some check on the volume to make sure it remains over 0.0. For the set_volume function, where I originally made the change, it makes sense to leave the user in charge of making sure that the volume is over 0. But I would still have put an assert or raise an error with a meaningful message, because as thing are for the moment, when you set the volume to a negative value, the script simply hangs with no explanation whatsoever and your only solution is to kill the process. That is the main reason as to why I originally made the modifications in the set_volume function and not in the change_all_chans, so that the function would fail "gracefully".

[mk-fg]

Oh, yeah, apparently another thing that I missed in the original report is without any real error message, thanks for repeating it. Will look into that, probably some check missing in callback.

[davinellulinvega]

Glad if I have been of some help. Keep up the good work.

Sincerely.

[mk-fg]

Apparently forgot to add check for whether all pa_operation* calls return immediate failure, such as pa_cvolume_valid(volume) check in this case. Should be fixed now for all such things - PulseOperationInvalid raised in these cases, with proper message from pa_strerror() (e.g. pulsectl.pulsectl.PulseOperationInvalid: Invalid argument for negative volume).