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)
Firefox OS Graveyard
Gaia::System
Tracking
(blocking-b2g:tef+, blocking-basecamp:-, b2g18+ fixed, b2g18-v1.0.0 fixed)
VERIFIED
FIXED
People
(Reporter: Firefox_Mozilla, Assigned: julienw)
References
Details
Attachments
(1 file, 1 obsolete file)
41.90 KB,
patch
|
alive
:
review+
Pike
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Summary: [power management]the interface of low power alarm appears erratic-617001930730 → [Open_][power management]the interface of low power alarm appears erratic-617001930730
Comment 1•11 years ago
|
||
Triage: QAWANTED to confirm the behavior of low power indication. It seems like the low power indicator pops up randomly to reporter
Keywords: qawanted
Updated•11 years ago
|
blocking-basecamp: --- → ?
Comment 2•11 years ago
|
||
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
Updated•11 years ago
|
blocking-basecamp: ? → -
tracking-b2g18:
--- → +
Comment 3•11 years ago
|
||
Andrew, Can someone write up what is the expected behavior of low power indication. I am a bit uninitiated.
blocking-basecamp: - → ?
tracking-b2g18:
+ → ---
Comment 5•11 years ago
|
||
(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)
Comment 6•11 years ago
|
||
See bug 814231. The latest UX input on low battery notification. And it's already fixed.
Comment 8•11 years ago
|
||
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..
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
Triage: BB- for now reporter, can you try to reproduce again with latest build? thanks
blocking-basecamp: ? → -
Flags: needinfo?(Firefox_Mozilla)
Comment 11•11 years ago
|
||
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)
Comment 12•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
Triage: BB-, tracking-b2g18+, would take a patch
blocking-basecamp: ? → -
tracking-b2g18:
--- → +
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
* 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)
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
blocking-b2g: --- → tef?
Ever confirmed: true
QA Contact: atsai
Comment 18•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: rlu → felash
Assignee | ||
Updated•11 years ago
|
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
Updated•11 years ago
|
blocking-b2g: tef? → tef+
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #703561 -
Flags: review?(alive) → feedback?(alive)
Assignee | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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+
Comment 23•11 years ago
|
||
(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
Assignee | ||
Comment 24•11 years ago
|
||
(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 !
Assignee | ||
Comment 25•11 years ago
|
||
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 ?
Comment 26•11 years ago
|
||
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.
Assignee | ||
Comment 27•11 years ago
|
||
Ok Rudy, will do that, thanks. Then I'll remove all the markup/css about this too.
Assignee | ||
Comment 28•11 years ago
|
||
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)
Assignee | ||
Comment 29•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
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 31•11 years ago
|
||
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+
Assignee | ||
Comment 32•11 years ago
|
||
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 33•11 years ago
|
||
Comment on attachment 703849 [details] [diff] [review] patch v2 Review of attachment 703849 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #703849 -
Flags: review?(alive) → review+
Assignee | ||
Comment 34•11 years ago
|
||
Pike> this will land to all 3 branches, so I guess we're ok.
Assignee | ||
Comment 35•11 years ago
|
||
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...
Assignee | ||
Comment 36•11 years ago
|
||
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
Assignee | ||
Comment 37•11 years ago
|
||
kaze told me we need the arabic locale for testing so I commited 2 follow-ups: master: https://github.com/mozilla-b2g/gaia/commit/8d99806e7270e8f3f12c69ecec0ec39ffa810cba v1: https://github.com/mozilla-b2g/gaia/commit/86cc151a64033dd611545c0032b4705866a41e02
Comment 38•11 years ago
|
||
(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)
Comment 39•11 years ago
|
||
(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
status-b2g18:
--- → fixed
Comment 40•11 years ago
|
||
... and, there is no flag to point out that we need this in v1-train and we have not landed it yet.
Assignee | ||
Comment 41•11 years ago
|
||
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).
Assignee | ||
Comment 42•11 years ago
|
||
Tim> reading the page it seems you're right :)
Comment 43•11 years ago
|
||
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.
status-b2g18-v1.0.0:
--- → affected
Comment 44•11 years ago
|
||
Also this seems like something we should have a test and/or smoketest for - adding qawanted and cc'ing Henrik.
Keywords: qawanted
Comment 45•11 years ago
|
||
Dave or Rob, is that something we could have as a Gaia test?
Assignee | ||
Comment 46•11 years ago
|
||
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).
Comment 47•11 years ago
|
||
The unit tests look like enough coverage here, I don't think we need Gaia UI tests for this.
Comment 48•11 years ago
|
||
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
Comment 49•11 years ago
|
||
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.
Description
•