Closed Bug 1061120 Opened 6 years ago Closed 5 years ago

[Calllog][Dialer] Time duration format is not showing 'min' or 's' in 'Call Information'

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S4 (12sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: echang, Assigned: davidg)

References

Details

(Keywords: late-l10n, Whiteboard: [planned-sprint c=3])

Attachments

(5 files)

Attached image 2014-09-01-16-20-32.png
### STR
1. Have a connected call in call log more than 0 second.

### Actual
E.G. 00:04 shown.
2014-09-01-16-20-32.png

### Expected
00 min 4 s
Page 5/Call info view/https://bugzilla.mozilla.org/attachment.cgi?id=8463237

#version
Gaia      2be78d83a760fa3b9638fe51c266b442d14597f1
Gecko     https://hg.mozilla.org/mozilla-central/rev/1db35d2c9a2f
BuildID   20140831160203
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230
[Blocking Requested - why for this release]: feature not complete
00 min 4 s
Page 5/Call info view/https://bugzilla.mozilla.org/attachment.cgi?id=8463237
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Gaia::Dialer]
Depends on: 877971
Attached image CallInformation.png
Hi Carrie, Got a question on the formatting: 
A call is more than 1 minute, shown as 1 min 5 s.
A call is exactly 1 minute, shown as 1 min.
A call is less than 1 minute, shown as 12 s.
[Q] A call is exactly 0 minute, shown as???  0 min 0 s? 
[Q] How about a call is more than 1 hour? 1hr 3 min 2 s?
Flags: needinfo?(cawang)
Whiteboard: [planned-sprint]
Target Milestone: --- → 2.1 S4 (12sep)
Whoever takes this should discuss this with the l10n team to see if we can share some code with l10n_date.js. See bug 1060355.

If we decide against sharing, it'll just be as simple as passing a different parameter to the formatting function, so it should be fairly trivial.
Whiteboard: [planned-sprint] → [planned-sprint c=3]
Assignee: nobody → david.garciaparedes
Eric,
Looking at the code it seems that when the duration is exactly 0, it will say 'Missed' if it is an incoming call or 'Canceled' if it is an outgoing call.
I discussed with :stas in IRC about moving prettyDuration to l10n_date:
Quoting him:
"semantically I don't see an issue, but I'd like to avoid growing l10n_date too much, b/c it's used in many apps. I wouldn't be opposed to having shared/js/l10n_duration.js if there's a needfor this logic to be used outside of Dialer"

Because currently this seems to be only used in dialer and I'm not sure other applications will want to show durations with this specific format, I think we can probably leave it in dialer/utils.
Actually, if you pick up and hang up an incoming call really quick, you will have a 00:00 call shown on call screen, and in Call information, that's how I got that.
https://bug1061120.bugzilla.mozilla.org/attachment.cgi?id=8482633

