873477 - [SMS][MMS] Group participants string formatting update per v8.0

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Rick Waldron [:rwaldron]]

Refer to HTML5_SMS-MMSUserStorySpecifications_20130503_V8.0.pdf

1. p.12, 05 module indicating group message
  
  +n others

2. p.13, 01 Header

  (+n)



#1 is partly satisfied by bug 868679

Comment 1

[Rick Waldron [:rwaldron]]

Attachment: page 12

Comment 2

[Rick Waldron [:rwaldron]]

Attachment: page 13

Comment 3

[Rick Waldron [:rwaldron]]

Attachment: Fix for 873477

Comment 4

[Corey Frang [:gnarf]]

Comment on attachment 751715 [details] [review]
Fix for 873477

r+ from me, however I'm not sure what we should be doing about the strings in the other locale - dietrich suggested I ask stas.

The question at hand is, with en_US changing this:

-contact-title-text        = {[ plural(n) ]}
-contact-title-text[zero]  = {{name}}
-contact-title-text[one]   = {{name}} and {{n}} other
-contact-title-text[two]   = {{name}} and {{n}} others
-contact-title-text[few]   = {{name}} and {{n}} others
-contact-title-text[many]  = {{name}} and {{n}} others
-contact-title-text[other] = {{name}} and {{n}} others
+others        = {[ plural(n) ]}
+others[zero]  = {{name}}
+others[one]   = {{name}} (+{{n}})
+others[two]   = {{name}} (+{{n}})
+others[few]   = {{name}} (+{{n}})
+others[many]  = {{name}} (+{{n}})
+others[other] = {{name}} (+{{n}})

Do we need to also change the other locales (which already used others[], but now have many more words than "{{name}} (+{{n}})"

Comment 5

[Fabien Cazenave [:kaze]]

Jumping in the conversation as Dietrich just pinged me…

I’m discovering the « + plural » madness in the JS part of the code, this is a complete non-sense. Doing a |git blame| to find out when it all went so wrong…

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #4)
> Comment on attachment 751715 [details] [review]
> Do we need to also change the other locales (which already used others[],
> but now have many more words than "{{name}} (+{{n}})"

Rule of thumb: developers should always leave non-English locales alone.

However, `others' is not a good l10n key in this case. Let’s use something more self-explanatory please — choosing a new name will ensure that the corresponding strings will be properly updated by our l10n contributors.

Comment 6

[Fabien Cazenave [:kaze]]

(In reply to Fabien Cazenave [:kaze] from comment #5)
> I’m discovering the « + plural » madness in the JS part of the code, this is
> a complete non-sense.

This has been done with bug 868679. The reviewer (Julien) was aware of the issue and agreed to fix this with a follow-up, see bug 873703.

Changing this l10n key without fixing bug 873703 along the way does not make any sense. Let’s fix this issue properly, please.

Comment 7

[Rick Waldron [:rwaldron]]

I'm not sure I follow... It seems to me that by using "contact-title-text", the app was broken in every other language—they all have "others"

Comment 8

[Rick Waldron [:rwaldron]]

(In reply to Fabien Cazenave [:kaze] from comment #5)
> Jumping in the conversation as Dietrich just pinged me…
> 
> I’m discovering the « + plural » madness in the JS part of the code, this is
> a complete non-sense. Doing a |git blame| to find out when it all went so
> wrong…

@Fabien, complete nonsense maybe, but it the i18n/l10n "magic" wasn't working correctly. The existing "contact-title-text" was producing strings like:

"Jane Doe and 1 others"

I just assumed that the additional entries, eg.

contact-title-text[zero]
contact-title-text[one]
contact-title-text[two]

were meant to be used to correct that sort of "non-sense"

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Rick Waldron from comment #7)
> I'm not sure I follow... It seems to me that by using "contact-title-text",
> the app was broken in every other language—they all have "others"

l10n 1st rule of thumb: when we change the string, we change the key. Always. This is what makes the localizers know that they need to change their locale. The localizations that are shipped are kept in a separate repository (http://hg.mozilla.org/gaia-l10n). The locales that are in the Gaia repo are here only to help testing easily, therefore you can change them if you feel like it, but it's not an obligation.

(In reply to Rick Waldron from comment #8)

> @Fabien, complete nonsense maybe, but it the i18n/l10n "magic" wasn't
> working correctly. The existing "contact-title-text" was producing strings
> like:
> 
> "Jane Doe and 1 others"
> 
> I just assumed that the additional entries, eg.
> 
> contact-title-text[zero]
> contact-title-text[one]
> contact-title-text[two]
> 
> were meant to be used to correct that sort of "non-sense"

I'll check all this today in Bug 873703.

This is completely possible that there is a bug in the l1àn library although it is unlikely.

(In reply to Fabien Cazenave [:kaze] from comment #5)
> Jumping in the conversation as Dietrich just pinged me…

Dietrich, really, I know this library very well; no need to disturb kaze about this. Thanks anyway Kaze !

Thanks all guys, I'll resolve Bug 873703 today as my top priority bug.

Comment 10

[Axel Hecht [:Pike]]

Should this be resolved as a DUPE or a dependent bug of bug 873703, then?

Also, should Kaze's comments be considered an r-?

Comment 11

[Julien Wajsberg [:julienw]]

clearing the review flags, we'll fix bug 873703 first.

Comment 12

[Fabien Cazenave [:kaze]]

Julien, Rick: I’ve proposed a patch for bug 873703, feel free to review it.

(In reply to Rick Waldron from comment #8)
> @Fabien, complete nonsense maybe, but it the i18n/l10n "magic" wasn't
> working correctly. The existing "contact-title-text" was producing strings
> like:
> 
> "Jane Doe and 1 others"

I can’t reproduce this behavior, but that sounds really bad and it should be reported as an l10n.js bug rather than doing a workaround.

BTW, I’m sorry if “non-sense” sounded harsh — that wasn’t intentional at all.

(In reply to Rick Waldron from comment #7)
> I'm not sure I follow... It seems to me that by using "contact-title-text",
> the app was broken in every other language—they all have "others"

Right, but that’s how the localization process works. I agree that other locales should be sync’ed more often with the master branch to ease the test, but that’s not a priority of the l10n team.

You can test l10n-related patches with a so-called “multilocale build”: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Building#Building_multilocale

Comment 13

[Fabien Cazenave [:kaze]]

Comment on attachment 751715 [details] [review]
Fix for 873477

Rick, I’ve just merged bug 873703. Please rebase your patch and use a more explicit l10n key (e.g. `contact-others' rather than just `others'). Thanks!

Comment 14

[Fabien Cazenave [:kaze]]

Comment on attachment 751715 [details] [review]
Fix for 873477

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/4d360cbcf8faea6df9128e3af6d18be4ababc0df

Comment 15

[Gareth Aye [:gaye] (back from PTO)]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 4d360cbcf8faea6df9128e3af6d18be4ababc0df
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 31ff278