819744 - Delay the light sensor response in screen_manager.js

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

External user feedback: currently the light sensor will brighten the screen and dim again simply with the finger accidentally move over the light sensor.

It should come with a delay to prevent that kind of response but we shouldn't delay too much for the user to think it's ill-responsive.

Comment 1

[Greg Green]

Attachment: pull request for light sensor delay

Added a delay for light sensor response. I don't have a device to test against v2.2. I have tested the patch on a device running the v2.1 branch.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8547337 [details] [review]
pull request for light sensor delay

Hi gpgreen,

It's very nice of you to organize the flow into different states. However, screen_manager.js is getting to large and I would like to prevent it to become bigger first. Please split the code into another class usable by ScreenManager instead.

Your patch does not come with any unit test either. Having a separate class created will help you to do that too. Thanks!

Comment 3

[Greg Green]

So do you want to move the just the automatic display adjustment to the new class, or more than that?

Comment 4

[Greg Green]

Attachment: pull request for light sensor delay

Updated patch to refactor code and add unit test

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8554198 [details] [review]
pull request for light sensor delay

Excellent clean up! Thanks for the patch. 

I've left a few comments on Github. There isn't a lot to fix except for the interface and the timer part. The ones marked with "nit" are really optional improvements so it's up to you if you want to fix them all.

Comment 6

[Greg Green]

Comment on attachment 8554198 [details] [review]
pull request for light sensor delay

Updated the pull request after review.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8554198 [details] [review]
pull request for light sensor delay

Please squash the commits into one commit. nits are optional (since they are not flagged by jshint let's not spend time manually go and forth)

After that, please set checkin-needed in the Bugzilla Keyword field and it will be merged.

Thanks!

Comment 8

[Greg Green]

I updated the pull request, cannot change the Keyword field. I assume that is because the bug hasn't been assigned to me.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've hit re-run on these intermittents, patch should land if we can sure of the errors are not related. 

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=fbf969956442

Comment 10

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/b063ab231cee3b965751ae7471ae9e133d5262c8