875852 - Reader Mode: Ambient light level changes are too sensitive

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Arun Balachandran Ganesan [:abc]]

Switching been light and dark modes while the setting is "Auto" is too sensitive to light. It keeps switching between dark and light modes that I was initially confused about what was going on.

Comment 1

[:Margaret Leibovic]

Maybe this is something Nicolas would be interested in helping with, since he's working on the transition styles for this now.

To fix this, we should tweak the values here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#324

Comment 2

[Nicolas Carlo [:nickecarlo]]

I'd like to see if I can do anything for this bug. I tested this on the device I had last night and I think the values would just need to be a little farther apart than they are right now.

At the moment, even if the light was on but it was a little dimmer (an old light bulb hehe) the mode kept switching between dark and light sporadically.

It will take me a little while to work on this bug though, not sure if there's a timeframe that this bug needs to be fixed in?

Comment 3

[:Margaret Leibovic]

(In reply to Nicolas Carlo from comment #2)

> It will take me a little while to work on this bug though, not sure if
> there's a timeframe that this bug needs to be fixed in?

We should fix this bug for Firefox 24, since that's the version this feature was added in. The next merge from Nightly->Aurora is June 24, so it would be best to try to land a fix on mozilla-central by then.

(This will be a small JS-only patch with isolated changes, so it would be low-risk enough to uplift to Aurora if necessary, but let's try not to need to do that :)

Let me know if you have any questions, and feel free to upload a WIP even if you're not sure if it's the best solution yet.

Comment 4

[Nicolas Carlo [:nickecarlo]]

Alright, I'll get to it tonight once I get my hands on an Android device.

Comment 5

[Nicolas Carlo [:nickecarlo]]

I've been testing this and it seems like we'd want something lower than 15(?) for this to be a little more reliable. I tested the lx values in the room I'm sitting in and every time my shadow was being cast on the phone, the lx value lowered to about 15-19. Of course this isn't a professional lx meter or anything, its just an app on Android. But it seemed pretty accurate in the terms of values that that function takes in (when the lx value was slightly above 30, it stayed in the light mode). I've also made the values for the first conditional a lot closer to each other which makes it a little more stable.

I am wondering though, what is the "goal" of this auto mode? When do you want the app to switch to "dark" as opposed to light? Is it for a room where the light's dim or is it for an unlit room (for which the values will have to be much lower)?

I'll adjust the values depending on what you tell me would be the ideal situation in which the modes should change.

Comment 6

[Nicolas Carlo [:nickecarlo]]

Attachment: WIP-Bug-875852

Just playing around with the settings. I've changed the values so the mode only changes when the light in the room is turned off. While walking around the house, moving from areas with light to dark ones, the change is pretty fast. I've kept the settings so that moving from dark areas to ones with light takes a little while before it switches to light mode so as not to burden the eyes too much.

Let me know what you think.

Comment 7

[:Margaret Leibovic]

Comment on attachment 755393 [details] [diff] [review]
WIP-Bug-875852

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

I think there may be a bit of confusion over what these values do, I tried to clarify down below. In bug 857987, mfinkle suggested I factor these out into constants, which I thought was unnecessary, but maybe making them named constants (and adding some comments), would help clarify what they're doing. Using the current values, maybe these would be named something like:

darkColorSchemeMax = 50
lightColorSchemeMin = 25
colorSchemeThreshold = 30

I actually don't really like these names, but I can't think of anything better right now. Maybe if my comments have helped clarify things, you could figure out how to come up with better names.

::: mobile/android/chrome/content/aboutReader.js
@@ +322,5 @@
>  
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
>      // Ignore changes that are within a certain threshold of previous lux values.
> +    if ((this._colorScheme === "dark" && luxValue < 15) ||
> +        (this._colorScheme === "light" && luxValue > 5))

This second line doesn't actually do anything useful anymore if the threshold value down below is 5, since if luxValue > 5, we would keep the color scheme as light anyway.

I think the main issue described in this bug is that we're flipping between light and dark too frequently, so maybe what we really need to do is increase the first value in this if statement and decrease the second value.

@@ +327,3 @@
>        return;
>  
> +    if (luxValue < 5)

So the idea here is that this value is the actual value that we're using as the threshold between dark/light, and the values above act to give us buffer zones, so that if there's a new value that's not too far from the threshold, we shouldn't change the color scheme.

I haven't tested with this patch, but I feel like 5 is probably too low for the threshold. I wonder what Ian thinks the threshold for the dark theme should be, but setting it to 5 means that we'll basically only use the dark theme in almost completely dark rooms.

Comment 8

[Nicolas Carlo [:nickecarlo]]

Yeah, I did misunderstand the exact reason for the threshold values. My bad really. I actually went over and read the comments on that bug after I submitted the WIP.

The value of 5 is will only work in the dark or in a room with very little light. I am a little unclear on when exactly the mode should change. I'm going to set the needinfo flag for ibarlow to see what he says or would want.

I'll revert the values back to your original ones and fiddle around with the upper bound for dark and lower bound for light and post some APK's for you to test and another patch so you can tell me if I'm on the right track or not.

Do you have a convention for constants you follow like all caps or something like that? Or just regular variable declarations are fine (like the examples you wrote above)?

Comment 9

[:Margaret Leibovic]

(In reply to Nicolas Carlo from comment #8)

> Do you have a convention for constants you follow like all caps or something
> like that? Or just regular variable declarations are fine (like the examples
> you wrote above)?

We do often use all caps for constants, but I feel like these can just be declared as local variables (with regular lower case variable naming conventions), since they're only used in this one place (and I don't see them being used elsewhere). In this case, declaring them as variables is more for code clarity than anything else.

Comment 10

[Nicolas Carlo [:nickecarlo]]

Margaret,

Could you please test the following build.

https://dl.dropboxusercontent.com/u/22352304/fennec-24.0a1.en-US.android-arm6.apk

This only tweaks the values slightly and I find this setting more stable than the previous one. But I would like your input on it.

So far, using the variable names provided by you above (I will change those when I submit the patch :) ), the values in this build are:

darkColorSchemeMax = 50
lightColorSchemeMin = 10
colorSchemeThreshold = 20

I can change these values but what I have noticed is that a difference of about 40-50 between darkColorSchemeMax and lightColorSchemeMin works best (or better). And a difference of 30-40 between darkColorSchemeMax and colorSchemeThreshold, and a difference of at least 10 between lightColorSchemeMin and colorSchemeThreshold works best.

I realize that light intensity may be different where you test it, compared to light where I'm testing it, so the result might be different but I think the difference in values as I've currently set them leaves a lot more room for a one off value change because of a shadow or something like that to be ignored. And with these values auto mode functioned quite "dependably".

PS:

I've dumped the idea of using timers since they weren't really helping.

I haven't been able to find a way to "collect" light values, like we discussed in chat, other than just fiddling with the two bounds that you had already set in your if-statement, and filtering values out that way. Any calculation I tried to do within the function crashed Reader mode. So if you have any advice on that, I'd appreciate it.

Comment 11

[:Margaret Leibovic]

(In reply to Nicolas Carlo from comment #10)

> https://dl.dropboxusercontent.com/u/22352304/fennec-24.0a1.en-US.android-
> arm6.apk

It looks like this is an ARMv6 build. If you post a patch, I can just apply it locally, and I'm curious to see what the patch looks like :)

> So far, using the variable names provided by you above (I will change those
> when I submit the patch :) )

You don't need to worry too much about the names if you can't come up with anything better :) I was just feeling nit-picky.

