[pulseaudio-discuss] [PATCH v2] bluetooth: Fix bluetooth.nrec property not updated

[Tanu Kaskinen]

Hi,

Thanks for the patch! I've pushed it now, with a couple of minor changes
(see below).

Thanks also to Luiz for checking the patch too - I was a bit worried
that others might see the new version of the patch as unnecessarily
complex, given that the original patch was much simpler.

On Tue, 2012-06-12 at 15:49 +0200, Frédéric Danis wrote:
> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
> index b786502..42b7a05 100644
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -138,8 +138,13 @@ static pa_bluetooth_device* device_new(const char *path) {
>  }
>  
>  static void transport_free(pa_bluetooth_transport *t) {
> +    int i;

I changed this to unsigned, just because I was editing the patch anyway
and I think that it's better to use unsigned types with variables that
represent inherently non-negative things, like table indexes. I don't
think it was really a coding style violation, because it certainly isn't
mentioned in the CodingStyle wiki page and I may be the only one who
cares about this issue...

> +
>      pa_assert(t);
>  
> +    for (i=0; i < PA_BLUETOOTH_TRANSPORT_HOOK_MAX ; i++)

There should be spaces around the equality sign, but there shouldn't be
a space after PA_BLUETOOTH_TRANSPORT_HOOK_MAX.

(Same comments for transport_new().)

> +        pa_hook_done(&t->hooks[i]);
> +
>      pa_xfree(t->path);
>      pa_xfree(t->config);
>      pa_xfree(t);
> @@ -756,8 +761,10 @@ int pa_bluetooth_transport_parse_property(pa_bluetooth_transport *t, DBusMessage
>              dbus_bool_t value;
>              dbus_message_iter_get_basic(&variant_i, &value);
>  
> -            if (pa_streq(key, "NREC"))
> +            if (pa_streq(key, "NREC") && t->nrec != value) {
>                  t->nrec = value;
> +                pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED], t);

I changed the call data to NULL, because t is already passed as hook
data.

> +            }
>  
>              break;
>           }
> @@ -979,7 +986,7 @@ const pa_bluetooth_device* pa_bluetooth_discovery_get_by_path(pa_bluetooth_disco
>      return NULL;
>  }
>  
> -const pa_bluetooth_transport* pa_bluetooth_discovery_get_transport(pa_bluetooth_discovery *y, const char *path) {
> +pa_bluetooth_transport* pa_bluetooth_discovery_get_transport(pa_bluetooth_discovery *y, const char *path) {
>      pa_bluetooth_device *d;
>      pa_bluetooth_transport *t;
>      void *state = NULL;
> @@ -1085,6 +1092,7 @@ static int setup_dbus(pa_bluetooth_discovery *y) {
>  
>  static pa_bluetooth_transport *transport_new(pa_bluetooth_discovery *y, const char *path, enum profile p, const uint8_t *config, int size) {
>      pa_bluetooth_transport *t;
> +    int i;
>  
>      t = pa_xnew0(pa_bluetooth_transport, 1);
>      t->y = y;
> @@ -1097,6 +1105,9 @@ static pa_bluetooth_transport *transport_new(pa_bluetooth_discovery *y, const ch
>          memcpy(t->config, config, size);
>      }
>  
> +    for (i=0; i < PA_BLUETOOTH_TRANSPORT_HOOK_MAX ; i++)
> +        pa_hook_init(&t->hooks[i], t);
> +
>      return t;
>  }
>  
> diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h
> index 2752a69..ebffd3d 100644
> --- a/src/modules/bluetooth/bluetooth-util.h
> +++ b/src/modules/bluetooth/bluetooth-util.h
> @@ -63,6 +63,11 @@ enum profile {
>      PROFILE_OFF
>  };
>  
> +typedef enum pa_bluetooth_transport_hook {
> +    PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED,
> +    PA_BLUETOOTH_TRANSPORT_HOOK_MAX
> +} pa_bluetooth_transport_hook_t;

I added comments about the hook data and call data types - it sometimes
annoys me when these are not documented in the hook enum definitions and
I have to check it from the code that fires the hook instead.

> +
>  struct pa_bluetooth_transport {
>      pa_bluetooth_discovery *y;
>      char *path;
> @@ -71,6 +76,9 @@ struct pa_bluetooth_transport {
>      uint8_t *config;
>      int config_size;
>      pa_bool_t nrec;
> +
> +    /* hooks */

I removed this comment, because it appeared redundant to me.

> +    pa_hook hooks[PA_BLUETOOTH_TRANSPORT_HOOK_MAX];
>  };
> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index 4901ef9..8886f1e 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -2073,6 +2052,17 @@ static pa_hook_result_t source_state_changed_cb(pa_core *c, pa_source *s, struct
>      return PA_HOOK_OK;
>  }
>  
> +static pa_hook_result_t nrec_changed_cb(pa_core *c, pa_bluetooth_transport *t, struct userdata *u) {

The first parameter is what you pass in pa_hook_init(), so the type is
actually pa_bluetooth_transport *, not pa_core *.

> +    pa_assert(c);
> +    pa_assert(t);
> +    pa_assert(u);
> +
> +    pa_log_debug("property 'NREC' changed to value '%s'", t->nrec ? "True" : "False");

I feel that this log message belongs to the code that fires the hook,
because if there would be multiple users for the hook, they might all
print the same message if it wasn't done centrally where the hook is
originally fired.

> +    pa_proplist_sets(u->source->proplist, "bluetooth.nrec", t->nrec ? "1" : "0");

pa_source_update_proplist() needs to be used, otherwise no notifications
will be sent about the updated source proplist.

-- 
Tanu