[Telepathy] Dispatcher branch comments

[Sjoerd Simons]

Hi guys,

Both Xavier and Cosimo commented on my empathy dispatcher branch (thanks!).
Most things are uncontroversial, but some things ask for some
explanation/discussion. Hence this mail :). It's long but some of it is usefull
for a larger audience as well, especially the later parts.

On Tue, Dec 23, 2008 at 07:00:15PM +0100, Cosimo Cecchi wrote:
> - in empathy_tp_chat_acknowledge_messages() you could avoid the
> EMPATHY_MESSAGE cast;

EMPATHY_MESSAGE is not just a cast, it's also a run-time type check. I prefer
to use them as it's a good indicator when things are totally fucked up and
much more clear then a random segv in some unrelated place :)

> also you could declare the variables |m|, | message| and |id| on top of the
> function and reassign the values; that is more clear to me and also avoids
> the creation of some variables.  Also, is there a reason why you copy the
> list there?

The list can be (and is in most cases) the list returned from
empathy_tp_chat_get_pending_messages. Which is directly the internal list from
the messages queue. empathy_tp_chat_acknowledge_messages modifies this list,
which is something you don't want to do on the list you're traversing, hence
the copy.

I've pondered about letting empathy_tp_chat_get_pending_messages return a copy
of the list. But that would need to get a ref on all messages and thus be freed
by a special function, which is inconvenient and very easy to get wrong.

> - why do you have a dispose_has_run variable? can _dispose be called
> multiple times?

Yes. Running dispose multiple times can happen, when purely in C code it's
uncommon. But (python/ruby/etc) bindings can cause it (when they try to break
reference loops). This is also why you should always have a _dispose if you
have refs to other objects. In empathy i often only see _finalize functions,
which is basically a bug.