> I've dumped the idea of using timers since they weren't really helping.

Yeah, that's what I suspected.

> I haven't been able to find a way to "collect" light values, like we
> discussed in chat, other than just fiddling with the two bounds that you had
> already set in your if-statement, and filtering values out that way. Any
> calculation I tried to do within the function crashed Reader mode. So if you
> have any advice on that, I'd appreciate it.

Well that sucks. Was there an error? This logic will probably be tricky to implement. I think what we really want is a rolling average, something like the average of the last 10 values we've seen. My first thought is to have a property on AboutReader that holds onto an array of the last 10 values we've seen (adding a new one and retiring the oldest one every time we get a devicelight event). It would kinda suck to also need to calculate an average every time we get an event, but maybe we can do something smart with math to also store an every value as a property, then appropriately update it by removing that oldest value from the mix and adding the newest value. What do you think of that plan?

Comment 12

[Nicolas Carlo [:nickecarlo]]

Attachment: WIP-2-Bug-875852

I've chosen variable names that I think are appropriate but if you don't like them I'll change them to what you suggested.

Also, I've added comments to each one but I am sure you could probably suggest something better and more succinct.

I noticed the use of 'let' but I wasn't exactly clear on scope so I went with var to keep the variables local within the function.

Comment 13

[Nicolas Carlo [:nickecarlo]]

(In reply to :Margaret Leibovic from comment #11)
> (In reply to Nicolas Carlo from comment #10)
> 
> > https://dl.dropboxusercontent.com/u/22352304/fennec-24.0a1.en-US.android-
> > arm6.apk
>
> It looks like this is an ARMv6 build. If you post a patch, I can just apply
> it locally, and I'm curious to see what the patch looks like :)

Oops, apologies for the confusing file name. Its the normal arm build but I have five other apk files in my dropbox so I named it arm6...hehe.
> 
> > So far, using the variable names provided by you above (I will change those
> > when I submit the patch :) )
> 
> You don't need to worry too much about the names if you can't come up with
> anything better :) I was just feeling nit-picky.
> 
> > I've dumped the idea of using timers since they weren't really helping.
> 
> Yeah, that's what I suspected.
> 
> > I haven't been able to find a way to "collect" light values, like we
> > discussed in chat, other than just fiddling with the two bounds that you had
> > already set in your if-statement, and filtering values out that way. Any
> > calculation I tried to do within the function crashed Reader mode. So if you
> > have any advice on that, I'd appreciate it.
> 
> Well that sucks. Was there an error? This logic will probably be tricky to
> implement. I think what we really want is a rolling average, something like
> the average of the last 10 values we've seen. My first thought is to have a
> property on AboutReader that holds onto an array of the last 10 values we've
> seen (adding a new one and retiring the oldest one every time we get a
> devicelight event). It would kinda suck to also need to calculate an average
> every time we get an event, but maybe we can do something smart with math to
> also store an every value as a property, then appropriately update it by
> removing that oldest value from the mix and adding the newest value. What do
> you think of that plan?

The problem was that if I tried to do any kind of calculation, when I switched to reader mode, it just showed a white screen. I didn't do any logging of the problem (I am not aware of how to do that, console.log?).

I'm going to see how to implement an array in the object. Any implementation of an array within the function didn't work either, but I realize I might be going about it the wrong way.

What do you mean by property on AboutReader that holds on to an array? You mean an array as a property of AboutReader or do you mean something else entirely?

I too am worried about the amount of calculation that would go into averaging though. However this array may be implemented, we could subtract the amount that gets taken out (the first one) and then add the tenth one that gets added and divide by ten instead of going through and adding all the numbers every single time and then dividing by 10. Does that make sense? And is it even possible? :-)

Comment 14

[:Margaret Leibovic]

