Bug 27397

Implemented in http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/set-avatar-27397

(Please use the patch keyword to get things in my review queue, and assign things you're actively working on to yourself.)

I'd like a better description of the parameters' constraints, enforced with g_return_if_fail. I think these assertions would be appropriate:

* avatar != NULL || len == 0 (you can only use NULL if there's no data)
* mime_type != NULL (if you really, truly want it unspecified for some reason, you can use "")

> +tp_account_set_avatar_async (TpAccount *account,

I generally prefer the first argument of a method (or at least, a thing that would be a method if you were writing C++) to be called self; telepathy-glib isn't 100% consistent on this though.

> +  GArray tmp = { (gchar *) avatar, len };

Now that GArray is refcounted and otherwise magical, I'm uncomfortable with allocating it on the stack like this, at least in new code. Please allocate an array, and (if len is nonzero) append the data to it.

> +  g_value_init (&value, TP_STRUCT_TYPE_AVATAR);

tp_value_array_build() would be clearer, I think. (Yes we have new API \o/ )

> +  tp_cli_dbus_properties_call_set (TP_PROXY (account), -1,

The first argument of TpProxy's tp_cli methods is gpointer, so you don't need this cast.

> +      _tp_account_property_set_cb, result, NULL, G_OBJECT (account));

It's useless to pass @account as the weak object; tp_cli calls hold a ref to the proxy anyway, so this weak ref will never die.

Oh, also... at least where it's easy (like here), please squash the change to sections.txt into the commit that adds the API in the first place (an instance of the general rule about trying to avoid commits that fail the tests - undocumented or un-section'd API is a test failure).

(In reply to comment #2)
> I'd like a better description of the parameters' constraints, enforced with
> g_return_if_fail. I think these assertions would be appropriate:
> 
> * avatar != NULL || len == 0 (you can only use NULL if there's no data)
> * mime_type != NULL (if you really, truly want it unspecified for some reason,
> you can use "")

done.

> > +tp_account_set_avatar_async (TpAccount *account,
> 
> I generally prefer the first argument of a method (or at least, a thing that
> would be a method if you were writing C++) to be called self; telepathy-glib
> isn't 100% consistent on this though.

So am I. I just based my implementation on set_nickname.
Fixed.

> > +  GArray tmp = { (gchar *) avatar, len };
> 
> Now that GArray is refcounted and otherwise magical, I'm uncomfortable with
> allocating it on the stack like this, at least in new code. Please allocate an
> array, and (if len is nonzero) append the data to it.

As you prefer. Done.

> > +  g_value_init (&value, TP_STRUCT_TYPE_AVATAR);
> 
> tp_value_array_build() would be clearer, I think. (Yes we have new API \o/ )

done.

> > +  tp_cli_dbus_properties_call_set (TP_PROXY (account), -1,
> 
> The first argument of TpProxy's tp_cli methods is gpointer, so you don't need
> this cast.
> 
> > +      _tp_account_property_set_cb, result, NULL, G_OBJECT (account));

done.

> It's useless to pass @account as the weak object; tp_cli calls hold a ref to
> the proxy anyway, so this weak ref will never die.

done.

Merged to master. Will be in 0.11.1