(In reply to David Garcia [:davidg] from comment #4)
> Eric,
> Looking at the code it seems that when the duration is exactly 0, it will
> say 'Missed' if it is an incoming call or 'Canceled' if it is an outgoing
> call.
I'm not sure we can distinguish a non-connected call and a call of 0 seconds. I don't think it really matters in real world usage though. And if it does, it should be handled in another bug.
Attached file github PR
Attachment #8483472 - Flags: review?(drs+bugzilla)
Eric, 
You are right. The call duration is in miliseconds, so it can be greater than 0 but less than a second. In that case im just showing '0 s'. 
I'm following your suggestion of using this format '1hr 12min 30s' when it is more than an hour.
Comment on attachment 8483472 [details] [review]
github PR

See my comments on the PR.
Attachment #8483472 - Flags: review?(drs+bugzilla) → review-
triage: major issue on new feature
blocking-b2g: 2.1? → 2.1+
Attachment #8483472 - Flags: review- → review?(drs+bugzilla)
Fixed comments
Keywords: late-l10n
Comment on attachment 8483472 [details] [review]
github PR

I'd like to see another version, but I think we'll be good to go after that. See comments on PR.
Attachment #8483472 - Flags: review?(drs+bugzilla) → review-
Attachment #8483472 - Flags: review- → review?(drs+bugzilla)
Status: NEW → ASSIGNED
Comment on attachment 8483472 [details] [review]
github PR

I left a few small comments on the PR, but this otherwise looks good.
Attachment #8483472 - Flags: review?(drs+bugzilla) → review+
https://github.com/mozilla-b2g/gaia/commit/b2bc7392fd1a1407ba3fc774fd84e185a5a09365
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Note that due to recent policy changes, all patches must have approval for uplift regardless of blocking status. Please request Gaia v2.1 approval on this patch when you get a chance. Sorry for the inconvenience :(
Flags: needinfo?(david.garciaparedes)
+callDurationTextSeconds={{ s }} s
+callDurationTextMinutes={{ m }} min {{ s }} s
+callDurationTextHours={{ h }} hr {{ m }} min {{ s }} s

Is this text coming from a spec? Because the mix of abbreviations looks awkward to me: hr/min/s. I'd expect h/m/s, or hr/min/sec. The first one would be probably more consistent with what we already use around Gaia.
Francesco,
In the spec [1] page 5 it says "we adopt “min” and “s” as the abbreviation of “minutes” and “seconds” to display the call length."

[1] https://bug877971.bugzilla.mozilla.org/attachment.cgi?id=8463237
Comment on attachment 8483472 [details] [review]
github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 877971
[User impact] if declined: Call duration in call info page will be displayed with this format "05:10" instead of this format "5 min 10 s"
[Testing completed]: Added unit tests in apps/sharedtest/test/unit/dialer/utils_test.js 
[Risk to taking this patch] (and alternatives if risky): The function changed it is also used in call screen for displaying call duration, but it has been tested that it works the same as before.
[String changes made]: Added 3 new string in apps/sharedtest/test/unit/dialer/utils_test.js
+callDurationTextSeconds={{ s }} s
+callDurationTextMinutes={{ m }} min {{ s }} s
+callDurationTextHours={{ h }} hr {{ m }} min {{ s }} s

Requesting approval for 2.1
Attachment #8483472 - Flags: approval-gaia-v2.1?(release-mgmt)
Flags: needinfo?(david.garciaparedes)
Setting a ni? for Stephany.

Have a look at comment 17 and comment 18 in particular.

Thanks.
Flags: needinfo?(swilkes)
Matej, is this flag for patch approval? If so, we need to be flagged on ui-review? for the actual patch so I know which one to pull and check on the device before this can be approved for uplift.

Won't the "min" and "s" have to be translated, and thus count as new strings and be checked for translation issues? It does look like new strings were added per comment #19, but I'm just wondering if this format actually localizes well or is what users expect in our markets. Regardless, has that happened or is this now in late-l10n territory?
Flags: needinfo?(swilkes)
(In reply to Stephany Wilkes from comment #21)
> Matej, is this flag for patch approval? If so, we need to be flagged on
> ui-review? for the actual patch so I know which one to pull and check on the
> device before this can be approved for uplift.
> 
> Won't the "min" and "s" have to be translated, and thus count as new strings
> and be checked for translation issues? It does look like new strings were
> added per comment #19, but I'm just wondering if this format actually
> localizes well or is what users expect in our markets. Regardless, has that
> happened or is this now in late-l10n territory?

I have to admit I know very little about this bug. I was just added yesterday, but didn't see any questions specifically for me. I just wanted to make sure you added it to the audit.

Sorry for the confusion.
(In reply to Stephany Wilkes from comment #21)
> Matej, is this flag for patch approval? If so, we need to be flagged on
> ui-review? for the actual patch so I know which one to pull and check on the
> device before this can be approved for uplift.

My question wasn't about uplift or late-l10n, more about the mix of abbreviations. I don't think we ever used 'hr' in Gaia for example.
Flagging Joe. Joe, do you know if this is a) a standard time display and really b) whether or not a standard display is actually required? Just double checking the requirements on time display before it lands.
Stephany: looks like you forgot to needinfo the Joe you're talking about :)
Flags: needinfo?(swilkes)
Ha - thanks for the catch, Rik! :)
Flags: needinfo?(swilkes) → needinfo?(jcheng)
Yes, 

1. If users pick up the call and hang up immediately so the call is exactly 0 minute, we shown "0 min 0 s" 
2. I'd say adopt "hr" as "hour", but I think we need to ask some localization folks to see if this is a standard format.
Thanks!
Flags: needinfo?(cawang)
After an IRC discussion, Carrie is going to ask on the world-ready mailing list what the standardized format should be. We may have to back this out or do a followup for it if the format is wrong.
Flags: needinfo?(cawang)
not sure if this is a requirement coming from any partner but i don't think so (ni? WDM)

my 2 cents
hour, min, sec probably varies per locale
this means we may need to keep a table of the standard format display per locale (if we don't already have such table) and run into risks of an overly crowded line for some locales. (not sure if in some locale min/sec goes before the numbers like min 00 sec 05?, just my speculation). Perhaps a locale independent solution can be considered here
Flags: needinfo?(jcheng) → needinfo?(wmathanaraj)
I'm not sure this is material for world-ready: since the string is localizable, people will localize 'hr' as they consider fit. 
Having said that.

1. If you use 'h' or 'hr', we can live without proper plural forms. If you use "hour", we need a proper plural form (1 hour, 2 hours, etc.) and a lot of space.

2. (my first and main question) en-US is not being consistent

We use 'h' here (I think for all notifications in the tray)
https://hg.mozilla.org/gaia-l10n/en-US/file/default/shared/date/date.properties#l172

And, if you're curious, you can also see how that string is currently localized
http://transvision.mozfr.org/string/?entity=shared/date/date.properties:hours-ago-short%5Bother%5D&repo=gaia
Being this a late-l10n bug, tomorrow is the last day for a possible land in 2.1.
Please let me know if you have any conclusion so I can prepare a patch  otherwise it can not be fixed in 2.1 version
Attachment #8487950 - Flags: review?(drs+bugzilla)
I've decided to take Francesco's suggestion since he's the only one who has shown any degree of certainty. attachment 8487950 [details] [review] moves us from the "hr min s" format to "h m s". Once it lands, we're going to request uplift of both patches.

If we want to change this afterwards, we can, just not within the 2.1 timeframe.
Attachment #8487950 - Flags: review?(drs+bugzilla) → review+
per comment 30, I think we should just adopt "h m s" for now. If there has any concern, we can change it later. Thanks!
Flags: needinfo?(cawang)
Comment on attachment 8487950 [details] [review]
Follow-up patch changing strings

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 877971
[User impact] if declined: Call duration format will be hr/min/s instead of h/m/s
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky):
[String changes made]: yes

This needs to be landed AFTER the previous patch in this bug.
Attachment #8487950 - Flags: approval-gaia-v2.1?(release-mgmt)
Keywords: verifyme
Comment on attachment 8483472 [details] [review]
github PR

Requesting QA verification on testing around call screen duration displaying as expected on landing.
Attachment #8483472 - Flags: approval-gaia-v2.1?(release-mgmt) → approval-gaia-v2.1+
Attachment #8487950 - Flags: approval-gaia-v2.1?(release-mgmt) → approval-gaia-v2.1+
Needs rebasing for v2.1 uplift.
Flags: needinfo?(david.garciaparedes)
Attached file 2.1 patch
Flags: needinfo?(david.garciaparedes)
This issue is verified fixed on  Flame 2.2 Master KK (319mb) (Full Flash) and  Flame 2.1 KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame 2.2 Master KK (319mb) (Full Flash)
BuildID: 20141017040208
Gaia: abef62c0623e5504a97b4fd411e879a67b285b52
Gecko: ae1dfa192faf
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 36.0a1 
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

--------------------------------------------------------------------------------------

Environmental Variables:
Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141017001201
Gaia: 1ea74943cfe525c76a074ca1d7de8e51a70f6b98
Gecko: 2befa902ff5c
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Result: Time duration format SHOWS 'min' or 's' in 'Call Information'
Status: RESOLVED → VERIFIED
QA Whiteboard: [COM=Gaia::Dialer] → [COM=Gaia::Dialer][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Fred, can you recheck this? It should be showing h m s. Please confirm.
QA Whiteboard: [COM=Gaia::Dialer][QAnalyst-Triage?] → [COM=Gaia::Dialer][QAnalyst-Triage-]
Flags: needinfo?(ktucker) → needinfo?(fmuyumba)
Confirming that issue is verified fixed in Flame 2.2, 2.1 (Full Flash, Nightly). 

Actual Results:  Elapsed time is correctly logged in Call Information screen.  Hours, seconds, and minutes are shown correctly. 

Device: Flame Master
Build ID: 20141020040204
Gaia: dc496d04907dd314f9736ff78bab3bd27156f79a
Gecko: ca6b46cbca3b
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1
Build ID: 20141020001201
Gaia: 2904ab80816896f569e2d73958427fb82aebaea5
Gecko: 12dc9b782f2a
Version: 34.0 (2.1)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [COM=Gaia::Dialer][QAnalyst-Triage-] → [COM=Gaia::Dialer][QAnalyst-Triage?]
Flags: needinfo?(fmuyumba) → needinfo?(ktucker)
Flags: needinfo?(wmathanaraj)
QA Whiteboard: [COM=Gaia::Dialer][QAnalyst-Triage?] → [COM=Gaia::Dialer][QAnalyst-Triage+_]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.