[Telepathy] Code review: telepathy-gabble-olpc2

Simon McVittie simon.mcvittie at collabora.co.uk
Fri Apr 20 08:28:39 PDT 2007


Daf/Guillaume,

Some comments on the use of generated code in -olpc2:

In extensions/Makefile.am you say "# This is a derived file but must be
checked-in to Darcs anyway, for bootstrapping." This is no longer the
case.

It would perhaps be good to have an extensions.h or something rather than
#including the ugly generated names in _gen directly?

The "should be generated" interface name constants can be - I'll prepare
a patch.

I haven't read the C code thoroughly, but with my telepathy-glib hat on:

In the ActivityPropertiesChanged spec you don't specify whether the
signal contains the changed properties, or all the properties.
Ditto in the BuddyInfo.PropertiesChanged spec. I assume from context
that the answer is "all the properties".

In olpc.c:setaliases_foreach it would be nice if the comment "User has done
SetAliases on themselves" was moved above the added code.

olpc.c:free_gvalue is unnecessary, use tp_g_value_slice_free from
<telepathy-glib/util.h>.

In olpc_buddy_info_set_activities you call tp_handle_is_valid with
error=NULL, then construct an InvalidArgument error if it failed. You
could just use the error argument of tp_handle_is_valid: the message
would be "handle %u is not currently a valid room handle (type 2)".


More information about the Telepathy mailing list