[Telepathy] Code review: telepathy-gabble-olpc2

[Dafydd Harries]

Ar 20/04/2007 am 16:28, ysgrifennodd Simon McVittie:
> 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.

Fixed.

> 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.

Done.

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

Done.

> 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)".

Done.

Thanks!

-- 
Dafydd