979558 - [l10n] Missing strings for new ringtones

Status: RESOLVED FIXED

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

Description

[Théo Chevalier]

Bug 964584 added new tones, but they appear in English in the middle of other localized tones.

Go to Settings > Sound > Tones, change the tone: all tones should be localized.

Comment 1

[Francesco Lodolo [:flod]]

List of ringtones from /shared
https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/media/ringtones/list.json

Then /apps/ringtones to get localized sound names
https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/pick.js#L127

So, for example, ringer_classic_courier.opus searches for key "ringer_classic_courier_opus". 
If this key doesn't exists, type ("ringer_") and suffix are stripped, then "_" is replace with " ".

For example code it's failing in English for "Classic Prism" (currently displayed as "classic prism"), because the file is named "ringer_classic_prism.opus", while the key is "ringer_classic_prism_ogg".

If I'm reading the code right, this change landed on 1.3 but it basically breaking the localization of ring tones, it's not just about missing strings.

Tim, is this right?

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #1)
> If I'm reading the code right, this change landed on 1.3 but it basically
> breaking the localization of ring tones, it's not just about missing strings.

Confused sentence. Localization is broken because strings are missing, but this affects also consistency for en-US on 1.3 and 1.4

Personally I would just drop the extension in the key name.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I would agree that the key should not be tied to the exact file name.  I'm sorry I did not realize this.

How should we proceed?  Back out bug 964584 until this can be fixed?  Or is there someone who can do the fix now?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Actually, looking at the code linked in comment 1, it tries to strip the suffix already:

  .replace(/\..+$/, '')     // strip suffix

I wonder if there is trailing whitespace breaking this regex.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Ignore comment 4.  Francesco pointed out on IRC that is just the "couldn't find it in l10n" case.

I think we should back out bug 964584.  It seems we cannot adjust the l10n strings for 1.3t now because they are shared with 1.3, so fixing this in code is not an option.  We can re-land bug 964584 for 1.5.

Tim, what do you think?

Sorry again for this mess.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I went ahead and reverted bug 964584 as it seems the correct action.  Tim, please let me know if you want me to do something else here.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks Ben.