On 04/07/2014 04:46 PM, Tanu Kaskinen wrote:
> On Mon, 2014-04-07 at 19:59 +0600, Alexander E. Patrakov wrote:
>> 07.04.2014 18:24, Tanu Kaskinen wrote:
>>>
>>> +/* Called from the main thread. */
>>> +static void set_reference_volume_internal(pa_sink *s, const pa_cvolume *volume) {
>>> + pa_cvolume old_volume;
>>> + char old_volume_str[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
>>> + char new_volume_str[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
>>> +
>>> + pa_assert(s);
>>> + pa_assert(volume);
>>
>> Note the assertions.
>>
>>> +
>>> +/* Called from the main thread. */
>>> +void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
>>> + pa_assert(s);
>>> + pa_assert(volume);
>>> +
>>> + set_reference_volume_internal(s, volume);
>>> +}
>>
>> What's the point of having this function in this patch? As demonstrated
>> above, set_reference_volume_internal does the same assertions.
>
> The "point" is in the function naming: I like to have a function called
> set_reference_volume_internal(), but it makes no sense to have a
> function with that name exported outside the .c file, so I added another
> function for interfacing with the outside world. I can't give good
> reasons for why I like the name set_reference_volume_internal(), so I
> think it's best that I drop set_reference_volume_internal() and move the
> implementation inside pa_sink_set_reference_volume_direct().
I've skimmed through the patches and it looks like a good refactoring.
And I don't mind extra hooks for changing volume, but the hooks should
fire when mute status changes too (or possibly, we should have another
set of hooks for that).
A question about the name - pa_sink_set_reference_volume_direct, is
there also an indirect way of setting reference volume? Or why is it not
simply just called pa_sink_set_reference_volume?
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
