1115921 - [User Story][Lock Screen]Display remaining charging time on lockscreen

Status: RESOLVED FIXED

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

When user is charging their phone, we should show them the remaining time until the phone is charged.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: 2014-12-27-15-20-48.png

That's a screenshot from Flame running my branch.

wrt. the code, I'd like to make sure that we don't have the event listener on while the screen is off to limit CPU wakeups.

It also seems that currently, at least on Flame, we battery.chargingTime is always Infinite. We'll need to fix that before we add this feature.

Comment 2

[Eric Pang [:epang] UX]

Attachment: Lockscreen_charging_spec.jpg

Hi Zibi, thanks for your patience with this! :).  I've talked with Amy and we  looked at all possible scenarios on the lockscreen.  Thanks for the patch but unfortunately it won't work because the space is taken up by notifications and/or the music player.

I've spec'ed out adding the charging string under the date.  But also note that I've also changed the vertically spacing of the time and date since the notifications were too close to the string.  Is adjusting the layout of these elements something that's easy to change or would this cause problems?

Thanks and please let me know if you have any questions!

Comment 3

[Eric Pang [:epang] UX]

Attachment: Lockscreen_charging_spec.jpg

Updated the spec to include the % since it will no longer appear in the status bar.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Eric Pang [:epang] from comment #3)
> Created attachment 8546019 [details]
> Lockscreen_charging_spec.jpg
> 
> Updated the spec to include the % since it will no longer appear in the
> status bar.

Thanks for the spec! I like it a lot and will be happy to hack it :)

Two questions:

1) We do not have the charging estimation (waiting for :dliang in bug 1116368). Does it make sense to implement the spec partially for now to just show "59% Charged" until we have the remaining piece?

2) One of the UX flows I had in mind while planning it was the scenario similar to the charging one, when the user is looking at an unlocked phone and he wants to learn battery charge status.

I can imagine this happening commonly in two user stories:

a) User is charging the phone, but his screen is unlocked as he is browsing the web or chatting. He'd like to check how much of the battery is charged.

b) User is not charging the phone. He'd like to quickly learn his battery status to estimate how long will the battery will hold.

In my initial approach based on how iOS/Android solve that, I was thinking about teaching the user that the battery percentage is an additional bit of information available next to the battery icon in certain modes (lockscreen and notification unfolded).

With your spec this will not be possible. Do you plan to address the flow to allow the user to quickly check battery status in such cases?

I don't have a strong opinion here, I'm just curious if you think it's something worth exploring later and we may end up wanting to show battery percentage on the statusbar in some cases?

If you think it's worth getting partial implementation, I'll update my patch tomorrow and will try to get it into 2.2 :)

Comment 5

[Eric Pang [:epang] UX]

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> (In reply to Eric Pang [:epang] from comment #3)
> > Created attachment 8546019 [details]
> > Lockscreen_charging_spec.jpg
> > 
> > Updated the spec to include the % since it will no longer appear in the
> > status bar.
> 
> Thanks for the spec! I like it a lot and will be happy to hack it :)
> 
> Two questions:
> 
> 1) We do not have the charging estimation (waiting for :dliang in bug
> 1116368). Does it make sense to implement the spec partially for now to just
> show "59% Charged" until we have the remaining piece?

This works for me

> 
> 2) One of the UX flows I had in mind while planning it was the scenario
> similar to the charging one, when the user is looking at an unlocked phone
> and he wants to learn battery charge status.
> 
> I can imagine this happening commonly in two user stories:
> 
> a) User is charging the phone, but his screen is unlocked as he is browsing
> the web or chatting. He'd like to check how much of the battery is charged.
> 
> b) User is not charging the phone. He'd like to quickly learn his battery
> status to estimate how long will the battery will hold.
> 
> In my initial approach based on how iOS/Android solve that, I was thinking
> about teaching the user that the battery percentage is an additional bit of
> information available next to the battery icon in certain modes (lockscreen
> and notification unfolded).
> 
> With your spec this will not be possible. Do you plan to address the flow to
> allow the user to quickly check battery status in such cases?

This all comes down to the real estate of the status bar in 2.2.  With the Rocket bar in there as well we already don't have enough room for all icons so it wouldn't make sense to squish the % in there.  But in the future if this changes we can consider something like this.  I'll make sure to speak with Rob about it when we start exploring future versions.
> 
> I don't have a strong opinion here, I'm just curious if you think it's
> something worth exploring later and we may end up wanting to show battery
> percentage on the statusbar in some cases?
> 
> If you think it's worth getting partial implementation, I'll update my patch
> tomorrow and will try to get it into 2.2 :)

