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.
