Created attachment 62176 [details] [review]
patch that replaces popt with goption in control-center

Created attachment 62177 [details] [review]
Fixed version

Could anyone review this, please ?

Created attachment 70524 [details] [review]
Updated patch.

Updated patch to apply in HEAD. Ok to commit. We'll need to wait until next development cycle because this patch adds new strings and we're string frozen now.

Don't forget to add libgnome 2.14.0 dependecy after commiting this patch.

gnome-control-center has been branched for gnome 2.17.

Lucas, ready to commit this patch to HEAD?

Andre, it's up to gnome-control-center maintainers. I can commit as soon as they say it's ok.

The background portion is OK save for a few minor details. The mixed use of char and gchar. Please just use gchar everywhere. And the "description" being passed in is wrong. It should say "Desktop Background Preferences" rather than "Properties".

Committed patch with corrections

Just a quick note: doing it like this doesn't work:

  context = g_option_context_new(_("- GNOME Theme Applier"));
  	         g_option_context_add_main_entries(context, options, NULL);

because you use _() before setlocale has been called.

Instead, you should use N_() (to mark the string for translation only) and g_option_context_set_translation_domain() (since glib 2.12).

reopening as per last comment

Created attachment 79619 [details] [review]
updated patch for themus-theme-applier 

thos reverted the patch for themus; this patch reapplies and fixes it.

More comments about the committed patches:

* for background capplet:

+  if (command_line_files != NULL) {
     const gchar ** p;
 
-    for (p = args; *p != NULL; p++) {
+    for (p = (const gchar **) command_line_files; *p != NULL; p++) {
       capplet->uri_list = g_slist_append (capplet->uri_list, (gchar *) *p);
     }
---> you should |g_strfreev (command_line_files)| here.
   }

- forgets to g_object_unref (program) just before return in main()

* common/capplet-util.c:

	gnome_program_init (argv[0], VERSION, LIBGNOMEUI_MODULE, argc, argv,
-			    GNOME_PARAM_POPT_TABLE, cap_options,
+			    GNOME_PARAM_GOPTION_CONTEXT, context,
 			    NULL);

Still leaks the GnomeProgram.

* keyboard:

 	gnome_program_init ("gnome-keyboard-properties", VERSION, LIBGNOMEUI_MODULE, argc, argv,
-			    GNOME_PARAM_POPT_TABLE, cap_options,
+			    GNOME_PARAM_GOPTION_CONTEXT, context,
 			    GNOME_PARAM_APP_DATADIR, GNOMECC_DATA_DIR,
 			    NULL);

Leaks the GnomeProgram;

* Mouse:

same.

* sound:

Same, plus:

 	gnome_program_init ("gnome-sound-properties", VERSION,
 			    LIBGNOMEUI_MODULE, argc, argv,
-			    GNOME_PARAM_POPT_TABLE, cap_options,
+ 			    GNOME_PARAM_GOPTION_CONTEXT, context,
 			    NULL);
 
+	gst_init (&argc, &argv);

No! You should instead use 

g_option_context_add_group (context,  gst_init_get_option_group ());

before calling gnome_program_init.

The updated patch for themus-theme-applier looks better, but there appears to be some extra glade changes at the end. Other than that, it seems to apply and build fine.

Created attachment 79641 [details] [review]
updated patch to apply cleanly to trunk

The 'extra glade changes' are a fix for wrong spacing in the dialogue that I had in my checkout tree. If you prefer, I can file it separately.

> ---> you should |g_strfreev (command_line_files)| here.

Sorry that was wrong, since the list takes the pointers... still |command_line_files| need to be free'd at the end of the programme.

Patch looks ok (removing the glade changes), except the use of goto will probably make people twitch a little. If there is a patch without the goto and without the extra glade changes, it should be fine to commit.

I committed a slightly modified version of Christian's patch (sans glade changes).