(In reply to Nicolas Carlo from comment #12)

> I noticed the use of 'let' but I wasn't exactly clear on scope so I went
> with var to keep the variables local within the function.

'let' has block scoping, so we prefer using it for local variables:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

(In reply to Nicolas Carlo from comment #13)
> (In reply to :Margaret Leibovic from comment #11)
> > (In reply to Nicolas Carlo from comment #10)
> > 
> > > https://dl.dropboxusercontent.com/u/22352304/fennec-24.0a1.en-US.android-
> > > arm6.apk
> >
> > It looks like this is an ARMv6 build. If you post a patch, I can just apply
> > it locally, and I'm curious to see what the patch looks like :)
> 
> Oops, apologies for the confusing file name. Its the normal arm build but I
> have five other apk files in my dropbox so I named it arm6...hehe.

Oh, haha, sorry. I should have just tried the build!

> > > I haven't been able to find a way to "collect" light values, like we
> > > discussed in chat, other than just fiddling with the two bounds that you had
> > > already set in your if-statement, and filtering values out that way. Any
> > > calculation I tried to do within the function crashed Reader mode. So if you
> > > have any advice on that, I'd appreciate it.
> > 
> > Well that sucks. Was there an error? This logic will probably be tricky to
> > implement. I think what we really want is a rolling average, something like
> > the average of the last 10 values we've seen. My first thought is to have a
> > property on AboutReader that holds onto an array of the last 10 values we've
> > seen (adding a new one and retiring the oldest one every time we get a
> > devicelight event). It would kinda suck to also need to calculate an average
> > every time we get an event, but maybe we can do something smart with math to
> > also store an every value as a property, then appropriately update it by
> > removing that oldest value from the mix and adding the newest value. What do
> > you think of that plan?
> 
> The problem was that if I tried to do any kind of calculation, when I
> switched to reader mode, it just showed a white screen. I didn't do any
> logging of the problem (I am not aware of how to do that, console.log?).

You should be able to use dump() statements. Have you used adb logcat before? If you do |adb logcat | grep GeckoConsole| you'll see any JS errors that are happening. It sounds like something was definitely going wrong there.

> I'm going to see how to implement an array in the object. Any implementation
> of an array within the function didn't work either, but I realize I might be
> going about it the wrong way.
> 
> What do you mean by property on AboutReader that holds on to an array? You
> mean an array as a property of AboutReader or do you mean something else
> entirely?

Yeah, that's what I meant, something like AboutReader._luxValues. I think it would be good to create this array where we add the "devicelight" listener, with something like |this._luxValues = []|, and then set it to null where we remove the listener.

> I too am worried about the amount of calculation that would go into
> averaging though. However this array may be implemented, we could subtract
> the amount that gets taken out (the first one) and then add the tenth one
> that gets added and divide by ten instead of going through and adding all
> the numbers every single time and then dividing by 10. Does that make sense?
> And is it even possible? :-)

Yeah, I think that's what I was trying to suggest, but I see now my comment didn't actually make sense :) In addition to the _luxValues array, we could also store an _averageLux value, that we update. I think the code for updating these properties would look something like:

// Add the newest value to the front of the array
this._luxValues.unshift(newLux);

// Remove the oldest value from the end of the array
let oldLux = this._luxValues.pop();

// Update the average lux value
this._averageLux = (this._averageLux*10 + newLux - oldLux)/10;

And we'd also need to do something smart to make sure this._luxValues is always length 10, which means doing something different before we have 10 values.

I know this is a challenging problem, but hopefully you're having fun thinking about it :)

Comment 15

[:Margaret Leibovic]

Comment on attachment 758342 [details] [diff] [review]
WIP-2-Bug-875852

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

This definitely looks like an improvement. I'd like to see if we can to the smarter array-based stuff, but this can be a backup if that proves too difficult. I think the variable names are good here.

::: mobile/android/chrome/content/aboutReader.js
@@ +321,5 @@
>    },
>  
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    // Upper bound value for "dark" color scheme beyond which it changes to
> +    // "light"

We don't really care about long lines, so this comment doesn't need to wrap.

@@ +327,5 @@
> +    // Lower bound value for "light" color scheme beyond which it changes to
> +    // "dark"
> +    var lowerBoundLight = 10;
> +    // Threshold for color scheme change
> +    var colorChangeThreshold = 20;

As I mentioned in my last comment, you should use let instead of var.

Comment 16

[Nicolas Carlo [:nickecarlo]]

Attachment: Backup patch for 875852

Margaret,

I'm submitting this patch as a backup patch with the corrections made according to your suggestions.

I've replaced 'var's with 'let's and unwrapped the comments.

I've also named the file 'backup-bug-875852.patch'.

Comment 17

[Nicolas Carlo [:nickecarlo]]

> You should be able to use dump() statements. Have you used adb logcat
> before? If you do |adb logcat | grep GeckoConsole| you'll see any JS errors
> that are happening. It sounds like something was definitely going wrong
> there.

Thanks! I have not used adb logcat before. But now that you've told me about it its definitely going to make it easier for me to know (or try to guess) what's going on without me having to put crazy stuff like alerts and whatnot.

> 
> Yeah, that's what I meant, something like AboutReader._luxValues. I think it
> would be good to create this array where we add the "devicelight" listener,
> with something like |this._luxValues = []|, and then set it to null where we
> remove the listener.

> Yeah, I think that's what I was trying to suggest, but I see now my comment
> didn't actually make sense :) In addition to the _luxValues array, we could
> also store an _averageLux value, that we update. I think the code for
> updating these properties would look something like:
> 
> // Add the newest value to the front of the array
> this._luxValues.unshift(newLux);
> 
> // Remove the oldest value from the end of the array
> let oldLux = this._luxValues.pop();
> 
> // Update the average lux value
> this._averageLux = (this._averageLux*10 + newLux - oldLux)/10;
> 
> And we'd also need to do something smart to make sure this._luxValues is
> always length 10, which means doing something different before we have 10
> values.
> 
> I know this is a challenging problem, but hopefully you're having fun
> thinking about it :)

