[pulseaudio-discuss] [PATCH 39/56] bluetooth: Handle PropertiesChanged for org.bluez.MediaTransport1

[João Paulo Rechi Vita]

On Mon, Jul 22, 2013 at 11:04 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Fri, 2013-07-12 at 15:06 -0300, jprvita at gmail.com wrote:
>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> ---
>>  src/modules/bluetooth/bluez5-util.c | 84 +++++++++++++++++++++++++++++++++++++
>>  src/modules/bluetooth/bluez5-util.h |  1 +
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
>> index e29a19a..45052a2 100644
>> --- a/src/modules/bluetooth/bluez5-util.c
>> +++ b/src/modules/bluetooth/bluez5-util.c
>> @@ -253,6 +253,79 @@ bool pa_bluetooth_device_any_transport_connected(const pa_bluetooth_device *d) {
>>      return false;
>>  }
>>
>> +static pa_bluetooth_transport_state_t transport_state_from_string(const char* value, pa_bluetooth_transport_state_t *state) {
>> +    pa_assert(value);
>> +    pa_assert(state);
>> +
>> +    if (pa_streq(value, "idle"))
>> +        *state = PA_BLUETOOTH_TRANSPORT_STATE_IDLE;
>> +    else if (pa_streq(value, "pending") || pa_streq(value, "active"))
>> +        *state = PA_BLUETOOTH_TRANSPORT_STATE_PLAYING;
>> +    else
>> +        return PA_BLUETOOTH_TRANSPORT_STATE_INVALID;
>
> Here you return an enum value...
>
>> +
>> +    return 0;
>
> ...and here you return an int.
>
> It seems that the only purpose of PA_BLUETOOTH_TRANSPORT_STATE_INVALID
> is to signal error from transport_state_from_string(). Since returning
> -1 is the normal way to signal error, I think it's best to not have the
> INVALID state at all, and just change the return value of
> transport_state_from_string() to int.
>

Ok

>> +}
>> +
>> +static void parse_transport_property(pa_bluetooth_transport *t, DBusMessageIter *i) {
>> +    const char *key;
>> +    DBusMessageIter variant_i;
>> +
>> +    key = pa_bluetooth_dbus_check_variant_property(i);
>> +    if (key == NULL)
>> +        return;
>> +
>> +    dbus_message_iter_recurse(i, &variant_i);
>> +
>> +    switch (dbus_message_iter_get_arg_type(&variant_i)) {
>> +
>> +        case DBUS_TYPE_STRING: {
>> +
>> +            const char *value;
>> +            dbus_message_iter_get_basic(&variant_i, &value);
>> +
>> +            if (pa_streq(key, "State")) {
>> +                bool old_any_connected = pa_bluetooth_device_any_transport_connected(t->device);
>> +
>> +                if (transport_state_from_string(value, &t->state) == PA_BLUETOOTH_TRANSPORT_STATE_INVALID) {
>> +                    pa_log_error("Transport %s has an invalid state: '%s'", t->path, value);
>> +                    return;
>> +                }
>> +
>> +                pa_log_debug("Transport %s state changed to '%s'", t->path, value);
>> +                pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED], t);
>> +
>> +                if (old_any_connected != pa_bluetooth_device_any_transport_connected(t->device)) {
>> +                    t->device->dead = old_any_connected;
>> +                    pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED], t->device);
>
> If the device is dead, I suppose it should be freed after firing the
> hook.
>

I was reviewing the dead logic and I found that it was only useful in
the BlueZ 4 modules, so I'll completely remove it for the next series.

-- 
João Paulo Rechi Vita
http://about.me/jprvita