Let's give a try and see how it looks implemented, flag me for ui-review when ready :).
Thanks again for all your work on this Zibi!

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Alive, Can you take a look at that?

I need to add tests, but it seems to work well. I'd like to make sure that I hooked up the code properly.

Also, if you can help me verify if I conformed to the spec CSS spacing, that would help. I did not adjusted date/time spacing because I didn't know how to translate spec sizes into CSS sizes that we use in the lockscreen.css.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: not charging screenshot

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging screenshot with no estimation

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging screenshot with estimation in minutes

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging screenshot with estimation over an hour

Eric, I did not adjust the spacing, so I only would like you to look at this. When the time of charging is above an hour I'd like to display it in a form of "Xh and Y mins". Instead of "147 mins remaining".

Can you review that piece?

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also, Alive, do I have to add the charging here: https://github.com/mozilla-b2g/gaia/blob/f5e481d4caf9ffa561720a6fc9cf521a28bd8439/apps/callscreen/index.html#L80-L85 ? I'm not sure what this screen is.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8546962 [details] [review]
pull request

Forwarding to Greg since this touches lockscreen and only lockscreen.

Comment 13

[Eric Pang [:epang] UX]

Comment on attachment 8546966 [details]
charging screenshot with estimation over an hour

R+ based on the string format.  Can you flag me for review again when the spacing has been updated? Thanks!

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8546962 [details] [review]
pull request

Thanks for this nice feature. I've left some comments on the PR page, please set review if you fixed them and add the unit test.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks a lot Greg! Your feedback is awesome! I based my patch on the Clock xlass which was probably not the best choice ;)

I'll refactor the patch tomorrow and add tests.

Can you provide me tips on how to apply spacing changew from the spec? I've never worked with this and the line-heights from the spec dont match to anything in the css file I can find and also I dont know what the hires version should be.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

"spacing change from the spec"? Do you mean media-query?

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

And I updated the title and keyword for management.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

In comment 2 you can see that Eric added the spec and modified vertically spacing of the time and date to fit the new notifications. So in the spec attachment in this bug the line height and vertical paddings are updated. I don't know how to translate the numbers from the spec into our lockscreen.css. can you provide me some guideline here?

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

So the problem is you want to use line-height to match the lines & spaces but it failed. I think to set the DIV element's height or margin could solve the issue.

(For UX:)
Eric: I've noticed the spec is only with one SIM. Should UX updated it with two SIMs case? If we only adjust the line height to match the one SIM case only, it may be broken while user is on a multi-SIM device.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Greg, unfortunately don't think I understand how to achieve that. Maybe I'll try to ask specific questions:

Question 1:

Eric's spec specifies the space between time and Sim info to be 1.6 rem (2.4 rem on fvga).
In lockscreen/index.html [0] we have #lockscreen-conn-states and #lockscreen-clock-time.
conn-states have padding-bottom: 1.4rem; while clock-time has margin-top: -1.4rem.

How 1.6rem translates to 1.4rem + -1.4rem here and I'm not sure if I should increase conn-states padding, clock-time margin, or change something else. I also don't know what to do with fvga.

Question 2:

Eric's spec specifies the size of clock-time to be 7.3333R rem (11 rem on fvga).
The font for clock-time is specifies as 8.5 rem.

Should I decrease it to 7.333 rem? What to do about fvga?

Question 3:

The line-height for #lockscreen-date is specified as 7.3333R rem (11 rem on fvga). I'm not sure if that's an error on Eric's side (copied from the previous line?), or should the line-height of this line match the line-height of the previous. Also, no margins/paddings are defined between date and clock-time.

Question 4:

The line-height for the new #lockscreen-charging line is defined in spec as 1.6 rem (2.4 on fvga). In my pull-request that's how I defined it. Not sure if I have to do anything special for fvga.

Can you help me with those? (also Eric, can you look at my question 3)

[0] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/lockscreen.html
[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/style/lockscreen.css

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

I updated the pull request. Waiting for the instructions wrt. CSS, will add tests and re-request review.

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

Unfortunately I'm afraid that I can't help you a lot: those margins & paddings are always a mess that inherited from very early version like v1.2. I once tried to re-organize them with more clear rules but I've found there are too much things would fail mysteriously and you need to fix them one-by-one. So I'm not sure if for this feature to re-arrange and re-check them all is a good idea (the intention in the spec).

In my recollection, for the v2.1 notification feature I even measured whether the size is correct with a physical ruler while I connected to the device via AppManager and adjusted them at the same time, since I failed to make the CSS rules more reasonable. Yes, this sucks, but the fact is I'm the only person for LockScreen and need to face so much messy code & styles should be refactored, which are all inherited from the old version and stuck there like hell's cornerstones which are also protected by the abyss named TBPL & regression, I barely have resources to fix them all (right now I'm refactoring another mess that I really want to kill it twice).

So no, I have no reasonable answers for your rem questions. What I can do is to suggest that we don't fix them all at this moment, but focus on the line your feature would add, and ask UX if they can review it as I said.

Comment 23

[Eric Pang [:epang] UX]

Attachment: Lockscreen_charging_spec.jpg

Hi Zibi/Greg,

Sorry the time should stay the same size, I've removed it from the spec now.

I'm wondering how the notifications portion of the screen works.  Does the gradient adjust vertically based on the space?  Or is it in a set position?  If it's variable we may not need to mess with the vertical spacing at all.

Which leaves us with two options:

1. If it's variable, I think we should leave all vertically spacing and see how it looks (we'll just need to make sure to test on hvga, flame with and without dual sim).
 
2. If it's in a set position we run into more issues.  As Grey mentioned, multi sim takes up even more vertical space (Which will need another spec). 

In this case I'm wondering if it will be less trouble to change the positioning of the notification gradient?

Let me know what you think,

Eric

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #22)
> Unfortunately I'm afraid that I can't help you a lot: those margins &
> paddings are always a mess that inherited from very early version like v1.2.
> I once tried to re-organize them with more clear rules but I've found there
> are too much things would fail mysteriously and you need to fix them
> one-by-one. So I'm not sure if for this feature to re-arrange and re-check
> them all is a good idea (the intention in the spec).

Oh, ok! I wasn't aware of that.

I would be happy to help you sorting this out, but that's definitely out of the scope for this bug.

Maybe LockScreen 2.0 next to System 2.0? ;)

I'll try to test the space with overflowing notifications and dual-sim and if it'll fit I'll post screenshots here.

I'm still not sure how to treat the fvga. I only test on Flame and I don't know if it's fvga or not.

Comment 25

[Greg Weng [:snowmantw][:gweng][:λ]]

For FVGA part I think you need to have a phone with such resolution, like Buri or Open. I'm not sure if B2G desktop is a good tool to simulate that, unless you know how to bridge your AppManager with the simulator. I could post a how-to after I asked others.

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

The argument for B2G desktop is:

  -start-debugger-server 6000

So I think the full line of command is:

  ./b2g-bin --screen=320x480@160 -profile /path/to/your/gaia/profile -start-debugger-server 6000

With the screen argument it can be FVGA

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot on Flame with all whistles

Eric, so I turned on dual sim, music and made a bunch of screenshots to see how it looks on flame.

Here's a screenshot.

I can adjust positioning of the elements around Date/Time/Sim if you want, I just need to know what to move/resize :)

I'll try to get the fvga from emulator now and finish tests.

Comment 28

[Eric Pang [:epang] UX]

(In reply to Zibi Braniecki [:gandalf] from comment #27)
> Created attachment 8552130 [details]
> Screenshot on Flame with all whistles
> 
> Eric, so I turned on dual sim, music and made a bunch of screenshots to see
> how it looks on flame.
> 
> Here's a screenshot.
> 
> I can adjust positioning of the elements around Date/Time/Sim if you want, I
> just need to know what to move/resize :)
> 
> I'll try to get the fvga from emulator now and finish tests.

Hey Zibi, thanks for working on this.  Is it possible to have the vertical alignment changed between 1 and 2 sim(s)?  So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine).  For 2 sims can we move the sim info, time, date and charging up 1.2rem (on HVGA or 1.8 on flame).  Would this be possible?

Also, from my understanding FWVGA is the flame device and HVGA is a device like the hamachi.  We need to find HVGA device to test on.

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Eric Pang [:epang] from comment #28)
> Hey Zibi, thanks for working on this.  Is it possible to have the vertical
> alignment changed between 1 and 2 sim(s)?

Sure.

> So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine). 
> For 2 sims can we move the sim info, time, date and charging up 1.2rem (on
> HVGA or 1.8 on flame).  Would this be possible?
> 
> Also, from my understanding FWVGA is the flame device and HVGA is a device
> like the hamachi.  We need to find HVGA device to test on.

Yeah. I got Buri with master so I can test on both Flame and Buri. I'll try to get the patch and screenshots for you tomorrow.

Comment 30

[Eric Pang [:epang] UX]

