948263 - [User Story] Warning when the battery is low while recording a video

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect, P2)

Description

[Sri Kasetti]

User Story:

As a user, while recording a video if the phone's battery is less than 2% I want a visual warning indicating that the battery status is low

Comment 1

[Youngjun Kim]

Ashish! Please check the user stories and update the current status.

Comment 2

[singhashish1887]

It can be Implemented.

Comment 3

[singhashish1887]

Attachment: Pointer to Pull Request.html

Hi David,

We have added low battery scenario for camera application.
We added code for message and indicator.
This is work in  progress code .We did not added test codes.

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8379639 [details]
Pointer to Pull Request.html

Adding Diego and Wilson

Comment 5

[David Flanagan [:djf]]

Where did the SVG files in this patch come from? If Mozilla visual design supplied them, we've need to have it redone. The file are really long because they include a text encoding of a bitmap image in addition to the vector image.  Some bogus Adobe Illustrator thing rather than pure SVG.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8379639 [details]
Pointer to Pull Request.html

There are many problems with this patch.  It is not localized, and I suspect it is not actually close to the correct design.  

Thumbs.db should not have been added to the commit.

I don't know why there are two PNG images in the patch. They appear to be unused.

The SVG images in the patch are way too big and include bitmaps.

There is probably a pre-defined building block that you can use for the "toast" messages.

This patch isn't anywhere close to being ready to land.

Comment 7

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 8

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8381342 [details] [review]
Pull Request (camera-dev)

Please review comments in the GitHub pull request and address them. Once you have updated the pull request, please set either myself or David (djf) to review. Thank you!

Comment 9

[singhashish1887]

Comment on attachment 8381342 [details] [review]
Pull Request (camera-dev)

>https://github.com/dmarcos/gaia/pull/46

Comment 10

[singhashish1887]

Attachment: Pull Request (camera-dev)

Comment 11

[Wilson Page [:wilsonpage]]

Comment on attachment 8381949 [details] [review]
Pull Request (camera-dev)

Please see github comments. This patch still need some work.

Comment 13

[singhashish1887]

Wilson your comments are required for https://github.com/singhashish1887/gaia/commit/1dddf2fda0ac301779fae766016da876e192d761 commit.

Comment 14

[Hema Koka [:hema]]

per comment 13

Comment 15

[Wilson Page [:wilsonpage]]

Comments added (r-)

Comment 16

[singhashish1887]

Attachment: Pull Request (camera-dev)

Updated the Indicator and Low battery comments.

Comment 17

[singhashish1887]

Wilson We did not move the Full screen to overlay because over lay is specifically designed for memory messages .So we make Notification  view and control to manage messages and notifications

Comment 18

[singhashish1887]

Attachment: Pull Request(Camera-new-feature)

Comment 19

[Hema Koka [:hema]]

Ashish,

We seem to have many patch attachments in this bug, as a result the review comments from previous are being lost. It is slowing down the review process. Please make sure to address the comments. 

Thanks
Hema

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

This is looking much better!  But there is still some work to do. Please see my comments on github.

Comment 21

[singhashish1887]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

We have updated the code as David suggested.

Comment 22

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

Thank you for making my suggested changes.

I'm giving r- again because I think you still have two different kinds of battery related events, and I don't think that is necessasry.

Also, it appears that you're emitting a battery event every time the battery level drops a tiny bit.  When the battery goes from 99% to 98% you'll send a healthy event even though you were at healthy before.

Note that I did not re-review the notifications part of this patch because I just did that in a separate bug.

If you want to merge those two things back into a single PR, that's okay with me.  Otherwise, try to keep the two separate, if you can.

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

Wilson offered very good review comments on github, and I just chimed in about some of them.

Wilson's point that the battery controller just needs to set state on the app and the app will send out a change:batteryStatus event seems like the key thing to work on here.

But also please modify the getStatus() function in the battery controller so that all it does is get the status. Don't do any notifications unless that status has actually changed from the last time.  Otherwise you're going to keep popping up low battery notifications over and over.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

