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).
Cheers,
Mikel
