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

[Tanu Kaskinen]

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

-- 
Tanu