-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Android - Keep screen on #1117
Android - Keep screen on #1117
Conversation
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.
💯
Thanks for pointing this out and creating a fix. I haven't gotten to test this, so please excuse if I'm not up to speed. I think the ideal behavior is that the screen stays on while you're playing the video, but if you pause the video the screen should be allowed to go to sleep as that's what other video apps do. If we set this flag, will the screen go to sleep while paused? Also, is this a problem on ExoPlayer as well? If so, would you mind creating a PR for that. |
You're right @cobarx the screen should be allowed to go to sleep if the video is paused so I changed the code to take that into account as well :) let me know what you think. As for the ExoPlayer I can't check it at the moment but I'll try to soon. Thanks! |
Thanks for tackling the pause conditions. Is there a reason we need to check the current state of the flag rather than applying the appropriate value any time we play or pause? It adds quite a bit of code & complexity unless there's a compelling need for it (like performance). |
Hi! I had a comment in my e-mail about this being fixed in another PR but i've just tested it and it is still happening with the latest version. Also I don't see your comment here :D do you still want me to continue with this PR? Let me know! |
@geriux I posted in the wrong PR, that comment was meant to go in the one about the iOS hanging while loading. Yes, I would like to get this included. Can you simplify the logic so that it's more straightforward? Unless there's a good reason to do otherwise, it's much simpler to set and clear the flag every time rather than tracking the current state. |
See here for how it could potentially be simplified: https://github.com/corbt/react-native-keep-awake/blob/master/android/src/main/java/com/corbt/keepawake/KCKeepAwake.java |
@cobarx alright! I'll simplify it and update the PR =) |
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've added a fix for one bug and also included some examples of how to simplify the code.
} | ||
|
||
public void setPreventScreenFromDimmingFlag(final boolean state) { | ||
if (!mMediaPlayerValid && mThemedReactContext == 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.
This should be !mMediaPlayerValid || mThemedReactContext == null
return; | ||
} | ||
|
||
final boolean isFlagOn = isPreventScreenFromDimmingFlagOn(); |
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 would remove this check and simply apply the flag every time.
public boolean isPreventScreenFromDimmingFlagOn() { | ||
int flags = mThemedReactContext.getCurrentActivity().getWindow().getAttributes().flags; | ||
|
||
if ((flags & WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON) == 0) { |
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.
If we need to keep this check for performance or other reasons, it can be written as:
return flags & WindowManager.LayoutParams.FLAG_KEEP_SCREEN_ON == 0
…act-native-video # Conflicts: # android/src/main/java/com/brentvatne/react/ReactVideoView.java
Thank you for the suggestions @cobarx ! I've fixed the bug and removed the check of the flag. Let me know what you think =) |
Sorry for the delay in getting this merged. I kept forgetting to bring along my cable when going to coffee shops and there's no way to test without using a real device #smh I went ahead and switched this over to setKeepScreenOn since it's much simpler and matches what we're using on the ExoPlayer side. We also need to check for when the video ends and allow the screen to sleep if the video isn't on repeat. I tested and it works great. I'm glad you brought this up, this is pretty important behavior, I have no idea how people got around this. Maybe everybody was using other modules to control sleeping. |
Is there a prop I need to pass in for this fix to work? I am still having this issue. |
Would be great to have a way to disable this, since sometimes video is just a background. |
This is still an issue for me too. |
any update for exoplayer ? |
Hello, |
Even though
setScreenOnWhilePlaying
is set totrue
in the MediaPlayer, on Android the screen will dim depending on the time the user has set on their phone.It was already reported in this issue #744.
This will prevent the screen to turn off while a video is playing and it only affects the Android platform.