Whoops, I missed these.

#: calendar/gui/dialogs/delete-comp.c:131
#, c-format
msgid "Are you sure you want to delete %d appointments?"

#: calendar/gui/dialogs/delete-comp.c:136
#, c-format
msgid "Are you sure you want to delete %d tasks?"

#: calendar/gui/dialogs/delete-comp.c:141
#, c-format
msgid "Are you sure you want to delete %d journal entries?"

#: calendar/gui/e-itip-control.c:606
#, c-format
msgid "Every %d days"

#: calendar/gui/e-itip-control.c:614
#, c-format
msgid "Every %d weeks"

#: calendar/gui/e-itip-control.c:619
#, c-format
msgid "Every %d weeks on "

This one probably needs to have first letter capitalized as well.

#: calendar/gui/e-itip-control.c:657
#, c-format
msgid "every %d months"

#: calendar/gui/e-itip-control.c:665
#, c-format
msgid "Every %d years"

#: calendar/gui/e-itip-control.c:677
#, c-format
msgid " a total of %d times"

#: filter/filter-datespec.c:65
msgid "1 second ago"

#: filter/filter-datespec.c:65
#, c-format
msgid "%d seconds ago"

#: filter/filter-datespec.c:66
msgid "1 minute ago"

#: filter/filter-datespec.c:66
#, c-format
msgid "%d minutes ago"

#: filter/filter-datespec.c:67
msgid "1 hour ago"

#: filter/filter-datespec.c:67
#, c-format
msgid "%d hours ago"

#: filter/filter-datespec.c:68
msgid "1 day ago"

#: filter/filter-datespec.c:68
#, c-format
msgid "%d days ago"

#: filter/filter-datespec.c:69
msgid "1 week ago"

#: filter/filter-datespec.c:69
#, c-format
msgid "%d weeks ago"

#: filter/filter-datespec.c:70
msgid "1 month ago"

#: filter/filter-datespec.c:70
#, c-format
msgid "%d months ago"

#: filter/filter-datespec.c:71
msgid "1 year ago"

#: filter/filter-datespec.c:71
#, c-format
msgid "%d years ago"

#: mail/mail-ops.c:1818
#, c-format
msgid "Retrieving %d message(s)"

#: mail/mail-ops.c:1902
#, c-format
msgid "Saving %d messsage(s)"


As a sidenote, easiest way to fix all of these is, IMHO, to load
evolution/po/sr.po (Serbian translation) into Emacs PO-mode, search
for occurence of "bug: plural", and press "s" once you get to the
relevant message, which will open source code at correct position for you.

I presume this means we should actually use something like

ngettext(N_("get %d file"), N_("get %d files"), n);

?

Yeah.

Do you need any help in making a patch?  As I said, the fastest and
easiest way to do that is to open up sr.po in Emacs po-mode, search
for "bug: plural" and use "s" to "go to source reference" which will
open up the code at the right place.

Uhm, actually no. The call should be like:
  ngettext("get %d file", "get %d files", n);

(no need to use N_, xgettext uses by default --keyword which
recognizes ngettext)

Created attachment 43392 [details] [review]
Use plural forms in Evolution -- part 1 of the patch

This is untested patch, and it doesn't handle the following a bit more
tricky (as in more thinking required) situation:

static const timespan timespans[] = {
	{ 1, N_("1 second ago"), N_("%d seconds ago"), 59.0 },
	{ 60, N_("1 minute ago"), N_("%d minutes ago"), 59.0 },
	{ 3600, N_("1 hour ago"), N_("%d hours ago"), 23.0 },
	{ 86400, N_("1 day ago"), N_("%d days ago"), 31.0 },
	{ 604800, N_("1 week ago"), N_("%d weeks ago"), 52.0 },
	{ 2419200, N_("1 month ago"), N_("%d months ago"), 12.0 },
	{ 31557600, N_("1 year ago"), N_("%d years ago"), 1000.0 },
};

The problem here is that we want to let xgettext know to extract these
into PO files as:
 msgid "%d minute ago"
 msgid_plural "%d minutes ago"

We can do that with introducing another noop for plural-forms (similar
to N_):
 #define NP_(a,b) 
and then create another table used simply to help xgettext extract
strings correctly:
 NP_("%d second ago", "%d seconds ago")
 NP_("%d minute ago", "%d minutes ago")
 NP_("%d hour ago", "%d hours ago")
 ...
