575080 - l10n review of test pilot/feedback

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Axel Hecht [:Pike]]

The l10n-side of the feedback code that landed in the tree isn't good enough.

Bug 573079 comment 30 plus has a few issues, as does bug 574772.

Every string should get looked at, really. I don't mind that much what the rest of the code does, but loose ends in l10n cause a mess for 90 teams out there.

Beltzner, can you get this owned and blocking beta 2?

Comment 1

[Rimas Kudelis [use needinfo to ping]]

Axel, if you don't mind, I'll name the problems we've already noticed.

1) testpilot.statusPage.uploadError in main.properties is empty and yelds a parsing error because of the line break at very beginning. It's bug 574772, which I think now could be duped against this one.

2) Strings should be using the single unicode character … instead of ... (Bug 573079 comment 30).

3a) "testpilot.statusPage.extensions = %S extension" should say "%S extensions" (Bug 573079 comment 30).

3b) Ideally and if possible, testpilot.statusPage.extensions and testpilot.statusPage.extension should be combined into one plural-form-aware string.

4) testpilot.quitPage.quitFoever has a mistake in entity name.

5) at least some strings would benefit from l10n comments.

Comment 2

[Alexander L. Slovesnik]

One more problem - testpilot.welcomePage.copyright contains copyright string. Copyright strings have been removed from Firefox code and it doesn't make sense to reintroduce them again.

Comment 3

[Jono Xia]

Hey guys,
I'm happy to fix these issues and will attach a patch as soon as possible.

Quick question: When using unicode characters like … in main.properties, should I use the literal character and save the file as UTF-8, or should I insert the unicode entity code (… in this case) ?

Comment 4

[Jono Xia]

Re: "one plural-form-aware string":  What does that mean exactly?  Is there documentation anywhere on the correct way to do this?  Thanks.

Comment 5

[Alexander L. Slovesnik]

(In reply to comment #4)
> Re: "one plural-form-aware string":  What does that mean exactly?  Is there
> documentation anywhere on the correct way to do this?  Thanks.
See https://developer.mozilla.org/en/Localization_and_Plurals

(In reply to comment #3)
> Hey guys,
> I'm happy to fix these issues and will attach a patch as soon as possible.
> 
> Quick question: When using unicode characters like … in main.properties, should
> I use the literal character and save the file as UTF-8, or should I insert the
> unicode entity code (… in this case) ?
Just use literal character.

Comment 6

[Jono Xia]

Attachment: Patch implementing the reqested changes.

Patch removes line breaks and HTML tags and fragments from all main.properties strings; uses UTF-8 literals instead of HTML entities for special characters; fixes mistake in property names; implements proper localized pluralization for "1 extension/2 extensions".

Comment 7

[Jono Xia]

It also removes the copyright string.

Comment 8

[Jono Xia]

Is this OK or are there any other changes needed?

How about the dates, which are currently expressed in the main.properties file as e.g. "Finished on %S", "Will start on %S", etc. and then the %S filled in by js code with the results of .toLocaleDateString().  Is that the right way to do it?

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 454627 [details] [diff] [review]
Patch implementing the reqested changes.

testpilot.statusPage.extensions should get the ID rev'ed, and a localization note like 

# LOCALIZATION NOTE (downloadsTitleFiles): Semi-colon list of plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 number of files
# example: 111 files - Downloads

testpilot.statusPage.uploadError should probably  be rev'ed, too.

The plain unicode conversions can stay without rev'ing the keys.

Not sure about the dates, don't have a lot experience there myself.

Comment 10

[Jono Xia]

Hi, sorry to be a n00b, but what do you mean by "rev'ed"?  Does it mean I should change the keyname in main.properties to include a version number?

Comment 11

[Alexander L. Slovesnik]

Comment on attachment 454627 [details] [diff] [review]
Patch implementing the reqested changes.

--- a/extension/locale/en-US/main.properties
+++ b/extension/locale/en-US/main.properties
@@ -1,3 +1,6 @@
+# plural rule
+testpilot.pluralRule = 1

Is separate plural rule for test pilot really needed? There is already global plural rule in http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/intl.properties#14

Comment 12

[Alexander L. Slovesnik]

(In reply to comment #6)
> Created an attachment (id=454627) [details]
> Patch implementing the reqested changes.
> 
> Patch removes line breaks and HTML tags and fragments from all main.properties
> strings; uses UTF-8 literals instead of HTML entities for special characters;
> fixes mistake in property names; implements proper localized pluralization for
> "1 extension/2 extensions".
BTW, you should also use single unicode character … instead of ... in main.dtd, there are 3 occurences of them.

(In reply to comment #10)
> Hi, sorry to be a n00b, but what do you mean by "rev'ed"?  Does it mean I
> should change the keyname in main.properties to include a version number?
You can either add version number to keyname or just rename keyname.

Comment 13

[Axel Hecht [:Pike]]

"rev'ed" should just mean "changed to a good new name" where "good" is meant in the sense of a variable name. Rarely, folks resort to rename foo to foo2, but that's really not a good naming. Usually there's something better.

You could sneak and just make testpilot.statusPage.uploadError statusPage.uploadError, as "testpilot" is already encoded in the file. Or make some other variation of "error while uploading".

And we shouldn't need a separate plural form, no.

Comment 14

[Alexander L. Slovesnik]

>testpilot.finishedTask.finishedStudy = Excellent! You finished the %S Study!

s/%S/"%S" perhaps? In all other strings name of study is surrounded by quotes.

Comment 15

[Jono Xia]

Attachment: Second patch, apply after first patch.

Second patch implementing more requested changes.

Comment 16

[Jono Xia]

OK, rev'd those properties, put quote around study name, used global plural rule, used elipsis character in main.dtd, and added localization note.

Comment 17

[Jono Xia]

Attachment: This diff combines the two previous patches into a single one; implements all change requests on this bug so far.

Here, I combined the two patches into a single one.  This patch implements all the requests on this bug so far.

Comment 19

[Rimas Kudelis [use needinfo to ping]]

Looks fine, IMHO. Have you tested if it works fine too? :)

Comment 20

[Jono Xia]

Yes, I've tested it (in as far as making sure all the affected strings still look correct in English, at least).

Comment 21

[Rimas Kudelis [use needinfo to ping]]

I think you're supposed to ask for Axel's review... ;)

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 454692 [details] [diff] [review]
This diff combines the two previous patches into a single one; implements all change requests on this bug so far.

Looks good to me, thanks.

Comment 24

[Axel Hecht [:Pike]]

Can we get this landed, please?

Comment 25

[Dave Townsend [:mossop] (PTO until July 20)]

http://hg.mozilla.org/mozilla-central/rev/b83ee53ea303