Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sdk] Brightness module audit #2408

Merged
merged 13 commits into from Jan 25, 2019

Conversation

@esamelson
Copy link
Member

commented Oct 12, 2018

Brightness module SDK audit. A couple of notes:

On Android there are two separate system settings for the brightness value in manual and automatic mode, and they are completely independent. While it is possible to read the automatic brightness value using the (undocumented) screen_auto_brightness_adj constant, the OS blocks any attempt to write to this value.

I ended up having getSystemBrightnessAsync get the automatic value if automatic brightness is turned on, since I think it makes sense to just return the actual brightness value of the phone at the moment. We could instead pass in a parameter to determine whether to read the manual or automatic value, but I really can't think of a use case where you would want to read the value you aren't using.

There are two things you cannot do with this PR's API that you would be able to with a fully native app:

  1. Write to the manual system brightness value while actually using automatic brightness
  2. Read the value of manual system brightness while using automatic brightness, or vice versa

I can't think of any reasonable use cases that require either of these things, but happy to discuss/rethink.

12/31/18: Updated this PR and added docs, since they're now in this repo. This is ready to merge once SDK 32 is out the door.

@esamelson esamelson requested a review from ide Oct 12, 2018

@esamelson esamelson changed the title Brightness module audit [WIP] Brightness module audit Oct 12, 2018

promise.resolve(null);
} else {
// TODO: what to do if we are rejecting but there is no native error?
// should we create a new Exception in order to get the native stack trace?

This comment has been minimized.

Copy link
@ide

ide Oct 12, 2018

Member

To this question -- the implementation of the Java "Promise" should be responsible for getting the stack trace (and pop off a frame or two) so that each caller doesn't need to do it.

This comment has been minimized.

Copy link
@ide

ide Oct 12, 2018

Member

To add to this -- I think this can happen in a separate PR at any time. It's pretty self-encapsulated in the sense that it's just the Promise / bridge that needs to be modified, while also cross-cutting across all modules => I don't think it needs to be grouped with this Brightness PR.

case 2:
return Settings.System.SCREEN_BRIGHTNESS_MODE_MANUAL;
default:
throw new Exception("Unsupported brightness mode");

This comment has been minimized.

Copy link
@ide

ide Oct 12, 2018

Member

We probably should include the invalid value in the error so it's easier to debug. It might also make sense to somehow specify the error code (ERR_BRIGHTNESS_INVALID_BRIGHTNESS_MODE) too, or we could have a more generic error like InvalidJSArgumentException, which would have a code like ERR_INVALID_ARGUMENT.

Show resolved Hide resolved packages/expo/src/Brightness.ts Outdated
Show resolved Hide resolved .../main/java/versioned/host/exp/exponent/modules/api/BrightnessModule.java Outdated
Show resolved Hide resolved .../main/java/versioned/host/exp/exponent/modules/api/BrightnessModule.java Outdated
@ide

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Also I agree with your take on the API tradeoffs - those cases seem exceedingly uncommon.

@esamelson esamelson force-pushed the @eric/brightness branch from 922c5af to 07e0573 Oct 15, 2018

@ide

ide approved these changes Oct 17, 2018

@esamelson esamelson force-pushed the @eric/brightness branch from 8c420af to 2b0d3e0 Oct 20, 2018

@esamelson esamelson requested a review from brentvatne as a code owner Oct 20, 2018

@ide
Copy link
Member

left a comment

Tests look good

Show resolved Hide resolved packages/expo/src/__tests__/Brightness-test.ts Outdated
Show resolved Hide resolved packages/expo/src/__tests__/Brightness-test.ts Outdated
Show resolved Hide resolved packages/expo/src/__tests__/Brightness-test.ts Outdated
Show resolved Hide resolved packages/expo/src/__tests__/Brightness-test.ts Outdated

@esamelson esamelson removed the request for review from brentvatne Oct 23, 2018

@esamelson esamelson force-pushed the @eric/brightness branch 2 times, most recently from 6e6967f to 685c8be Oct 23, 2018

@EvanBacon

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@ide

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

@EvanBacon Yep, this PR will address that request. Thanks for pointing it out.

}
}

private int brightnessModeNativeToJS(int nativeValue) {

This comment has been minimized.

Copy link
@EvanBacon

EvanBacon Nov 13, 2018

Contributor

Does JS make sense being that these should be agnostic of React Native.

@esamelson esamelson force-pushed the @eric/brightness branch from 685c8be to d7a19fb Nov 16, 2018

@esamelson esamelson referenced this pull request Nov 30, 2018

Merged

expo-errors package #2863

@esamelson esamelson force-pushed the @eric/brightness branch 2 times, most recently from 41684b1 to 46be274 Dec 31, 2018

@esamelson esamelson changed the title [WIP] Brightness module audit [sdk] Brightness module audit Dec 31, 2018

@esamelson esamelson force-pushed the @eric/brightness branch from 46be274 to c1d7c1d Dec 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.