Re: [PATCH v4 i18n 1/1] add pgettext() and npgettext() support for context-aware translations

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Kefu Chai <k.chai@proxmox.com> writes:

> Context-aware translations (msgctxt in gettext) distinguish identical
> msgids that mean different things in different parts of the UI, for
> example "Monitor" for Ceph versus QEMU.
>
> To keep msgid+msgctxt pairs distinct in the generated JS catalog,
> po2js.pl now hashes the two together with the standard gettext EOT
> separator ("\x04") between them. Messages without a context fall
> through to the existing fnv31a(msgid). Locale::PO::load_file_ashash()
> conflates rows that share the same msgid, so switch to
> load_file_asarray() to preserve the distinct entries. The generated
> catalog gains JS pgettext() and npgettext() helpers that perform the
> same lookup at runtime.
>
> The Makefile change adds --keyword=npgettext:1c,2,3 to xgettext.
> pgettext is in xgettext's default JavaScript keyword list, but
> npgettext is not: it was omitted from the defaults in gettext 0.18.3
> (2013) when JavaScript support first landed, and never added since.
> Without the explicit flag, xgettext silently drops npgettext() calls
> during 'make update_pot'.
>
> See "xgettext Invocation" in the GNU gettext manual:
> https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
>
> Signed-off-by: Kefu Chai <k.chai@proxmox.com>
> ---
>  Makefile |  1 +
>  po2js.pl | 69 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 16aab32..e1d9633 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -155,6 +155,7 @@ define potupdate
>  	  --package-version="$(shell cd $(2);git rev-parse HEAD)" \
>  	  --msgid-bugs-address="<support@proxmox.com>" \
>  	  --copyright-holder="Copyright (C) Proxmox Server Solutions GmbH <support@proxmox.com> & the translation contributors." \
> +	  --keyword=npgettext:1c,2,3 \
>  	  --output="$(1)".pot
>  endef
>  
> diff --git a/po2js.pl b/po2js.pl
> index 316c0bd..4b7b044 100755
> --- a/po2js.pl
> +++ b/po2js.pl
> @@ -8,10 +8,6 @@ use Getopt::Long;
>  use JSON;
>  use Locale::PO;
>  
> -# current limits:
> -# - we do not support plural. forms
> -# - no message content support
> -
>  my $options = {};
>  GetOptions($options, 't=s', 'o=s', 'v=s') or die "unable to parse options\n";
>  
> @@ -34,6 +30,15 @@ sub fnv31a {
>      return $hval & 0x7fffffff;
>  }
>  
> +# Hash function for messages with context
> +sub fnv31a_ctxt {
> +    my ($msgctxt, $msgid) = @_;
> +    return fnv31a($msgid) if !length($msgctxt);  # Empty context = no context
> +    # Use EOT character (0x04) as separator, standard in gettext
> +    my $combined = $msgctxt . "\x04" . $msgid;
> +    return fnv31a($combined);
> +}
> +
>  my $catalog = {};
>  my $plurals_catalog = {};
>  
> @@ -41,11 +46,20 @@ my $nplurals = 2;
>  my $plural_forms = "n!=1";
>  
>  foreach my $filename (@ARGV) {
> -    my $href = Locale::PO->load_file_ashash($filename)
> +    my $aref = Locale::PO->load_file_asarray($filename)
>          || die "unable to load '$filename'\n";
>  
>      my $charset;
> -    my $hpo = $href->{'""'} || die "no header";
> +    # Find header entry (msgid "")
> +    my $hpo;
> +    foreach my $po (@$aref) {
> +        if ($po->msgid eq '""') {
> +            $hpo = $po;
> +            last;
> +        }
> +    }
> +    die "no header" if !$hpo;
> +
>      my $header = $hpo->dequote($hpo->msgstr);
>      if ($header =~ m|^Content-Type:\s+text/plain;\s+charset=(\S+)$|im) {
>          $charset = $1;
> @@ -58,8 +72,7 @@ foreach my $filename (@ARGV) {
>  	$plural_forms = $2;
>      }
>  
> -    foreach my $k (keys %$href) {
> -        my $po = $href->{$k};
> +    foreach my $po (@$aref) {
>          next if $po->fuzzy(); # skip fuzzy entries
>          my $ref = $po->reference();
>  
> @@ -76,9 +89,18 @@ foreach my $filename (@ARGV) {
>          my $qmsgid_plural = decode($charset, $po->msgid_plural);
>          my $msgid_plural = $po->dequote($qmsgid_plural);
>  
> +        # Extract message context if present
> +        my $msgctxt = '';
> +        if (defined($po->msgctxt)) {
> +            my $qmsgctxt = decode($charset, $po->msgctxt);
> +            $msgctxt = $po->dequote($qmsgctxt);
> +        }
> +
>          next if !length($msgid) && !length($msgid_plural); # skip header
>  
> -        my $digest = fnv31a($msgid);
> +        my $digest = length($msgctxt) > 0
> +            ? fnv31a_ctxt($msgctxt, $msgid)
> +            : fnv31a($msgid);
>  
>          die "duplicate digest" if $catalog->{$digest};
>  
> @@ -150,6 +172,35 @@ function ngettext(singular, plural, n) {
>      }
>      return translation[msg_idx];
>  }
> +
> +function fnv31a_ctxt(context, text) {
> +    // Use EOT character (0x04) as separator
> +    var combined = context + "\\x04" + text;
> +    return fnv31a(combined);
> +}
> +
> +function pgettext(context, msgid) {
> +    var digest = fnv31a_ctxt(context, msgid);
> +    var data = __proxmox_i18n_msgcat__[digest];
> +    if (!data) {
> +        return msgid;  // Return msgid (not context) as fallback
> +    }
> +    return data[0] || msgid;
> +}
> +
> +function npgettext(context, singular, plural, n) {
> +    const msg_idx = Number($plural_forms);
> +    const digest = fnv31a_ctxt(context, singular);
> +    const translation = __proxmox_i18n_plurals_msgcat__[digest];
> +    if (!translation || msg_idx >= translation.length) {
> +        if (n === 1) {
> +            return singular;
> +        } else {
> +            return plural;
> +        }
> +    }
> +    return translation[msg_idx];
> +}
>  __EOD
>  
>  if ($outfile) {

Tested that strings using pgettext are correctly extracted:

#: pve-manager/www/manager6/ceph/Monitor.js:36
#: pve-manager/www/manager6/node/Config.js:373
msgctxt "ceph"
msgid "Monitor"
msgstr ""

#: pve-manager/www/manager6/qemu/Config.js:350
msgctxt "qemu-guest-agent"
msgid "Monitor"
msgstr ""

and that they are displayed correctly on the web UI after translation.

This would be a good addition for a couple of strings where we have two
versions of the same string which might require different translations.
lgtm.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano