-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add wakelock support #1984
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
base: master
Are you sure you want to change the base?
Add wakelock support #1984
Conversation
|
There could absolutely be cases where this can be useful, but we can't assume that all users want to prevent their screen from locking when they are connected to a session. It should probably be an opt-in feature. |
It should probably be an opt-in feature. 100% agree. This feature is controlled by the newly added If you want, I can try to plumb this configuration option into the (advanced) settings in the UI. This would also require me to add support for toggling the configuration at runtime. |
That sounds great! 😃 |
|
How are you progressing on this? 🙂 |
644c520 to
78695ba
Compare
|
Thanks for checking up on this. I have rewritten this, but am currently working on tests. I am at a conference this/next week, but will try to get the tests finished after that. I have pushed my new changes to this PR, but I want to clean them up a little:
|
63a63d1 to
428b1f7
Compare
|
Actually... I ended up finding some time to hack on the last few things I wanted with this. I have updated this PR. Not sure if you would like me to squash these down commits. |
428b1f7 to
a6f157d
Compare
|
This change should be in a good state to review. |
|
Works great! Tested on Android 15 I would suggest we put this option as ON by default. |
What's your thinking here? I'd consider it very misbehaved for any other application to not respect my screen timeout. Applications that are passively used are normally the exception, not the rule. |
I would personally expect a remote desktop client to keep my screen awake. But perhaps that isnt the case for everyone. If we dont want to enable this by default, I would at least move it out of "advanced". |
|
In what way do you think using remote applications differs from using local ones? Perhaps there is some room for automation here. |
I use desktop applications and mobile applications differently. In my mind the norm is to have a significantly longer screen timeout, if any, on desktop. While a short timeout is probably quite common on mobile. When connecting to a remote desktop via noVNC I'd like things to act more like a desktop. I'd be very annoyed if my desktop screen automatically locked after 30 seconds of inactivity. |
7ece518 to
1719d4b
Compare
For the moment, I will keep the default set to As for moving it out of "advanced", I can if you want... I am just very hesitant to move it, given that most other settings are in "advanced" (like |
I'm fine with that. I understand your reasoning for keeping it set to false by default. Having it under advanced or not isn't a big deal. Given that @CendioOssman is satisfied with the review comments, I think we are ready to merge! |
CendioOssman
left a comment
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.
There are some minor details left. And the linter isn't happy.
Add a new configuration option `keep_device_awake` to allow noVNC to stop the local display from going to sleep. This is especially useful with view-only sessions. This new option has been added to the configuration UI, making it easier for users to configure. When this option is changed at runtime, we will request/release the wake lock. We only hold the view lock while connected to a server. We will also attempt to reacquire the wakelock if we lost it due to a visibility change (the tab becoming inactive, or during the transition into/from fullscreen). All existing unittests have been run, and the change has been manually tested in Firefox 145. Additional tests will be added later.
Add an error state to the WakeLockManager state machine. This adds a transition that can be detected from tests (it otherwise serves no purpose, and the system immediatly transitions back into the released state).
Dispatch an event on each state transition inside the WakeLockManager. This gives the unit tests something to synchronise on, allowing us to write fast, flake-free tests.
Add tests to for both the `rfb` side (calling into the new wakelock code), and the new wakelock class (which tracks the desired state and how to get there).
1719d4b to
988e9da
Compare
Done.
I believe I have fixed these now. |
CendioOssman
left a comment
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.
Looks good to me!
Thanks for your work!
Add a new configuration option
use_wakelockto allow noVNC to stop the local display from going to sleep. This is especially useful with view-only sessions.We only hold the view lock while connected to a server. We will also attempt to reacquire the wakelock if we lost it due to a visibility change (the tab becoming inactive, or during the transition into/from fullscreen).
All existing unittests have been run (with #1983 manually patched into my working directory), and the change has been manually tested in Firefox 142.