GNOME Bugzilla – Bug 328988
translatable string conflict: "second"
Last modified: 2013-09-13 00:52:40 UTC
Please describe the problem: In recurrence-page.c, "second" means "2nd". static const char *options[] = { N_("first"), N_("second"), N_("third"), N_("fourth"), N_("last") }; But in misc.c and util.c, "second" is used for time.. str = g_strdup_printf (_("(%d %s %d %s)"), minutes, ngettext(_("minute"), _("minutes"), minutes), seconds, ngettext(_("second"), _("seconds"), seconds)); These "second"s should be distinguished or translators couldn't translate them correctly. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
Confirming. Adding appropriate keywords.
fixed in cvs, thanks very much for reporting this. http://cvs.gnome.org/viewcvs/evolution/calendar/gui/dialogs/recurrence-page.c?r1=1.74&r2=1.75 http://cvs.gnome.org/viewcvs/evolution/calendar/gui/alarm-notify/util.c?r1=1.6&r2=1.7 http://cvs.gnome.org/viewcvs/evolution/calendar/gui/misc.c?r1=1.6&r2=1.7
While translator comments are pretty much appreciated, I don't think the problem is really solved. Translators still cannot provide different translations for different meanings, as they still share the same msgid entry. Maybe, gettext context helps? Please re-open the bug.
Created attachment 59175 [details] [review] proposed additional patch with gettext context
reopening as per comment 4.
Agreed to Theppitak in comment 4. Actaully, I just came across this and was about to propose the very same. :) The patch looks good to me. Unfortunetaly I lack the gettext experience to actually approve the patch.
Ah, nope. As discused on IRC, when using prefixes we need to use Q_() rather than _(). Probably going to fix this in a few...
Created attachment 59222 [details] [review] proposed fix use prefixes and Q_()
Comment on attachment 59222 [details] [review] proposed fix >Index: gui/misc.c >=================================================================== >RCS file: /cvs/gnome/evolution/calendar/gui/misc.c,v >retrieving revision 1.7 >diff -p -u -r1.7 misc.c >--- gui/misc.c 11 Feb 2006 19:14:49 -0000 1.7 >+++ gui/misc.c 13 Feb 2006 01:42:17 -0000 >@@ -109,7 +109,7 @@ calculate_time (time_t start, time_t end > seconds = difference % 60; > if (seconds) > /* TRANSLATORS: here, "second" is the time division (like "minute"), not the ordinal number (like "third") */ >- str = g_strdup_printf (_("(%d %s %d %s)"), minutes, ngettext(_("minute"), _("minutes"), minutes), seconds, ngettext(_("second"), _("seconds"), seconds)); >+ str = g_strdup_printf (_("(%d %s %d %s)"), minutes, ngettext(_("minute"), _("minutes"), minutes), seconds, ngettext(Q_("time|second"), _("seconds"), seconds)); > else > str = g_strdup_printf (_("(%d %s)"), minutes, ngettext(_("minute"), _("minutes"), minutes)); The ngettext() usage in this function looks incorrect both before and after this patch. The arguments to ngettext() should not be marked for translation to begin with (it is the "minute"/"minutes" pair that is used as a key when looking up the translation). It is possible that if they weren't marked, that this would not be an issue ... >Index: gui/alarm-notify/util.c >=================================================================== >RCS file: /cvs/gnome/evolution/calendar/gui/alarm-notify/util.c,v >retrieving revision 1.7 >diff -p -u -r1.7 util.c >--- gui/alarm-notify/util.c 11 Feb 2006 19:14:49 -0000 1.7 >+++ gui/alarm-notify/util.c 13 Feb 2006 01:42:17 -0000 The same issue applies here. >Index: gui/dialogs/recurrence-page.c >=================================================================== >RCS file: /cvs/gnome/evolution/calendar/gui/dialogs/recurrence-page.c,v >retrieving revision 1.77 >diff -p -u -r1.77 recurrence-page.c >--- gui/dialogs/recurrence-page.c 12 Feb 2006 21:53:32 -0000 1.77 >+++ gui/dialogs/recurrence-page.c 13 Feb 2006 01:42:17 -0000 >@@ -995,7 +995,7 @@ make_recur_month_num_menu (int month_ind > * (dropdown menu options are in [square brackets]). This means that after 'second', either the string 'day' or > * the name of a week day (like 'Monday' or 'Friday') always follow. > */ >- N_("second"), >+ N_("ordinal|second"), > /* TRANSLATORS: Entire string is for example: This appointment recurs/Every [x] month(s) on the [third] [Monday] [forever]' > * (dropdown menu options are in [square brackets]). This means that after 'third', either the string 'day' or > * the name of a week day (like 'Monday' or 'Friday') always follow. >@@ -1021,7 +1021,7 @@ make_recur_month_num_menu (int month_ind > > /* Relation */ > for (i = 0; i < sizeof (options) / sizeof (options[0]); i++) { >- item = gtk_menu_item_new_with_label (_(options[i])); >+ item = gtk_menu_item_new_with_label (Q_(options[i])); > gtk_menu_shell_append(GTK_MENU_SHELL(menu), item); > gtk_widget_show (item); > } This change looks good, although it might be worth adding the same context to the other strings.
So we don't need _() inside negttext... There is a bunch of other usage of the same broken code. Plus, there are kind of duplicated strings marked for translation. Like "minutes" and "%s minutes". I am so not going to fix this tonight...
For reference, you can tell if the ngettext() call has been extracted correctly by looking at the PO file template. If you see an entry like this, then the string is being handled correctly: msgid "minute" msgid_plural "minutes" msgstr[0] "" msgstr[1] "" If they are being treated as separate translations, you'd see: msgid "minute" msgstr "" msgid "minutes" msgstr "" Note that while the second case works okay for languages with the same plural rules as English, it is not enough for other languages (which might have more than two plural forms).
Created attachment 59246 [details] [review] proposed patch using ngettext and Q_() I totally agree with using ngettext(). My first patch was so quick and dirty indeed. So, I propose another patch based on what suggested here. Note that context is still required for the ordinal case.
Created attachment 59265 [details] [review] more polished patch As guenther suggested on IRC, I have prepared an improved patch which further unifies the translations of the plural forms.
That last patch looks good to me by a quick glimpse. :) Obsoletes my patch.
Actually, there is a reason to prefer "%u minutes" as a translatable string over "minutes". With the former, the translator can reorder the words in the phrase (which is important for some languages).
In that case, _(" %u %s") in the front still provides the possibility?
It is possible if you use C99 format string extensions to reorder the arguments, but it is nicer not to have to rely on that if possible, since it isn't supported on all platforms yet. It is worth noting that a translatable string like "%u %s" would almost definitely need some context ...
Hmm.. That means the code in if (difference > 60 && difference < 3600) ... case also needs to be changed. And, with the "%d" or "%u", with or without leading space, that would result in many combinations. To minimize duplicated strings to translate, wouldn't it be better to prepare the "%u second(s)", "%u minute(s)" and "%u hour(s)" fragments separately without the leading space concern, and then combine them afterwards case by case?
I am definitely not qualified to comment on that, therefore reassigning.
Created attachment 59279 [details] [review] rewritten calculate_time() + using Q_() In this patch, calculate_time() is rewritten as shown below (without new strings introduced, as necessary strings are already in POT): char * calculate_time (time_t start, time_t end) { time_t difference = end - start; char *str; int hours, minutes; char *s_hours = NULL, *s_minutes = NULL, *s_seconds = NULL; if (difference >= 3600) { hours = difference / 3600; difference %= 3600; s_hours = g_strdup_printf (ngettext("%d hour", "%d hours", hours), hours); } if (difference >= 60) { minutes = difference / 60; difference %= 60; s_minutes = g_strdup_printf (ngettext("%d minute", "%d minutes", minutes), minutes); } /* TRANSLATORS: here, "second" is the time division (like "minute"), not the ordinal number (like "third") */ s_seconds = g_strdup_printf (ngettext("%d second", "%d seconds", difference), (int)difference); if (s_hours) { if (s_minutes) { if (s_seconds) { str = g_strconcat ("(", s_hours, " ", s_minutes, " ", s_seconds, ")", NULL); } else { str = g_strconcat ("(", s_hours, " ", s_minutes, ")", NULL); } } else { if (s_seconds) { str = g_strconcat ("(", s_hours, " ", s_seconds, ")", NULL); } else { str = g_strconcat ("(", s_hours, ")", NULL); } } } else { if (s_minutes) { if (s_seconds) { str = g_strconcat ("(", s_minutes, " ", s_seconds, ")", NULL); } else { str = g_strconcat ("(", s_minutes, ")", NULL); } } else { str = g_strconcat ("(", s_seconds, ")", NULL); } } g_free (s_hours); g_free (s_minutes); g_free (s_seconds); return g_strchug(str); }
As a notice, this still breaks string freeze, anyway, in the prefixed strings part.
*ping* anyone qualified (james? ;-) willing to review thep's patch? thanks very much in advance...
The code looks like it uses ngettext() correctly, but I have a few comments on the code changes themselves. For a start, the code for putting together the time components looks more complicated than it needs to be. How about something like this: char *times[4] = { NULL, }; int i = 0 if (has_hour) { times[i++] = g_strdup_printf(ngettext("%d hour", ... } if (has_minute) { times[i++] = g_strdup_printf(ngettext("%d minute", ... } if (has_second) { times[i++] = g_strdup_printf(ngettext("%d second", ... } joined = g_strjoin(" ", times); str = g_strconcat("(", joined, ")", NULL); for (i = 0; times[i] != NULL; i++) g_free(times[i]); g_free(joined); This is easier to understand, and doesn't lead to a combinatorial explosion when assembling the time quantities. Secondly, this code appears to be duplicated almost exactly in the two places. Is there a place this code could be placed to make it useful to both parts of Evolution? Lastly, it would be good to decide how 0 seconds should be handled. With the pseudo-code I listed above, you'd probably get "()" for 0 seconds. It would be good to make the function return "(0 seconds)" or similar in this case.
Created attachment 60207 [details] [review] Simplified patch Thank you for the review. I have simplified the patch as you suggested. For the case of 0 seconds, I have added a check in my patch so that the text is generated only if no hours nor minutes texts are generated before. (In previous patch, it always generates, to guarantee the calculated string not to be empty. But that's too loose for cases like 60 seconds.) I won't touch the refactoring for now. I don't think I know the source tree well enough.
Comment on attachment 60207 [details] [review] Simplified patch Looks great. The only change I'd make now is to remove the g_strchug() call at the end of the function. The string shouldn't ever end up with leading whitespace, so there should be no reason to trim it. I guess you just need the sign off from one of the Evo developers now.
Created attachment 60321 [details] [review] Updated patch, without string freeze breaking g_strchug() removed as noticed. Another notice is that the "ordinal|" prefixes are now unnecessary, as the "second" strings are now totally removed from time calculation. So, this bug could be fixed without breaking string freeze!
That is pretty lucky that the plural form translations exist in calendar/gui/e-alarm-list.c. I'd expect the translators to be in favour of this patch then, since they'd fall into one of the following two categories: * they translate "second" to the same thing in both ordinal and time based contexts, so the change doesn't affect them. * "second" should be translated differently by context, so their translation was incorrect before this patch but shouldn't be any less correct with the patch. Good work, Theppitak.
So, can I commit it?
srag will take care of this so we get this in before 2.6
srag: *poke*.
Committed to HEAD. Thanks for your patch.