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

Android - Keep screen on #1117

Merged
merged 8 commits into from
Aug 6, 2018
Merged

Android - Keep screen on #1117

merged 8 commits into from
Aug 6, 2018

Conversation

geriux
Copy link
Contributor

@geriux geriux commented Jul 10, 2018

Even though setScreenOnWhilePlaying is set to true 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.

Copy link

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@cobarx
Copy link
Contributor

cobarx commented Jul 10, 2018

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.

@geriux
Copy link
Contributor Author

geriux commented Jul 11, 2018

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!

@cobarx
Copy link
Contributor

cobarx commented Jul 12, 2018

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).

@geriux
Copy link
Contributor Author

geriux commented Jul 19, 2018

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!

@cobarx
Copy link
Contributor

cobarx commented Jul 20, 2018

@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.

@djw27
Copy link

djw27 commented Jul 23, 2018

@geriux
Copy link
Contributor Author

geriux commented Jul 23, 2018

@cobarx alright! I'll simplify it and update the PR =)

Copy link
Contributor

@cobarx cobarx left a 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) {
Copy link
Contributor

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();
Copy link
Contributor

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) {
Copy link
Contributor

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

@geriux
Copy link
Contributor Author

geriux commented Jul 25, 2018

Thank you for the suggestions @cobarx ! I've fixed the bug and removed the check of the flag. Let me know what you think =)

@cobarx
Copy link
Contributor

cobarx commented Aug 6, 2018

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.

@cobarx cobarx merged commit 7854a23 into TheWidlarzGroup:master Aug 6, 2018
@tkstang
Copy link

tkstang commented Apr 9, 2020

Is there a prop I need to pass in for this fix to work? I am still having this issue.

@auseika
Copy link

auseika commented Apr 14, 2020

Would be great to have a way to disable this, since sometimes video is just a background.

@sadikyalcin
Copy link

Is there a prop I need to pass in for this fix to work? I am still having this issue.

This is still an issue for me too.

@mehthabux
Copy link

any update for exoplayer ?

@mezalejandro
Copy link

Hello,
This still doesn't work in version 6.0.0-beta.5,
any solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants