LiveText: pumped generator follow-up#20216
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new “Beep for skipped lines” terminal setting and related terminal flood-handling controls, while updating release notes for the live-region freeze fix.
Changes:
- Documented and surfaced a new Advanced setting: “Beep for skipped lines”.
- Added config options to cap/batch reported terminal lines and to configure the skipped-lines beep.
- Updated changelog entry to include additional issue/credit references.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/userGuide.md | Documents the new “Beep for skipped lines” Advanced setting. |
| user_docs/en/changes.md | Updates the bug-fix entry with additional references/credits. |
| source/gui/settingsDialogs.py | Adds the Advanced Settings checkbox, default handling, and persistence. |
| source/config/configSpec.py | Introduces new [terminals] tuning options (limits, batching, beep params). |
| source/NVDAObjects/behaviors.py | Implements line dropping, batching, and optional beep when lines are skipped. |
… the new LiveText speech generator
8ac79a8 to
f003e22
Compare
|
@codeofdusk I like this, fully agree it should be done. |
| | . {.hideHeaderRow} |.| | ||
| |---|---| | ||
| |Options |Disabled, Enabled| | ||
| |Default |Enabled| |
There was a problem hiding this comment.
please move this table to below the option description
| maxNewLines = integer(min=0, default=100) | ||
| newLinesBatchSize = integer(min=0, default=5) | ||
| beepForSkippedLines = boolean(default=true) | ||
| skippedLinesBeepHz = integer(default=550, min=20) | ||
| skippedLinesBeepMinDurationMs = integer(default=10, min=5, max=5000) | ||
| skippedLinesBeepMaxDurationMs = integer(default=100, min=5, max=5000) |
There was a problem hiding this comment.
Please remove the hidden config options, in favour of constants stored in code
There was a problem hiding this comment.
Why? This makes them immutable at runtime.
Might it be better to expose a visible feature flag to control the new behaviour instead?
There was a problem hiding this comment.
At minimum, "beep for skipped lines" (already in settings) and a toggle (hidden or user-facing) to switch back to the old behaviour. Right now, maxNewLines of 0 disables the new behaviour and setting different values tunes it.
There was a problem hiding this comment.
I think exposing maxNewLines is fine, but the rest I think should remain in code and be removed from config
|
Is this something you intend to continue to work on? |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Link to issue number:
#20177
Summary of the issue:
After #20177:
Description of how this pull request fixes the issue:
This PR:
Testing strategy:
Verified that all settings and the beep work as expected. Alpha testing.
Known issues with pull request:
None known
Code Review Checklist: