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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Francesco Lodolo [:flod]]

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.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Francesco Lodolo [:flod]]

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).

Comment 3

[Raymond Lee [:raymondlee]]

(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".

Comment 4

[Francesco Lodolo [:flod]]

(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 ;-)

Comment 5

[Axel Hecht [:Pike]]

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.

Comment 6

[Francesco Lodolo [:flod]]

I didn't think of that. If %2$S can be 1, we also need plural form. ("1 altra", "2+ altre").

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v2

Added plural form support

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2

Minor update

Comment 9

[Francesco Lodolo [:flod]]

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.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 11

[Dão Gottwald [:dao]]

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.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v4

Comment 13

[Dão Gottwald [:dao]]

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

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 15

[Dão Gottwald [: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).

Comment 16

[Raymond Lee [:raymondlee]]

(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.

Comment 17

[Dão Gottwald [:dao]]

(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 18

[Raymond Lee [:raymondlee]]

Attachment: v6

I've found an example and followed hat.

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#177

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#605

Comment 19

[Dão Gottwald [:dao]]

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.

Comment 20

[Axel Hecht [:Pike]]

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.

Comment 21

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=87ee92c63181

Comment 22

[Raymond Lee [:raymondlee]]

(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

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f88227ac42f8

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Should we merge this back into Aurora? Bug 588593 landed for Fx 15.

Comment 25

[Axel Hecht [:Pike]]

Let me ask in .l10n, this is actually an interesting border line case of breaking string freeze for the better of the web.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f88227ac42f8

Comment 27

[Axel Hecht [:Pike]]

Quorum in .l10n is to request this to get on aurora.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 630476 [details] [diff] [review]
Patch for checkin

[Triage Comment]
Approving for Aurora 15 w/ string change, given Axel's comment.

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/edc666ffeeb2