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

RESOLVED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: sarsenyev, Assigned: flod)

Tracking

({l12y})

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.4 affected, b2g-v2.0 fixed)

Details

(Whiteboard: [mentor=:julienw] LocRun1.2, LocRun1.3)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 822451 [details]
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
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...
(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.
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.
(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?
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.
Duplicate of this bug: 942705
This still happens on v1.3 Moz Ril
Label's aren't translated
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
Summary: [B2G][1.2][l10n][SMS] The labels (Home, Work...) aren't localized in the contact's message thread → [B2G][l10n][SMS] The labels (Home, Work...) aren't localized in the contact's message thread
Whiteboard: LocRun1.2 → LocRun1.2, LocRun1.3
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.
blocking-b2g: --- → 1.3?
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

4 years ago
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.
(Assignee)

Comment 12

4 years ago
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
(Assignee)

Comment 13

4 years ago
Created attachment 8386790 [details]
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.
Let's find out if this reproduces on 1.1.
Keywords: qawanted
(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.
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
Keywords: qawanted
(Assignee)

Comment 17

4 years ago
Created attachment 8387442 [details] [review]
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?
Attachment #8387442 - Flags: feedback?(kaze)
Actually, we also want to get rid of "|" and use only "," (see bug 883911)

All this is not very clear though...
(Assignee)

Comment 19

4 years ago
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.
You're right, let's keep the status-quo in this bug.
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.
Attachment #8387442 - Flags: feedback?(kaze) → feedback+
(Assignee)

Comment 22

4 years ago
(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.
(Assignee)

Comment 23

4 years ago
(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 '').
(Assignee)

Updated

4 years ago
Attachment #8387442 - Flags: review?(felash)
(Assignee)

Updated

4 years ago
Assignee: nobody → francesco.lodolo
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.
(Assignee)

Comment 25

4 years ago
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...).
Too much risk for 1.3, so moving to backlog.
blocking-b2g: 1.3? → backlog
Francesco: oki, let's land this after branching then. (which also gives us some more time to do it right). Thanks !
Hey Rick, would you have time reviewing this? Especially because you did most of this initial code. Thanks in advance!
Flags: needinfo?(waldron.rick)
(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 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...
Attachment #8387442 - Flags: review?(felash)
(Assignee)

Comment 31

4 years ago
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?
Flags: needinfo?(felash)
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.
Attachment #8387442 - Flags: review?(felash)
Flags: needinfo?(felash)
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.
Attachment #8387442 - Flags: review?(felash)
(Assignee)

Comment 34

4 years ago
Makes sense. I'll look into it and request a new review when it's ready.
(Assignee)

Comment 35

4 years ago
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.
Flags: needinfo?(waldron.rick)
(Assignee)

Comment 36

4 years ago
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.
Attachment #8387442 - Flags: review?(felash)
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.
Attachment #8387442 - Flags: review?(felash) → review+
(Assignee)

Comment 38

4 years ago
Thanks Julien. Rebased to master, waiting for Travis to run green before setting checkin-needed.
Status: NEW → ASSIGNED
(Assignee)

Comment 39

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

Setting checkin-needed
Keywords: checkin-needed
Whiteboard: LocRun1.2, LocRun1.3 → [mentor=:julienw] LocRun1.2, LocRun1.3
Master: https://github.com/mozilla-b2g/gaia/commit/3557372133b8ca7e1e3b1387ce68ed4decd732fd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.2: affected → wontfix
status-b2g-v1.3: affected → wontfix
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Duplicate of this bug: 1019356
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.