On Tue, 2015-10-20 at 15:25 +0200, David Henningsson wrote:
>
> On 2015-10-20 06:20, Tanu Kaskinen wrote:
> > On Tue, 2015-05-05 at 17:01 +0200, David Henningsson wrote:
> > > diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> > > index 5c55cec..5d278c1 100644
> > > --- a/src/modules/module-card-restore.c
> > > +++ b/src/modules/module-card-restore.c
> > > @@ -375,8 +385,44 @@ finish:
> > > Â Â Â Â Â Â return PA_HOOK_OK;
> > > Â }
> > >
> > > +static const char* profile_name_for_dir(pa_card_profile *cp, pa_direction_t dir) {
> > > +Â Â Â Â if (dir == PA_DIRECTION_OUTPUT && cp->output_name)
> > > +Â Â Â Â Â Â Â Â return cp->output_name;
> > > +Â Â Â Â if (dir == PA_DIRECTION_INPUT && cp->input_name)
> > > +Â Â Â Â Â Â Â Â return cp->input_name;
> > > +Â Â Â Â return cp->name;
> > > +}
> > > +
> > > +static void update_profile_for_port(struct entry *entry, pa_card *card, pa_device_port *p) {
> > > +Â Â Â Â struct port_info *p_info;
> > > +Â Â Â Â const char *profilename;
> > > +
> > > +Â Â Â Â if (p == NULL)
> > > +Â Â Â Â Â Â Â Â return;
> > > +
> > > +Â Â Â Â if (!(p_info = pa_hashmap_get(entry->ports, p->name))) {
> > > +Â Â Â Â Â Â Â Â p_info = port_info_new(p);
> > > +Â Â Â Â Â Â Â Â pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> > > +Â Â Â Â }
> > > +
> > > +Â Â Â Â profilename = profile_name_for_dir(card->active_profile, p->direction);
> > > +Â Â Â Â if (pa_safe_streq(p_info->profile, profilename))
> > > +Â Â Â Â Â Â Â Â return;
> > > +
> > > +Â Â Â Â pa_xfree(p->preferred_profile);
> > > +Â Â Â Â p->preferred_profile = pa_xstrdup(profilename);
> >
> > I don't think updating the preferred sink or source belongs in module-
> > card-restore. module-card-restore should only concern itself with
> > updating the database, and restoring things when new cards appear.
> > pa_card_set_profile() seems like a better place to me to set
> > pa_device_port.preferred_device (my previous suggestion to name the
> > field "preferred_sink" was obviously bad, since we need to cover
> > sources too). I'd also use add a function to the pa_device_port API to
> > set the preferred device name, to keep writes to the struct
> > encapsulated within device-port.c.
>
> Trying to summarize the IRC discussion and add my own reflections:
>
> Moving the assignment to the preferred_profile variable is okay, I don't
> mind either way. (That said, I have a slight preference for keeping it
> as it is because it opens up for other modules using the field in other
> ways.)
> I assume that "restoring things when new cards appear" means that the
> code in card_new_hook_callback remains as it is (modulo a wrapper
> function in device-port.c)?
Yes.
> The rest: changing input_name / output_name to hashmaps of
> sinks/sources, and/or changing preferred_profile to
> preferred_sink/source (and have correct sink/source names) seems to grow
> out of proportion, to the extent that we need to change namereg to
> reserve all possible sink names at card creation time.
> While the comment that a port prefers a sink/source rather than a
> profile is technically correct, I don't see an easy split-up of all this
> that can fix some of your comments and doing everything is probably too
> much effort for little gain IMO.
>
> Would you like me to send out a new patch set with the assignment moved?
> Would it be okay to let the rest of the patches in as they are?
So you think it's ok to have pa_device_port.preferred_profile, even if
the field doesn't actually refer to a profile? I'm definitely not ok
with that. But if you think it's "too much effort for little gain" to
make the code correct, it's probably the least hassle if we apply your
patches, and I fix it up, since I have more motivation, assuming that
you won't be blocking patches that add name reservation to the namereg
etc. (By the way, name reservation is something that I have wanted on
several occasions, because it has caused trouble that we can't rely on
the name in the foo_new_data struct to be the final object name.)
--Â
Tanu
