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