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

Prevent system sleep when reading lon pieces of text in say all #10643

Merged
merged 1 commit into from Jan 30, 2020

Conversation

leonardder
Copy link
Collaborator

@leonardder leonardder commented Dec 19, 2019

Link to issue number:

Follow up of #10111

Summary of the issue:

When a system is set up to lock or go to sleep within one minute, say all is interrupted when reading long paragraphs in virtual buffers. Possibly in other cases as well.

Description of how this pull request fixes the issue:

Similar as in #10111. We instruct the system not to sleep. This happens on every line we reach.

Testing performed:

Tested with a long paragraph of text in Firefox browse mode. My system no longer locks within one minute.

Known issues with pull request:

In theory, say all could still be interrupted when reading a very long line at a very slow rate.

Change log entry:

  • Changes
    • NVDA prevents the system from locking or going to sleep when in say all.

@leonardder leonardder marked this pull request as ready for review Dec 20, 2019
@leonardder leonardder requested a review from feerrenrut Jan 30, 2020
@leonardder
Copy link
Collaborator Author

leonardder commented Jan 30, 2020

@feerrenrut: I'd like to shamelessly ask you to review this, as this really interrupts my current work flow in a major way. I have a system that has a lock of the screen enforced at one minute by means of a group policy, and this makes it very hard to use say all in Word and on the web.

Copy link
Member

@feerrenrut feerrenrut left a comment

Thanks @leonardder

@feerrenrut feerrenrut merged commit b94442a into nvaccess:master Jan 30, 2020
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jan 30, 2020
@feerrenrut feerrenrut modified the milestones: 2019.3, 2020.1 Jan 30, 2020
feerrenrut added a commit that referenced this issue Jan 30, 2020
@btman16
Copy link

btman16 commented May 2, 2020

Hi,

On systems without group policies set, it's preventing my laptop from turning off the screen which is consuming more power on my laptop. My laptop has a very small battery, so I need to get as much power out of it as I can, and this will have negative effects.
This is a huge show stopper for me, and I'd like to ask if this can be made optional please? Or could there be a way to do a say all without this change?

Thank you very much and I look forward to any input you may have.

Thanks,

Brandon

@feerrenrut
Copy link
Member

feerrenrut commented May 4, 2020

@leonardder Did you investigate using only winKernel.SetThreadExecutionState(winKernel.ES_SYSTEM_REQUIRED) E.G. leaving out winKernel.ES_DISPLAY_REQUIRED?

@btman16 It would be best if you create a new issue to explain the problem. In the new issue please answer the following:

  • Do you expect the computer to sleep, or just let the display turn off and continue with say-all?
  • Is the screen being kept on even when not using say-all?

@leonardder
Copy link
Collaborator Author

leonardder commented May 4, 2020

This flag was added on request of @michaelDCurran in #10111 (comment) . I"m happy to remove it for this case and provide a pr. Does this block 2020.1 from being released, though?

@feerrenrut
Copy link
Member

feerrenrut commented May 4, 2020

I"m happy to remove it for this case and provide a pr.

I'd like to understand the use-cases from @btman16 better first.

Does this block 2020.1 from being released

I don't think this is a serious enough issue to block the release at this late stage.

From the MS docs:

Multimedia applications, such as video players and presentation applications, must use ES_DISPLAY_REQUIRED when they display video for long periods of time without user input. Applications such as word processors, spreadsheets, browsers, and games do not need to call SetThreadExecutionState.

This makes me think that ES_DISPLAY_REQUIRED isn't appropriate for audio only users, though, of course this depends on the user. Low-vision users may still want the screen to be kept on. It's certainly annoying for me having the screen turn off while I'm trying to read a long thing.

Also the end of the MS docs has an answer to the screen saver question. Though the use of screen savers is pretty rare these days!

This function does not stop the screen saver from executing.

@leonardder
Copy link
Collaborator Author

leonardder commented May 4, 2020

@btman16 Would you be able to try this build of NVDA to see whether this fixes it for you?

@leonardder
Copy link
Collaborator Author

leonardder commented May 4, 2020

Low-vision users may still want the screen to be kept on. It's certainly annoying for me having the screen turn off while I'm trying to read a long thing.

That's what I figured as well. Especially if we intend the NVDA highlighter to follow along say all at some point.

@btman16 nevertheless, it would be helpful to see how the try build behaves in your case. Also, wouldn't it be possible for you to disable your screen with windows+p entirely (choose second screen)? That works for several older pc's that still have a dedicated VGA connector, like my five year old laptop.

@btman16
Copy link

btman16 commented May 4, 2020

@btman16
Copy link

btman16 commented May 4, 2020

@josephsl
Copy link
Collaborator

josephsl commented May 4, 2020

@btman16
Copy link

btman16 commented May 4, 2020

@feerrenrut
Copy link
Member

feerrenrut commented May 5, 2020

While low vision or sighted users likely want to keep their screen on during long periods of reading, I think they are more likely to windows settings appropriate for keeping their screen on, and familiar with using the mouse to keep the screen on.

It's certainly too late 2020.1, but I would accept a PR for this change.

@btman16
Copy link

btman16 commented May 5, 2020

@XLTechie
Copy link
Contributor

XLTechie commented May 6, 2020

@btman16
Copy link

btman16 commented May 6, 2020

@XLTechie
Copy link
Contributor

XLTechie commented May 6, 2020

@feerrenrut
Copy link
Member

feerrenrut commented May 8, 2020

Would you still like me to make a new issue describing this case?

@btman16 No new issue required. Thanks!

@btman16
Copy link

btman16 commented May 8, 2020

@feerrenrut
Copy link
Member

feerrenrut commented May 11, 2020

Will it be showing up in Alpha?

Yes, it was delivered with PR #11118 and should now be available in alpha.

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.

None yet

6 participants