[pulseaudio-discuss] [PATCH] role-ducking: Add support for ducking group

[Sangchul Lee]

Thanks for your comment.
Before making new version of patch, I replied inline.

2016-03-24 20:46 GMT+09:00 Tanu Kaskinen <tanuk at iki.fi>:
> On Fri, 2016-02-19 at 22:22 +0900, Sangchul Lee wrote:
>>  struct userdata {
>>      pa_core *core;
>>      const char *name;
>> -    pa_idxset *trigger_roles;
>> -    pa_idxset *ducking_roles;
>> -    pa_idxset *ducked_inputs;
>> +    uint32_t n_group;
>
> I'd prefer "n_groups".

 OK, I'll revise it.

>
> Why is the type for volumes changed from pa_volume_t to char*? It's
> better to parse the volumes just once during the initialization rather
> than every time the volume values are needed.
>
> Rather than having four arrays containing the various attributes of a
> group, it's better to define a struct for a group and have just one
> array for storing the group structs.
>
> You could also add the volume factor name to the group struct so that
> it doesn't have to be constructed every time it's needed.

 I got what you mean. I'll try that. :)

>> +    u->trigger_roles = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
>> +    u->ducking_roles = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
>> +    u->ducked_inputs = pa_xmalloc0(u->n_group * sizeof(pa_idxset*));
>> +    u->volumes = pa_xmalloc0(u->n_group * sizeof(char*));
>> +    while (i < u->n_group) {
>
> Since you're going to loop exactly u->n_group times, a for loop would
> be the idiomatic choice.

 I'll replace it with a for loop.

>> +    roles = pa_modargs_get_value(ma, "trigger_roles", NULL);
>> +    if (roles) {
>> +        const char *group_split_state = NULL;
>> +        char *roles_in_group = NULL;
>> +        i = 0;
>> +        while ((roles_in_group = pa_split(roles, "/", &group_split_state))) {
>> +            if (roles_in_group[0] != '\0') {
>> +                const char *split_state = NULL;
>> +                char *n = NULL;
>> +                while ((n = pa_split(roles_in_group, ",", &split_state))) {
>> +                    if (n[0] != '\0')
>> +                        pa_idxset_put(u->trigger_roles[i], n, NULL);
>> +                    else
>> +                        pa_xfree(n);
>
> If an empty string is given as a role name, I think that should be an
> error.

 I understood your meaning that if an empty string is given as a role
name, then not proceed next step and return error. Am I right?

>> +                }
>> +                i++;
>> +            } else
>> +                pa_xfree(roles_in_group);
>
> Likewise, an empty group string should be interpreted as an empty role
> name, which should be an error.

 Similar as above.

>> +    volumes = pa_modargs_get_value(ma, "volume", NULL);
>> +    if (volumes) {
>> +        const char *group_split_state = NULL;
>>          char *n = NULL;
>> -        while ((n = pa_split(roles, ",", &split_state))) {
>> +        i = 0;
>> +        while ((n = pa_split(volumes, "/", &group_split_state))) {
>> +            pa_log_debug("%s", n);
>
> This log message seems like something that you meant to remove from the
> final version.

 You are right, it remains by mistake and will be removed.

>> +
>> +    for (i = 0; i < u->n_group; i++) {
>> +        if (pa_parse_volume(u->volumes[i], &volume) == -1) {
>> +            pa_log("Failed to parse a volume parameter: volume");
>
> This message looks strange to me (I know it was there already earlier).
> I think "Failed to parse volume" would be better.

 OK, no problem to fix.