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

Fix the screen saver blocker never getting disabled #3286

Merged

Conversation

absidue
Copy link
Member

@absidue absidue commented Mar 11, 2023

Fix the screen saver blocker never getting disabled

Pull Request Type

  • Bugfix

Related issue

closes #1369
might partially solve this discussion #3248
closes #3140

Description

Fixes the screen saver blocking being turned on but never off.

(functions have their own this scope that can be overriden, so what was happening is that the play event was actually assigning the blocker token to the player instance instead of the ft-player component, so when other functions went to try and disable the blocker with the token, they didn't find it and therefore couldn't disable the blocker)

Testing

The screen saver seems to be flaky on my comptuer regardless of whether i'm using FreeTube or not, but other people have tested this and it works for them.

https://github.com/absidue/FreeTube/actions/runs/4393284869

The screen saver should be disabled when the video is playing but should be reenabled if the video is paused, ends or a player error occurs or the ft-player component is destroyed.

In each test case the screen saver should be able to come on after performing these steps

Test case 1:
Start playing a video, then pause it

Test case 2:
Start playing a video, then leave the watch page

Test case 3:
Start playing a video, seek almost to the end, wait until the video ends

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.18.0

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 11, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 11, 2023 17:17
PikachuEXE
PikachuEXE previously approved these changes Mar 12, 2023
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

I have no idea how to test screensaver quickly
Code looks fine though

@efb4f5ff-1298-471a-8973-3d47447115dc

Might also addresses #3140

@oldlamps
Copy link

Hi, thanks for addressing this issue. I've recently shared my experience over in #3140 (comment) .. I downloaded the appImage from https://github.com/absidue/FreeTube/actions/runs/4393284869 , and tested a few scenarios that were keeping the screen saver lock on.

I'm glad to report that this seems to fix the issue on my end.

Test case 1:
Playing a video creates a "user session inhibited block", pausing removes the block

Test case 2:
While playing I navigate away to the subscriptions page. The block is removed!

Test case 3:
Playing a video, the lock is create. Skip to the end of the video, and let it finish. Upon finishing the block is removed!

These are the 3 scenarios that were keeping the block in place. I have verified with the release version, and the bugs are persistent.

Very happy about this , thank you for looking into it again. I'll be using this AppImage, and hopefully the fix will find its way in.

@absidue absidue changed the title Rework the screen saver blocking while watching a video Fix the screen saver blocking while watching a video Mar 12, 2023
@absidue absidue changed the title Fix the screen saver blocking while watching a video Fix the screen saver blocker never getting disabled Mar 12, 2023
@absidue
Copy link
Member Author

absidue commented Mar 12, 2023

@oldlamps Thank you so much for confirming it works for you :).

@efb4f5ff-1298-471a-8973-3d47447115dc

@John4003 is it possible for u to test this?

@absidue absidue disabled auto-merge March 12, 2023 21:43
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 12, 2023 21:43
@PikachuEXE
Copy link
Collaborator

Let me test on MacOS if this is not approved yet tomorrow
#3140

@PikachuEXE
Copy link
Collaborator

Tested with MacOS 12.6.3, all 3 test cases passed

Steps (per case):

  • Update display to sleep after 1 minute
  • Perform test case
  • wait 1 minute
Test case 1:
Start playing a video, then pause it

Test case 2:
Start playing a video, then leave the watch page

Test case 3:
Start playing a video, seek almost to the end, wait until the video ends

Also tested that the display won't go to sleep when playing a video (played a video with 1:00 length)

@FreeTubeBot FreeTubeBot merged commit 6c3ff9e into FreeTubeApp:development Mar 14, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 14, 2023
@absidue absidue deleted the rework-power-save-blocker branch March 14, 2023 07:22
@DnickC
Copy link

DnickC commented Mar 15, 2023

Great to see this issue being fixed!

I have no idea how to test screensaver quickly Code looks fine though

I use 'inhibit applet' on mint 19, this shows me power management being blocked or not in a panel icon, great tool for testing, no need to wait for the screensaver.

I tested on mint 19.3, all 3 of the test cases succeed now, but this fails for me:
start playing a video, reload the page with ctrl+r

@oldlamps
Copy link

Great to see this issue being fixed!

I have no idea how to test screensaver quickly Code looks fine though

I use 'inhibit applet' on mint 19, this shows me power management being blocked or not in a panel icon, great tool for testing, no need to wait for the screensaver.

I tested on mint 19.3, all 3 of the test cases succeed now, but this fails for me: start playing a video, reload the page with ctrl+r

I can confirm that is still one case where the block still sticks. While the video is loading / loaded and playing hitting ctrl+r will cause the inhibit block to stay once the page reloads, and continue despite pausing / unpausing, letting the video finish, or navigating away.

@absidue absidue mentioned this pull request Mar 30, 2023
5 tasks
@absidue
Copy link
Member Author

absidue commented Mar 30, 2023

Reloading is a special case, when you reload a page you force it to throw away all of it's javascript state including variables (the same thing happens when you reload a page in a web browser), so you make that window forget it's token to disable the power save blocker. We'll need to see if we can detect a reload before it happens and remove the blocker before the reload.

@EvilGremlin
Copy link

EvilGremlin commented May 14, 2023

+1, reload totally must remove any previous locks, issue is unresolved until then.

@absidue
Copy link
Member Author

absidue commented May 14, 2023

@EvilGremlin The issue is essentially solved if you use the nightly builds. There should never be a need to use the reload function anyway, pressing the backwards and forwards keys does the same but a lot less destructive (reloading reloads that entire window, forcing it to reinitialise everything, it's almost the same as closing and reopening FreeTube).

@EvilGremlin
Copy link

pressing ctrl+r is much faster and convenient thann going to history to open that video again

@absidue
Copy link
Member Author

absidue commented May 14, 2023

You can just use the backward and forward buttons.

Also avoids having load your settings, subscriptions, watch history, playlists and everything else again.

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.

[Bug]: Freetube blocks MacOs from auto sleep [feature] screensaver disabled only if video is playing (linux)
8 participants