GNOME Bugzilla – Bug 380617
Use GOption argument parsing instead of popt
Last modified: 2007-01-26 14:56:21 UTC
+++ This bug was initially created as a clone of Bug #338954 +++ Following these guidelines: http://live.gnome.org/GnomeGoals/PoptGOption The remote server option parsing is still completely busted, needs fixing.
Here are my comments on the patch from bug 338954 that I was writing just as you were committing the patch... better late than never :) +#if 0 /* FIXME: how to handle non totem options with goption ? */ Since you use LIBGNOMEUI_MODULE in gnome_program_init, they're handled automatically by libgnome[ui]. #ifdef HAVE_GTK_ONLY gtk_init (&argc, &argv); #else In the gtk-only case, you should just add the gtk option group (gtk_get_option_group) to the option context, and use if (!g_option_context_parse (...., &error)) { print error & exit } +extern const GOptionEntry *options; Why not just put the GOptionEntry[] table in the same file? And same for +extern TotemCmdLineOptions optionstate; The patch to src/totem-video-indexer.c is incomplete: it lacks removal of bacon_video_widget_init_backend (&argc, &argv); (replaced by getting its option group), constructing an option context and adding the bvw and the gtk option groups to it and actually parsing the arguments! Same goes for totem-video-thumbnailer.c. + gint seek; [...] + {"seek", '\0', 0, G_OPTION_ARG_INT, &optionstate.seek, N_("Seek"), NULL}, [...] + /* Handle --seek */ + totem->seek_to = options->seek; /* FIXME: Should be a GINT64, but GOptions doesn't support it ATM */ It supports it now, use G_OPTION_ARG_INT64. (And use gint64 in TotemCmdLineOptions struct!) +#if 0 + /* FIXME: Hangs since popt->GOption migration, can this be removed safely ? */ g_once (&bvw_init_once, (GThreadFunc) bacon_video_widget_init_gst, NULL); It seems that the gst option group handles this internally, so you can remove that. + for (i = 0; options->filenames[i] != NULL; i++) You don't check anywhere that options->filenames != NULL ? + program = gnome_program_init (argv[0], VERSION, Should pass PACKAGE instead of argv[0], since this is used internally to locate help files, and store accels etc; so this must not be variable.
Here are further comments from reading the committed patch: hadess: void hadess: bacon_video_widget_init_backend (int *argc, char ***argv) hadess: { hadess: - /* FIXME: Is this ever called? what about the args? */ hadess: g_once (&bvw_init_once, (GThreadFunc) bacon_video_widget_init_gst, NULL); hadess: } Do we need this function at all, anymore? Everyone should parse options using the bvw option group. (The plugin viewer fails to do this currently, which should be fixed.) hadess: + return (GOptionGroup*) gst_init_get_option_group (); No need for the cast here. hadess: + uri = g_filename_to_uri (path_id, NULL, NULL); hadess: argv[i++] = path_id; hadess: + g_free (uri); Don't you want |argv[i++] = uri;| here? Also, g_filename_to_uri CAN fail, so you must check for NULL.
(In reply to comment #2) > Here are further comments from reading the committed patch: > > hadess: void > hadess: bacon_video_widget_init_backend (int *argc, char ***argv) > hadess: { > hadess: - /* FIXME: Is this ever called? what about the args? */ > hadess: g_once (&bvw_init_once, (GThreadFunc) bacon_video_widget_init_gst, > NULL); > hadess: } > > Do we need this function at all, anymore? Everyone should parse options using > the bvw option group. (The plugin viewer fails to do this currently, which > should be fixed.) The nautilus properties plugin can't do that... > hadess: + return (GOptionGroup*) gst_init_get_option_group (); > > No need for the cast here. Fixed. > hadess: + uri = g_filename_to_uri (path_id, NULL, NULL); > hadess: argv[i++] = path_id; > hadess: + g_free (uri); > > Don't you want |argv[i++] = uri;| here? Also, g_filename_to_uri CAN fail, so > you must check for NULL. Yeah. Thinko. And the free needs to be done after the argv[] has been used as well. As for g_filename_to_uri failing, it can't unless totem_session_create_key() gives us garbage, in which case, we'd be pretty screwed already. (In reply to comment #1) > Here are my comments on the patch from bug 338954 that I was writing just as > you were committing the patch... better late than never :) > > +#if 0 /* FIXME: how to handle non totem options with goption ? */ > > Since you use LIBGNOMEUI_MODULE in gnome_program_init, they're handled > automatically by libgnome[ui]. Yep, sorted. > #ifdef HAVE_GTK_ONLY > gtk_init (&argc, &argv); > #else > > In the gtk-only case, you should just add the gtk option group > (gtk_get_option_group) to the option context, and use > > if (!g_option_context_parse (...., &error)) { print error & exit } Done. > +extern const GOptionEntry *options; > > Why not just put the GOptionEntry[] table in the same file? And same for > +extern TotemCmdLineOptions optionstate; There's already too much stuff in totem.c. I put the options table in totem-options.c (just like the GtkAction based menu is in totem-menu.c) > The patch to src/totem-video-indexer.c is incomplete: it lacks removal of > bacon_video_widget_init_backend (&argc, &argv); (replaced by getting its option > group), constructing an option context and adding the bvw and the gtk option > groups to it and actually parsing the arguments! Fixed, except I couldn't find a way to either force having exactly one remaining option, or showing the help when one file isn't passed. Ideas? > Same goes for totem-video-thumbnailer.c. I'll add it to the list of TODOs for this bug. > + gint seek; > [...] > + {"seek", '\0', 0, G_OPTION_ARG_INT, &optionstate.seek, N_("Seek"), > NULL}, > [...] > + /* Handle --seek */ > + totem->seek_to = options->seek; /* FIXME: Should be a GINT64, but > GOptions doesn't support it ATM */ I know, I added GINT64 support to glib myself :) > It supports it now, use G_OPTION_ARG_INT64. (And use gint64 in > TotemCmdLineOptions struct!) Forgot that last bit though :) > +#if 0 > + /* FIXME: Hangs since popt->GOption migration, can this be removed safely ? > */ > g_once (&bvw_init_once, (GThreadFunc) bacon_video_widget_init_gst, NULL); > > It seems that the gst option group handles this internally, so you can remove > that. I removed it. > + for (i = 0; options->filenames[i] != NULL; i++) > > You don't check anywhere that options->filenames != NULL ? Done. > + program = gnome_program_init (argv[0], VERSION, > > Should pass PACKAGE instead of argv[0], since this is used internally to locate > help files, and store accels etc; so this must not be variable. Done
2006-11-29 Bastien Nocera <hadess@hadess.net> * src/backend/bacon-video-widget-gst-0.10.c: (bacon_video_widget_get_option_group): Remove useless cast * src/backend/bacon-video-widget-xine.c: (bacon_video_widget_get_option_group), (bacon_video_widget_init_backend): Fix xine-lib build * src/totem-options.c: (totem_options_process_for_server): Return early if we don't have any filenames to pass * src/totem-options.h: Put the "seek to" option in a gint64 * src/totem-session.c: (totem_save_yourself_cb): Fix thinko, and properly set the playlist name to be a URI * src/totem-video-indexer.c: (main): Finish porting to GOption * src/totem.c: (main): Fix option parsing when using the GTK+ only version GOption-related fixes, with help from Christian Persch <chpe@cvs.gnome.org> The video thumbnailer, and the original bug still stands.
Found a few more nits in the new patch: hadess: #ifdef HAVE_GTK_ONLY hadess: gtk_init (&argc, &argv); hadess: + if (g_option_context_parse (context, &argc, argv, &error) == FALSE) { If you add g_option_context_add_group (context, gtk_get_option_group (TRUE)) you can just kill the gtk_init call. Likewise, totem-video-indexer doesn't add the gtk option group to its context. hadess: +struct GOptionEntry * hadess: +bacon_video_widget_get_option_group (void) hadess: { hadess: - /* Xine backend does not need any options */ hadess: - return (struct poptOption *) xine_options; hadess: + return entries; hadess: } That doesn't match the signature in b-v-w.h which is GOptionGroup* bacon_video_widget_get_option_group (void); I think you can just return NULL here, and make the g_option_context_add_group (context, bvw_context) conditional on bvw_context != NULL in totem.c and totem-video-indexer.c. > Fixed, except I couldn't find a way to either force having exactly one > remaining option, or showing the help when one file isn't passed. Ideas? For now, just |if (!remaining || g_strv_length (remaining) != 1) { g_print ("Expect exactly one filename"; exit (1); } should suffice. Bug 336089 is about adding a way to make the option context print its help, but it's still unsolved.
> > hadess: + uri = g_filename_to_uri (path_id, NULL, NULL); > > hadess: argv[i++] = path_id; > > hadess: + g_free (uri); > > > > Don't you want |argv[i++] = uri;| here? Also, g_filename_to_uri CAN fail, so > > you must check for NULL. > > Yeah. Thinko. And the free needs to be done after the argv[] has been used as > well. As for g_filename_to_uri failing, it can't unless > totem_session_create_key() gives us garbage, in which case, we'd be pretty > screwed already. I admit it's more a theoretical concern. The docs say that it might return any of the GConvert errors, so I imagine a non-UTF-8 locale with a buggy homedir name might trigger it, but we probably don't have to care about that :)
(In reply to comment #5) > Found a few more nits in the new patch: > > hadess: #ifdef HAVE_GTK_ONLY > hadess: gtk_init (&argc, &argv); > hadess: + if (g_option_context_parse (context, &argc, argv, &error) == > FALSE) { > > If you add g_option_context_add_group (context, gtk_get_option_group (TRUE)) > you can just kill the gtk_init call. Done, thanks. > Likewise, totem-video-indexer doesn't add the gtk option group to its context. Notice that the totem-video-indexer doesn't use GTK+ :) > > hadess: +struct GOptionEntry * > hadess: +bacon_video_widget_get_option_group (void) > hadess: { > hadess: - /* Xine backend does not need any options */ > hadess: - return (struct poptOption *) xine_options; > hadess: + return entries; > hadess: } > > That doesn't match the signature in b-v-w.h which is > GOptionGroup* bacon_video_widget_get_option_group (void); > > I think you can just return NULL here, and make the g_option_context_add_group > (context, bvw_context) conditional on bvw_context != NULL in totem.c and > totem-video-indexer.c. I think I was pretty tired when I wrote that... I avoided the warnings by pushing an empty option group instead. > > Fixed, except I couldn't find a way to either force having exactly one > > remaining option, or showing the help when one file isn't passed. Ideas? > > For now, just |if (!remaining || g_strv_length (remaining) != 1) { g_print > ("Expect exactly one filename"; exit (1); } should suffice. > Bug 336089 is about adding a way to make the option context print its help, but > it's still unsolved. Thanks, CC:'ed myself.
2007-01-26 Bastien Nocera <hadess@hadess.net> * src/totem-options.c: (totem_option_create_line), (totem_options_process_for_server): Handle remote options properly, add the ability to pass multiple commands in one go (Closes: #380617) * src/totem.c: (playlist_changed_cb): add mention of bug #364311