I've submitted the backup patch just in case but I'm going to start working on this right away and hopefully I can come up with something soon.

Your suggestions about the array implementation should help me get going but I'll ask here or at #mobile if I get stuck somewhere.

This is pretty challenging but its helping me learn a lot. I didn't know Javascript but I'm now learning it because of this bug :D. But I am definitely enjoying all the challenges. Thanks a lot for the opportunity. :)

Comment 18

[Nicolas Carlo [:nickecarlo]]

Attachment: WIP patch with array applied to collect light values

Margaret,

I'm submitting this to make sure that I am on the right track. Sorry it took me a while, I had to not only learn a bit of Javascript but also familiarize myself with JSON.

This isn't the final version, the variable and array names are not final either. I will change the names to what you suggested above when I submit the final version for the patch.

I have also left a bunch of dump statements in the code which I will remove, of course, before I submit a final version.

This is what's going on:

1) I declare the array property for AboutReader (I am still not sure if this is the right way to do it or not).

2) I rename the method _handleDeviceLight to _handleDeviceLights so I don't mess with what is already working and use the method named _handleDeviceLight to collect the array values (I will change method names later too :)).

3) Every time the mode is set to auto, the array is created and in _handleDeviceLight as long as the light values in the array are less than 10, all the new values are pushed on to the array and no calculation is done. Only after there are 10 values to play with, I calculate the average (currently it is inefficient since the for loop runs every single time for all 10 values. I will work on it to make it better like we talked about before).

4) After I calculate the average, I call the _handleDeviceLights method and pass the average value to it. This function works the same as it used to. I haven't changed anything in it (except for variable names and the values as per our discussion before and the backup patch).

5) When the mode is switched from auto to either light or dark, the array is set to null as per your suggestion.

6) I have tested it and now it seems a lot more stable than it did before. I would like your input on it though and see if I am even on the right track or not. Also, let me know if you want to increase the size of the array or decrease it.

Comment 19

[:Margaret Leibovic]

Comment on attachment 760597 [details] [diff] [review]
WIP patch with array applied to collect light values

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

You're definitely on the right track here, this is looking good. I left some comments, but I think you mostly know what you need to do :)

::: mobile/android/chrome/content/aboutReader.js
@@ +319,5 @@
>  
>      Services.prefs.setIntPref("reader.font_size", this._fontSize);
>    },
>  
> +  _luxArray: [],

You don't need to declare this property on the prototype like this, since you're setting its value dynamically in _setColorSchemePref.

@@ +325,2 @@
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    let totalLight = 0;

You should declare variables as close as possible to where they're used. So this could be declared right above the for loop down below. This will probably change as you change the logic down below, but I figured it's a good style issue to mention.

