[Telepathy] Code review: telepathy-gabble-olpc2
dafydd.harries at collabora.co.uk
Fri Apr 20 09:12:08 PDT 2007
Ar 20/04/2007 am 16:28, ysgrifennodd Simon McVittie:
> 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
> It would perhaps be good to have an extensions.h or something rather than
> #including the ugly generated names in _gen directly?
Good idea. Done.
> 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".
Yes. Not sure how to word this though.
> 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
> 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