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).
--
Tanu
https://www.patreon.com/tanuk
