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.
