Bug 467603 - anaconda should use ngettext
Summary: anaconda should use ngettext
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: anaconda
Version: 10
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: David Cantrell
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-10-19 07:44 UTC by Miloš Komarčević
Modified: 2009-06-03 19:33 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-06-03 19:33:14 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
0001-Change-megabytes-to-MB-to-avoid-change-to-N_.patch (1.27 KB, patch)
2009-05-28 03:11 UTC, David Cantrell
no flags Details | Diff
0002-Use-gettext.ldngettext-when-necessary-467603.patch (5.80 KB, patch)
2009-05-28 03:12 UTC, David Cantrell
no flags Details | Diff
Makefile changes for extracting ngettext messages (986 bytes, patch)
2009-05-28 14:44 UTC, Miloš Komarčević
no flags Details | Diff
0001-Use-gettext.ldngettext-when-necessary-467603.patch (9.18 KB, patch)
2009-05-29 20:06 UTC, David Cantrell
no flags Details | Diff

Description Miloš Komarčević 2008-10-19 07:44:36 UTC
Hope bug #460728 didn't get anaconda devs completely off ngettext, here are some examples where it could be used appropriately:

"Flashing %s port lights for %d seconds..."

"The passphrase must be at least %d characters long."

"Giving up attempting to connect after %d tries!\n"

Comment 1 Miloš Komarčević 2008-10-19 10:29:52 UTC
More potential candidates for ngettext:

"A RAID device of type %s requires at least %s members."

"This RAID device can have a maximum of %s spares. To have more spares you will need to add members to the RAID device."

"Your %s partition is less than %s megabytes which is lower than recommended for a normal %s install."

"%s Bytes"

