[Telepathy] Re: Some tp-glib feedback

[Simon McVittie]

On Thu, 29 Mar 2007 at 12:47:30 +0100, Simon McVittie wrote:
> 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.

Fixed in Darcs.

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

Fixed in Darcs.

> >   - For some reason the instance fields are missing from the documentation
> 
> I'm puzzled by that too, but will try to fix it.

Fixed in Darcs, eventually.

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

Fixed in Darcs.

> > * None of the *_return_from_<whatever> functions end up in the documentation. 
> 
> I'll see what I can do. More code-generation-fu...

Fixed in Darcs.

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

Still to be done.

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

Fixed in Darcs.

> > * tp_channel_factory_iface_request takes a guint handle, instead of TpHandle
> >   handle
> 
> Well spotted.

Fixed in Darcs.

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

Fixed in Darcs, sort of. Once I release Gabble 0.5.7, if you #define
_TP_CM_UPDATED_FOR_0_5_7 before including any headers, the first
argument will become GObject* and GObjectClass* instead of
TpSvcChannelInterfaceGroup* and TpSvcChannelInterfaceGroupClass*.
(It's just a case of relabelling a pointer which always points to a GObject
that implements TpSvcChannelInterfaceGroup, so no real change...)

In Gabble 0.5.8 (and in Darcs just after the release of 0.5.7) the #define
will no longer be necessary, it'll always be a GObject-based API.