@@ +325,5 @@
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    let totalLight = 0;
> +
> +    if (this._luxArray.length < 10) {
> +      this._luxArray.unshift(luxValue);

I like this idea to avoid changing the color scheme until we get 10 values that we can average, but we should add some logic to set an initial color scheme with the first value we get, since we should make sure we're starting out with the correct color scheme.

Instead of using this if/else, you can just use a return statement to bail in the case where we don't want to do anything to update the color scheme.

@@ +341,5 @@
> +        this._handleDeviceLights(averageLight);
> +    }
> +  },
> +
> +  _handleDeviceLights: function Reader_handleDeviceLights(luxValue) {

I know you said this is just WIP logic, but I think you can just put all this logic in _handleDeviceLight, and use return statements in the cases where you don't want to do the comparisons down below.

@@ +384,5 @@
>      } else {
>        this._win.removeEventListener("devicelight", this, false);
>        this._setColorScheme(colorSchemePref);
> +      dump("\nBEFOREOFFARRAY: " + this._luxArray + "\n");
> +      this._luxArray = null;

I know this is what I suggested, but something better to do would be to just delete the property entirely (using |delete this._luxArray|).

Comment 20

[Nicolas Carlo [:nickecarlo]]

Thanks a lot for the feedback Margaret.

(In reply to :Margaret Leibovic from comment #19)
> Comment on attachment 760597 [details] [diff] [review]
> WIP patch with array applied to collect light values
> 
> Review of attachment 760597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You're definitely on the right track here, this is looking good. I left some
> comments, but I think you mostly know what you need to do :)
> 
> ::: mobile/android/chrome/content/aboutReader.js
> @@ +319,5 @@
> >  
> >      Services.prefs.setIntPref("reader.font_size", this._fontSize);
> >    },
> >  
> > +  _luxArray: [],
> 
> You don't need to declare this property on the prototype like this, since
> you're setting its value dynamically in _setColorSchemePref.

Hehe, didn't know this. Thanks.

> @@ +325,2 @@
> >    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> > +    let totalLight = 0;
> 
> You should declare variables as close as possible to where they're used. So
> this could be declared right above the for loop down below. This will
> probably change as you change the logic down below, but I figured it's a
> good style issue to mention.
> 
> @@ +325,5 @@
> >    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> > +    let totalLight = 0;
> > +
> > +    if (this._luxArray.length < 10) {
> > +      this._luxArray.unshift(luxValue);
> 
> I like this idea to avoid changing the color scheme until we get 10 values
> that we can average, but we should add some logic to set an initial color
> scheme with the first value we get, since we should make sure we're starting
> out with the correct color scheme.
> 
> Instead of using this if/else, you can just use a return statement to bail
> in the case where we don't want to do anything to update the color scheme.
> 
> @@ +341,5 @@
> > +        this._handleDeviceLights(averageLight);
> > +    }
> > +  },
> > +
> > +  _handleDeviceLights: function Reader_handleDeviceLights(luxValue) {
> 
> I know you said this is just WIP logic, but I think you can just put all
> this logic in _handleDeviceLight, and use return statements in the cases
> where you don't want to do the comparisons down below.
> 
> @@ +384,5 @@
> >      } else {
> >        this._win.removeEventListener("devicelight", this, false);
> >        this._setColorScheme(colorSchemePref);
> > +      dump("\nBEFOREOFFARRAY: " + this._luxArray + "\n");
> > +      this._luxArray = null;
> 
> I know this is what I suggested, but something better to do would be to just
> delete the property entirely (using |delete this._luxArray|).

So this is what I'm thinking. Until we have 10 values, I can just send the first ever value we take in (which will always be the last value in the array since we're using unshift) as the luxValue to be used to determine what color scheme to choose. This will make sure the whole experience is consistent since after that we will average each 10 values that we take in, so on and so forth.

I am unclear a bit about the return statements. If I understand them right, each time I use them, the point where they're used is where the function would quit. What I'm thinking is that even at the time we have less than 10 values, we want to use a luxValue (maybe as I tried to explain above, if you like) to set the correct color scheme. In that case, wouldn't it be better to actually use if/else instead of return like:

   let averageLuxValue
   if (this._luxValues.length < 10) {
       this._luxValues.unshift(luxValue);
       // set averageLuxValue to the first luxValue we received until we have a size 10 array
       averageLuxValue = this._luxValues[this._luxValues.length-1];
   } else {
      // here we continue with the logic for when the array has size 10;
   }

   // down here we pass the averageLuxValue to the _setColorScheme method
   // using all the checks you'd already put in place in _handleDeviceLight

