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

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

Attachment: 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

Comment 1

[Eric Chang [:ericcc] [:echang]]

[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

Comment 2

[Eric Chang [:ericcc] [:echang]]

Attachment: 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?

Comment 3

[Doug Sherk (:drs) (inactive)]

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.

Comment 4

[David Garcia [:davidg][retired]]

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.

Comment 5

[David Garcia [:davidg][retired]]

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.

Comment 6

[Eric Chang [:ericcc] [:echang]]

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.

Comment 7

[Anthony Ricaud (:rik)]

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.

Comment 8

[David Garcia [:davidg][retired]]

Attachment: github PR

Comment 9

[David Garcia [:davidg][retired]]

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 10

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8483472 [details] [review]
github PR

See my comments on the PR.

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: major issue on new feature

Comment 12

[David Garcia [:davidg][retired]]

Fixed comments

Comment 13

[Doug Sherk (:drs) (inactive)]

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.

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8483472 [details] [review]
github PR

I left a few small comments on the PR, but this otherwise looks good.

Comment 15

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/b2bc7392fd1a1407ba3fc774fd84e185a5a09365

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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 :(

Comment 17

[Francesco Lodolo [:flod]]

+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.

Comment 18

[David Garcia [:davidg][retired]]

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 19

[David Garcia [:davidg][retired]]

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

Comment 20

[Matej Novak [:matej]]

Setting a ni? for Stephany.

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

Thanks.

Comment 21

[Stephany Wilkes]

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?

Comment 22

[Matej Novak [:matej]]

(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.

Comment 23

[Francesco Lodolo [:flod]]

(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.

Comment 24

[Stephany Wilkes]

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.

Comment 25

[Anthony Ricaud (:rik)]

Stephany: looks like you forgot to needinfo the Joe you're talking about :)

Comment 26

[Stephany Wilkes]

Ha - thanks for the catch, Rik! :)

Comment 27

[Carrie Wang [New account: [e-mail]]]

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!

Comment 28

[Doug Sherk (:drs) (inactive)]

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.

Comment 29

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 30

[Francesco Lodolo [:flod]]

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

Comment 31

[David Garcia [:davidg][retired]]

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

Comment 32

[David Garcia [:davidg][retired]]

Attachment: Follow-up patch changing strings

Comment 33

[Doug Sherk (:drs) (inactive)]

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.

Comment 34

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/8cbf8501495d871e0a2783b41ddbba2c1ec46993

Comment 35

[Carrie Wang [New account: [e-mail]]]

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!

Comment 36

[David Garcia [:davidg][retired]]

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.

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 8483472 [details] [review]
github PR

Requesting QA verification on testing around call screen duration displaying as expected on landing.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.1 uplift.

Comment 39

[David Garcia [:davidg][retired]]

Attachment: 2.1 patch

Comment 40

[Doug Sherk (:drs) (inactive)]

v2.1: https://github.com/mozilla-b2g/gaia/commit/59e5c2467b7b8219ed194a0d0a94c6ed59af95be

Comment 41

[Fred Muyumba [:FredM]]

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'

Comment 42

[KTucker [:KTucker][Inactive 3/4/2016]]

Fred, can you recheck this? It should be showing h m s. Please confirm.

Comment 43

[Duane Dixon [:ddixon]]

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