GNOME Bugzilla – Bug 622151
[patch] port totem to GtkApplication
Last modified: 2011-02-26 02:00:01 UTC
Created attachment 164105 [details] [review] 0002-port-from-unique-to-GtkApplication.patch As per our conversation on IRC, attached is a patch for porting Totem to GtkApplication.
Review of attachment 164105 [details] [review]: ::: src/totem-options.c @@ +111,3 @@ } +static gchar * Use char * @@ +134,2 @@ commands = NULL; + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); Make this a macro. @@ +149,3 @@ } else if (options->replace) { + g_free (action); + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); Make those 2 commands use the same macro as above. ::: src/totem.c @@ +73,3 @@ + GApplication requires the platform_data to be of type a{sv}. The + parameter is a dictionary of string value pairs, with a single item, + a string value mapped to url, which is the URL. */ You can remove that. Just explain that it's a{sv}. @@ +150,3 @@ +static GVariant * +variant_from_argv (int argc, + char **argv) Move to totem-options.[ch] @@ +242,3 @@ + Because default-quit is a constructible-only property, we are + unable to set it after the constructor call. As a result, we + must explicitly init the gtk application. */ No need for the comment.
(In reply to comment #1) > Review of attachment 164105 [details] [review]: > > ::: src/totem-options.c > @@ +111,3 @@ > } > > +static gchar * > > Use char * Done. > @@ +134,2 @@ > commands = NULL; > + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); > > Make this a macro. Make a macro for the free/lookup or make the totem_get_action_for_command a macro? > @@ +149,3 @@ > } else if (options->replace) { > + g_free (action); > + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); > > Make those 2 commands use the same macro as above. > > ::: src/totem.c > @@ +73,3 @@ > + GApplication requires the platform_data to be of type a{sv}. The > + parameter is a dictionary of string value pairs, with a single item, > + a string value mapped to url, which is the URL. */ > > You can remove that. Just explain that it's a{sv}. Done. > @@ +150,3 @@ > +static GVariant * > +variant_from_argv (int argc, > + char **argv) > > Move to totem-options.[ch] Why? Ideally, once there is a way to set the default-quit property as well, this will simply go away. The only user of it is the constructor for the GtkApplication, so in my mind it makes little sense to move this to totem-options.[ch]. > @@ +242,3 @@ > + Because default-quit is a constructible-only property, we are > + unable to set it after the constructor call. As a result, we > + must explicitly init the gtk application. */ > > No need for the comment. Removed.
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 164105 [details] [review] [details]: > > > > ::: src/totem-options.c > > @@ +111,3 @@ > > } > > > > +static gchar * > > > > Use char * > > Done. > > > @@ +134,2 @@ > > commands = NULL; > > + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); > > > > Make this a macro. > > Make a macro for the free/lookup or make the totem_get_action_for_command a > macro? For the free/lookup. > > @@ +149,3 @@ > > } else if (options->replace) { > > + g_free (action); > > + action = totem_get_action_for_command (TOTEM_REMOTE_COMMAND_REPLACE); > > > > Make those 2 commands use the same macro as above. > > > > ::: src/totem.c > > @@ +73,3 @@ > > + GApplication requires the platform_data to be of type a{sv}. The > > + parameter is a dictionary of string value pairs, with a single item, > > + a string value mapped to url, which is the URL. */ > > > > You can remove that. Just explain that it's a{sv}. > > Done. > > > @@ +150,3 @@ > > +static GVariant * > > +variant_from_argv (int argc, > > + char **argv) > > > > Move to totem-options.[ch] > > Why? Ideally, once there is a way to set the default-quit property as well, > this will simply go away. The only user of it is the constructor for the > GtkApplication, so in my mind it makes little sense to move this to > totem-options.[ch]. Fair enough, can you file a bug against gtk+ to that effect? > > @@ +242,3 @@ > > + Because default-quit is a constructible-only property, we are > > + unable to set it after the constructor call. As a result, we > > + must explicitly init the gtk application. */ > > > > No need for the comment. > > Removed.
Created attachment 164219 [details] [review] 0002-port-from-unique-to-GtkApplication.patch Updated the patch, please let me know if I may push. I had filed a bug for the default-quit property. Turns out it is a duplicate of bug #622152.
Doesn't work for me:
+ Trace 222511
The "server" part of Totem crashes. glib2-2.25.9 gtk3-2.90.3
Interesting, I cant seem to reproduce that. From the backtrace you provided, the issue seems to be in gtk_main? I have the same versions of glib and gtk+ installed. Can you please provide me with a concrete way to reproduce this? Ive tried all the methods, and even with quick succession, have not had any issues.
(In reply to comment #6) > Interesting, I cant seem to reproduce that. From the backtrace you provided, > the issue seems to be in gtk_main? > > I have the same versions of glib and gtk+ installed. > > Can you please provide me with a concrete way to reproduce this? Ive tried all > the methods, and even with quick succession, have not had any issues. 1. totem & 2. wait for it to start 3. totem /path/to/movie That should load the movie in the instance we just started.
Created attachment 164299 [details] [review] port from unique to GtkApplication
Updated to apply to master.
Created attachment 164327 [details] [review] Port from unique to GApplication
Comment on attachment 164327 [details] [review] Port from unique to GApplication Committed this patch. I'm debugging why I'm getting crashes with the GtkApplication version of the patch.
+ GEnumValue *value; [...] + if (platform_data) { + GVariantIter iter; + GVariant *value; Shadows the first |value| variable. + gchar *key; + + g_variant_iter_init (&iter, platform_data); + if (g_variant_iter_next (&iter, "{sv}", &key, &value)) + url = g_variant_get_string (value, NULL); + } This looks wrong. First, you should use "{&sv}" to save a strdup and not leak the |key| (and use |const char *key|). Secondly, should check that key == "url" before using it as such. Also this leaks |value|. And it assumes "url" is the only entry in the dict; should use a "while (next (...)) { if (...) { ...; break; } }
Thank Christian for the notice. commit 350b94324c2120bc042f4330f76ead789f90242f Author: Bastien Nocera <hadess@hadess.net> Date: Sun Jun 27 14:30:23 2010 +0100 Fix GApplication handling bugs As spotted by Christian Persch. https://bugzilla.gnome.org/show_bug.cgi?id=622151#c12
I'm guessing this is solved now.