Closed Bug 575080 Opened 9 years ago Closed 9 years ago

l10n review of test pilot/feedback

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b2
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Pike, Assigned: jono)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
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.
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.
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) ?
Re: "one plural-form-aware string":  What does that mean exactly?  Is there documentation anywhere on the correct way to do this?  Thanks.
Status: NEW → ASSIGNED
Assignee: nobody → jdicarlo
(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.
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".
It also removes the copyright string.
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 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.
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 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
(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.
"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.
>testpilot.finishedTask.finishedStudy = Excellent! You finished the %S Study!

s/%S/"%S" perhaps? In all other strings name of study is surrounded by quotes.
Second patch implementing more requested changes.
OK, rev'd those properties, put quote around study name, used global plural rule, used elipsis character in main.dtd, and added localization note.
Here, I combined the two patches into a single one.  This patch implements all the requests on this bug so far.
Attachment #454627 - Attachment is obsolete: true
Attachment #454688 - Attachment is obsolete: true
Duplicate of this bug: 574772
Looks fine, IMHO. Have you tested if it works fine too? :)
Yes, I've tested it (in as far as making sure all the affected strings still look correct in English, at least).
I think you're supposed to ask for Axel's review... ;)
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.
Attachment #454692 - Flags: review+
Duplicate of this bug: 575453
Can we get this landed, please?
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b83ee53ea303
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7b2
blocking2.0: ? → betaN+
You need to log in before you can comment on or make changes to this bug.