931119 - [B2G][l10n][SMS] The labels (Home, Work...) aren't localized in the contact's message thread

Status: RESOLVED FIXED

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

Description

[sarsenyev]

Attachment: 2013-10-25-10-48-18.png

Description:
When choosing the contact from from the message thread, the 
label isn't localized and appears in English

Prerequisite:
Manually created contacts with label (Work,Mobile,Home) and at list one sent or received message from the contact

Repro Steps:
1) Updated Buri to BuildID: 20131025004000
2) Open the message thread
3) Choose the existing contact with label (Home, Work...)
4) Observe the label

Actual:
The labels aren't translated in the contacts thread

Expected:
All labels are translated

Environmental Variables:
BuildID: 20131025004000
Gaia: 606517ceafe0950c2b89822d5f13353743334f2c
Gecko: 5eabd267ef04
Version: 26.0a2
Firmware Version: US_20131015
RIL Version: 01.02.00.019.082

Notes:

Repro frequency: 100
See the attached screenshot

Comment 1

[Julien Wajsberg [:julienw]]

The labels are translated in the Contacts app.

All apps that load contacts should translate them then ? It seems suboptimal but I don't see better solutions...

Comment 2

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> The labels are translated in the Contacts app.
> 
> All apps that load contacts should translate them then ? It seems suboptimal
> but I don't see better solutions...

That's exactly what I thought as I read this. What happens when there are custom tel record types? eg. I can create one that's some New England colloquialism and it will be impossible to translate.

Comment 3

[Julien Wajsberg [:julienw]]

I'd say custom ones will be added by the user anyway, so I think this is expected by him that they won't get translated.

Comment 4

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> I'd say custom ones will be added by the user anyway, so I think this is
> expected by him that they won't get translated.

How would Messages app know if it wasn't supposed to translate user-added labels?

Comment 5

[Julien Wajsberg [:julienw]]

In Contacts, I think we try to translate but uses the original string if the translation fails. We'll translate only some known labels.

It makes sense to share this translation files between apps.

Comment 7

[Delphine Lebédel [:delphine - OOO]]

This still happens on v1.3 Moz Ril
Label's aren't translated

Comment 8

[Delphine Lebédel [:delphine - OOO]]

Possibly related to bug 911415 ?

Comment 9

[Delphine Lebédel [:delphine - OOO]]

Nominating this as the user is loosing a lot of sense when writing his sms here: depending on the language, the user won't know if he's texting to the mobile phone of a contact, or the house phone, or the work phone, etc.

Comment 10

[Julien Wajsberg [:julienw]]

Is it ok to add new l10n strings in 1.3?

