Ensure screen stays on during say all#17651
Conversation
|
@LeonarddeR |
|
The only sleep state I have on this system is hybernate, since I unfortunately don't have S3 here. I once disabled s0 in the bios and was too lazy to re-enable it. |
|
btw, on some computers, it is not available to turn off the s0.. |
| |Default |Disabled| | ||
|
|
||
| This option ensures that the screen stays on when reading with say all or with braille (e.g. when pressing scroll buttons). | ||
| This avoids the situation where the screen unexpectedly locks during a say all. |
There was a problem hiding this comment.
The option name talks about system lock whereas here, you talk about screen lock. I have not reviewed this PR in detail, but reading the UG does not clarify things for me.
Are we talking about system lock, I guess session lock?) or the screen turning off (black)?
I think that you should describe both the use cases for enabled and for disabled here. Thanks.
There was a problem hiding this comment.
More specifically and reading #17651 (comment) by @btman16, I have concerns regarding braille.
I'd say that systems using modern standby should never enter standby, thus never lock, when reading braille.
|
Hi,
Based on the description, prevent system lock is ok, because the behavior
here actually depends on the system.
If the system supports the s3 sleep state, this simply refers to the screen
turning off while at least in a say all, whereas on a system supporting
modern standby, you would notice the system locking.
Does this help to clarify this?
Thanks,
Brandon
…On Thu, Jan 30, 2025 at 2:05 AM Cyrille Bougot ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In user_docs/en/userGuide.md
<#17651 (comment)>:
> @@ -3333,6 +3333,18 @@ Some GDI applications will highlight text with a background color, NVDA (via dis
In some situations, the text background may be entirely transparent, with the text layered on some other GUI element.
With several historically popular GUI APIs, the text may be rendered with a transparent background, but visually the background color is accurate.
+##### Prevent system lock during say all or reading with Braille {#preventSystemLock}
+
+| . {.hideHeaderRow} |.|
+|---|---|
+|Options |Default (Enabled), Disabled, Enabled|
+|Default |Disabled|
+
+This option ensures that the screen stays on when reading with say all or with braille (e.g. when pressing scroll buttons).
+This avoids the situation where the screen unexpectedly locks during a say all.
The option name talks about system lock whereas here, you talk about
screen lock. I have not reviewed this PR in detail, but reading the UG does
not clarify things for me.
Are we talking about system lock, I guess session lock?) or the screen
turning off (black)?
I think that you should describe both the use cases for enabled and for
disabled here. Thanks.
—
Reply to this email directly, view it on GitHub
<#17651 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD6UHRVUFFSZZULYF7UQPH32NHFK7AVCNFSM6AAAAABVZXTRPGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOBTGA3DMOBRGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
“Be what you are. This is the first step towards becoming better than you
are.”
– J. C. Hare & A. W. Hare
|
|
@CyrilleB79 I think you are raising valid concerns here.
I'm not sure how s3/s0 standby is related here. The problem is not the system idle timer, but the display idle timer. I don't think that's related to system power state, but to display power state, which is a different aspect. |
| #: @type: [int, ...] | ||
| self.brailleCells = [] | ||
| self._windowRowBufferOffsets: list[tuple[int, int]] = [(0, 1)] | ||
| self._windowRowBufferOffsets: list[tuple[int, int]] = [(0, 0)] |
There was a problem hiding this comment.
there are appears to be merge conflicts here
There was a problem hiding this comment.
I think this is fixed now.
c9f9b9a to
25d7063
Compare
|
There was something wrong in the merge process, I rebased on master to get rid of it. |
|
Frankly, the UX of this new option still seems strange and I am not sure to understand it if I only read the User Guide (i.e. not the PR). The option is called "Prevent system lock during say all or reading with braille". As a simple user, I would wonder: As a user, I would prefer an option "Allow the screen to sleep / become black during say all or while reading braille" or something similar. And NVDA should evaluate itself if the options is relevant or not depending on the standby capabilities of the system. The option should be greyed out on systems where screen standby = session lock because there is no point in enabling session lock while reading braille. |
|
What exactly is the purpose of this setting? Is it that users using say all want to have longer batery time while using NVDA? When I use NVDA actively with the keyboard, the screen doesn't get black either. So why should this be optional when using say all? |
|
I initially thought this needn't be an option, but then @btman16 chimed in, saying that it was negatively impacting his battery life. SO that's why it is an option now. |
When NVDA is reading with review cursor say all, the physical cursor isn't moving so there's basically nothing happening from a visible point of view. It makes sense not to prevent the display from turning off in that case. |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
See test results for failed build of commit a318b80cac |
Qchristensen
left a comment
There was a problem hiding this comment.
Looks good, I know this will be appreciated by users.
|
Yes, using the menus it do not open and NVDA crashs when closing or restarting. |
Does this option apply only to the case where there is no visible move of the cursor, i.e. browse mode cursor on webpages or review cursor? Or does it also apply when there are visible cursor moves, e.g. system caret moved during say all in Notepad, Word, Outlook, etc.? |
Fixes #17705 Fixup of #17651 Summary of the issue: Pr #17651 introduced a feature flag in the general section of the config, but this is not supported yet. Description of user facing changes General settings panel can be reopened after saving. Description of development approach Rename the flag and make it a boolean.
Link to issue number:
Fixes #17649
Partially reverts #11118
Follow up of #10111, #10643,
Summary of the issue:
On some systems (mine, for example 😉) the screen still locks during a say all even though we reset the system idle timer. We also reset the display timer in the past, but this was reverted in #11118, particularly on request of @btman16
Description of user facing changes
Added a new advanced setting that, when enabled, also resets the display timer.
Description of development approach
ES_CONTINUOUSflag. This should avoid an edge case where reading a line takes more then a minute and the sleep timer is a minute, in which case the system will still lock.Testing strategy:
Tested on my system. Noted that with the feature flag enabled, the system doesn't lock, whereas when disabled, it does lock.
Known issues with pull request:
None known
Code Review Checklist:
@coderabbitai summary