GNOME Bugzilla – Bug 567596
Empathy plugin should use the new dispatcher
Last modified: 2009-02-28 00:17:08 UTC
Empathy got a new channel dispatcher recently and so the current empathy plugin should be updated.
Created attachment 126349 [details] [review] Update empathy plugin to use the new dispatcher A few things: I'm not sure about my usage of gtk_main_quit in send_file_cb. Is that okay? I couldn't see any more appropriate nst-esque approach, but I'm prepared to be told I'm blind. Also, gtk_dialog_run: I used it because it was easy at the time.. I'll remove it if you're totally against it being used. I've upped the dep on libempathy in configure.in, but bear in mind that Empathy currently hasn't been released with this new dispatcher, so this patch should probably wait here until it does. The next release should be in the next week or so, I think.
What's the gtk_main_quit() supposed to stop? If it's some mainloop running in Empathy, then Empathy should provide a "empathy_dispatch_operation_finish()" or somesuch. The rest looks fine.
The gtk_main_quit is meant to close the main window (which is greyed out at the time as a result of returning FALSE in send_files). If send_files returns TRUE, the dialog is destroyed before the send_file_cb is called. Therefore, I'm doing the equivalent of the function destroy_dialog in nautilus-sendto-command.c.
Fair enough, looks fine to commit when it's been released then.
Oh, just make sure you add a comment when you start and finish your new main loop.
I don't start any new main loop. I use the loop created by gtk_init by nst itself...
Some comments: * init() --> empathy_debug_set_flags is not needed, empathy_init() does it for you. * send_files() --> empathy_dispatcher_dup_singleton (); --> it is leaked. * send_files() --> info is leaked. * validate_destination() --> contact is leaked if there is an error message. * get_selected_contact() --> why do you return a boolean instead of directly return the contact or NULL if no selection? * get_contacts_widget() --> store is leaked. * I'm against gtk_main_quit(), if the first file fails, all others will be aborted. Also does it call destroy()? I think Nst plugin API should have a way to tell asyncronously when you are done. Or at least wait for send_file_cb() to be called for all sent files. * send_files() --> You shouldn't return if one file fails to get info but continue and try others.
Created attachment 126578 [details] [review] Second patch So, for some reason, I must have been blind drunk when I wrote the first patch, which clearly wouldn't work. This is better. (In reply to comment #7) > * init() --> empathy_debug_set_flags is not needed, empathy_init() does it for > you. Done. > * send_files() --> empathy_dispatcher_dup_singleton (); --> it is leaked. Heh, yeah, so if it's unrefed, nst segfaults.. I showed Sjoerd and he told me to let it leak.. > * send_files() --> info is leaked. Fixed. > * validate_destination() --> contact is leaked if there is an error message. Fixed. > * get_selected_contact() --> why do you return a boolean instead of directly > return the contact or NULL if no selection? Meh, changed. > * get_contacts_widget() --> store is leaked. Fixed. > * I'm against gtk_main_quit(), if the first file fails, all others will be > aborted. Also does it call destroy()? I think Nst plugin API should have a way > to tell asyncronously when you are done. Or at least wait for send_file_cb() to > be called for all sent files. Yeah it would be nice if there was something I could call.. So, perhaps something like destroy_dialog (from nautilus-sendto-command.c which simply calls gtk_main_quit anyway) should be exposed or something like that? Anyway, I fixed the "if one fails they all stop" thing. > * send_files() --> You shouldn't return if one file fails to get info but > continue and try others. Yeah, fixed.
Created attachment 126580 [details] [review] third Sorry, I forgot to free some memory.
Oh, a detail, you should use empathy_gtk_init() instead of empathy_init() :)
... and make sure threads are initialized. I think you can't do that in the plugin's init, that must be the first thing you do when the process starts.
Created attachment 126873 [details] [review] Final Right, this is it. This update reflects some changes in libempathy HEAD, and the change from empathy_init to empathy_gtk_init as Xavier mentioned. Again, it shouldn't be committed until Empathy is released with the new dispatcher.
I guess this can be committed now :-)
Created attachment 129698 [details] [review] Updated to HEAD I agree. I've updated the patch to current HEAD. Bastien, could you commit this or shall I?
As long as it compiles without warnings, and works as it should, go for it!
2009-02-27 Jonny Lamb <jonny.lamb@collabora.co.uk> * configure.in: Bumped dependency on libempathy. * src/plugins/empathy/empathy.c: Moved plugin to use the new channel dispatcher in libempathy. (Closes: #567596)