[Telepathy] Review of Will's telepathy-glib requestotron branch

[Simon McVittie]

On Wed, 17 Sep 2008 at 18:35:16 +0100, Will Thompson wrote:
> On 17/09/08 05:51, Simon McVittie wrote:
> > I reviewed up to commit afa0599419bfd12b2663339673d469da37052623 (I'd have
> > looked for a newer version, but Three connectivity in Heathrow airport
> > seems to fail).
> >   
> Really? That's an unrelated commit made by you a week ago, just before
> the merge of the self-handle-changed branch, according to my git log…

Right, if I had been less tired I might have correctly said fba3d7e. I
have now reviewed up to eb424a3.

> I have made a requestotron-trivia branch, based on the requestotron
> branch, in which I have fixed the [trivia]-annotated items, to make
> reviewing the fixes to less trivial complaints more straightforward.

Ideal. I've re-reviewed and it all looks good (so just merge
requestotron-trivia into requestotron), except:

> > c_r_r_validate_handle: tp_handle_ensure (usually? always?) raises
> > NotAvailable on error, but CreateChannel is specified to raise
> > InvalidHandle (slightly bending the usual semantics) if the TargetID is
> > inappropriate - either propose a small spec patch, or transform the error
> > (you can just alter error->code in-place, I think)
> >   
> Fixed in 74c605f, altering error->code in place in the event of either
> TargetID or TargetHandle being invalid.

Can tp_handle_is_valid really ever raise something other than
InvalidHandle? No harm in what you did, though.

However... tp_handle_ensure could theoretically raise any GError of any
domain, so you should probably hack ->domain too.

> > Any thoughts on a better (than POINTER) GType for a
> > GHashTable<TpExportableChannel, GSList<request token> > as per the FIXME
> > I put in exportable-channel.c? We can't dup request tokens, though (the
> > TpChannelManager semantics are that the manager MUST forget about all requests
> > that it has mentioned in a signal) so trying to invent a boxed type may just
> > be an exercise in futility
> >   
> Presumably we could use (something equivalent to, if dbus_g_type_map is
> not meant to be use to construct random map gtypes):
> 
>     dbus_g_type_get_map ("GHashTable", TP_TYPE_EXPORTABLE_CHANNEL,
>     G_TYPE_POINTER);
> 
> but it's not obvious what that would buy us. I don't see a GType for
> GSLists, and again I don't really know what creating one would improve…

No, I think you're right. Let's keep POINTER - please drop the FIXME
comment.

    Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 155 bytes
Desc: Digital signature
Url : http://lists.freedesktop.org/archives/telepathy/attachments/20080917/fabbaeb2/attachment.pgp