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

[David Henningsson]

On 07/20/2012 09:09 AM, Tanu Kaskinen wrote:
> On Thu, 2012-07-19 at 16:34 +0200, David Henningsson wrote:
>> If we know if a certain port is available/unavailable, we can print
>> that out, as a help to the user (and as debugging for ourselves).
>> A profile is also available/unavailable if all ports which have that
>> profile is available/unavailable.
>> ---
>>   src/mainwindow.cc  |   44 ++++++++++++++++++++++++++++++++++++++------
>>   src/pavucontrol.cc |   16 ++++++++++++++++
>>   src/pavucontrol.h  |    1 +
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mainwindow.cc b/src/mainwindow.cc
>> index dc84682..b843d03 100644
>> --- a/src/mainwindow.cc
>> +++ b/src/mainwindow.cc
>> @@ -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.

>> +                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);



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

 > Also, the period after "belonging" shouldn't be there.

True. Will be fixed.

>> +                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).



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