All of the above will be done in one method (_handleDeviceLight). Not sure if its clear enough what I'm thinking. If not, let me know and I'll explain again or post another WIP. What I'm thinking is that if I use return instead of doing the normal if/else above, I won't be able to pass a value to _setColorScheme method before we have a size 10 array (unless I do if (array size == 1) add value to array and pass value, if (array size > 1 && < 10) add value to array and return, but I'm thinking that's more of a hack and looks disgusting).

Comment 21

[Nicolas Carlo [:nickecarlo]]

Margaret,

I'm posting some APKs for you to try to see how many values you prefer we collect for the averaging we're trying to do. I just want to make sure collecting 10 values doesn't make it too "unrealistic" or slow for the color scheme change to take affect:

1: https://dl.dropboxusercontent.com/u/22352304/Reader-mode-collect-10-values.apk

This one collects 10 values.

2: https://dl.dropboxusercontent.com/u/22352304/Reader-mode-collect-7-values.apk

This one collects 7 values.

3: https://dl.dropboxusercontent.com/u/22352304/Reader-mode-collect-5-values.apk

This one collects 5 values.

Let me know, if you can, which one you prefer. Or if you have any problem downloading and/or using any of these Apks.

Comment 22

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 875852

I'm attaching this patch in case you're happy with 10 values. If not, I'll change it and post a new patch.

I also wanted to post the patch for you to see what it was that I was trying to explain in my other comment about if/else and see if you're okay with what I'm doing or you want me to do something else entirely.

You will also notice that I left the for loop in there. I tried to implement this logic without the for loop with a property that held average values like |this._averageLux| or something but the more the device stayed in the dark, the more it had tendency to go in the negative. And when the device came back in areas with light, it took a LONG time for it to register the added values and change modes.

For now it seems the for loop is faster and keeps it more natural than something like |this._averageLux|. Although its quite possible I wasn't doing the math right (I followed your suggestions from one of the previous comments :s).

Let me know what you think.

I hope I haven't veered too far away from what you had in mind with this patch.

I have marked the backup patch and the last WIP as obsolete with this patch since it does everything the backup patch is doing too.

I'm also marking this for review just in case I am going about this the right way.

Comment 23

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 875852

Okay, I finally got rid of the for-loop. There is no other way to put it but to say that I was being a first grade idiot. I didn't have enough coffee I guess or whatever it was but I was doing this:

    let newLux = this._luxValues.unshift(luxValue);

And then trying to add this to |this._totalLux| (I have no idea why in the world I thought that this is how it should be), of course I was getting negative values. Sorry about this confusion here. Hope you didn't waste your time with the last patch.

So now only the if/else situation remains unclear. Other than that the patch works as expected. The three APKs should tell you how many values you'd prefer we collect for this (if you want, I can post more APKs with different values).

Some comments on the patch itself:

1) I created a variable |maxLuxValuesSize = 10| so if anyone in the future wanted to change the amount of values needed to collect, they can easily change that one number instead of changing values all over the method.

2) I'm leaving |averageLuxValue| variable so we can easily pass the first luxValue (before we have 10 luxValues collected) without messing around with |this._totalLux|.

3) I've named the property that adds the 10 values |this._totalLux|. It gets created when the array property gets created and I also destroy it along with the array property in _setColorSchemePref. Let me know if you're unhappy with the name or anything else I'm doing with this property.

Comment 24

[:Margaret Leibovic]

Comment on attachment 763205 [details] [diff] [review]
Patch for bug 875852

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

This is looking really good, thanks for putting so much effort into this. I agree with you that we mainly just need to clean up this if/else logic. We actually have 3 cases here:

1) The first lux value we get. In this case this._luxValues.length would be 0, and we would just want to update the color scheme with that new value.
2) The next 8 lux values we get. In this case, we don't have enough information for a good average, so we want to just update this._luxValues and this._totalLux.
3) All lux values after this. In this case, we actually do compute the average and use that to see if we should update the color scheme.

