GNOME Bugzilla – Bug 618244
port to GDBus
Last modified: 2013-03-27 13:16:10 UTC
Port totem to GDBus.
Created attachment 160704 [details] [review] Port TotemScrsaver to GDBus (Untested) Bug #618244.
Is that a straight port, or does it fix bug 567908 as well?
It is a straight port from dbus/dbus-glib to GDBus. I wasn't aware of the other bug. (Also it's untested, since my system is broken (I can't get totem to play anything b/c of pulse errors).) Something else that would be nice to fix afterwards is the problem that totem-scrsaver makes sync dbus calls in functions called from the main UI thread; should be using async calls instead.
Feel free to commit this patch after a GLib version with GDBus support has been released.
commit 199d02674bcfb105cb6c2e4f2cc4e00585966cfd Author: Christian Persch <chpe@gnome.org> Date: Mon May 10 12:13:51 2010 +0200 Port TotemScrsaver to GDBus (Untested) Bug #618244.
There's 3 uses of dbus[-glib] in totem: TotemScrsaver: evince uses this, and has ported it to gdbus; can be taken back unmodified. media player keys: evince uses this, and has it ported to gdbus; can be taken back with some changes related to that in totem it derives from TotemPlugin which evince of course removed. browser plugin/plugin viewer: not yet ported, but should be easy enough.
Not sure I'd call this 'fixed' since there's stuff remaining :) Should probably backport the TotemScrsaver from evince, it was improved not to make sync dbus calls, and the patch here doesn't compile against older gdbus (some API changes). Also: +GLIB_REQS=2.25.4 This should probably be 2.25.7 now.
Yep, probably. I just broke the build :)
The part of the plugin viewer that gets loaded by the browser is now ported. Only the plugin viewer left.
Created attachment 239549 [details] [review] browser-plugin: Port to GDBus UNTESTED I only tested the SetWindow property so far, to check whether the communication between plugin and viewer worked. The PropertyChanged signal could do with using the fd.o Properties interface instead, and a number of parameters are unused and could be cleaned up.
Created attachment 239551 [details] [review] browser-plugin: Port to GDBus UNTESTED I only tested the SetWindow property so far, to check whether the communication between plugin and viewer worked. The PropertyChanged signal could do with using the fd.o Properties interface instead, and a number of parameters are unused and could be cleaned up.
Created attachment 239552 [details] [review] build: Remove dbus-glib dependency Now that totem-plugin-viewer is ported to GDBus.
Review of attachment 239551 [details] [review]: A couple of FIXMEs in there which need addressing. Other than that, it looks good. I haven’t compiled/run it; just reviewed by inspection. ::: browser-plugin/totem-plugin-viewer.c @@ +506,3 @@ } + + //FIXME D-Bus error? Don’t forget about this FIXME. @@ +671,2 @@ { g_message ("totem_embedded_clear_playlist"); Unrelated, but shouldn’t this be a g_debug()? @@ +737,3 @@ + if (invocation) + g_dbus_method_invocation_return_value (invocation, NULL); + return; Might be tidier to do this as a “goto end”, so the g_dbus_method_invocation_return_value() call only appears once in the function. @@ +866,2 @@ if (!emb->is_browser_stream) + return; Shouldn’t this be a g_dbus_method_invocation_return_value()? @@ +1018,3 @@ g_error_free (err); + g_dbus_method_invocation_return_value (invocation, NULL); + return; Again, might be tidier to use “goto end” here (and again below). @@ +1318,2 @@ else + totem_embedded_set_fullscreen (emb, NULL, TRUE); It would be neater to just have: totem_embedded_set_fullscreen (emb, NULL, (totem_fullscreen_is_fullscreen (emb->fs) == FALSE))); @@ +1467,2 @@ totem_embedded_set_error (emb, BVW_ERROR_GENERIC, message); + totem_embedded_set_logo_by_name (emb, "image-missing"); Would be more maintainable to #define the "image-missing" string so it isn’t replicated several times in the file. @@ +2139,3 @@ + guint time; + + //FIXME uri is unused Another FIXME here. @@ +2171,3 @@ + } else { + g_warning ("Unimplemented method call '%s'", method_name); + //FIXME return error Another FIXME here. @@ +2313,3 @@ } else { g_print ("Sleeping....\n"); + g_usleep (3 * 1000 * 1000); /* 10s */ The comment’s now wrong. @@ +2399,3 @@ } + //FIXME error checking Another FIXME. @@ +2400,3 @@ + //FIXME error checking + char *introspection_xml; Declaration in the body of a method.
Review of attachment 239552 [details] [review]: \o/ Mustn’t forget to update jhbuild once this is committed.
Attachment 239551 [details] pushed as 757c643 - browser-plugin: Port to GDBus Attachment 239552 [details] pushed as 477f319 - build: Remove dbus-glib dependency
(In reply to comment #13) > Review of attachment 239551 [details] [review]: > > A couple of FIXMEs in there which need addressing. Other than that, it looks > good. I haven’t compiled/run it; just reviewed by inspection. > > ::: browser-plugin/totem-plugin-viewer.c > @@ +506,3 @@ > } > + > + //FIXME D-Bus error? > > Don’t forget about this FIXME. Fixed. > @@ +671,2 @@ > { > g_message ("totem_embedded_clear_playlist"); > > Unrelated, but shouldn’t this be a g_debug()? We'll need to review our debugging strategy at some point. The plugins themselves are overly verbose as well. > @@ +737,3 @@ > + if (invocation) > + g_dbus_method_invocation_return_value (invocation, NULL); > + return; > > Might be tidier to do this as a “goto end”, so the > g_dbus_method_invocation_return_value() call only appears once in the function. I have a clean up for the fullscreen functions in a separate patch. > @@ +866,2 @@ > if (!emb->is_browser_stream) > + return; > > Shouldn’t this be a g_dbus_method_invocation_return_value()? Fixed. > @@ +1018,3 @@ > g_error_free (err); > + g_dbus_method_invocation_return_value (invocation, NULL); > + return; > > Again, might be tidier to use “goto end” here (and again below). Fixed. > @@ +1318,2 @@ > else > + totem_embedded_set_fullscreen (emb, NULL, TRUE); > > It would be neater to just have: > totem_embedded_set_fullscreen (emb, NULL, (totem_fullscreen_is_fullscreen > (emb->fs) == FALSE))); Fixed in a separate fullscreen cleanup patch. > @@ +1467,2 @@ > totem_embedded_set_error (emb, BVW_ERROR_GENERIC, message); > + totem_embedded_set_logo_by_name (emb, "image-missing"); > > Would be more maintainable to #define the "image-missing" string so it isn’t > replicated several times in the file. Fixed in a separate patch. > @@ +2139,3 @@ > + guint time; > + > + //FIXME uri is unused > > Another FIXME here. It's something I noticed. The uri wasn't used before either, so the API might actually be broken. I'll remove it and add the FIXME separately. > @@ +2171,3 @@ > + } else { > + g_warning ("Unimplemented method call '%s'", method_name); > + //FIXME return error > > Another FIXME here. Fixed. > @@ +2313,3 @@ > } else { > g_print ("Sleeping....\n"); > + g_usleep (3 * 1000 * 1000); /* 10s */ > > The comment’s now wrong. I've fixed this locally. > @@ +2399,3 @@ > } > > + //FIXME error checking > > Another FIXME. > > @@ +2400,3 @@ > > + //FIXME error checking > + char *introspection_xml; > > Declaration in the body of a method. Those 2 are fixed by a separate patch that uses GResources to avoid any possibility of it failing.