I guess we could add a new shared l10n file (I don't exactly know how this works though) to be shared with communications (contacts & dialer) and sms. This looks somewhat risky though.

Comment 11

[Axel Hecht [:Pike]]

Sorry, but we're way past the point where we can add strings to 1.3.

IMHO, moving stuff to shared isn't much of an answer either, because that won't fix 3rd party apps using the contacts API, right? If the contacts API could return a localized version of the contact, that'd be optimal, but I have no constructive proposal on how to do that. In particular with out weak l10n story on the gecko side.

Comment 12

[Francesco Lodolo [:flod]]

I've just realized that touching that area, you get another dialog where "mobile" is correctly localized, because we actually already have those strings in sms.properties (duplicated from Contacts).
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.properties#L166

Comment 13

[Francesco Lodolo [:flod]]

Attachment: Test screenshot

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L222

Replacing this line of code with something along this line (possibly smarter) would work

if (found.type && found.type[0]) {
  if (navigator.mozL10n.get(type) != '') {
    type = navigator.mozL10n.get(type);
  }
} else {
  type = '';
}

This would fix the immediate problem, but that file is all but l10n friendly: hard-coded separators ('|'), multiple concatenations.

Comment 14

[Jason Smith [:jsmith]]

Let's find out if this reproduces on 1.1.

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Francesco Lodolo [:flod] from comment #12)
> I've just realized that touching that area, you get another dialog where
> "mobile" is correctly localized, because we actually already have those
> strings in sms.properties (duplicated from Contacts).
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/locales/sms.en-US.
> properties#L166

oh right, that's from before I joined the project, never noticed them. Are they still sync-ed?

That would still not fix 3rd party apps. I don't have a good idea for this.

(In reply to Francesco Lodolo [:flod] from comment #13)
> 
> This would fix the immediate problem, but that file is all but l10n
> friendly: hard-coded separators ('|'), multiple concatenations.

Yeah but there are so many different cases that it's difficult to do very differently. But you're right, we should probably try to do better.

Comment 16

[Brogan Zumwalt [Inactive]]

Issue DOES occur on 1.1

Result:
The labels aren't translated in the contacts thread

Environmental Variables:
Device: Buri v1.1 Mozilla RIL
BuildID: 20140306041201
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 1421a6b7fc51
Version: 18.0
Firmware Version: v1.2-device.cfg

Comment 17

[Francesco Lodolo [:flod]]

Attachment: Pull request on Github

This is what I learned so far. We have at least two hard-coded separators: ' | ' and ', '.

Two examples: thread header is displayed when opening a conversation, contact details is displayed when tapping the contact in the header.

thread header: mobile | +1234567
contact detail: mobile | +1234567

If a phone number has a carrier associated
thread header: mobile | carriername
contact detail: mobile | carriername, +1234567

I'm not a fan of those \u20 spaces, but I don't see alternatives. Also, I'm not getting rid of all concatenations, but at least it's a step ahead from the current status (no hard-coded string, translated labels).

:kaze
Any suggestion before finding a reviewer?

Comment 18

[Julien Wajsberg [:julienw]]

Actually, we also want to get rid of "|" and use only "," (see bug 883911)

All this is not very clear though...

Comment 19

[Francesco Lodolo [:flod]]

To be honest I discovered most of the formats we're using from failing tests. 

The idea of this PR was to keep the status-quo for en-US and fix the localizability issues. 
Replacing ' | ' with ', ' should be relatively simple in this patch, not sure if it would break something else. That bug is not that clear though.

Comment 20

[Julien Wajsberg [:julienw]]

You're right, let's keep the status-quo in this bug.

Comment 21

[Fabien Cazenave [:kaze]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

Looks OK to me so far. I’ve let a few comments on your PR, and I’ll leave the tests to the reviewer — we could use a real *.properties file to test the l10n part.

(In reply to Francesco Lodolo [:flod] from comment #17)
> I'm not a fan of those \u20 spaces, but I don't see alternatives. Also, I'm
> not getting rid of all concatenations, but at least it's a step ahead from
> the current status (no hard-coded string, translated labels).

Agreed, that’s a step ahead — and we better aim for a small patch if we want to uplift this to 1.3. I’d prefer to get rid of all concatenations, but maybe you intend to do that in bug 883911?

Please r? Julien when you’re ready.

Comment 22

[Francesco Lodolo [:flod]]

(In reply to Fabien Cazenave [:kaze] — on PTO until 2013-03-14 from comment #21)
> Agreed, that’s a step ahead — and we better aim for a small patch if we want
> to uplift this to 1.3. I’d prefer to get rid of all concatenations, but
> maybe you intend to do that in bug 883911?

From what I see, that's a big task, so I'll leave it to real Gaia developers ;-)

I would say we need a shared way between apps to display that information, using string with placeholders for position and separator characters. 

As it is now, every single piece of code does its own thing, while a function that given info like (name, carrier, type, number) returns a nicely formatted string would ensure consistency and heavily simplify the existing code.

I'll review your comments and ask Julien for review, and thanks again for this Friday sprint.

It's not going to be 1.3, since this adds new strings, but definitely wanted for 1.4.

Comment 23

[Francesco Lodolo [:flod]]

(In reply to Fabien Cazenave [:kaze] — on PTO until 2013-03-14 from comment #21)
> Looks OK to me so far. I’ve let a few comments on your PR, and I’ll leave
> the tests to the reviewer — we could use a real *.properties file to test
> the l10n part.

Comments addressed. Not sure it would be faster to load a real .properties files, since I'm testing two case: different separators, empty separators (this one is fast, just make mozL10n.get always return '').

Comment 24

[Julien Wajsberg [:julienw]]

I didn't really review it yet (will do tomorrow) but I see it's quite risky. I recall we had some difficulty to make this code behave correctly, and we were told to not land even slightly risky non-necessary patch to 1.4.

Therefore, how necessary do you think this patch is? I mean, we could live with this for several versions now.

Comment 25

[Francesco Lodolo [:flod]]

It would definitely be nice to have, but not at risk of creating regressions, so I'll let you evaluate the risk of taking it in 1.4

I'm clearly not a good judge, based on what just happened in another bug (4 lines of code, 4 different people reviewed my patch, and still managed to screw things up...).

Comment 26

[Jason Smith [:jsmith]]

Too much risk for 1.3, so moving to backlog.

Comment 27

[Julien Wajsberg [:julienw]]

Francesco: oki, let's land this after branching then. (which also gives us some more time to do it right). Thanks !

Comment 28

[Julien Wajsberg [:julienw]]

Hey Rick, would you have time reviewing this? Especially because you did most of this initial code. Thanks in advance!

Comment 29

[Julien Wajsberg [:julienw]]

(Note: this should not land in master before the 1.4 branching. If it's ready to land before the branching, just add [ready-to-land-after-1.4-branching] in the whiteboard).

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

This looks good to me. Just a few nits to fix + maybe more fix to the test.

I asked about having more consistency between '|' and ',' in bug 883911, I don't like having so many different places where we do this magic...

Comment 31

[Francesco Lodolo [:flod]]

Julien, what needs be done in this bug to move it forward? I commented on Github on the test part, should I switch to use only "," as separator?

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

I think it needed another review request ;) I think I wanted to wait for some information in bug 883911.

I've mixed feelings between landing your patch and then do it again in bug 883911 or directly doing it right. Probably better to land your patch because it's ready and it still improves the situation.

Comment 33

[Julien Wajsberg [:julienw]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

Sorry, I have the same concerns about changing the MockL10n file. The explanation is that we'd like to eventually have only one mock for the whole project, in shared, and therefore we want no generic things in it.

Comment 34

[Francesco Lodolo [:flod]]

Makes sense. I'll look into it and request a new review when it's ready.

Comment 35

[Francesco Lodolo [:flod]]

I've updated my PR, restoring the original mockl10n.js and moving stubs into individual suites.

All SMS tests pass locally, not sure if there are cleaner/more elegant way to do this (I hope so).

Unfortunately Travis is acting up, so I'll wait tomorrow to update the PR and check if Travis is green.

Comment 36

[Francesco Lodolo [:flod]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

Green build
https://travis-ci.org/mozilla-b2g/gaia/builds/23179740

As I said before, I'm not sure if the approach for tests is the best possible.

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8387442 [details] [review]
Pull request on Github

r=me
this is as good as it gets, thanks !

please squash, rebase, and add 'checkin-needed' as keyword -if you don't have commit rights of course.

Comment 38

[Francesco Lodolo [:flod]]

Thanks Julien. Rebased to master, waiting for Travis to run green before setting checkin-needed.

Comment 39

[Francesco Lodolo [:flod]]

https://travis-ci.org/mozilla-b2g/gaia/builds/23612553

Setting checkin-needed

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/3557372133b8ca7e1e3b1387ce68ed4decd732fd