On Fri, 2017-03-10 at 21:24 +0100, Georg Chini wrote:
> On 10.03.2017 19:21, Tanu Kaskinen wrote:
> > On Fri, 2017-03-10 at 08:13 +0100, Georg Chini wrote:
> > > On 10.03.2017 00:33, Tanu Kaskinen wrote:
> > > > On Thu, 2017-03-02 at 17:04 +0100, Georg Chini wrote:
> > > > > -pa_bluetooth_backend *pa_bluetooth_native_backend_new(pa_core *c, pa_bluetooth_discovery *y) {
> > > > > +pa_bluetooth_backend *pa_bluetooth_native_backend_new(pa_core *c, pa_bluetooth_discovery *y, pa_bluetooth_backend *native_backend, bool enable_hs_role) {
> > > > > pa_bluetooth_backend *backend;
> > > > > DBusError err;
> > > > >
> > > > > + /* If the backend already exists just switch the HS role on or off */
> > > >
> > > > I find this interface weird. If the goal is just to switch the HS role
> > > > on or off, I think it would be better to have function
> > > > pa_bluetooth_native_backend_enable_hs_role().
> > > >
> > >
> > > It was the most simple way to achieve the goal. The function originally
> > > created
> > > the native backend. Now it creates the backend if it does not exist or
> > > switches
> > > the HS role on or off if the backend is already there. Otherwise I have
> > > to replace
> > > the calls to pa_bluetooth_native_backend_new() with something like
> > >
> > > if (native_backend)
> > > pa_bluetooth_native_backend_enable_hs_role()
> > > else
> > > pa_bluetooth_native_backend_new()
> > >
> > > And then I would still have to check within
> > > pa_bluetooth_native_backend_new()
> > > if the HS role needs to be enabled or not. So I do not see the advantage of
> > > implementing a separate function. If you prefer, I can change it anyway.
> >
> > The advantage is avoiding weird APIs. I expect foo_new() to create a
> > new instance of foo, not to sometimes create a new instance and
> > sometimes do something else. Am I the only one who finds the proposed
> > API weird?
> >
> > I don't think there's need to call pa_bluetooth_native_backend_new()
> > from pa_bluetooth_discovery_set_ofono_running() anyway, so there
> > shouldn't be need to compromise on simplicity either. Your patch
> > removes the only place where y->native_backend was being set to NULL
> > during y->ofono_backend's life time (when y->ofono_backend is NULL,
> > pa_bluetooth_discovery_set_ofono_running() won't get called).
> >
>
> We still need to call pa_bluetooth_native_backend_new() from
> pa_bluetooth_discovery_set_ofono_running() because with
> headset=auto the native backend is not created initially. It cannot
> be created there, because we don't know if ofono is running.
> I sent a new patch.
Oh, I missed the condition in get_managed_objects_reply() that prevents
native_backend from being created if ofono_backend is not NULL.
However, isn't it so that now we want to always set up the AG role
regardless of whether ofono is running or not? So
get_managed_objects_reply() can be changed so that native_backend is
always created when headset=auto.
--
Tanu
https://www.patreon.com/tanuk
