-
Notifications
You must be signed in to change notification settings - Fork 255
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
Jon/detect-android-batt-saver #5262
Conversation
src/components/services/Services.tsx
Outdated
previousState = state | ||
} | ||
|
||
await checkPowerSavingMode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just inline the function. No need to declare it then call it.
src/components/services/Services.tsx
Outdated
|
||
const checkPowerSavingMode = async () => { | ||
// This method is only available for Android | ||
if (powerSavingOn != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check at the top of the useAsyncEffect
if (Platform.OS !== 'android') return
This eliminates all the checks for powerSavingOn or powerSavingModeChanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider this when I wrote the PR
Those functions are defined as optional by the lib, I'd still need to check for the existence of the function even if I made this change.
To implement your suggestion and not check for the function existence, I'd need to also patch the lib's source itself. The way I had it seemed like the simpler approach in case the contents of the lib change in the future.
src/components/services/Services.tsx
Outdated
} | ||
|
||
const handlePowerSavingModeChanged = async (state: boolean) => { | ||
if (state && !previousState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have a previousState
. showBatterySaverWarning
already checks if it's showing and early exits.
Also fixed auto-dismissing the warning when battery saver was turned off but the user never previously manually dismissed the warning |
airshipBridge = undefined | ||
}) | ||
} else if (airshipBridge != null) { | ||
// Dismiss the alert when power-saving mode turns off and there's an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause the alert to be dismissed if isPowerSavingModeOn
and the modal is showing. While you shouldn't get this case, you are relying on distributed logic to ensure this doesn't break. Logic in the SDK in addition to logic in your code.
This should be
if (isPowerSavingModeOn && airshipBridge == null) {
// showmodal
} else if (!isPowerSavingModeOn && airshipBridge != null) {
// close modal
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I originally had it like this but assumed you'd prefer it less verbose since it shouldn't happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verbose isn't bad if it makes the logic crystal clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to hear, that's my preference too
34eaf37
to
1c2d140
Compare
1c2d140
to
071f1f3
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: