Closed
Bug 931119
Opened 11 years ago
Closed 10 years ago
[B2G][l10n][SMS] The labels (Home, Work...) aren't localized in the contact's message thread
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(tracking-b2g:backlog, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.4 affected, b2g-v2.0 fixed)
People
(Reporter: sarsenyev, Assigned: flod)
References
Details
(Keywords: l12y, Whiteboard: [mentor=:julienw] LocRun1.2, LocRun1.3)
Attachments
(3 files)
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•10 years ago
|
||
This still happens on v1.3 Moz Ril Label's aren't translated
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
Comment 8•10 years ago
|
||
Possibly related to bug 911415 ?
Updated•10 years ago
|
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
Updated•10 years ago
|
Whiteboard: LocRun1.2 → LocRun1.2, LocRun1.3
Comment 9•10 years ago
|
||
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?
Comment 10•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
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 15•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
Actually, we also want to get rid of "|" and use only "," (see bug 883911) All this is not very clear though...
Assignee | ||
Comment 19•10 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.
Comment 20•10 years ago
|
||
You're right, let's keep the status-quo in this bug.
Comment 21•10 years ago
|
||
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•10 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•10 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•10 years ago
|
Attachment #8387442 -
Flags: review?(felash)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → francesco.lodolo
Comment 24•10 years ago
|
||
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•10 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...).
Comment 27•10 years ago
|
||
Francesco: oki, let's land this after branching then. (which also gives us some more time to do it right). Thanks !
Comment 28•10 years ago
|
||
Hey Rick, would you have time reviewing this? Especially because you did most of this initial code. Thanks in advance!
Flags: needinfo?(waldron.rick)
Comment 29•10 years ago
|
||
(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•10 years ago
|
||
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•10 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 32•10 years ago
|
||
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 33•10 years ago
|
||
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•10 years ago
|
||
Makes sense. I'll look into it and request a new review when it's ready.
Assignee | ||
Comment 35•10 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.
Updated•10 years ago
|
Flags: needinfo?(waldron.rick)
Assignee | ||
Comment 36•10 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 37•10 years ago
|
||
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•10 years ago
|
||
Thanks Julien. Rebased to master, waiting for Travis to run green before setting checkin-needed.
Status: NEW → ASSIGNED
Assignee | ||
Comment 39•10 years ago
|
||
https://travis-ci.org/mozilla-b2g/gaia/builds/23612553 Setting checkin-needed
Keywords: checkin-needed
Updated•10 years ago
|
Whiteboard: LocRun1.2, LocRun1.3 → [mentor=:julienw] LocRun1.2, LocRun1.3
Comment 40•10 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/3557372133b8ca7e1e3b1387ce68ed4decd732fd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•