[Telepathy] Re: Some tp-glib feedback

[Simon McVittie]

(Redirected to telepathy at l.f.o from private mail)

On Thu, 29 Mar 2007 at 12:49:24 +0200, Sjoerd Simons wrote:
> * TpDynamicHandleRepo
>     - Not documented that default-normalize-context and normalize-function
>       properties are not mandatory or what their default is

Good point, I'll fix that. For reference, the default normalize function
is (equivalent to) g_strdup and the default default-normalize-context is
NULL.

> * TpBaseConnection
>   - TpBaseConnectionProcWithError start_connecting says that it must calculate
>     and ref the self_handle. But salut can only calculate it during the
>     connection (as soon as the name is estabished on dns-sd)

I think it should be OK to change start_connecting's behaviour to be
"by the time you call t_b_c_change_status (CONNECTED), you must have
calculated and reffed the self handle".

>   - For some reason the instance fields are missing from the documentation

I'm puzzled by that too, but will try to fix it.

>   - Reading the documentation in the header it's not clear which instance 
>     fields are meant to be set by the base class and which by the subclass

Fair point, I'll fix that.

> * None of the *_return_from_<whatever> functions end up in the documentation. 

I'll see what I can do. More code-generation-fu...

I need to document the generated code in a general way, really - a
chapter in the API docs entitled "How to use the TpSvc... GInterfaces"
or something.

> * ERROR_IF_NOT_CONNECTED_ASYNC would be nice to have as a utility on the base
>   connection.

You'll probably still want a #define, the properly namespaced name will be a
bit unwieldy... but yes.

> * tp_channel_factory_iface_request takes a guint handle, instead of TpHandle
>   handle

Well spotted.

> * tp_channel_factory_iface_request, the request argument must be include if the
>   signalled new-channel's handle is 0.. but handles can't be 0 right? Also
>   handle and handle_type can be 0, but i'm unsure how to handle those cases (or
>   why they would ever be 0)

The channel might not really have a handle, which is modelled as "handle
0 of type 0". This asks for a new "anonymous" channel: for instance, media
channels are created with handle/type 0, then you add the callee to the
channel afterwards. If Salut chat rooms are unnamed but just exist as a
tuple of (multicast IP, port, key) or something, then they should have
"handle" 0 too.

> * The Test mixin functions want a gobject instance, the group mixin 
>   functions want a TpSvcChannelInterfaceGroup.. Would be good to be more
>   consistent here :)

Perhaps they should both take a gpointer to avoid unnecessary casts...

The motivation for some of the group mixin functions taking a
TpSvcChannelInterfaceGroup* was to make them usable as implementations
for the GInterface methods straight away - some of the group methods
were also public API though, so I changed the rest of the group API in
order to be consistent. With hindsight, that was probably a bad move.

-- 
Simon McVittie, Collabora Ltd.: http://www.collabora.co.uk/