Closed Bug 760794 Opened 10 years ago Closed 10 years ago

tabView2.title and tabview2.moveToUnnamedGroup.label need l10n comments (browser.properties)

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(firefox15 fixed)

RESOLVED FIXED
Firefox 16
Tracking Status
firefox15 --- fixed

People

(Reporter: flod, Assigned: raymondlee)

References

Details

Attachments

(2 files, 6 obsolete files)

http://hg.mozilla.org/mozilla-central/rev/1c5f48f57ef1

tabview2.moveToUnnamedGroup.label=%S and %S more

Needs a l10n comment explaining what those "%S" stand for.

Since we are in the process, tabView2.title should be fixed too.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #629691 - Flags: review?(ttaubert)
I don't think it's wise to change from
tabview2.moveToUnnamedGroup.label=%S and %S more
to
tabview2.moveToUnnamedGroup.label=%1$S and %2$S more
without changing the key name (CCing Axel, this would probably raise a warning in compare-locales).
(In reply to Francesco Lodolo [:flod] from comment #2)
> I don't think it's wise to change from
> tabview2.moveToUnnamedGroup.label=%S and %S more
> to
> tabview2.moveToUnnamedGroup.label=%1$S and %2$S more
> without changing the key name (CCing Axel, this would probably raise a
> warning in compare-locales).

The reason I changed that is because it's possible that in some languages, the structure order might be "%2$S foo %1$S bar".
(In reply to Raymond Lee [:raymondlee] from comment #3)
> The reason I changed that is because it's possible that in some languages,
> the structure order might be "%2$S foo %1$S bar".

I absolutely agree, in fact I'm not questioning the change, but if this can be done without changing also the key name ;-)
Changing implicit order to explicit order is fine without key change. The semantics don't change, they just get more crisp.

The question I have is whether "foo and 15 more" needs to use plural forms for the second half. CC more community, in particular Polish and Japanese as spot checks.
I didn't think of that. If %2$S can be 1, we also need plural form. ("1 altra", "2+ altre").
Attached patch v2 (obsolete) — Splinter Review
Added plural form support
Attachment #629691 - Attachment is obsolete: true
Attachment #629691 - Flags: review?(ttaubert)
Attachment #629716 - Flags: review?(ttaubert)
Attached patch v2 (obsolete) — Splinter Review
Minor update
Attachment #629716 - Attachment is obsolete: true
Attachment #629716 - Flags: review?(ttaubert)
Attachment #629717 - Flags: review?(ttaubert)
I don't think that's the right way to support plural forms (but I'm not a developer)
https://developer.mozilla.org/en/Localization_and_Plurals#Developing_with_PluralForm

BTW, that new string will need a new key name for sure.
Attachment #629717 - Flags: review?(ttaubert)
Attached patch v3 (obsolete) — Splinter Review
Attachment #629717 - Attachment is obsolete: true
Attachment #629829 - Flags: review?(ttaubert)
Comment on attachment 629829 [details] [diff] [review]
v3

"UnnamedGroupAndMore" doesn't make sense, since "more" refers to tabs...
Also, drop the "2" in tabview2 when changing the string name.
Attached patch v4 (obsolete) — Splinter Review
Attachment #629829 - Attachment is obsolete: true
Attachment #629829 - Flags: review?(ttaubert)
Attachment #629846 - Flags: review?(ttaubert)
Comment on attachment 629846 [details] [diff] [review]
v4

>+tabView.title=%S - Group Your Tabs
>+tabview.moveToUnnamedGroup.label=%1$S and 1 more;%1$S and #1 more

either tabview or tabView for both strings
Attached patch v5 (obsolete) — Splinter Review
Attachment #629846 - Attachment is obsolete: true
Attachment #629846 - Flags: review?(ttaubert)
Attachment #630177 - Flags: review?(dao)
Comment on attachment 630177 [details] [diff] [review]
v5

>-tabView2.title=%S - Group Your Tabs
>-tabview2.moveToUnnamedGroup.label=%S and %S more
>+# LOCALIZATION NOTE (tabview.title): %S is the application name.
>+# LOCALIZATION NOTE (tabview.moveToUnnamedGroup.label): 
>+# %1$S is the page title of the first tab in the unnamed group, 
>+# #1 is the number of remaining tabs
>+tabview.title=%S - Group Your Tabs
>+tabview.moveToUnnamedGroup.label=%1$S and 1 more;%1$S and #1 more

Use %S instead of %1$S, since you removed the second %S.

You should probably note that tabview.moveToUnnamedGroup.label uses the plural form (http://mxr.mozilla.org/mozilla-central/search?string=http%3A%2F%2Fdeveloper.mozilla.org%2Fen%2Fdocs%2FLocalization_and_Plurals).
(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 630177 [details] [diff] [review]
> v5
> 
> >-tabView2.title=%S - Group Your Tabs
> >-tabview2.moveToUnnamedGroup.label=%S and %S more
> >+# LOCALIZATION NOTE (tabview.title): %S is the application name.
> >+# LOCALIZATION NOTE (tabview.moveToUnnamedGroup.label): 
> >+# %1$S is the page title of the first tab in the unnamed group, 
> >+# #1 is the number of remaining tabs
> >+tabview.title=%S - Group Your Tabs
> >+tabview.moveToUnnamedGroup.label=%1$S and 1 more;%1$S and #1 more
> 
> Use %S instead of %1$S, since you removed the second %S.

If I use %S instead of %1$S, I would have to pass the same string twice to getFormattedString() as below.  Do you think it's better to use %1$S?

gNavigatorBundle.getFormattedString("tabview.moveToUnnamedGroup.label", [topChildLabel, topChildLabel]);

> 
> You should probably note that tabview.moveToUnnamedGroup.label uses the
> plural form
> (http://mxr.mozilla.org/mozilla-central/search?string=http%3A%2F%2Fdeveloper.
> mozilla.org%2Fen%2Fdocs%2FLocalization_and_Plurals).

Yes, we want to use plural form.
(In reply to Raymond Lee [:raymondlee] from comment #16)
> If I use %S instead of %1$S, I would have to pass the same string twice to
> getFormattedString() as below.  Do you think it's better to use %1$S?
> 
> gNavigatorBundle.getFormattedString("tabview.moveToUnnamedGroup.label",
> [topChildLabel, topChildLabel]);

I see. Do we have a precedent for this?

> > You should probably note that tabview.moveToUnnamedGroup.label uses the
> > plural form
> > (http://mxr.mozilla.org/mozilla-central/search?string=http%3A%2F%2Fdeveloper.
> > mozilla.org%2Fen%2Fdocs%2FLocalization_and_Plurals).
> 
> Yes, we want to use plural form.

You should add it to the localization note.
Comment on attachment 630206 [details] [diff] [review]
v6

>+# #1 the page title of the first tab in the unnamed group, 
>+# #2 the number of remaining tabs

# #1 is the page title of the first tab in the unnamed group,
# #2 is the number of remaining tabs.
Attachment #630206 - Flags: review?(dao) → review+
Comment on attachment 630206 [details] [diff] [review]
v6

Review of attachment 630206 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +310,5 @@
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +# #1 the page title of the first tab in the unnamed group, 
> +# #2 the number of remaining tabs
> +tabview.title=%S - Group Your Tabs
> +tabview.moveToUnnamedGroup.label=#1 and 1 more;#1 and #2 more

Please put the comments on top of the entries that they're for, see also my previous comment.
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=87ee92c63181
(In reply to Raymond Lee [:raymondlee] from comment #21)
> Created attachment 630476 [details] [diff] [review]
> Patch for checkin
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=87ee92c63181

Passed try
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f88227ac42f8
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
Should we merge this back into Aurora? Bug 588593 landed for Fx 15.
Let me ask in .l10n, this is actually an interesting border line case of breaking string freeze for the better of the web.
https://hg.mozilla.org/mozilla-central/rev/f88227ac42f8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Quorum in .l10n is to request this to get on aurora.
Comment on attachment 630476 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 588593
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: Yes, please see comment #27
Attachment #630476 - Flags: approval-mozilla-aurora?
Comment on attachment 630476 [details] [diff] [review]
Patch for checkin

[Triage Comment]
Approving for Aurora 15 w/ string change, given Axel's comment.
Attachment #630476 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.