[pulseaudio-discuss] [PATCH 1/4] sink, source: Assign to reference_volume from only one place

[Tanu Kaskinen]

On Tue, 2014-04-08 at 09:12 +0200, David Henningsson wrote:
> 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).

We should have another set of hooks for that. I'm currently working on
mute controls for the Tizen volume API, and I will need mute hooks for
that, so patches for that should appear soonish.

> 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?

The word "direct" refers to the lack of extra stuff happening. The
"indirect" way is to use pa_sink_set_volume(), which is the primary
interface for setting the reference volume, but that function does more
stuff than just update the reference_volume variable.

I don't mind calling the function pa_sink_set_reference_volume(), so
I'll change the name to that.

-- 
Tanu