Closed Bug 1059057 Opened 6 years ago Closed 6 years ago

"(+0)" mistakenly appear on the main view of Message app

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timdream, Assigned: stas)

Details

Attachments

(2 files)

STR: 

1. Update to OTA as of yesterday
2. Inspect Messages app, looking for a number that is not in the address book.

Expected:

1. Seeing "+8861234567890"

Acutal:

1. Seeing "+8861234567890 (+0)"


[Blocking Requested - why for this release]: regression
IMO it looks like problem with pseudo-locale vs plurals. I see similar issues in other apps (eg. Contacts).

Zibi, do you know if we have any issues on how pseudo-locale handles plurals (in particular "zero" case)?

Thanks!
Flags: needinfo?(gandalf)
Stas and Flod may have a better sense of where it may be coming from.

Does it not happen with regular locales?
Flags: needinfo?(stas)
Flags: needinfo?(gandalf)
Flags: needinfo?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf] from comment #2)
> Stas and Flod may have a better sense of where it may be coming from.
Ah, sorry, thanks for forwarding!

> Does it not happen with regular locales?
Nope, I don't see such issues for regular locale. Tim do you see it for any other regular locale?
Flags: needinfo?(timdream)
Right, it's gone after I switch to normal English.

So it's not a regression of recent build.
Flags: needinfo?(timdream)
Keywords: regression
blocking-b2g: 2.1? → ---
It should fall back to the default plural form
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1009

Which should be 'en-US'. And I guess getPluralRule doens't know how to deal with 'en-US'.

I'm seeing the issue with Accented English, not with Italian in Messages (moving to Gaia::L10n)
Component: Gaia::SMS → Gaia::L10n
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #5)
> Which should be 'en-US'. And I guess getPluralRule doens't know how to deal
> with 'en-US'.

Scrap that, I stopped reading the code too early (we use just "en" from "en-US" to determine the plural form).
What I've found so far: Accented English is correctly identified as pseudolocale and gets the right plural rule for the default locale (en-US, rule #3).

        if (n === 1) {
          return 'one';
        }
        return 'other';

So, for '0' it should return string[other].

Strings in SMS are wrong, so at some point this bug should go back to Gaia::SMS
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.properties#L6-L13

This shouldn't be a plural form, eventually 2 separate strings. There's no argument that needs to be declined according to {{n}}.

thread-header-text-name = {{name}}
thread-header-text-name-plus = {{name}} (+{{n}})

What I don't understand is why English and Italian are not showing the same issue, since they're using rule #3 as well and they pass the same value (0) to thread-header-text.
(In reply to Francesco Lodolo [:flod] (offline from Aug 31 to Sep 7) from comment #7)
> What I've found so far: Accented English is correctly identified as
> pseudolocale and gets the right plural rule for the default locale (en-US,
> rule #3).
> 
>         if (n === 1) {
>           return 'one';
>         }
>         return 'other';
> 
> So, for '0' it should return string[other].
> 
> Strings in SMS are wrong, so at some point this bug should go back to
> Gaia::SMS
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.
> properties#L6-L13
> 
> This shouldn't be a plural form, eventually 2 separate strings. There's no
> argument that needs to be declined according to {{n}}.
> 
> thread-header-text-name = {{name}}
> thread-header-text-name-plus = {{name}} (+{{n}})
> 
> What I don't understand is why English and Italian are not showing the same
> issue, since they're using rule #3 as well and they pass the same value (0)
> to thread-header-text.

Mmm, but is that rule wrong "thread-header-text[zero]" [1] ?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.properties#L8
For anything but one it should return [other]
https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L336-L341

thread-header-text[other] = {{name}} (+{{n}})

Again, that's not a plural rule, so it's even confusing to explain. Plural form shouldn't be used to distinguish 0 vs some
Tested by changing the strings in en-US properties: English is picking [zero], Accented English is picking  [other].
Thanks for all the investigation, flod!  I was able to find the root cause of this bug:  in pseudolocales, all plural variants where correctly transformed to the pseudo-form, but the index of the string (plural(n)) was not copied to the new entity.  I'll attach a patch in a moment.
Flags: needinfo?(stas)
Attached patch PatchSplinter Review
Pull request: https://github.com/mozilla-b2g/gaia/pull/23418
Try: https://tbpl.mozilla.org/?rev=ed3e53eb3747&tree=Gaia-Try
Assignee: nobody → stas
Attachment #8480515 - Flags: review?(gandalf)
Comment on attachment 8480515 [details] [diff] [review]
Patch

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

::: shared/js/l10n.js
@@ -874,5 @@
>      }
>  
>      var rv = {};
>      for (var key in node) {
> -      if (key !== '_index' && (key in node)) {

(key in node) here is a fallout from node.hasOwnProperty(key), which we removed, so it's no longer needed.

@@ +875,5 @@
>  
>      var rv = {};
>      for (var key in node) {
> +      if (key === '_index') {
> +        rv[key] = node[key];

node[key] when key is '_index' is an array, so this copies by reference.  I think that's fine and safe and I don't want to make this code more complex.  Zibi, are you okay with this?
Attachment #8480515 - Flags: review?(gandalf) → review+
You need to log in before you can comment on or make changes to this bug.