This is looking much better, but there are still issues. I put my comments on the latest commit: https://github.com/singhashish1887/gaia/commit/bc296085f1943ba9455c8a1ec799abba49bf0ec4

Note that these notifications seem to appear on top of the settings menu rather than undeneath it. This will be particularly problematic for persistent battery notifications.

I think that this is getting close to ready to land.

I haven't looked at your tests. Please ensure that they still work.

Decide whether you want to land the notifications and the battery work in one patch or two separate patches.

And start getting ready for UX review. Attaching screenshots of the battery level notifications (including different orientations of the phone) would help with UX review.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

I'm not treating this pull request as the official pull request for both notifications and battery status.  Let's stop working on the other notification-only bug and just get this one landed.

r- because:

- there is still a notification controller test that cannot possibly run

- the overlay controller code needs some refactoring. See the very detailed comment I left on github about how to do this.

- some of the battery status logic is much more complicated than it needs to be

- some of the notifcation view logic is also more complicated than it should be and does too many hides and shows.

Thanks for your continued work on this patch.

Please also try to make the time to attach screenshots for UX review.  Our UX team is not going to want to sit around and wait for their battery to discharge, so we need to see all the states in screenshots.  Note that you can easily simulate various states by tweaking the numbers in config/app.js to define critical to be 99% or whatever you need to get a screenshot.  Be sure to include screenshots that demonstrate the indicators and the notifications and the overlay in both portait and landscape orientations.

Comment 26

[singhashish1887]

Attachment: ScreenShots.zip

Comment 27

[singhashish1887]

Comment on attachment 8393877 [details]
ScreenShots.zip

Screen shots are uploaded for UX review

Comment 28

[Amy Lee [:amylee] UX]

Attachment: CSS_Pixels_Camera_Low_Battery.png

Hi, 

I've reviewed the screenshots and here is my feedback. I've also attached the spec for reference. Thanks!

Feedback:

Portrait View: There should be a distance of 38px between the top of the shutter button and the baseline of the text (See spec). 

Font: Should be Fira Sans Medium/ 20px size

Text Spacing: Please decrease the spacing between the 1st and 2nd line of text by 2px

Battery Icon: Please remove drop shadow

Landscape View: There should be a distance of 48px between baseline of text and edge of screen (see spec).

Low battery indicator (Landscape/Portrait): There should be equal distance between indicators (12px). Distance of battery indicator should be the same as between HDR and Timer indicators

Landscape: Warning Message and battery icon needs to be horizontally centred to screen

Comment 29

[Tiffanie Shakespeare [:tif]]

A couple issues with the strings (text) in the screenshots:

- remove the "You have" from string. The text should read "<x>% battery remaining" where x is the number.

- in the full screen low battery alert, the text should be "The battery is too low to use the camera." Period is missing and "camera" should not be capitalized.

Everything else seems good.

Comment 30

[Hema Koka [:hema]]

Youngjun/Ashish

When can we have the review comments addressed and landed on branch. Please provide a target date for this

Thanks
Hema

Comment 31

[Youngjun Kim]

Today we can upload new patch including ux comments and Wilson's comments.

Comment 32

[singhashish1887]

Attachment: ScreenShots.zip

Updated Screen Shots are uploaded for review.

Comment 33

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

r- because of a bug in the way overlays are destroyed. See my detailed comments on github.

I'm pretty sure that the UX spec requires notifiations when focus mode changes. (I'm not sure about when we switch front/rear camera or switch photo/video mode).  Please either add the focus mode notifications to this patch or be sure that we have a followup bug filed so we don't forget to add those.

Comment 34

[Amy Lee [:amylee] UX]

Attachment: Review.png

Hi, 

Here is my feedback:

Landscape: Distance of confirmation message and edge of screen is still wrong. Should be 48px (See attached).

Landscape: Distance between indicators is still wrong. Should be 12px distance between indicators (See attached).

Landscape: Message is still not horizontally centered to the screen.


Font style/Size should be Fira Sans Medium/20px. There is a separate bug filed for universal styling of all confirmation messages and position. See Bug 985638.

