gnome-games ?  Had me confused here for a second  :-)

D'oh. Yeah, I'm confused. Thanks for fixing the subject.

I am going to attach a patch with a proposed fix for the strings in
launcher.c.  If this is correct, I will create patches for the others
as well.

Created attachment 17303 [details] [review]
proposed fix for first noted instance

Created attachment 17305 [details] [review]
ignore the previous patch -- this one should fix both instances in launcher.c.

Created attachment 17341 [details] [review]
Ugh, I attached the wrong file last time.  Here is the real patch.

Couple of nitpicks:

1) Don't bother using gchar ... char is the same thing
2) Do

  char *msg;

  msg = g_str...

instead of

  char *msg = g_str..


Apart from that, it looks fine to commmit. Please go ahead. Thanks
much :-)

I don't have CVS access (this is actually my first patch submission),
but I will attach a patch for all of the files (let me know if you'd
prefer a separate patch for each file).  I assume that whoever commits
the patches does the ChangeLog entry?

Created attachment 17367 [details] [review]
patch for the 5 files mentioned above

Thanks a million Thomas. I've committed your patch:

2003-06-10  Mark McLoughlin  <mark@skynet.ie>
                                                                     
                                         
        Re-order message construction throughout the panel so
        that Pango markup is not marked for translation. Patch
        from Thomas Benson <tbenson@cs.utk.edu> in #113433.
                                                                     
                                         
        * launcher.c: (launch_cb), (launcher_show_help):
        * menu-ditem.c: (ditem_properties_close),
        (really_add_new_menu_item):
        * menu.c: (about_gnome_cb), (activate_app_def), (remove_menuitem),
        (add_to_run_dialog), (show_help_on):
        * panel-run-dialog.c: (panel_run_dialog_launch_command),
        (panel_run_dialog_show_url):
        * panel-util.c: (panel_show_help), (panel_lock_screen):
        Don't translate pango markup.
                                                                     
                                         

Thanks Thomas for the patch. Unfortunately there's a problem in it
that I overlooked previously. It's this change:

 		esc = g_markup_escape_text (sim->item_loc, -1);
+		msg = g_strdup_printf ("<b>%s %s</b>\n\n%s: %s\n",
+			_("Could not remove the menu item"),
+			esc,
+			_("Details"),
+			gnome_vfs_result_to_string (result));
 
 		panel_error_dialog (
 			menuitem_to_screen (sim->menuitem),
 			"cant_remove_menu_item",
-			_("<b>Could not remove the menu item %s</b>\n\n"
-			  "Details: %s\n"), 
-			esc,
-			gnome_vfs_result_to_string (result));
+			msg);


Some languages may need to reorder the words in the sentence "Could
not remove the menu item %s" and so the %s might need to move around
when translated. Hence, the full sentence "Could not remove the menu
item %s" must be in a single message. I hope it's solvable without too
much added trouble...

Oops, sorry about that.  Isn't this also a problem?

+		msg = g_strdup_printf ("<b>%s</b>\n\n%s: %s %s",
+			_("Can't execute 'About GNOME'"),
+			_("Details"), program_path,
+			_("probably does not exist"));

I'll attach a patch for those two.  Are the two panel-run-dialog.c
changes okay?

Created attachment 17419 [details] [review]
Patch for the two instances noted above in menu.c

Uggh, thanks guys.

So hmm, Thomas - you're patch isn't going to work AFAICT. printf
doesn't have recursive behaviour so the "%s probably doesn't exist"
bit won't have the %s substituted. So you'd actually need to do a
second printf ... at which point I think its getting too complicated
to actually be beneficial.

Christian: what do you think ?

Oh yeah, you're right...I don't see any reasonable way to do it either
then.

To answer Thomas' first comment, yes, that second message also has the
same problem. That problem is more severe than having markup in the
message, so the current state is kind of worse than the original one.

I still believe this can be solved, with printf and a second string.

                detail_msg = g_strdup_printf (
                        _("%s probably does not exist"),
                        program_path);
		msg = g_strdup_printf ("<b>%s</b>\n\n%s %s",
			_("Can't execute 'About GNOME'"),
			_("Details:"), detail_msg);

Not pretty, but should work, I think. Can perhaps be made prettier and
more general with a seperate function for outputting this kind of
error messages.

Created attachment 17475 [details] [review]
Well, here's a patch that does it with two strings if Mark decides that's how it should be done.  It's ugly though.

Christian: I'm not saying it can't be solved ... I'm saying that this
seriously obfuscates the code and makes more use of the heap ...

At what point is it no longer worth the effort ? 

My personal opinion is that this is well within the limits of what's
reasonable to still do.

An important aspect of issues like this, that is easy to forget, is
that a "simple" solution tends to cause problems in other areas. One
needs to look at the big picture, and realize that the source is the
source for many translations as well, so there's a one-to-many mapping
with one line of potentially elegant and easily maintainable but
localization-wise troublesome source code causing trouble a multitude
of times, one for each and every translation that has to be created
and maintained. Remember that gnome-panel has 56 translations.
Seen that way, it can often be regarded as more efficient to fix the
problem in the source code rather than trying to work around it 56
times over. That's at least one interesting way of looking at it. 

Sorry, I consider this way of solving the problems as bogus. I'll
submit a better patch at #112527.
Menthos: a printf ("%s %s", _("Details:"), detail_msg) solution is
totally inacceptable for RTL languages - you definitly need a way to
set the details output order. We ought to g_strconcat the first part
(like "<b>", _("firststring"), "</b>\n\n", _("Details: %s"), NULL) and
then insert the rest using printf (concattedstring, detail_msg).

regs,
 Chris

Created attachment 17573 [details] [review]
I'm a patch. Click me! ;)

Mark: Could you please review this patch?

regs,
 Chris

cneumair: Sorry, yes, overlooked that part.

Christian: I hadn't seen this patch (no PATCH keyword) ... but the
same thing goes here as in #112527. I am not reviewing a patch that
has things like:

-	g_warning (G_STRLOC ": gconf error : '%s'", our_error->message);
+	g_warning (G_STRLOC ": GConf error : '%s'", our_error->message);

(If you want to go through all the panel's strings and do
s/gconf/GConf/, please do and send me the patch that you commit.
Personally, though I think this is just a waste of translators time to
have go and re-translate all the messages)

or 

- 
- gtk_dialog_set_default_response (GTK_DIALOG (dialog),
GTK_RESPONSE_REJECT);
+ gtk_dialog_set_default_response (GTK_DIALOG (dialog),
+				 GTK_RESPONSE_REJECT);

I don't want random code formatting changes.


and I don't understand why you do this:

+	tmp_str = g_strconcat (_("Force this application to exit?"),
+			       "\n",
+			       _("(Any open documents will be lost.)"),
+			       NULL);
+
 	dialog = gtk_message_dialog_new (NULL, 0,
 					 GTK_MESSAGE_QUESTION,
 					 GTK_BUTTONS_NONE,
-					 _("Force this application to exit?\n"
-					   "(Any open documents will be lost.)"));
- 
+					 tmp_str);

What's wrong with using gtk_message_dialog like this ?

Oh, I see it now - you don't want to translate "\n". You do realise
this patch makes the panel use the heap *much* more ? I find it hard
to justify this runtime expense in my own mind ... please open a new
bug about this where we can discuss it.


But most importantly, you've just dumped a *huge* patch into this bug
when most of it has *nothing* to do with the bug ! :/

Created attachment 18108 [details] [review]
New gnome_panel_error implementation, arg3+4 form the format string, the rest is done by a va_list. This saves many lines + slight unrelated changes to function calls I needed to change anyway.

I'm a little bit unhappy that panel_error_dialog is a bit confusing to
use with this ... e.g.

+		panel_error_dialog (screen,
+				    "cannot_show_url_dialog",
+				    _("Cannot show %s"),
+				    error->message,
+				    url);
+

Here "url" looks like a coding error and error->message would actually
get substitued in for %s.

On the other hand, the hackiness is confined to panel_error_dialog and
it does clean up like a lot of stuff.

Please go ahead and commit to HEAD Manny. Great work, thanks :-)

In fact we haven't got any possibility to avoid that - or does
[arg1,2]
format1,
va_list1,
format2,
va_list2

work, too?

regs,
 Chris

No, that can't really be made to work I don't think. Its fine the way
you've done it. Please commit ...

Done. Closing.

regs,
 Chris