(and also remove all N_ occurences above, they're not needed this way)

Next, we'd need to pass --keyword=NP_:1,2 to xgettext, and with
intltool we can currently do this in two ways: either from environment
variable or through XGETTEXT_KEYWORDS in po/Makefile.in.  None of
these is suitable for Evolution (there's no po/Makefile.in in CVS, and
translators won't like to have to set environment variable before
calling eg. "intltool-update de").  So, we'd have to patch intltool
(again, since we've done this recently for Gtk+, when
XGETTEXT_KEYWORDS was added).

In the code then, we'd have to change following snippet:

if (count == 1)
	/* 1 (minute|day|...) ago (singular time ago) */
	strcpy(buf, _(timespans[span].singular));
else
	/* N (minutes|days|...) ago (plural time ago) */
	sprintf(buf, _(timespans[span].plural), count);

to something like:

sprintf(buf, ngettext(timespans[span].singular,
timespans[span].plural, count), count);

I don't like this approach, but it requires slightest changes to the
code.  Other approach, which would not require changes to intltool is
to drop get_best_span and timespans array, and use ngettext in
ordinary ways.  Perhaps creating a function get_best_span_string(int
seconds) would be better, and keeping a timespans array without the
strings? (on the second thought, this seems like the best idea)

What do you think?

uhm, replace above get_best_span_seconds with something like
get_string_for_span(int span, int count) {
	switch (span) {
	case 0:
            return g_strdup_printf(ngettext("%d second ago",
                                            "%d seconds ago",
                                            count), count);
        case 1:
             ....
}

Created attachment 43395 [details] [review]
Use plural forms in Evolution -- part 2 of the patch (again, untested)

These two patches should be sufficient.  I'll probably test them later
today (I first need to recompile all the other dependencies of
evolution).  So, I'm adding a "patch" keyword.

and some brain-fart in second patch above: s/g_strdup_printf/g_strdup/g

Created attachment 43396 [details] [review]
Use plural forms in Evolution -- part 2, attempt 2 (more brain-fart) -- I promise to stop spamming now

Upping prority so patch gets noticed.

This patch at least compiles, and works in some instances I was able
to trigger (I've also got an updated Serbian translation which eases
testing at least for me, not sure how much it would help anyone else).

Unfortunately, I'm getting some crashes in Evolution (bug 255491), so
I'm unable to test it throughout.

gerardo, patches get noticed by beign posted to evolution-patches ... :)

Danilo, could you please submit the first patch to evolution-patches?
 Please also include appropriate ChangeLog entries for the changes.

"Uhm, actually no. The call should be like:
  ngettext("get %d file", "get %d files", n);

(no need to use N_, xgettext uses by default --keyword which
recognizes ngettext)"

This means it looks for ngettext() as well as _() and N_() etc for
strings to snarf, i guess?

How do you put such strings in a table or file?  For example your
second patch should keep the strings in a table, not implement a nasty
switch statement.  Is using N_() like it did before adequate? (i'm
only responsible for the code in the second patch, and it must
continue to use a table before i'll accept it).

Not Zed, thanks for looking at this.

Yes, xgettext (tool that extracts strings into translatable PO files,
which translators work on) looks at ngettext(a, b,...) as well. 

Still, a ngettext is a function call, so it cannot be put inside a
table, and by default xgettext doesn't look at anything like
ngettext_noop (like it does on "gettext_noop", "ngettext" or "N_"). 
Also, it requires two parameters, so the only way I see for keeping
this in a table is the ugly method I outlined above:
- rework table to be
#define NP_(a,b)  a, b
static const timespan timespans[] = {
	{ 1, NP_("%d second ago", "%d seconds ago"), 59.0 },
	{ 60, NP_("%d minute ago", "%d minutes ago"), 59.0 },
	{ 3600, NP_("%d hour ago", "%d hours ago"), 23.0 },
	{ 86400, NP_("%d day ago", "%d days ago"), 31.0 },
	{ 604800, NP_("%d week ago", "%d weeks ago"), 52.0 },
	{ 2419200, NP_("%d month ago", "%d months ago"), 12.0 },
	{ 31557600, NP_("%d year ago", "%d years ago"), 1000.0 },
};
(notice the very ugly hack here, where C preprocessor actually makes
this table appear to the compiler the same as it looks now, and this
may confuse many readers because two strings seem to be "related",
while they're separate fields in the table)

- change the relevant code to use ngettext (the snippet I change in
the patch):
sprintf(buf, ngettext(timespans[span].singular,
timespans[span].plural, count), count);

- pass --keyword=NP_:1,2 to xgettext; this is what makes this solution
"uglier" than what I propose.  

We'd need to patch intltool for this last step, and it means Evolution
would have to require latest intltool (which is not even released yet,
and there isn't even a patch that provides this functionality),
instead of being safe with even a year-old intltool.

I can provide a patch for intltool to be able to cope with this, since
I've commited some patches against it recently.  Still, some changes
in the build system would be required if you want this to work as
expected.  That's why I still opt for "ugly" solution using a switch
statement.  Also, patches won't be accepted for intltool before Gnome
2.6.0 release is made, so that means there won't be suitable intltool
for this at least until March 22nd.

If you want me to work on a patch for intltool, let me know so we can
discuss best ways to support it in the Evolution build system.

I think the approach with the switch statement is still the best
solution, because a static table is not a place to keep strings that
depend on a number in. Unless you're targetting English and Germanic
languages only, what I doubt is the case with Evolution.

Of course, I hope you won't even consider leaving this as is, because
in some languages that's like dropping "1 day", "1 month"... in
English, and having things like "1 days".

I'll add ChangeLog entry to the first patch and post it to
evolution-patches later today.

Recently was added another string which requires usage of ngettext:

#: mail/mail-ops.c:695
#, c-format
msgid "Failed to send %d of %d messages"

ngettext should be used on the second of the numbers here.

Why exactly do you need the NP_() stuff?

This is only to get the strings in the po files right?  What
difference would there be just using two N_()'s.

The function call will just be a plain function call.

Even your messy solution is neater than breaking out the table into a
case statment.

(It is neater, apart from having to patch intltool :)

Yes, NP_ is needed to extract strings into PO files.  The catch here
is that there can be more than two plural forms (that's the entire
purpose of ngettext; for instance, Serbian and Russian have 3
different forms, Slovenian has 4! at the other extreme, there are
languages which have only one form, instead of 2 in English), so
related strings (one for "singular" and other for "plural") need to be
grouped together.  In PO files, instead of the regular two messages
N_() would give:
msgid "1 item"
msgstr ""

msgid "%d items"
msgstr ""

We end up with (for example, with 3 plural forms):
msgid "%d item"
msgid_plural "%d items"
msgstr[0] ""
msgstr[1] ""
msgstr[2] ""

If they're separate messages, xgettext doesn't know the relation
between them when extracting strings, and cannot produce messages like
this.  Which means that translators won't have them in PO files, even
though Evolution might be able to use them if they existed.

If it will be any easier to grasp, it's (in a way, not exactly)
similar to how 1st, 21st, 22nd, 33rd, and 77th are constructed in
English: it's not 33th or 22th.

Various explitives deleted.

would this work:

#define ngettext(a, b) a, b
#define foo(a, b) ngettext(a, b)

use ngettext macro in the table
use foo in the code

Yeah, it would work perhaps even easier than that:
Simply do:
#define ngettext(a,b) a, b
/* construct table here */
#undef ngettext
// ... and use ngettext(.singular,.plural,number) in the code.

Still, while this is a nice trick, it seems that we're getting to even
harder to understand code.  Though, it's your call.

(Also note that the call you #defined as "foo" above should have 3
parameters; xgettext looks only at the first two, so that's not a
problem; just pointing it out if you choose to use "foo" and only add
#undef ngettext before that)

what if ngettext is already a macro though?

anyway, i guess its ok to assume it isn't.

well if it has 3 args, will the detection code find it if its only
used with 2?

if you care to, could you re-do a patch with the macro stuff in it?

otherwise i can do it if you're sick of this bit of the patch :)
 

hmm, i think i accidentally changed the component

I'll write the patch as explained above, but I'm also concerned with
the case of ngettext being a macro -- that happens when developers
want to support older gettextes which don't have ngettext function.

Currently, I'll simply use #ifdef ngettext and then #undef it prior to
this "modified" definition for the table -- that seems at least a bit
better if someone later makes ngettext a macro in Evolution if
function is not present (a compile or link time failure is way better
than having mysterious borkages during runtime).

Also, xgettext won't have problems extracting strings since it cares
only about parameters 1 and 2 in the function call; the real ngettext
function needs the number (3rd parameter) as well to determine which
of the strings to use.

I'll post a patch later today, so you could review it.

Created attachment 43439 [details] [review]
Use ngettext in filter/filter-datespec.c -- second version of the patch

Here's a patch along with the ChangeLog entry (patch will ignore it).
It does exactly as described above, it was tested to compile, and
xgettext/intltool extract messages as expected.

I've also added some comments, but you may strip them (or if I end up
commiting it, I may strip them before committing at your request).

Also, if you're satisfied with it, should it go to evolution-patches?

Thanks for the effort so far.

*** bug 253528 has been marked as a duplicate of this bug. ***

ok this patch looks good as is, please commit, or let me know we need to.

Maybe add a blank line after the #define ngettext stuff, but its not a
biggy.

thanks this!

Ok, second patch commited (along with the newline, wow! :).

Patch one applied to CVS HEAD as well. Marking as RESOLVED/FIXED.

(Uhm, I'm having some login problems with Bugzilla, I always need to
login twice, or rather, to reload this page once I login.)