On Sun, 2017-03-12 at 16:45 +0100, Georg Chini wrote:
> On 16.02.2017 11:09, Tanu Kaskinen wrote:
>
> "Refactor" is the wrong word, you are changing the logic.
> Maybe redesign is a better verb.
Indeed. I think I'll change it to "improve" ("redesign" feels a bit
strong).
> > Currently the default sink policy is simple: either the user has
> > configured it explicitly, in which case we always use that as the
> > default, or we pick the sink with the highest priority. The sink
> > priorities are currently static, so there's no need to worry about
> > updating the default sink when sink priorities change.
> >
> > I intend to make things a bit more complex: if the active port of a sink
> > is unavailable, the sink should not be the default sink, and I also want
> > to make sink priorities dependent on the active port, so changing the
> > port should cause re-evaluation of which sink to choose as the default.
> > Currently the default sink choice is done only when someone calls
> > pa_namereg_get_default_sink(), and change notifications are only sent
> > when a sink is created or destroyed. That makes it hard to add new rules
> > to the default sink selection policy.
> >
> > This patch moves the default sink selection to
> > pa_core_update_default_sink(), which is called whenever something
> > happens that can affect the default sink choice. That function needs to
> > know the previous choice in order to send change notifications as
> > appropriate, but previously pa_core.default_sink was only set when the
> > user had configured it explicitly. Now pa_core.default_sink is always
> > set (unless there are no sinks at all), so pa_core_update_default_sink()
> > can use that to get the previous choice. The user configuration is saved
> > in a new variable, pa_core.configured_default_sink.
> >
> > pa_namereg_get_default_sink() is now unnecessary, because
> > pa_core.default_sink can be used directly to get the
> > currently-considered-best sink. pa_namereg_set_default_sink() is
> > replaced by pa_core_set_configured_default_sink().
>
> It seems a bit strange to me, that module-default-device-restore and
> module-switch-on-connect set the configured_default_* variables, because
> these should be the user configured values.
> If a user has configured his default sink/source,
> module-switch-on-connect will change
> it if some new device (for example a bluetooth headset) turns up. This
> is then saved
> as the new default sink/source, so the original user setting is lost and
> on the next start
> of pulseaudio, it will use the (possibly not existent) device as the
> default.
> It also means, that if there is no user configured value, it suddenly
> will be there
> because module-default-device-restore saves the current default as the
> user default.
I don't think this is a regression. I tried to make as few behavioural
changes as possible. I'm aware that module-default-device-restore needs
to be improved, which requires changes in the core too. Basically, I
think configured_default_sink should be the sink name (a string) so
that it can remain set even when the configured sink is not present.
> I would use two variables, user_configured_default_* and
> system_configured_default_*
> and let module-default-device-restore save both values. When loading the
> configuration,
> the module can check if the system configured default still exists and
> else revert to
> the user setting. Module-switch-on-connect would use the system_ variables.
> The system_configured_default_* variables would take the role of the current
> configured_default_* variables and the user_configured_default_*
> variables would
> only get copied into them if the sink/source is valid.
>
> This would also mean, that the user can configure any value for the
> default, if the
> sink or source does not exist, it won't be used. The user configured
> value will
> also survive any temporary changes done by module-switch-on-connect.
I acknowledge that it feels a bit weird to have module-switch-on-
connect set a variable that's supposed to be set by the user, but
again, this is not a regression, and I'm not convinced that there is
any better alternative.
Regarding your proposal, you didn't specify in detail how the system
and user default devices are supposed to be prioritized. When a device
is plugged in, we certainly want to override the user's previous
configuration, which would suggest prioritizing the system default over
the user default. But if the user then manually selects some other
device as the default, that should override the system default. If the
rule is "whichever was set last wins", then we lose no information by
folding the two variables into a single default device variable.
> > +void pa_core_set_configured_default_sink(pa_core *core, pa_sink *sink) {
> > + pa_sink *old_sink;
> > +
> > + pa_assert(core);
> > +
> > + old_sink = core->configured_default_sink;
>
> Why do you use a new variable here? If you move the assignment after
> the pa_log_info() the variable is not necessary. This also applies to a few
> other functions.
I find the extra variable to make the code a bit clearer. I don't feel
very strongly about this, though, so if it bothers you, I can change
it.
Thanks for the review!
--
Tanu
https://www.patreon.com/tanuk
