Closed Bug 948263 Opened 6 years ago Closed 6 years ago
[User Story] Warning when the battery is low while recording a video
1.60 MB, image/png
203.14 KB, image/png
109.26 KB, image/png
562.96 KB, application/zip
4.79 MB, application/zip
46 bytes, text/x-github-pull-request
|Details | Review|
46 bytes, text/x-github-pull-request
|Details | Review|
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
Ashish! Please check the user stories and update the current status.
Assignee: nobody → singhashish1887
It can be Implemented.
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.
Attachment #8379639 - Flags: review?(dflanagan)
Comment on attachment 8379639 [details] Pointer to Pull Request.html Adding Diego and Wilson
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 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.
Attachment #8379639 - Flags: review?(dflanagan) → review-
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!
Attachment #8381342 - Flags: review?(wilsonpage) → review-
Comment on attachment 8381342 [details] [review] Pull Request (camera-dev) >https://github.com/dmarcos/gaia/pull/46
Attachment #8381342 - Attachment is obsolete: true
Comment on attachment 8381949 [details] [review] Pull Request (camera-dev) Please see github comments. This patch still need some work.
Attachment #8381949 - Flags: review?(wilsonpage) → review-
Wilson your comments are required for https://github.com/singhashish1887/gaia/commit/1dddf2fda0ac301779fae766016da876e192d761 commit.
per comment 13
Comments added (r-)
Updated the Indicator and Low battery comments.
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
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 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 on attachment 8389195 [details] [review] Pull Request(Camera-new-feature) We have updated the code as David suggested.
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 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 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 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 on attachment 8393877 [details] ScreenShots.zip Screen shots are uploaded for UX review
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
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.
Youngjun/Ashish When can we have the review comments addressed and landed on branch. Please provide a target date for this Thanks Hema
Today we can upload new patch including ux comments and Wilson's comments.
Updated Screen Shots are uploaded for review.
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.
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
Hi, shutDown.png: Please move "the" to next line. Period is still missing at end of the sentence (See attached). Thanks!
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.
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.
Whiteboard: [ucid:media71, 1.4:p2, ft:media] → [ucid:media71, 1.4:p2, ft:media] [m+]
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.
(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.
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.
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 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.
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
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?
(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 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.
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.
(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
(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.
We have made the suggested changes and upload the screen shots for review
Comment on attachment 8389195 [details] [review] Pull Request(Camera-new-feature) Notification View Unit test cases , CSS changes are done. Uploaded for the review
(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 on attachment 8396577 [details] [review] pull-request (camera-new-features) Looks good. Thanks!
Attachment #8396577 - Flags: ui-review?(amlee) → ui-review+
djf: Just waiting on your technical review. See unit-tests for module specs. Fingers crossed, good luck!
I have left most recent commits un-squashed for review, but we'll obvo need to squash them before merging.
Just caught a thing. Self timer should be "Self-Timer" This should be the same in the settings menu as well. Thanks
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.)
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.
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.
djf: I've addressed your comments and added some additional unit-tests to prove they work.
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.
Attachment #8396577 - Flags: review?(dflanagan) → review?(dmarcos)
Rebased PR on master and carried over ui-review+
Attachment #8397792 - Flags: review?(wilsonpage) → review+
Conditional r+: - Needs unit-tests to prove flash-notification is cleared when the mode or selectedCamera changes.
Attachment #8396577 - Flags: review?(dmarcos) → review+
Landed in master: https://github.com/mozilla-b2g/gaia/commit/1dd1d524c56033b332de189bf4250e2aea0fcbd6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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!
Attachment #8396577 - Flags: ui-review?(tshakespeare) → ui-review+
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
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
You need to log in before you can comment on or make changes to this bug.