On Thu, Apr 25, 2013 at 5:11 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
> Hi Jo?o Paulo,
>
> On Thu, Apr 25, 2013 at 9:36 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
>> Hi Jo?o,
>>
>> On Wed, Apr 24, 2013 at 7:51 PM, Jo?o Paulo Rechi Vita
>> <jprvita at openbossa.org> wrote:
>>> On Wed, Apr 24, 2013 at 3:29 AM, Mikel Astiz <mikel.astiz.oss at gmail.com> wrote:
>>>> Hi Jo?o Paulo,
>>>>
>>>> On Tue, Apr 23, 2013 at 10:48 PM, <jprvita at gmail.com> wrote:
>>>>> From: Jo?o Paulo Rechi Vita <jprvita at openbossa.org>
>>>>>
>>>>> According to the BlueZ API documentation the 'Alias' property should be
>>>>> preffered over the 'Name' property. Also, if the device doesn't have a
>>>>> remote name the 'Name' property may not be set, but the 'Alias' property
>>>>> will always be.
>>>>
>>>> Can you point to the documentation you refer to? 'Alias' is a
>>>> user-modifiable property, while 'Name' is what the remote device
>>>> reports. They just have different purposes, so I don't think one of
>>>> them is preferred over the other as a general rule.
>>>>
>>>
>>> doc/device-api.txt in BlueZ repository explicitly states that 'Alias'
>>> should be preferred over 'Name'. If you go back in history you'll see
>>> that it was first mentioned before BlueZ 4.0.
>>
>> I'm still not convinced. These are two different properties, both
>> exposed by BlueZ for some reason, and it can be misleading that we
>> start using a different terminology inside PA, and map alias->name.
>>
>> I'm fine encouraging the use of 'bluez.alias' over 'bluez.name', but
>> renaming it internally should be avoided IMO.
>>
>>>
>>>>> ---
>>>>> src/modules/bluetooth/bluetooth-util.c | 7 +------
>>>>> src/modules/bluetooth/bluetooth-util.h | 1 -
>>>>> src/modules/bluetooth/module-bluetooth-device.c | 4 ++--
>>>>> 3 files changed, 3 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>>>>> index c15ecd1..b0ee62c 100644
>>>>> --- a/src/modules/bluetooth/bluetooth-util.c
>>>>> +++ b/src/modules/bluetooth/bluetooth-util.c
>>>>> @@ -179,7 +179,6 @@ static pa_bluetooth_device* device_new(pa_bluetooth_discovery *discovery, const
>>>>>
>>>>> d->device_info_valid = 0;
>>>>>
>>>>> - d->name = NULL;
>>>>> d->path = pa_xstrdup(path);
>>>>> d->paired = -1;
>>>>> d->alias = NULL;
>>>>> @@ -228,7 +227,6 @@ static void device_free(pa_bluetooth_device *d) {
>>>>> uuid_free(u);
>>>>> }
>>>>>
>>>>> - pa_xfree(d->name);
>>>>> pa_xfree(d->path);
>>>>> pa_xfree(d->alias);
>>>>> pa_xfree(d->address);
>>>>> @@ -364,10 +362,7 @@ static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i, boo
>>>>> const char *value;
>>>>> dbus_message_iter_get_basic(&variant_i, &value);
>>>>>
>>>>> - if (pa_streq(key, "Name")) {
>>>>> - pa_xfree(d->name);
>>>>> - d->name = pa_xstrdup(value);
>>>>> - } else if (pa_streq(key, "Alias")) {
>>>>> + if (pa_streq(key, "Alias")) {
>>>>> pa_xfree(d->alias);
>>>>> d->alias = pa_xstrdup(value);
>>>>> } else if (pa_streq(key, "Address")) {
>>>>> diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h
>>>>> index 3361b0f..1040d5b 100644
>>>>> --- a/src/modules/bluetooth/bluetooth-util.h
>>>>> +++ b/src/modules/bluetooth/bluetooth-util.h
>>>>> @@ -120,7 +120,6 @@ struct pa_bluetooth_device {
>>>>> int device_info_valid; /* 0: no results yet; 1: good results; -1: bad results ... */
>>>>>
>>>>> /* Device information */
>>>>> - char *name;
>>>>
>>>> I don't see the removal as part of this patch, regardless of what you
>>>> do with pa_proplist_sets() below.
>>>>
>>>
>>> Ok, I can split it in a separate patch.
>>>
>>>>> char *path;
>>>>> pa_bluetooth_transport *transports[PA_BLUETOOTH_PROFILE_COUNT];
>>>>> int paired;
>>>>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
>>>>> index cd0a515..ddf658f 100644
>>>>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>>>>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>>>>> @@ -2237,7 +2237,7 @@ static int add_card(struct userdata *u) {
>>>>> data.driver = __FILE__;
>>>>> data.module = u->module;
>>>>>
>>>>> - n = pa_bluetooth_cleanup_name(device->name);
>>>>> + n = pa_bluetooth_cleanup_name(device->alias);
>>>>> pa_proplist_sets(data.proplist, PA_PROP_DEVICE_DESCRIPTION, n);
>>>>> pa_xfree(n);
>>>>> pa_proplist_sets(data.proplist, PA_PROP_DEVICE_STRING, device->address);
>>>>> @@ -2250,7 +2250,7 @@ static int add_card(struct userdata *u) {
>>>>>
>>>>> pa_proplist_sets(data.proplist, "bluez.path", device->path);
>>>>> pa_proplist_setf(data.proplist, "bluez.class", "0x%06x", (unsigned) device->class);
>>>>> - pa_proplist_sets(data.proplist, "bluez.name", device->name);
>>>>> + pa_proplist_sets(data.proplist, "bluez.name", device->alias);
>>>>
>>>> What's the purpose of replacing this existing property instead of
>>>> adding 'bluez.alias'?
>>>>
>>>
>>> As mentioned before, we should only use the 'Alias' value and not the
>>> 'Name' one, so there is no point in storing it. I'm not sure when
>>> (regarding PulseAudio release versions) we can change the property
>>> from 'bluez.name' to 'bluez.alias', since IIRC these properties are
>>> exposed to PulseAudio clients.
>>
>> I'd first add bluez.alias and we can discuss afterwards whether
>> bluez.name should be dropped. Changing the internal mapping seems
>> obscure and error-prone to me, not to mention that we'd be breaking
>> the API if any client is actually using this property, strictly
>> speaking.
>>
>>>
>>>> Btw, I'm not seeing any user of this property so I'm wondering why
>>>> you're interested in such a change. Is it for development and testing
>>>> purposes?
>>>>
>>>
>>> As said previously, IIRC these properties are available for PulseAudio
>>> clients as a source of information about the card. But what led me
>>> into this was a misbehavior introduced by the patch 2/16 on your
>>> series, found while doing PTS testing. There on
>>> parse_device_properties() you're treating this property as mandatory,
>>
>> BlueZ's documentation guarantees that this property is always present,
>> including 4.99 (which we depend on) and any later version.
>>
>>> but if the remote name is not set (which is the case for PTS) the
>>> 'Name' property is not present on the device object in BlueZ and the
>>> device information in PulseAudio is assumed invalid. That lead to
>>> problems when trying to associate a transport with a device.
>>
>> I also checked the implementation in several BlueZ versions. In BlueZ
>> 4, the property seems to be added always, so the issue you mention
>> should not exist
>>
>> Also BlueZ 5 should guarantee (according to the documentation) that
>> the property exists, but the implementation says otherwise, starting
>> with 826023de. One or the other is wrong, so we have to clarify this
>> first (patch sent to BlueZ's mailing list).
>
> This has been confirmed to be a bug in BlueZ 5's documentation and it
> has now been fixed.
>
> Based on this, I propose the following steps:
> 1. Add "bluez.alias" property.
> 2. Use the alias as PA_PROP_DEVICE_DESCRIPTION.
> 3. Remove the "bluez.name" property.
>
I agree.
> This is basically a split version of your patch, except that you're
> removing the name from the internal data structure (which IMO is not
> necessary).
>
I don't see why we should keep the name field in the internal
structure after the 3rd step above, since there will be any users of
it left.
> I believe the first two patches should make it to the stable branch,
> and perhaps even the third one.
>
I would like to hear the opinion of either Tanu, Arun or Colin (since
they're the ones I believe have been taking care of the releases
lately) about when it's better to break the API. Since we already had
a release candidate (if I understood correctly the release process) I
think patch 3 (and possibly 4) should go to next instead of master and
patches 1 and 2 should go to both master and next.
> After this, I'd update the Bluez5 patchset according to the new
> scenario where 'Name' is an optional property.
>
Ok. BTW, I'm doing a little bit of experimenting in the split backends
idea as well, to see if we can have BlueZ 4 and 5 as backends separate
from the core. If we conclude that it's better to do that way, we can
start merging things in the following order:
1. create backends infrastructure
2. split bluez4 out of the core as a backend
3. add bluez4 support as a backend
4. add ofono as a backend
My plan is to finish this experimenting this week, so we can have a
decision on the beginning of next week.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