Also note that functions on the object can potentially be called after dispose
has run (although it's very uncommon);

> empathy-chatroom-manager.c
> - being both this and the dispatcher a singleton, why do you have a
> (public) empathy_chatroom_observe() called from the dispatcher? Isn't it
> better to get the dispatcher in chatroom_manager_init () and connect to
> the signal there?

Maybe. The problem is that we might want to have the dispatcher used in
multiple processes (currently not really possible). In which case you don't
want all the processes writing to .gnome2/Empathy/chatrooms.xml. Which is also
an explicit FIXME in that code. In a later patch i'll probably move that to
empathy.c instead..

> Just as a side question: which is the usual order in which the
> dispatcher signals are called for a new channel? From the code I can
> guess it's (OBSERVE, APPROVE, DISPATCH), is that right? To which
> connection/channel events do these signals relate?

It's slightly more complicated, but not much. A DispatchOperation has two
important concepts. Approving and Claiming. Approving is only applicable to
incoming channels. It basically means that the user wants to accept an incoming
channel, for example for a Filetransfer this indicated the user wants to
receive the file. For a streamed media channel it means start the call etc.
It's important to note that no modifications to the channel should be done
during approving (as in one is only allowed to inspect the channel).

Claiming the channel means you want to handle it and afterwards you're allowed
to do modifications to the channel. Note that an approver is allowed to claim
the channel, this is usefull for cases where the user wants to reject the
channel. In that case the approver claims the channel and closes it (after some
potential channel type specific operation).

So when new channel comes in (either by means of return from a request or by a
NewChannel(s) signal) the following happens:

  * The TpChannel and the channel wrapper (dependant on the channel type) are
    created and the DispatchOperation waits untill both of those are ready.
  * OBSERVE is emitted. Observers aren't allowed to do any modifications to
    the dispatch operation. But they get a chance to see the channel before
    anything that would modify it. This is usefull for things like loggers.
  * The callbacks of all requestors is called with the operation. Requestors
    are allowed to claim the channel. In which case dispatching stops here.
  * APPROVE is emitted if the channel is incoming and wasn't already approved
    (a channel that was requested is by definition approved). In case the
    approve claims the channel, dispatching stops here. If the operation is
    approved dispatching continues to the next step.
  * DISPATCH is emitted. One of the objects listening to this signal is assumed
    to claim it.

I haven't implemented the case where nobody claims it yet.  When it a channel
requested by empathy, we should probably close it (but that should never
happen).. When it was an incoming channel, we apparently couldn't handle it
(channel type we don't know about..), but something else might, so closing it
is not the right thing, so we probably should ignore it.. Ofcourse in the
future the real ChannelDispatcher would take care of this properly, as it knows
who can handle which channels. I don't think it's usefull to worry too much
about this now tough..

Also i should make a flowchart of this and put it on the empathy wiki.

Xaviers comments:
>- libempathy-gtk/empathy-chat.c
>  * Why do you call show_pending_messages from chat_create_ui and
>  empathy_chat_set_tp_chat? I think you can remove from chat_create_ui.

On EmpathyChat construction empathy_chat_set_tp_chat is called before there is
a UI. When a contact disconnected and came back again, empathy_chat_set_tp_chat
is called after the UI got created.  Hence why we need to call
it in both places.

This wasn't necessary before as the TpChat got recreated and reemitted the
signals on reentering the mainloop. So they were always all received after the
UI creation.

> - libempathy/empathy-chatroom-manager.c:
> +      /* Remove the chatroom from the list, unless it\'s in the list of
>+       * favourites.. (seems strange to handle this at such a low level.. */
>     that doesn\'t seems strange to me, I think it is OK. If you don\'t agree
>     please either find a better place or add a FIXME in the comment.

I'll tag it as a fixme. I find it strange that something in (what is meant to
be, but never will be) such a low-level lib does policy like this. And
especailly that it writes stuff in ~/.gnome2/Empathy in the first place.

>- libempathy/empathy-chatroom.c:
>  * The \"tp-channel\" property that you removed is still used in
>    libempathy-gtk/empathy-contact-menu.c and libempathy/empathy-dispatcher.c.
>    That kind of problem can be avoided by using
>    empathy_chatroom_get_channel() instead of g_object_get (chatroom,
>    \"tp-channel\"). We should probably add getters for EmpathyChatroom\'s
>    property, I see that the \"is-favorite\" property is also accessed via
>    g_object_get.

Hmm, i seem to have missed those usages, thanks.. I also thinks the tp-channel
stuff is a slight abuse, but i'll put it back for now, that's something to fix
for another time. (As in it should probably call a method on the EmpathyTpChat
related to the chatroom, instead of doing a dbus call with no error handle or
further feedback)...

> - libempathy/empathy-contact.c:
>   * What happens if the contact is finalized while some call_when_ready are
>     still pending? Should we assert that priv->ready_callbacks is NULL in
>     finalize because callers are responsible to keep a ref? or should we call
>     all cb because it is canceled? Or ReadyCbData struct could keep a ref to
>     the contact? I guess we should do the same as tp-glib.

It can't be finalized because we have a ref to it. But yeah, if it fails to go
to ready the callbacks should probably be called with an error argument like in
tp-glib land, also the weak objects tp-glib has may be usefull here.

>- libempathy/empathy-dispatcher.c:
>  * You always give NULL for the callback and user_data params of
>  empathy_dispatcher_join_muc and empathy_dispatcher_chat_with_contact. Should
>  they be removed? Or the callback should be used?

The callback should be used, but isn't currently. more on that in a bit.

>  * empathy_dispatcher_send_file_to_contact(): I don\'t like it to get all
>  info as param. The code to get those infomartion from a gfile will have to
>  be duplicated in nautilus for example... Maybe we could add a helper
>  empathy_dispatch_send_gfile_to_contact(EmpathyContact*, GFile*, GCallback,
>  gpointer) that does the offer and call the cb with an error if either the
>  channel request or the offer fail?

I don't agree with most things you say here. It's definately a problem that
something like nautilus needs to duplicate a lot of code currently, but having
the dispatcher provide that functionality is wrong.

All the dispatcher should be able to do is dispath and request channels them
(with some convenience functions like chat_with_contact.. ). The steps needed
to go from function call to the actual dbus call in the dispatcher should be
minimal. As in, getting the handle from an EmpathyContact is about as much work
as it should do before requesting the channel (although strictly speaking, an
empathy contact should always know it's handle, but that's something to fix
another time).

Getting load of information of about a file is definately not the work of the
dispatcher. Especially when it calculating a hash for the file (which for some
reason isn't implemented in empathy atm, but should have been). To be honest i
think the way empathy does FT currently is completely broken. (Which is why i
asked cosimoc to put it high on his todo list).


In general the design of empathy to bounce so often through the dispatcher is
wrong. For example when you double-click a user to start a new chat with them,
first channel is requested and only on the final stap of the dispatching is the
UI actually shown to the user. Without any feedback or error reporting
inbetween. Requesting and dispatching a text channel currently involves at
least several D-Bus roundrips. Any of which can take several seconds when the
CM is blocking on something. The fact that this doesn't happen in practise is
because our CMs are usually quite high quality and don't tend to block on
things..

What should happen instead is that when you request a new chat/call/whatever.
The respective UI should pop up immediately while starting a channel request in
the background. The callbacks in the dispatcher API can be used to give
feedback when something goes wrong and claim the channel right away when it
comes back from the request. Also when you already have a chat with a contact
there is no reason to bounce through the dispather.. (The exception ofcourse
being the case where you don't know the handle yet (the user typed in some
identifier), so you can't uniquely identify them yet, but that's rare).


In the filetransfer case this is even more important as you need to do a lot
more steps before you can request the channel and for some of them you want to
give the user some feedback (hashing the file!). It's not something
you stuff into the dispatcher, look the other way and then wait for something
to happen :)

What i would do is create a FileTransferManager which does have a
_send_gfile_to_contact function, which returns a FileTransfer object and also
acts as a dispatcher for incoming filetranfers. The UI can then listen on the
FileTransferManager for new FileTransfer objects to appear and show some nice
dialog/transferlist/whatever.

The FileTransfer object itself then does all the needed steps of actually
getting the tranfer going. Which would include poking the GFile with a stick to
get all the needed information and afterwards doing the actual channel request.
And obviously it would have a nice set of signals about it's state to the
UI (hashed 400kbyte, transferred 200 bytes etc etc).

> - libempathy/empathy-message.c:
>   * id should be a property

I wasn't planning to, id is only intended for internal use, as in for the TpChat to be able to
acknowledge messages. There is no use in exposing it as a property (as in, it's
completely useless for bindings)

  Sjoerd
-- 
A lifetime isn't nearly long enough to figure out what it's all about.