Thinking about it as 3 cases, I think would actually be good to factor out the second half of this function to a new function, _updateColorScheme(luxValue) or something like that (sorry for the churn from a previous comment against this). Then that would make it easier to clearly organize the logic.

This patch is getting close! Playing around with it, the color scheme definitely feels more stable. Nice job.

::: mobile/android/chrome/content/aboutReader.js
@@ +320,5 @@
>      Services.prefs.setIntPref("reader.font_size", this._fontSize);
>    },
>  
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    let maxLuxValuesSize = 10;

Add a comment explaining that this is the size of the array that we want.

@@ +321,5 @@
>    },
>  
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    let maxLuxValuesSize = 10;
> +    let averageLuxValue = 0;

No need to initialize this variable, since we always assign it a new value (and we never actually intend for it to be 0). A comment here would also be nice, explaining that this is an average of the recent lux values we've seen.

@@ +322,5 @@
>  
>    _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> +    let maxLuxValuesSize = 10;
> +    let averageLuxValue = 0;
> +    let newLux = luxValue;

It doesn't look like you use luxValue anywhere after this, so instead of declaring this new variable, you can just change the parameter name to newLux (I think it's good that you did decide to use a clearer name, now that we have other values hanging around).

@@ +326,5 @@
> +    let newLux = luxValue;
> +
> +    if (this._luxValues.length < maxLuxValuesSize) {
> +      // Add the newest value to the front of the array.
> +      this._luxValues.unshift(newLux);

Pull this out of the if/else, since it's the same in both cases.

@@ +328,5 @@
> +    if (this._luxValues.length < maxLuxValuesSize) {
> +      // Add the newest value to the front of the array.
> +      this._luxValues.unshift(newLux);
> +      // Set averageLuxValue to the first luxValue we received until _luxValues.length equals maxLuxValuesSize.
> +      averageLuxValue = this._luxValues[this._luxValues.length-1];

This feels a bit too tricky for my taste, since you're assinging averageLuxValue to be something that isn't actually an average. It also seems like we're doing more work than necessary since this will always be the same value.

Comment 25

[:Margaret Leibovic]

I guess I didn't say it before, but I think 10 values is good for now. We can tweak it after this lands if people complain about it being too unresponsive.

Comment 26

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 875852

Comments on the patch:

1) I'm updating this._luxValues and this._totalLux at the top to avoid redundancy since those two properties are updated each time the method is called.

2) The style for if statements (with a nested if) looks cleaner to me. I can change it to:

    if (the length == 1) {
        update color scheme
        return
    }

    if (the length < 10) {
        return
    }

if you want.

3) I am initially updating the color scheme when the length is 1 (as opposed to 0) since the array is updated with newLux at the beginning of the method.

4) Factored the lower half out into a separate method as you suggested.

Comment 27

[:Margaret Leibovic]

Comment on attachment 764110 [details] [diff] [review]
Patch for bug 875852

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

This looks great! I just have two nits, but then we can land this.

::: mobile/android/chrome/content/aboutReader.js
@@ +326,5 @@
> +    // Add new lux value at the front of the array.
> +    this._luxValues.unshift(newLux);
> +    // Add new lux value to this._totalLux for averaging later.
> +    this._totalLux += newLux;
> +    // Don't update when length of array is less than luxValuesSize except when it is 1.

Style nit: Put a blank line above this line to make things more readable.

@@ +331,5 @@
> +    if (this._luxValues.length < luxValuesSize) {
> +      // Use the first lux value to set the color scheme until our array equals luxValuesSize.
> +      if (this._luxValues.length == 1) {
> +        this._updateColorScheme(newLux);
> +        return;

Nit: Get rid of this return, since we'll always return on the next line anyway.

Comment 28

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for check-in

Addressed the nits above.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf6498d1353

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbf6498d1353

Comment 31

[Aaron Train [:aaronmt]]

Played around with this experimenting with light and dark luminance; seems to work well now.

Comment 32

[Teodora Vermesan (:TeoVermesan)]

Test case added in moztrap:
https://moztrap.mozilla.org/manage/case/8775/