[Telepathy] Empathy File Transfer review

[Marco Barisione]

Il giorno dom, 30/12/2007 alle 23.12 +0100, Xavier Claessens ha
scritto: 
> - extensions dir should be generated using new tp-glib stuff. See my
> tpGlibClient branch on my git repos [2].

I will fix this when I have time (unlikely before February).

> - empathy-ft-enums.h: Shouldn't that be generated by libtp/tp-glib ?

Yep, but it's not :)
I put them in a separate file so it can be removed easily when the FT
spec is reviewed and approved.

> - it should depend on glib >= 2.15.0 instead of gio. Or maybe it could
> check for glib >= 2.15 and fallback to gio if API is the same in both.

I know but I didn't have the time to fix it.

> - in EmpathyTpFT's constructor dbus_g_object_register_marshaller and
> dbus_g_proxy_add_signal calls are (or should be) done by generated code.

They are not with the script I'm using ATM.

> if (cond) {
> 	stuff;
> }

I will fix this.

> - get_time_msec() why not using API from empathy-time.h ?

Because it returns the time in milliseconds while
empathy_time_get_current() returns seconds.

> I'm not quiet sure about the separation between EmpathyTpFT and
> EmpathyFT. It seems EmpathyFT does nothing, it's just a wrapper around
> EmpathyTpFT functions. I didn't read the code in details but as I
> understand, one EmpathyTpFT object represents one FT channel which can
> contain many ongoing FTs. So I support each running FT are represented
> by one EmpathyFT object... so what's FT struct in EmpathyTpFT? It
> seems
> there is a circular depend between EmpathyTpFT and EmpathyFT. I have
> to
> read more in details the spec and implementation to have a better idea
> on that, but I have the intuition there is a problem here.
> What I imagined first was something like that:
>  - EmpathyTpFT does all the Telepathy calls and creates EmpathyFT
> objects.
>  - Instead of things like empathy_ft_accept(EmpathyFT*) that calls
> empathy_tp_ft_accept(EmpathyTpFT*,guint ft_id), I imagined something
> like empathy_tp_ft_accept(EmpathyTpFT*, EmpathyFT*). Like that
> EmpathyFT
> don't uses EmpathyTpFT API, that solves the circular dep.
> 
> So EmpathyTpFT does the Telepathy stuff and EmpathyFT does the IO
> stuff.

We discussed this at GUADEC, the problem is that an EmpathyFT is not so
useful without its EmpathyTpFT, so you will probably end up keeping an
EmpathyTpFT and an EmpathyFT (and you know how annoying is to pass more
than a single variable as user data to a callback).

Basically, EmpathyTpFT does everything and EmpathyFT can be considered
just as a tuple containing the EmpathyTpFT and an id. For instance
empathy_ft_accept(ft) is implemented as _empathy_tp_ft_accept(ft->tp_ft,
ft->id), where _empathy_tp_ft_accept() is not exported.


Note that there is some hacky code (added before deciding who should close a channel) to control the life cycle of EmpathyTpFT and EmpathyFT, it should be removed when MC will close FT channels without active file transfers.


-- 
Marco Barisione
http://www.barisione.org/