Thanks

Comment 35

[Amy Lee [:amylee] UX]

Attachment: shutDown.png

Hi,

shutDown.png: Please move "the" to next line. Period is still missing at end of the sentence (See attached). 

Thanks!

Comment 36

[Wilson Page [:wilsonpage]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

I did a quick tidy to bring the patch inline with existing code style. I also rebased on top of mozilla/camera-new-features, so you should discard you local branch and pull down the one from your Github.

Icons continue to be a source of pain when multiple devs are making changes to the icon-font. I've done my best to resolve theses conflicts.

I did notice a bug:

1. Change grid to 'on'
2. Before 3 seconds is up change grid to 'off'
3. Observe the notification doesn't stay shown for 3 seconds

Expected: Each time a setting is changed the timer should be reset back to 3 seconds, so that every notification stays shown for 3 seconds.

Also we are missing unit tests for NotificationView.

Comment 37

[Diego Marcos [:dmarcos]]

Wilson, this patch removes all the selected values in the config/app.js file for all the settings options. It's for example reverting the fix for bug 984730 where the default flash value for video was torch.

Comment 38

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

Given Wilson's r- and the changes Amy has requested, I'm not going to review this carefully myself, and will instead just echo Wilson's r-.

Note the bug that Wilson caught: when showNotification() is called with a non-persistent message you are no longer calling _clearMessage(), which is good because that method does to many things you don't need. But you are also not clearing the timeout, which you do need to do.

Please fix the UX issues that Amy has identified, but don't worry about getting the word 'the' onto a different line. That is out of your control, and I don't think we should be putting HTML markup into our localization files.

Comment 39

[David Flanagan [:djf]]

(In reply to Amy from comment #35)

> 
> shutDown.png: Please move "the" to next line. 

Amy,

I've told Ashish to ignore this particular request from you. We don't have much control over line breaks: they're a factor of the font size and the line width, both of which are set by the building block we're using to display this overlay.  I'm not sure whether we could add an HTML <br> tag to the string. It might work, but I think it is frowned on to do that in a localization file.  In any case, this message will only be seen by a tiny fraction of users.  And right now, almost none of our users will use English as their locale anyway.  You're not going to be able to give this sort of typographic critique to every string in every language, so I don't think it makes sense to go out of our way to make it look just perfect in English.

Comment 40

[singhashish1887]

Attachment: ScreenShots.zip

Latest Screen shots are updated.
1) In full screen Message we have to make changes in overlay for moving 'The' in next line. That will effect the other messages Like Memory . SD card etc. That is why we  are did not changed.
2) Indicator related changes will be done in Indicator.

Comment 41

[Wilson Page [:wilsonpage]]

dmarcos: I don't know why that has got caught up in this patch, but in this case, technically they are redundant. Settings will pick the first option if no `selected` key is given.

Comment 42

[Wilson Page [:wilsonpage]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

- There are still no unit-tests for the new NotifationView.
- It is not clear from the diff of the latest commit what has changed. It appears to show every change. Did you discard you local branch after the rebase as I suggested?
- You have left no communication on Github or Bugzilla to indicate what you have changed.
- Every commit you land has the same commit message, please describe *what* you changed with each incremental patch. When we squash we will make sure the final message is matches the Bugzilla bug title.

Comment 43

[Amy Lee [:amylee] UX]

Attachment: screens.zip

Hi here is my feedback:

1. Please move all portrait/landscape confirmation messages 4px down (see attached)

2. Why is the small battery indicator icon missing from portrait views?

3. In “critical_1.png” the small battery indicator icon looks faded out. Is it cause it’s blinking?

I will file a bug for the indicator spacing if that is needed to get it fixed. 

Thanks

Comment 44

[Tiffanie Shakespeare [:tif]]

Didn't see a new updated screenshot for the full screen error message. Can you attach so we can confirm the string has been updated and correct?

Comment 45

[David Flanagan [:djf]]

(In reply to Tiffanie Shakespeare from comment #44)
> Didn't see a new updated screenshot for the full screen error message. Can
> you attach so we can confirm the string has been updated and correct?

I can confirm (from the source code) that the message now has a period at the end of it. And per comment #39 I've overruled the request to adjust the line break as incompatible with localization.

Comment 46

[David Flanagan [:djf]]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

Ashish,

We really can't afford the time to have two Mozilla engineers review every version of your patch. Wilson has given r-, so I will do the same, but I have not looked at your code.

This bug has gotten long and confusing. I think, however, that the only issues remaining are:

1) Amy's UX requests about adjusting the positioning of the notifications.

2) We need tests for NotificationView. You had tests for NotificationController, but when you removed that class at my request, you never replaced those tests with new ones.

