[Telepathy] Dispatcher branch comments

[Xavier Claessens]

Le jeudi 25 décembre 2008 à 16:11 +0100, Sjoerd Simons a écrit :
> > - 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);

Good to know.

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

I think code that can't be used from multiple process should be moved to
src/ Probably EmpathyChatroomManager and EmpathyDispatcher should be
moved there. I think we can assume Empathy is the only running process
to handle channels, other uses cases will wait for MC5's dispatcher.

I agree that the chatroom manager should connect the "observer" signal
on the dispatcher instead of the dispatcher to call explicitely into the
chatroom manager.

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

Right.

> > - 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 does a lot of policy unfortunatly, I'm OK with a FIXME
message.

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

Hm, I'm not sure we want to spend time on this, TpContact already fix
the issue anyway... Probably we can just free priv->ready_callbacks in
finalize and print a warning about it not being NULL.

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

Ok, so add a FIXME on all usage of those function that tell we want to
give a callback. Or preferably fix it :)

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

Ok, so those functions should go to empathy-message-priv.h, empathy
never do such thing even if there is loooots of API that should be moved
to such headers. Maybe add a FIXME about this?

I'm not sure if we can have private headers in libempathy that are used
in libempathy-gtk, but not from other modules... probably a module in
libempathy-gtk can have some #include "../libempathy/empathy-foo-priv.h"

>   Sjoerd
Xavier