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 _______________________________________________ Telepathy mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/telepathy