Comment 47

[Tiffanie Shakespeare [:tif]]

Cool, thanks David! :)

(In reply to David Flanagan [:djf] from comment #45)
> (In reply to Tiffanie Shakespeare from comment #44)
> > Didn't see a new updated screenshot for the full screen error message. Can
> > you attach so we can confirm the string has been updated and correct?
> 
> I can confirm (from the source code) that the message now has a period at
> the end of it. And per comment #39 I've overruled the request to adjust the
> line break as incompatible with localization.

Comment 48

[Amy Lee [:amylee] UX]

(In reply to singhashish1887 from comment #40)
> Created attachment 8395522 [details]
> ScreenShots.zip
> 
> Latest Screen shots are updated.
> 1) In full screen Message we have to make changes in overlay for moving
> 'The' in next line. That will effect the other messages Like Memory . SD
> card etc. That is why we  are did not changed.
> 2) Indicator related changes will be done in Indicator.

Hi, 

I've filed a bug for the spacing of the indicators. See Bug 987189. Thanks

Comment 49

[singhashish1887]

(In reply to Amy from comment #43)
> Created attachment 8395680 [details]
> screens.zip
> 
> Hi here is my feedback:
> 
> 1. Please move all portrait/landscape confirmation messages 4px down (see
> attached)
> 
> 2. Why is the small battery indicator icon missing from portrait views?
> 
> 3. In “critical_1.png” the small battery indicator icon looks faded out. Is
> it cause it’s blinking?
> 
> I will file a bug for the indicator spacing if that is needed to get it
> fixed. 
> 
> Thanks

1) We will set the positions of the portrait/landscape confirmation messages 4px down
2)for Critical small battery icon (battery indicator will be blinking). That's why in portrait view while capturing screen shot it is missing and in critical_1.png it is faded out.

Comment 50

[singhashish1887]

Attachment: ScreenShots.zip

We have made the suggested changes and upload the screen shots for review

Comment 51

[singhashish1887]

Comment on attachment 8389195 [details] [review]
Pull Request(Camera-new-feature)

Notification View Unit test cases , CSS changes are done. Uploaded for the review

Comment 52

[Amy Lee [:amylee] UX]

(In reply to singhashish1887 from comment #50)
> Created attachment 8396167 [details]
> ScreenShots.zip
> 
> We have made the suggested changes and upload the screen shots for review

Hi, 

Am I reviewing all confirmation messages in this bug? Because you have also attached Grid and Self-Timer notifications. I had filed a separate bug for all confirmation messages to be styled the same way. See Bug 985638. Can you please provide the patch for this bug? I would like flash it on my device. 

The font should be Fira Sans Medium (500 weight) for all confirmation messages

I don't see the battery icon in the screenshots (I assume it's because it's blinking?). I will need to review this in the patch. 

Confirmation Messages:

Timer: off
Should be “Self-Timer OFF” (OFF should be on 2nd line)

Grid: on 
Should be “Grid ON” (ON should be on 2nd line)

Timer: 5 seconds
Should be  “Self-Timer 5 SECONDS” (5 SECONDS should be on 2nd line)

I had provided a spec for styling confirmation messages in Bug 985638. Please check that for visual reference. Thanks.

Comment 53

[Wilson Page [:wilsonpage]]

Attachment: pull-request (camera-new-features)

Comment 54

[Amy Lee [:amylee] UX]

Comment on attachment 8396577 [details] [review]
pull-request (camera-new-features)

Looks good. Thanks!

Comment 55

[Wilson Page [:wilsonpage]]

djf: Just waiting on your technical review. See unit-tests for module specs. Fingers crossed, good luck!

Comment 56

[Wilson Page [:wilsonpage]]

I have left most recent commits un-squashed for review, but we'll obvo need to squash them before merging.

Comment 57

[Amy Lee [:amylee] UX]

Just caught a thing. 

Self timer should be "Self-Timer"

This should be the same in the settings menu as well. Thanks

Comment 58

[David Flanagan [:djf]]

Wilson: you changed a lot more in this patch than I expected you to, and I didn't have time to finish the review tonight.  

Please comment here (or in the patch) explaining the framework changes in model and setting proxy, because I don't understand what that's about.

Also, documentation in the notifications view would be really helpful.  I've left some github comments on that. (I just noticed that you said to see the unit tests for the specs, and I suppose I could do that if I have to, but I'll be pissy about it.)

