Bug 27397 - Need tp_account_set_avatar_{async,finish}
Summary: Need tp_account_set_avatar_{async,finish}
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Guillaume Desmottes
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ca...
Whiteboard: review?, minor changes, patch
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-31 08:09 UTC by Guillaume Desmottes
Modified: 2010-04-01 07:42 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Guillaume Desmottes 2010-03-31 08:09:34 UTC
<smcv> cassidy: it should probably take (const guchar *avatar, gsize len, const gchar *mime_type)
Comment 2 Simon McVittie 2010-04-01 05:56:04 UTC
(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.
Comment 3 Simon McVittie 2010-04-01 06:07:20 UTC
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).
Comment 4 Guillaume Desmottes 2010-04-01 07:16:10 UTC
(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.

Comment 5 Guillaume Desmottes 2010-04-01 07:42:35 UTC
Merged to master. Will be in 0.11.1


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.