Closed Bug 1172621 Opened 6 years ago Closed 6 years ago

Switch lockscreen from l10n_date to Intl

Categories

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
gsvelto
: review+
drs
: review+
gweng
: review+
Details | Review
No description provided.
Blocks: 1170963
Depends on: 1172609
Attached file pull request
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Hi Gabriele. In bug 1060333 you changed the behavior of the lockscreen to not display the am/pm portion in 12h scenario.

In this bug I'm migrating the lockscreen to use Intl API which has hour12 setting. The current patch takes hour12 from navigator.mozHour12 and displays it.
Do we really not want to have am/pm on the lockscreen? I thought it was supposed to be there, just smaller.

If that's the case, I can just set hour12: false and remove the eventlistener on mozhour12change.
Flags: needinfo?(gsvelto)
Ah, no, I cannot regardless of if we want the am/pm portion. I still need to react to mozHour12 since it controls if time is shown in 12/24 format and I need to remove the am/pm portion.

I updated the patch to incorporate callscreen. I didn't update tests yet.

One thing I'd like to consider is actually not removing am/pm from the lockscreen's clock. I think it's useful for users who use 12h clock format and I'm not sure what are we achieving by removing it. But that may be a question to someone from UX.
I've been wondering why the original code was doing that; since I wasn't involved in the callscreen/lockscreen at the time I assumed it must have been an UX decision. It turns out that it wasn't as the UI mock in bug 887802 shows the 'PM' string besides the clock (see  attachment 803600 [details] and attachment 803629 [details]). The string does look smaller than the rest of the clock so maybe it was hidden as a hack because they hadn't made it the right size yet.

I'd be in favor of showing it again but you'll need UX feedback about it and probably you'll also need to put some code in place to make it use a smaller font.

NI'ing Steve Chung and Greg Weng too since they worked on this code back in the day and might know more about it.
Flags: needinfo?(schung)
Flags: needinfo?(gweng)
We don't show PM/AM on LockScreen according to UX' opinon and spec of 2.1 (or 2.2). However, you know how chaotic our spec management is, so if you want the new spec we need to dig in it to find that. The spec you found is an very old one and it's for 1.2. So, if Zibi only concerns about whether to show or not to show the PM/AM, the answer is we don't show that now.
Flags: needinfo?(gweng)
Sorry that it's kind of out of ballpark to me that I only knew very little about the old pre 2.0 callscreen and the AM/PM was still there at that time. Maybe you can ask Rob(:robmac) if you have any thought about the lockscreen design.
Flags: needinfo?(schung) → needinfo?(rmacdonald)
Thanks for your feedback guys, so short answer is: we don't want to show the AM/PM part for now. We can always file a bug to reconsider that after this one lands.
Flags: needinfo?(gsvelto)
Comment on attachment 8616901 [details] [review]
pull request

This patch keeps the "no am/pm" logic, while removing LazyL10n, and l10n_date.js uses migrating them to Intl for the Lockscreen clock/date.
Attachment #8616901 - Flags: review?(gsvelto)
Attachment #8616901 - Flags: review?(drs)
This is the second patch after the statusbar time icon.
Flags: needinfo?(jlorenzo)
Comment on attachment 8616901 [details] [review]
pull request

The callscreen bits LGTM but you'll need :gweng's review for the lockscreen ones.
Attachment #8616901 - Flags: review?(gsvelto) → review+
Comment on attachment 8616901 [details] [review]
pull request

Oh, I thought :drs is for lockscreen :) Reassigning r? to gweng.
Attachment #8616901 - Flags: review?(drs) → review?(gweng)
Comment on attachment 8616901 [details] [review]
pull request

Greg, could you take a look at the Lockscreen parts of this?
Attachment #8616901 - Flags: review?(gweng)
Attachment #8616901 - Flags: review+
Whoops, mid-air collision there. I'm a Dialer peer, so Gabriele's review was enough.
Doug, Gabriele, also. I don't know how often the callscreen's "showClock" function is called, but if it's called often we can move the Intl.DateTimeFormat out of that function the same way as we do this in system's lockscreen and then set an eventlistener on languageschange/mozhourchange to regenerate it.

It only makes sense if showClock is performance sensitive. Let me know if you think it makes sense.
Flags: needinfo?(gsvelto)
Flags: needinfo?(drs)
It's probably one of the least performance-intensive function in the whole of gaia, we call it only once when showing the callscreen's lockscreen mock so don't worry :-)
Flags: needinfo?(gsvelto)
Flags: needinfo?(drs)
Comment on attachment 8616901 [details] [review]
pull request

Thanks for patching. I have some concerns and commented them on the PR page.
Attachment #8616901 - Flags: review?(gweng)
Comment on attachment 8616901 [details] [review]
pull request

Gerg, I responded to your concerns and also fixed the last Gij test so it should be green now.
Attachment #8616901 - Flags: review?(gweng)
Comment on attachment 8616901 [details] [review]
pull request

Okay, with Zibi's explanation on the PR page, it looks good to me. Thanks.
Attachment #8616901 - Flags: review?(gweng) → review+
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Contact: ddixon
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Comment on attachment 8616901 [details] [review]
> pull request
> 
> This patch keeps the "no am/pm" logic, while removing LazyL10n, and
> l10n_date.js uses migrating them to Intl for the Lockscreen clock/date.

Hi Zibi, 

I have tested your patch: "1172621-move-lockscreen-to-intl" with the latest Flame engineering build. 

Environmental Variables:
Device: Flame 2.5 (Shallow Flash)
Build ID: 20150724130844
Gaia: 199bf066a34a8ad5f4b84e1cf3f6083a2f44c9f5
Gecko: a1829935d87e
Version: 42.0a1 (2.5)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Actual Results: 
If the user manually changes the Region to anything other than "America", the device will display AM or PM at the lockscreen.
Flags: needinfo?(ktucker)
Keywords: qawanted
(In reply to Duane Dixon [:ddixon] from comment #19)
> Actual Results: 
> If the user manually changes the Region to anything other than "America",
> the device will display AM or PM at the lockscreen.

That's unfortunately once again bug 1172609.

What happens is:

We don't have a clean way to display 12-hour clock without am/pm portion using Intl (we also didn't have a clean way to do this before. We'll try to add it to Intl API v2).

So what we do is we generate the am/pm portion for a given locale and then use string.replace to remove it from the time string.

Now, since we have the timezone bug, it may happen that we generate the amPm portion for the *old timezone* (for example "AM") and then try to replace it in the time string with the *new timezone* (for example "7:43 PM"). The result is that the removal piece doesn't work.

It will get fixed when bug 1172609 lands.
Flags: needinfo?(rmacdonald)
Flags: needinfo?(ktucker)
Flags: needinfo?(jlorenzo)
(also, restarting B2G fixes it of course)
Commit: https://github.com/mozilla-b2g/gaia/commit/899e795e94aa71964443e08d6861964a5d2b75e3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1188162
You need to log in before you can comment on or make changes to this bug.