Comment 59

[Wilson Page [:wilsonpage]]

amlee: This was fixed in the self-timer bug, so we shouldn't fix here as well. When that lands it will be fixed across the app.

Comment 60

[Wilson Page [:wilsonpage]]

djf: Apologies for the larger than expected change. Upon fillign the patch out with unit-tests I found weaknesses in the NotificationView. The changes I made addressed these weaknesses and made the view more flexible/reusable.

The biggest change is probably returning a handle on the notification displayed to allow the callee to remove the exact notification that they created. This means that when a controller calls `.clear()` they will never accidentally clear another controller's notification.

The NotificationView is now unaware of 'icons'. Instead each notification can just accept a 'className' that allows them to be styled however we like, including added pseudo element icons. This leaves us prepared for any styling eventually in the future.

I also added the ability for 'temporary' notifications to temporarily obscure 'persistent' notifications. This means don't drop temporary notifications when a persistent notification is in place.

The unit tests (test/unit/views/notification_test.js) as clearly as possible outline all these intentions and assert that they are working as expected.

Again, apologies for the amount of changes. I would have liked to have discussed them with you before implementing, but time wasn't on our side :(

I'll set about addressing your current feedback now.

Comment 61

[Wilson Page [:wilsonpage]]

djf: I've addressed your comments and added some additional unit-tests to prove they work.

Comment 62

[David Flanagan [:djf]]

Comment on attachment 8396577 [details] [review]
pull-request (camera-new-features)

Delegating the review to Diego so I can move on to 1.3T+ reviews.

Diego: note that I did a partial review and Wilson addressed some of my comments. But I never had time to finish.  I haven't tried running any of the code either.  But I think we're in pretty good shape here.

Comment 63

[Diego Marcos [:dmarcos]]

Attachment: Pull request on master

Comment 64

[Diego Marcos [:dmarcos]]

Rebased PR on master and carried over ui-review+

Comment 65

[Wilson Page [:wilsonpage]]

Conditional r+:

- Needs unit-tests to prove flash-notification is cleared when the mode or selectedCamera changes.

Comment 66

[Diego Marcos [:dmarcos]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/1dd1d524c56033b332de189bf4250e2aea0fcbd6

Comment 67

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8396577 [details] [review]
pull-request (camera-new-features)

reviewed master and was able to see the low battery icon, the warning string, and the low battery dialogue. All seemed fine!

Comment 68

[David Flanagan [:djf]]

Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530

Comment 69

[sarsenyev]

The issue is no longer reproduces on 1.4 and master builds
Warning messages appear on low battery according to specification

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140409000202
Gaia: 26983f356ecb1bcf30e862d334b5de790071803e
Gecko: e450e07e3a58
Version: 30.0a2
Firmware Version: v1.2-device.cfg

1.5 Environmental Variables:
Device: Buri 1.5 Master
BuildID: 20140409040202
Gaia: 650e8c2c611ed07495d3bf3769f44a0efd88a492
Gecko: 5811efc11011
Version: 31.0a1
Firmware Version: v1.2-device.cfg