"%s of %s packages completed" (actually this is probably best handled as in bug #460728)

"You currently have %s software RAID partition(s) free to use.\n"

Comment 2 Bug Zapper 2008-11-26 04:00:13 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 10 development cycle.
Changing version to '10'.

More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Comment 3 David Cantrell 2009-03-23 20:22:31 UTC
Milos,

Would it be possible for you to provide line numbers and source files for where we should be using ngettext?

Comment 4 Miloš Komarčević 2009-03-23 22:08:53 UTC
Thanks for picking this up David.

Below are the excerpts from the POT file which should have the appropriate source file(s) and line numbers from where the message was extracted embedded in the comment.

Please note my additional comments as well.


#: ../partRequests.py:758
#, python-format
msgid "A RAID device of type %s requires at least %s members."
msgstr ""

#: ../partRequests.py:767
#, python-format
msgid ""
"This RAID device can have a maximum of %s spares. To have more spares you "
"will need to add members to the RAID device."
msgstr ""

# NOTE: ngettext is not really necessary if "magabytes" are changed to "MB"
#: ../partitions.py:1285 ../partitions.py:1296
#, python-format
msgid ""
"Your %s partition is less than %s megabytes which is lower than recommended "
"for a normal %s install."
msgstr ""

#: ../text.py:367
#, python-format
msgid "The passphrase must be at least %d characters long."
msgstr ""

#: ../vnc.py:204
#, python-format
msgid "Giving up attempting to connect after %d tries!\n"
msgstr ""

# NOTE: Can be rolled in together with the next message when using ngettext
#: ../yuminstall.py:82
#, python-format
msgid "%s Byte"
msgstr ""

#: ../yuminstall.py:84
#, python-format
msgid "%s Bytes"
msgstr ""

# NOTE: cannot be correctly translated in many languages even using ngettext,
#       see bug #460728
#: ../yuminstall.py:209
#, python-format
msgid "%s of %s packages completed"
msgstr ""

#: ../iw/lvm_dialog_gui.py:760
#, python-format
msgid "You cannot create more than %s logical volumes per volume group."
msgstr ""

#: ../iw/partition_gui.py:1267
#, python-format
msgid ""
"Software RAID allows you to combine several disks into a larger RAID "
"device.  A RAID device can be configured to provide additional speed and "
"reliability compared to using an individual drive.  For more information on "
"using RAID devices please consult the %s documentation.\n"
"\n"
"You currently have %s software RAID partition(s) free to use.\n"
"\n"
msgstr ""

#: ../loader/net.c:1675
#, c-format
msgid "Flashing %s port lights for %d seconds..."
msgstr ""

Comment 5 David Cantrell 2009-05-28 02:59:47 UTC
F-11 is done, so I'm picking this up to get it fixed up in rawhide.  Below are my comments/questions.

(In reply to comment #4)
> Thanks for picking this up David.
> 
> Below are the excerpts from the POT file which should have the appropriate
> source file(s) and line numbers from where the message was extracted embedded
> in the comment.
> 
> Please note my additional comments as well.
> 
> 
> #: ../partRequests.py:758
> #, python-format
> msgid "A RAID device of type %s requires at least %s members."
> msgstr ""
> 
> #: ../partRequests.py:767
> #, python-format
> msgid ""
> "This RAID device can have a maximum of %s spares. To have more spares you "
> "will need to add members to the RAID device."
> msgstr ""
> 
> # NOTE: ngettext is not really necessary if "magabytes" are changed to "MB"
> #: ../partitions.py:1285 ../partitions.py:1296
> #, python-format
> msgid ""
> "Your %s partition is less than %s megabytes which is lower than recommended "
> "for a normal %s install."
> msgstr ""

The partRequests.py and partitions.py files are gone in the latest version of anaconda.  All of the storage backend code was rewritten.

> #: ../text.py:367
> #, python-format
> msgid "The passphrase must be at least %d characters long."
> msgstr ""
> 
> #: ../vnc.py:204
> #, python-format
> msgid "Giving up attempting to connect after %d tries!\n"
> msgstr ""
> 
> # NOTE: Can be rolled in together with the next message when using ngettext
> #: ../yuminstall.py:82
> #, python-format
> msgid "%s Byte"
> msgstr ""
> 
> #: ../yuminstall.py:84
> #, python-format
> msgid "%s Bytes"
> msgstr ""
> 
> # NOTE: cannot be correctly translated in many languages even using ngettext,
> #       see bug #460728
> #: ../yuminstall.py:209
> #, python-format
> msgid "%s of %s packages completed"
> msgstr ""
> 
> #: ../iw/lvm_dialog_gui.py:760
> #, python-format
> msgid "You cannot create more than %s logical volumes per volume group."
> msgstr ""

I've changed these in anaconda.  I created a new lambda function called N_(), which calls gettext.ldngettext.  For the regular translations done by _(), we were just calling gettext.ldgettext.

From what I understand of ldngettext(), I have to pass it two strings and an indicator as to whether or not use the plural form.  Simple example:

numberOfBytes = 13
logEntry = N_("Wrote %d byte.", "Wrote %d bytes.", numberOfBytes) % (numberOfBytes,)
print logEntry

So, I give it the singular form, then the plural form, and then a count value.  If the count value is greater than one, ldngettext will give the plural form.

Is this correct?

> #: ../iw/partition_gui.py:1267
> #, python-format
> msgid ""
> "Software RAID allows you to combine several disks into a larger RAID "
> "device.  A RAID device can be configured to provide additional speed and "
> "reliability compared to using an individual drive.  For more information on "
> "using RAID devices please consult the %s documentation.\n"
> "\n"
> "You currently have %s software RAID partition(s) free to use.\n"
> "\n"
> msgstr ""

I don't think this one needs to be changed in the code.  There will never be a non-plural usage of software RAID.

> #: ../loader/net.c:1675
> #, c-format
> msgid "Flashing %s port lights for %d seconds..."
> msgstr ""  

I can't change this one either because loader is weird.

I am attaching a patch to this bug.  Could you look over it and see if I'm missing anything.  I am not really a gettext expert.

Comment 6 David Cantrell 2009-05-28 03:11:43 UTC
Created attachment 345696 [details]
0001-Change-megabytes-to-MB-to-avoid-change-to-N_.patch

Comment 7 David Cantrell 2009-05-28 03:12:03 UTC
Created attachment 345697 [details]
0002-Use-gettext.ldngettext-when-necessary-467603.patch

Comment 8 Miloš Komarčević 2009-05-28 08:40:02 UTC
Looking mostly ok from where I sit, thanks. Just need to handle the string extraction now - we have to make xgettext and/or intltool aware of the new keyword/alias when generating the new POT file. (Haven't checked anaconda's build toolchain yet, so not sure yet if this will just work or if some effort is needed.)

A few comments though:

(In reply to comment #5)
> I've changed these in anaconda.  I created a new lambda function called N_(),
> which calls gettext.ldngettext.  For the regular translations done by _(), we
> were just calling gettext.ldgettext.

Yep, that's the idea. To be pedantic though, I think N_() is traditionally used to alias gettext_noop() in C:

http://www.gnu.org/software/hello/manual/gettext/Comparison.html

I think people generally use P_() for aliasing ngettext, e.g.:

https://r-forge.r-project.org/plugins/scmsvn/viewcvs.php/pkg/highlight/src/highlight.h?rev=46&root=highlight&sortby=rev&view=markup

> From what I understand of ldngettext(), I have to pass it two strings and an
> indicator as to whether or not use the plural form.  Simple example:
> 
> numberOfBytes = 13
> logEntry = N_("Wrote %d byte.", "Wrote %d bytes.", numberOfBytes) %
> (numberOfBytes,)
> print logEntry
> 
> So, I give it the singular form, then the plural form, and then a count value. 
> If the count value is greater than one, ldngettext will give the plural form.
> 
> Is this correct?

For English and untranslated locales, yes. In the translated PO file there will be one or several additional plural forms for a particular language based on the Plural-Forms header, and which one is returned depends on that number passed to ngettext().

For e.g. Serbian, the corresponding PO file will look like:

"Plural-Forms: nplurals=3;    plural=n%10==1 && n%100!=11 ? "
"0 :    n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2;\n"

#, c-format
msgid "Wrote %d byte."
msgid_plural "Wrote %d bytes."
msgstr[0] "Записан је %d бајт."
msgstr[1] "Записана су %d бајта."
msgstr[2] "Записано је %d бајтова."

so the three choices will be returned for e.g. n = 1, 2 and 5.

> > #: ../iw/partition_gui.py:1267
> > #, python-format
> > msgid ""
> > "Software RAID allows you to combine several disks into a larger RAID "
> > "device.  A RAID device can be configured to provide additional speed and "
> > "reliability compared to using an individual drive.  For more information on "
> > "using RAID devices please consult the %s documentation.\n"
> > "\n"
> > "You currently have %s software RAID partition(s) free to use.\n"
> > "\n"
> > msgstr ""
> 
> I don't think this one needs to be changed in the code.  There will never be a
> non-plural usage of software RAID.

Just change the string to use "partitions" then so translators don't have to think about it, and ideally add an automatically extracted  # TRANSLATORS: comment immediately above [1] saying this will always be plural.

> I am attaching a patch to this bug.  Could you look over it and see if I'm
> missing anything.  I am not really a gettext expert.  

The last remaining issue is the weird "%d of %d packages completed" case in yuminstall.py:209, because the sentence in many languages actually depends on both numbers. See bug #460728 for alternative wording, e.g. "Packages completed: %d of %d".

I'll also have a pass through the new storage code when I get a chance.

[1] http://en.wikipedia.org/wiki/Gettext

Comment 9 Miloš Komarčević 2009-05-28 14:42:11 UTC
Just checked, N_ is definitely used as noop, even in the current anaconda python code (see constants.py and platfrom.py), so we must introduce P_.

I'm attaching a patch for po/Makefile that should then work for the extraction of the new messages (including a typo fix).

Also had a peak at the loader C code - the reason it's weird is that it doesn't use gettext ;) It's reimplementing translated string matching from scratch, so one would have to either reimplement ngettext as well, or switch loader to gettext (it still takes advantage of xgettext to extract messages from source though).

Comment 10 Miloš Komarčević 2009-05-28 14:44:05 UTC
Created attachment 345771 [details]
Makefile changes for extracting ngettext messages

Comment 11 David Cantrell 2009-05-28 21:49:56 UTC
(In reply to comment #9)
> Just checked, N_ is definitely used as noop, even in the current anaconda
> python code (see constants.py and platfrom.py), so we must introduce P_.

I'll make that change.

> I'm attaching a patch for po/Makefile that should then work for the extraction
> of the new messages (including a typo fix).

I'll combine this patch with mine.

> Also had a peak at the loader C code - the reason it's weird is that it doesn't
> use gettext ;) It's reimplementing translated string matching from scratch, so
> one would have to either reimplement ngettext as well, or switch loader to
> gettext (it still takes advantage of xgettext to extract messages from source
> though).  

Oh, I am well aware of what loader does.  I really have no interest in converting it to using gettext right now for this bug, but I would like to work on changing it in the future.  I spend a lot of time working on loader code, but I don't want to get in to loader gettext code right now.

Comment 12 David Cantrell 2009-05-29 20:06:38 UTC
Created attachment 345952 [details]
0001-Use-gettext.ldngettext-when-necessary-467603.patch

Updated patch incorporating your changes.  I decided to go with the P_() conversion on the software RAID line anyway because that message could appear to users when the installer is only detecting a single software RAID partition, so we should have it pick between singular and plural forms.

Comment 13 David Cantrell 2009-05-29 20:07:25 UTC
Can you have a look at the latest patch and comment?

Thanks.

Comment 14 Miloš Komarčević 2009-06-01 10:35:23 UTC
Thanks David, the patch looks great. To slightly improve readability and maintainability, I'd maybe split out that last sentence of the long RAID paragraph and only use P_() when required.

All clear about the loader code.

Comment 15 David Cantrell 2009-06-03 19:33:14 UTC
Patch committed to master branch of anaconda, will appear in the next build of anaconda for rawhide.


Note You need to log in before you can comment on or make changes to this bug.