[pulseaudio-discuss] [PATCH] pavucontrol: Show available status

[David Henningsson]

On 11/03/2012 09:22 PM, Colin Guthrie wrote:
> Hi,
>
> Did you ever get around to finishing this patch off David? I agree it's
> useful info to expose in pavucontrol.
>
> If you don't have time, would you like me to finish it of as per Tanu's
> comments?

Adjusting the patch to fit into to the work done by poljar (which wasn't 
written/merged at the time) probably means rewriting most of the patch.

I don't know if I'll have time/priority to do so in the near future. As 
such it'll be up to the two of you to decide whether you want to rewrite 
it, accept it as it is, or throw it away.

>
> Col
>
> 'Twas brillig, and Tanu Kaskinen at 21/07/12 09:09 did gyre and gimble:
>> On Fri, 2012-07-20 at 10:00 +0200, David Henningsson wrote:
>>> On 07/20/2012 09:09 AM, Tanu Kaskinen wrote:
>>>> On Thu, 2012-07-19 at 16:34 +0200, David Henningsson wrote:
>>>>> @@ -285,8 +285,22 @@ void MainWindow::updateCard(const pa_card_info &info) {
>>>>>        }
>>>>>
>>>>>        w->profiles.clear();
>>>>> -    for (std::set<pa_card_profile_info>::iterator i = profile_priorities.begin(); i != profile_priorities.end(); ++i)
>>>>> -        w->profiles.push_back(std::pair<Glib::ustring,Glib::ustring>(i->name, i->description));
>>>>> +    for (std::set<pa_card_profile_info>::iterator i = profile_priorities.begin(); i != profile_priorities.end(); ++i) {
>>>>> +        gchar* desc = NULL;
>>>>> +        int sums[3] = {0, 0, 0};
>>>>> +#ifdef HAVE_PORT_AVAILABILITY
>>>>> +        for (pa_card_port_info** cp = info.ports; *cp; cp++)
>>>>> +            for (pa_card_profile_info** pp = (*cp)->profiles; *pp; pp++)
>>>>
>>>> I'd prefer "port" instead of "cp" and "profile" instead of "pp" for
>>>> readability reasons. I guess you have different preferences?
>>>
>>> I usually follow the conventions around the code that I write, so that
>>> the code remains consistent in coding style. In this case, "i" (rather
>>> than "profile", "profile_iterator" etc) was used as iterator, so
>>> abbreviations were used for the inner iterators as well.
>>
>> Before your patch the "i" variable didn't cause any problems, because
>> the code was so simple. With multiple nested loops it becomes harder to
>> keep in mind what the variables refer to, especially because both port
>> and profile start with "p".
>>
>>>>> +                if ((*pp)->name == i->name && (*cp)->available < 3)
>>>>> +                    sums[(*cp)->available]++;
>>>>> +        if (sums[PA_PORT_AVAILABLE_NO] && !sums[PA_PORT_AVAILABLE_YES] && !sums[PA_PORT_AVAILABLE_UNKNOWN])
>>>>> +            desc = g_strdup_printf(_("%s (unplugged)"), i->description);
>>>>> +        else if (sums[PA_PORT_AVAILABLE_YES] && !sums[PA_PORT_AVAILABLE_NO] && !sums[PA_PORT_AVAILABLE_UNKNOWN])
>>>>> +            desc = g_strdup_printf(_("%s (plugged in)"), i->description);
>>>>
>>>> I don't like this kind of cleverness, I mean the sums array. Having the
>>>> array may help with minimizing the amount of lines of code, but it
>>>> relies on the AVAILABLE constants to have certain values, which in my
>>>> opinion is bad style.
>>>
>>> Yeah, it's not optimal. What do you think about this instead? It would
>>> be more resilient to us messing around with the available enum later on.
>>>
>>> bool hasYes = false, hasNo = false, hasOther = false;
>>> for (pa_card_port_info** cp = info.ports; *cp; cp++)
>>>       for (pa_card_profile_info** pp = (*cp)->profiles; *pp; pp++) {
>>>           if ((*pp)->name != i->name)
>>>               continue;
>>>           switch ((*cp)->available) {
>>>           case PA_PORT_AVAILABLE_YES:
>>>               hasYes = true;
>>>               break;
>>>           case PA_PORT_AVAILABLE_NO:
>>>               hasNo = true;
>>>               break;
>>>           default:
>>>               hasOther = true;
>>>               break;
>>>           }
>>>       }
>>> if (hasNo && !hasYes && !hasOther)
>>>       desc = g_strdup_printf(_("%s (unplugged)"), i->description);
>>> else if (hasYes && !hasNo && !hasOther)
>>>       desc = g_strdup_printf(_("%s (plugged in)"), i->description);
>>
>> This looks good to me, apart from the missing indentation in the switch
>> block. (And I'd also like to have braces for the outer for - I think we
>> agreed to make that an official coding style rule?)
>>
>>>>> diff --git a/src/pavucontrol.cc b/src/pavucontrol.cc
>>>>> index 72ec980..6a5ff8b 100644
>>>>> --- a/src/pavucontrol.cc
>>>>> +++ b/src/pavucontrol.cc
>>>>> @@ -426,6 +426,22 @@ void subscribe_cb(pa_context *c, pa_subscription_event_type_t t, uint32_t index,
>>>>>                        return;
>>>>>                    }
>>>>>                    pa_operation_unref(o);
>>>>> +
>>>>> +                /* Because the port availability might have changed, and we don't send
>>>>> +                   update events for that, we must update sources and sinks belonging.
>>>>> +                   to that card. Do it for all sources and sinks for simplicity. */
>>>>
>>>> Trailing whitespace after "belonging." Please do
>>>> "mv .git/hooks/pre-commit.sample .git/hooks/pre-commit" to prevent
>>>> trailing whitespace from slipping in the commits.
>>>
>>> Hmm, these are standard in pulseaudio, but not pavucontrol. If we think
>>> this is an issue, why not add the same checks to pavucontrol, so that
>>> these hooks become default on checkout?
>>
>> Right, I didn't know how it was done in pulseaudio. It turns out that
>> bootstrap.sh contains a snippet that sets up the pre-commit hook. I have
>> now copied that snippet to pavucontrol as well.
>>
>>>>> +                if (!(o = pa_context_get_sink_info_list(c, sink_cb, w))) {
>>>>> +                    show_error(_("pa_context_get_sink_info_list() failed"));
>>>>> +                    return;
>>>>> +                }
>>>>> +                pa_operation_unref(o);
>>>>> +
>>>>> +                if (!(o = pa_context_get_source_info_list(c, source_cb, w))) {
>>>>> +                    show_error(_("pa_context_get_source_info_list() failed"));
>>>>> +                    return;
>>>>> +                }
>>>>> +                pa_operation_unref(o);
>>>>
>>>> Do we really want to make two more round-trips here? The port
>>>> availability information is already included in the card info.
>>>
>>> As the comment explains: /* Do it for all sources and sinks for
>>> simplicity. */
>>>
>>> There is no object model that connects cards and sinks/sources in
>>> pavucontrol, but if there were, we could consider poking into the
>>> sink/source object, set its port available status, etc. That would
>>> likely at least triple the size of the patch, so it felt like much work
>>> for little gain (and two more round-trips are not that expensive, really).
>>
>> Having an object model that properly links ports to both cards and
>> sinks/sources would very likely be useful in the future also. Actually,
>> poljar has had the same problem with his latency offset feature. I don't
>> know if he already has a solution that you could re-use...
>
>
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic