Closed Bug 824551 Opened 11 years ago Closed 11 years ago

Always display the low power window when the battery level go below 10%, but only once

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:tef+, blocking-basecamp:-, b2g18+ fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
blocking-b2g tef+
blocking-basecamp -
Tracking Status
b2g18 + fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: Firefox_Mozilla, Assigned: julienw)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0
Build ID: 20120625030537

Steps to reproduce:

1、When the phone battery is greater than 20% and the battery box is in yellow, then operate the phone and view.

2、When the phone battery is less than 20% and the battery box is in red, then operate the phone and view.

build infomation:
gecko:   	 revision="3cbade1974968bb1e0fbb0c3386239715244a7a7"
gaia: 	 	 revision="aab72f365d73f624ede32b522f27d072c409e42e"
gonk-misc:   revision="654358494ba601a46ef9838debc95417ae464cc6"
dalvik:      revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce"
librecovery: revision="e1bd90051c9e937221eb1f91c94e3cde747311a7"
moztt:       revision="6ee1f8987ef36d688f97064c003ad57849dfadf2"
external/jsmin:    revision="cec896f0affaa0226c02605ad28d42df1bc0e393"
external/opensans: revision="b5b4c226ca1d71e936153cf679dda6d3d60e2354"
device/qcom/b2g_common/mozilla-b2g: revision="41c17a6abfd5f488ec99d9aa246f5b07583403c7"



Actual results:

1、When the phone battery is greater than 20% and the battery box is in yellow, the interface of power alarm appears erratic.

2、When the phone battery is less than 20% and the battery box is in red , the interface of power alarm appears erratic.Sometimes it does not appear once in an hour, sometimes it prompted twice in less than ten minutes. 


Expected results:

when Non-low-power,  battery indicator interface do not appear, if it appears, it should appear regularly.
Summary: [power management]the interface of low power alarm appears erratic-617001930730 → [Open_][power management]the interface of low power alarm appears erratic-617001930730
Triage: QAWANTED to confirm the behavior of low power indication. It seems like the low power indicator pops up randomly to reporter
Keywords: qawanted
blocking-basecamp: --- → ?
I've CC'd some people who might be able to help with QA verification.  We discussed this during triage and thought it might be a Gaia issue but please move back to Hardware if we're wrong.
Component: Hardware → Gaia::System
blocking-basecamp: ? → -
tracking-b2g18: --- → +
Andrew, 
   Can someone write up what is the expected behavior of low power indication. 
I am a bit uninitiated.
blocking-basecamp: - → ?
tracking-b2g18: + → ---
Triage: needinfo for comment 3
Flags: needinfo?(overholt)
(In reply to dclarke@mozilla.com  [:onecyrenus] from comment #3)
>    Can someone write up what is the expected behavior of low power
> indication. 

Josh can provide UX input or pointers to existing documentation.
Flags: needinfo?(overholt) → needinfo?(jcarpenter)
See bug 814231. The latest UX input on low battery notification. And it's already fixed.
Triage: QAWANTED to confirm if the bug is already fixed as per comment 6
Joe, from looking at bug 814231 it occurs to me that battery notification will be erratic depending on your usage pattern.  Battery discharge rates are not constant, and the algorithm seems to be to display based that rather than in a linear fashion. 

Do we want a different behavior pattern here ? I think this is a UX issue whether we want to prod at a linear rate, or have it tied to the battery discharge rate. 

I think a linear fashion might make things more predictable for the user..
I can't speak to the algorithms, but FWIW, per bug #814231, we decided that the indicator should only appear when the charge level drops below 10%, in order to reduce the problem of battery level indicators popping up too often. Here was Rebecca's confirmation of implementation in the final comment of that bug:

> Verified the battery warning displays for 10% and under, goes away above 10%, returns 
> when back under 10%. I did not check the 100% status due to comments above. 
> Tested on the 12/21 unagi build.
Flags: needinfo?(jcarpenter)
Triage: BB- for now
reporter, can you try to reproduce again with latest build? thanks
blocking-basecamp: ? → -
Flags: needinfo?(Firefox_Mozilla)
Triage: according to issue reporter, there is no more erratic low power indicator popping up on the new build. however, it seems like there is no more low power indicator popping up at all even at low power.

so the question is, is the low power indicator totally removed now? and the only low power indication is the upper right battery icon showing orange and red?
Flags: needinfo?(jcarpenter)
No, we should still have a 10% low battery indicator. This is important, and we should add it back in if it's been removed for some reason.
Flags: needinfo?(jcarpenter)
looks like a regression
Assignee: nobody → rlu
blocking-basecamp: - → ?
Triage: BB-, tracking-b2g18+, would take a patch
blocking-basecamp: ? → -
tracking-b2g18: --- → +
note: it was found that the low power indicator pops up only @ 10%. if your screen is not on @ 10%, you will not see it.

jcarpenter will add some notes on the expected behavior
Flags: needinfo?(Firefox_Mozilla) → needinfo?(jcarpenter)
* If device is awake and power level drops below 10%, indicator should appear.
* If device is asleep and power level drops below 10%, indicator should appear the next time the device is woken and unlocked.
* Once the indicator has been shown, it should not be shown again until the next time the power goes from over-to-under 10%.

Thanks guys, please let me know if I've missed anything.
Flags: needinfo?(jcarpenter)
Status: UNCONFIRMED → NEW
blocking-b2g: --- → tef?
Ever confirmed: true
QA Contact: atsai
Concerned about the effect of never seeing power notifications combined with need to make emergency called.

Vivien, can you assign to someone who can fix this *today*? Or ping Dylan/David to do so?
Flags: needinfo?(21)
Assignee: rlu → felash
Summary: [Open_][power management]the interface of low power alarm appears erratic-617001930730 → Always display the low power window when the battery level go below 10%, but only once
blocking-b2g: tef? → tef+
I added some stuff to Josh's logic:

Josh's logic:
* If device is awake and power level drops below 10%, indicator should appear.
* If device is asleep and power level drops below 10%, indicator should appear the next time the device is woken and unlocked.
* Once the indicator has been shown, it should not be shown again until the next time the power goes from over-to-under 10%.

I added:
* if we plug the cable, then we hide the notification if it was shown
* if we unplug the cable and the battery level is still low, show the indicator, even if we didn't go over 10%
* at startup, if the battery level is low, show the indicator

Josh, is it ok for you ?
Flags: needinfo?(21) → needinfo?(jcarpenter)
Attached patch patch v1 (obsolete) — Splinter Review
apply the patch with |git am|
see also github PR : https://github.com/mozilla-b2g/gaia/pull/7671

- display only once the notification
- hide it as soon as we plug the charger
- display it again when we go above the threshold, and then go down again
- display it again when we unplug the charger, even if we didn't go up the
  threshold
- added a test file, and had to change some things in BatteryManager to make
  this possible. Most notably, the navigator.battery object is now stored in a
  property so that we can change it in the test. Also, some properties are now
  initialized in init().
---
 apps/system/js/battery_manager.js               |   82 ++++++----
 apps/system/test/unit/battery_manager_test.js   |  198 +++++++++++++++++++++++
 apps/system/test/unit/mock_gesture_detector.js  |    7 +
 apps/system/test/unit/mock_navigator_battery.js |   55 +++++++
 apps/system/test/unit/mock_sleep_menu.js        |    5 +
 5 files changed, 320 insertions(+), 27 deletions(-)
 create mode 100644 apps/system/test/unit/battery_manager_test.js
 create mode 100644 apps/system/test/unit/mock_navigator_battery.js
 create mode 100644 apps/system/test/unit/mock_sleep_menu.js
Attachment #703561 - Flags: review?(alive)
Attachment #703561 - Flags: review?(alive) → feedback?(alive)
The patch works for me, however the notification is empty so I suspect some style is incorrect. 

I'll work on this tomorrow, however I'd like an early feedback on the JS code.
Comment on attachment 703561 [details] [diff] [review]
patch v1

Review of attachment 703561 [details] [diff] [review]:
-----------------------------------------------------------------

looks nice but I couldn't test it manually now.
Attachment #703561 - Flags: feedback?(alive) → feedback+
(In reply to Julien Wajsberg [:julienw] from comment #21)
> The patch works for me, however the notification is empty so I suspect some
> style is incorrect. 
> 
> I'll work on this tomorrow, however I'd like an early feedback on the JS
> code.
I guess you forget to assign dataset.level
See battery_manager.css for more info
(In reply to Alive Kuo [:alive] from comment #23)
> (In reply to Julien Wajsberg [:julienw] from comment #21)

> I guess you forget to assign dataset.level
> See battery_manager.css for more info

I think it was done already but I'll definitely check this, thanks !
Another question for Josh: in the markup where is a "low level" battery (that was supposed to be for 30% battery level) and a "near empty level" battery indicator (that was supposed to be for 10%).

In my current patch, it's only displayed for 10%, do you think it should be displayed when we go below 30% too ?
Hi Julien,


According comment 7 of Bug 814231, we only need to show the low battery notification for below 10%,
https://bugzilla.mozilla.org/show_bug.cgi?id=814231#c7

Thanks.
Ok Rudy, will do that, thanks.

Then I'll remove all the markup/css about this too.
Attached patch patch v2Splinter Review
PR https://github.com/mozilla-b2g/gaia/pull/7671 was updated too.

:rlu feel free to review this too if you want.

Differences to the first patch :
- changed all references to "low" to "empty" instead
- removed all indicator stuff for low or full notification in HTML, CSS, locales.
- some style changes to make the notification more like the update toaster that I know. Still not exactly the same (no animation when it appears, height is smaller, etc) but I want to land this now.
---
 apps/system/index.html                             |    4 +-
 apps/system/js/battery_manager.js                  |   91 ++++++---
 apps/system/locales/system.ar.properties           |  104 ----------
 apps/system/locales/system.en-US.properties        |    2 -
 apps/system/locales/system.fr.properties           |    2 -
 apps/system/locales/system.zh-TW.properties        |    2 -
 .../style/battery_manager/battery_manager.css      |   43 ++---
 .../battery_manager/images/battery_empty_big.png   |  Bin 2067 -> 0 bytes
 .../battery_manager/images/battery_full_big.png    |  Bin 2058 -> 0 bytes
 .../battery_manager/images/battery_full_small.png  |  Bin 3008 -> 0 bytes
 .../battery_manager/images/battery_low_big.png     |  Bin 2072 -> 0 bytes
 .../battery_manager/images/battery_low_small.png   |  Bin 3001 -> 0 bytes
 apps/system/test/unit/battery_manager_test.js      |  204 ++++++++++++++++++++
 apps/system/test/unit/mock_gesture_detector.js     |    7 +
 apps/system/test/unit/mock_navigator_battery.js    |   55 ++++++
 apps/system/test/unit/mock_sleep_menu.js           |    5 +
 16 files changed, 343 insertions(+), 176 deletions(-)
 delete mode 100644 apps/system/locales/system.ar.properties
 delete mode 100644 apps/system/style/battery_manager/images/battery_empty_big.png
 delete mode 100644 apps/system/style/battery_manager/images/battery_full_big.png
 delete mode 100644 apps/system/style/battery_manager/images/battery_full_small.png
 delete mode 100644 apps/system/style/battery_manager/images/battery_low_big.png
 delete mode 100644 apps/system/style/battery_manager/images/battery_low_small.png
 create mode 100644 apps/system/test/unit/battery_manager_test.js
Attachment #703561 - Attachment is obsolete: true
Attachment #703849 - Flags: review?(alive)
(In reply to Julien Wajsberg [:julienw] from comment #24)
> (In reply to Alive Kuo [:alive] from comment #23)
> 
> > I guess you forget to assign dataset.level
> > See battery_manager.css for more info
> 
> I think it was done already but I'll definitely check this, thanks !

The problem was that the CSS file was using "level=10" whereas the level could be like 9 etc. I removed all this because we only have one indicator now anyway.
Comment on attachment 703849 [details] [diff] [review]
patch v2

Asking for a review from the l10n guys as I change the locales here. But I only retire some keys, I'm no adding or changing any.
Attachment #703849 - Flags: review?(stas)
Comment on attachment 703849 [details] [diff] [review]
patch v2

Review of attachment 703849 [details] [diff] [review]:
-----------------------------------------------------------------

This is only OK if we can have this on shira as well.

Otherwise, we can drop the usage of the strings, but we'd need a follow-up to remove them from the .properties once all branches stopped using them.

r=me with that.
Attachment #703849 - Flags: review?(stas) → review+
Pike> actually, in the current code, the place where the removed strings are used is never shown. So I even think this is safe in all cases.

I don't understand your sentence with shira (I'm not so fluent with planned branches and new codenames).
Comment on attachment 703849 [details] [diff] [review]
patch v2

Review of attachment 703849 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #703849 - Flags: review?(alive) → review+
Pike> this will land to all 3 branches, so I guess we're ok.
master: https://github.com/mozilla-b2g/gaia/commit/7ec86afb7e7bc0f0747e538ae67ce0a06ec522f3

that doesn't apply cleanly on v1-train and v1.0.0, I'm currently merging...
v1.0.0 : https://github.com/mozilla-b2g/gaia/commit/c2cc9ce55a5a394e560338a66d44c06c4694977a

I've seen that shira/v1-train are very old so I'm not merging into this yet.

Jason, how could we mark that this patch should be merged in v1-train if necessary ?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Julien Wajsberg [:julienw] from comment #19)
> I added some stuff to Josh's logic:
> 
> Josh's logic:
> * If device is awake and power level drops below 10%, indicator should
> appear.
> * If device is asleep and power level drops below 10%, indicator should
> appear the next time the device is woken and unlocked.
> * Once the indicator has been shown, it should not be shown again until the
> next time the power goes from over-to-under 10%.
> 
> I added:
> * if we plug the cable, then we hide the notification if it was shown
> * if we unplug the cable and the battery level is still low, show the
> indicator, even if we didn't go over 10%
> * at startup, if the battery level is low, show the indicator
> 
> Josh, is it ok for you ?

Yes, those are smart additions. 

(In reply to Julien Wajsberg [:julienw] from comment #25)
> Another question for Josh: in the markup where is a "low level" battery
> (that was supposed to be for 30% battery level) and a "near empty level"
> battery indicator (that was supposed to be for 10%).
> 
> In my current patch, it's only displayed for 10%, do you think it should be
> displayed when we go below 30% too ?

Hmm, I thought the second indicator was for 20%, not 30. In any case, I suggest we get everything verified as working properly for 10%, and then revisit adding an indicator for a second level. I do think it's a good idea. The only reason we pulled it was because the behavior was so erratic and annoying.
Flags: needinfo?(jcarpenter)
(In reply to Julien Wajsberg [:julienw] from comment #37)
> v1:
> https://github.com/mozilla-b2g/gaia/commit/
> 86cc151a64033dd611545c0032b4705866a41e02

My guts told me that we should set the status-b2g18 flag after deciphering this document here:
https://wiki.mozilla.org/Release_Management/B2G_Landing
... and, there is no flag to point out that we need this in v1-train and we have not landed it yet.
Tim> I thought that the status-b2g18 flag was only for gecko patches.

About v1-train, I believe that :vingtetun will have this sorted out of master (as we have still been very prudent in master for one week).
Tim> reading the page it seems you're right :)
It's not clear that this has been uplifted to the v1.0.0 gaia branch - status flags are for all, not just Gecko, and we now have the status-b2g18-v1.0.0 flag to confirm that something is in either gecko and/or gaia v1.0.0 shipping code.  Please update the flag when this landing is verified.
Also this seems like something we should have a test and/or smoketest for - adding qawanted and cc'ing Henrik.
Keywords: qawanted
Dave or Rob, is that something we could have as a Gaia test?
Lukas, Henrik> fwiw we do have Gaia unit tests in this commit.

Lukas> I personally landed this to v1.0.0. I verified that this is in both v1.0.0 and v1-train (with the same commit actually).
The unit tests look like enough coverage here, I don't think we need Gaia UI tests for this.
Device no longer repros.  It works according to Comment 19 on.....  
    Kernel Dec 5
    Build ID 20130214070203
    gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
    Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49
Verified fixed.  the dialog only pops up now each time if you plug/unplug the USB cable in. l otherwise it works as expected.
Status: RESOLVED → VERIFIED
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: