563270 - Cocoa print UI isn't localizable on 1.9.2

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Proof-of-concept patch

Half of the new Cocoa print UI (e.g. stuff like http://mxr.mozilla.org/mozilla1.9.2/source/widget/src/cocoa/nsPrintDialogX.mm#309) uses a "localizable string" construct (http://mxr.mozilla.org/mozilla1.9.2/source/widget/src/cocoa/nsPrintDialogX.mm#250), but a) they're not NSLocalizable.strings and there's no Localizable.strings file for the code to pull strings from, and b) there's no Gecko string bundle to pull the strings from, either.  (For instance, "As Laid Out on the Screen" appears only in a Gnome-related properties file, a .dtd, which non-XUL UI can't use, and nsPrintDialogX.mm.)

The other half of the UI (the labels, e.g. http://mxr.mozilla.org/mozilla1.9.2/source/widget/src/cocoa/nsPrintDialogX.mm#334) aren't using localizable string calls at all.

This is a major regression for Camino.

If there's no Gecko stringbundle we can include to cover all of this UI (and make it localizable for everyone), it is possible to unregress Camino by changing

[self localizedString:"As Laid Out on the Screen"]

to

NSLocalizedString(@"As Laid Out on the Screen", nil)

and all of the *Label:"foo" calls from

self addLabel:"Frames:"

to

addLabel:[NSLocalizedString(@"Frames:", nil) UTF8String]

and then add all the strings-as-keys to Camino's Localizable.strings file; Camino will do string lookup for the key, and everyone else will fall back to the key (which is the string they're currently seeing), and no Gecko l10n freeze will be broken.[1]

It's kind-of a hack, but I think it satisfies the constraints of "Camino still gets this UI localized", "there is no available Gecko stringbundle to use", and "No Gecko string freeze is broken" (and, at least with this particular patch, "Smokey doesn't have to touch any code he really, really doesn't understand").  

I'm of course open to suggestions of better/more correct/cleaner code to accomplish this same goal, but the proof-of-concept patch attached here works.  I need to test against Firefox to sanity-check for completely unexpected breakage, but I think for that to happen Widget would have to be grossly subverting the mechanics of NSLocalizedString in some hidden fashion, or I'd have to have messed up the syntax completely ;) 

[1] (Even better, if anyone wanted to localize this UI on their own in their non-Camino app, they should just be able to insert a Localizable.strings file in their app's langCode.lproj folder and have it be picked up; have to test to be sure, but it seems like it should work based on what we see with Camino.)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Note to self: Need to test with double-byte languages, too.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Japanese PrintPDE.nib

Yeah, this patch works.  Namoroka[1] still has the English strings by default (and my theory about manual localization was correct, for people who want to do that).

Camino pulls the strings from our Localizable.strings file, as mentioned before; there's enough room for the current German strings, and double-byte languages like Japanese show up correctly, as this screenshot of with my franken-Localizable.strings file shows.

[1] https://build.mozilla.org/tryserver-builds/[e-mail]-cocoa_print_UI_l10n/

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Camino pulling print UI strings from a franken-language Localizable.strings (for real)

Grr, I renamed the wrong file :(

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 443022 [details] [diff] [review]
Proof-of-concept patch

Markus, I assume you're the best person to review this?

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 443022 [details] [diff] [review]
Proof-of-concept patch

I've never worked with localization in Cocoa before, so I'm not really comfortable reviewing this. Delegating to Josh.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Josh: ping?  ETA on looking at this?

Comment 7

[Josh Aas]

Comment on attachment 443022 [details] [diff] [review]
Proof-of-concept patch

Do we need a patch for mozilla-central? This doesn't appear to be a problem there, just checking.

This does seem like a reasonable, minimal-impact solution. If it lands on 1.9.2 we should make sure QA tests for regressions in Firefox for multiple languages.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> (From update of attachment 443022 [details] [diff] [review])
> Do we need a patch for mozilla-central? This doesn't appear to be a problem
> there, just checking.

Right, mozilla-central has a stringbundle: http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsPrintDialogX.mm#259 (and so long as the stringbundle isn't initialized before the nsIStringBundleOverride service and then cached forever, Camino will be able to use CHStringBundleOverride to localize this via the stringbundle code.  The early-initialization-and-caching-forever is an unusual behavior, though a few low-level Gecko components do use it; I assume here the stringbundle is only initialized once we go to print the first time, so I expect all will be well on m-c).

> This does seem like a reasonable, minimal-impact solution. If it lands on 1.9.2
> we should make sure QA tests for regressions in Firefox for multiple languages.

Certainly.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 443022 [details] [diff] [review]
Proof-of-concept patch

Requesting approval for this patch on 1.9.2; it fixes a major localizability regression for Camino on 1.9.2, but it is designed not affect Firefox or other XUL apps at all.  No baking on the trunk is possible because the trunk code is different.

Once it lands, QA should be sure double-check multiple languages' Mac Namaroka nightly builds to be sure nothing has changed there.

Comment 10

[LegNeato]

We'll be looking at this in triage Friday morning and making a decision

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 443022 [details] [diff] [review]
Proof-of-concept patch

Approved for 1.9.2.6, a=dveditz for release-drivers

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on 1.9.2.6: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e4036897f465

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> Once it lands, QA should be sure double-check multiple languages' Mac Namaroka
> nightly builds to be sure nothing has changed there.

I checked today's ar, de, en-US, es-ES, fr, ja-JP-mac, and zh-CN Namoroka 3.6.7pre builds, and the print UI is unchanged there, as expected.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr; rv:1.9.2.7pre) Gecko/20100626 Namoroka/3.6.7pre

I'll follow up with having someone verify the Camino side of the fix, but I wanted to assuage any fears about unexpected behaviors in Firefox l10n builds right away ;)

Comment 14

[LegNeato]

Thanks for that Smokey!

Comment 15

[philippe (part-time)]

On the Camino side: I verified this with
2.1a1pre (1.9.2.7pre 20100626000849)
built from http://hg.mozilla.org/releases/mozilla-1.9.2/rev/98888b2f3bf8

by translating a couple of strings in a similar way as comment 3, and then opening the print dialog. The strings are pulled correctly and used.