(In reply to Zibi Braniecki [:gandalf] from comment #29)
> (In reply to Eric Pang [:epang] from comment #28)
> > Hey Zibi, thanks for working on this.  Is it possible to have the vertical
> > alignment changed between 1 and 2 sim(s)?
> 
> Sure.
> 
> > So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine). 
> > For 2 sims can we move the sim info, time, date and charging up 1.2rem (on
> > HVGA or 1.8 on flame).  Would this be possible?
> > 
> > Also, from my understanding FWVGA is the flame device and HVGA is a device
> > like the hamachi.  We need to find HVGA device to test on.
> 
> Yeah. I got Buri with master so I can test on both Flame and Buri. I'll try
> to get the patch and screenshots for you tomorrow.

Great, thanks Zibi!

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Flame, empty, charging

Comment 32

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Flame, notifications, charging

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Flame, music, charging

Comment 34

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Flame, music and notifications, charging

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Buri, empty, charging

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Buri, notifications, charging

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Buri, music, charging

Comment 38

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Buri, music and notifications, charging

Comment 39

[Zibi Braniecki [:zbraniecki][:gandalf]]

Eric, so, actually adjusting CSS to how many card state lines we have is not trivial.

I took a different approach and modified the CSS to make room for charging. 
Those are the changes I made: https://github.com/zbraniecki/gaia/commit/35e2f08ecfcccab3e8d780be626f65e2b843ad84

I attached screenshots from Flame and Buri to show the outcome.

I also confirmed that "Emergency calls only (no SIM)" two-liner takes exactly the same lines as two SIM lines.
In Buri scenario, when there's music there is only one notification at a time.
In both scenarios, when there's one SIM card, all other header components go up making more room between the charging line and notifications.

I can probably adjust the spacing more for the Buri, notifications scenario, by either reducing the header top-padding a bit more (I reduced it by 0.5rem) or reducing the space between time and date (I reduced the line-height of time from 1.0 to 0.9 already).

Let me know what you think. :) With Buri and Flame at hand, I can easily test and iterate.

Comment 40

[Eric Pang [:epang] UX]

(In reply to Zibi Braniecki [:gandalf] from comment #39)
> Eric, so, actually adjusting CSS to how many card state lines we have is not
> trivial.
> 
> I took a different approach and modified the CSS to make room for charging. 
> Those are the changes I made:
> https://github.com/zbraniecki/gaia/commit/
> 35e2f08ecfcccab3e8d780be626f65e2b843ad84
> 
> I attached screenshots from Flame and Buri to show the outcome.
> 
> I also confirmed that "Emergency calls only (no SIM)" two-liner takes
> exactly the same lines as two SIM lines.
> In Buri scenario, when there's music there is only one notification at a
> time.
> In both scenarios, when there's one SIM card, all other header components go
> up making more room between the charging line and notifications.
> 
> I can probably adjust the spacing more for the Buri, notifications scenario,
> by either reducing the header top-padding a bit more (I reduced it by
> 0.5rem) or reducing the space between time and date (I reduced the
> line-height of time from 1.0 to 0.9 already).
> 
> Let me know what you think. :) With Buri and Flame at hand, I can easily
> test and iterate.

Hey Zibi, this looks really good (thanks for attaching all the screens for me to see!).  Only a couple of comments. 

1. In comment 36 the notifications are overlapping, but I feel this is an edge case since the user will more then likely have a sim card in.  Once in we shouldn't have any problems.  So I think this is fine.

2. In comment 38 is there anyway we can vertically center the notification between the charge and music player (since there's only 1)?

Let me know, thanks again!

Comment 41

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screenshot: Buri, music and notifications, charging

Centered.

Comment 42

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546962 [details] [review]
pull request

Hi Greg,

I think the patch is ready for your review.

I followed Eric's feedback on CSS updates.

I also added unit tests. One limiting feature is that I can't overwrite navigator.battery so I can't mock it which makes it impossible to test detailed result of chargingstatus.refresh() since it reads navigator.battery properties.

I hope that once we update the API to match the spec (bug 1047119), we will be able to override it and test more details.

For now, I hope it covers the feature well enough.

Comment 43

[Eric Pang [:epang] UX]

Comment on attachment 8558168 [details]
Screenshot: Buri, music and notifications, charging

Looks good to me, thanks again Zibi!

Comment 44

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8546962 [details] [review]
pull request

Okay, I think the code is good, thanks.

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/3c43615cf728840be9e0165f16388672d8e97f2e
Merge: https://github.com/mozilla-b2g/gaia/commit/c23229ab79d46070dc8f0743ab592df552be7506

Thanks Eric and Greg!

Comment 46

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8546962 [details] [review]
pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): feature
[User impact] if declined: no charging info on lockscreen
[Testing completed]: unit tests and tests on Flame and Buri
[Risk to taking this patch] (and alternatives if risky): no known risks
[String changes made]: two additional strings for System/Lockscreen app

Comment 47

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/70730ccda6465b846b152fee8404d163b417aec8